From bug 1127445 comment 35: Note: the reader mode "add to reading list" button has inconsistent behavior with the "add to reading list" button just added. I believe this is because LocalReadingListAccessor distinguishes between reader and non-reader urls while the JS implmentation does not.

I just noticed this today. Using the overflow menu to "add to reading list" when viewing a reader-ized page will add the about:reader URL to the Reading List. This is not what the JS implementation does, which adds the original page URL to the Reading List along with an excerpt. Tracking fx38 since bug 1127445 is tracking fx38

mcomella, do you have time to work on a fix for this?

Sure, I'll look into it.

/r/5557 - Bug 1142196 - Make LocalReadingListAccessor strip about:reader urls to get the base url. r=margaret /r/5559 - Bug 1142196 - Match about:reader add/remove button state with the toolbar's on press. r=margaret Pull down these commits: hg pull review -r 9ca9c4a69d73dbcf44ffdeb5c4b5e5fdf4df9843

Not sure what to do with delete w/ ID - I would be fine just making it a followup bug.

https://reviewboard.mozilla.org/r/5559/#review4571 ::: mobile/android/base/db/LocalReadingListAccessor.java (Diff revision 1) > + // TODO: For completness, we should send a "Reader:Removed" We may address this in bug 1133158.

https://reviewboard.mozilla.org/r/5557/#review4573 I don't love the fact that we need to use this stripURI call all over the place... I feel like it would be nicer if all consumers would just send the appropriately stripped URLs to ReadingListAccessor, but I also realize it's hard to guarantee that. Maybe in the future we'll have the time to do a thorough audit of the way we're passing around reader URLs, but this seems like the easiest path forward given we want to uplift this.

https://reviewboard.mozilla.org/r/5559/#review4575 Ship It!

Comment on attachment 8578928 [details] MozReview Request: bz://1142196/mcomella https://reviewboard.mozilla.org/r/5555/#review4577 Ship It!

(In reply to :Margaret Leibovic from comment #6) > https://reviewboard.mozilla.org/r/5559/#review4571 > > ::: mobile/android/base/db/LocalReadingListAccessor.java > (Diff revision 1) > > + // TODO: For completness, we should send a "Reader:Removed" > > We may address this in bug 1133158. Then I'll pretend it's the followup bug I filed. :)

(In reply to :Margaret Leibovic from comment #7) > https://reviewboard.mozilla.org/r/5557/#review4573 > > I don't love the fact that we need to use this stripURI call all over the > place... I feel like it would be nicer if all consumers would just send the > appropriately stripped URLs to ReadingListAccessor, but I also realize it's > hard to guarantee that. Assertions! :) > Maybe in the future we'll have the time to do a > thorough audit of the way we're passing around reader URLs, but this seems > like the easiest path forward given we want to uplift this. I'm pretty sure that JS and the Java menu are the only places these are called if you wanted to do it that way. But this is written, working, and not terrible so I'm going to land it - feel free to file a followup.

remote: https://hg.mozilla.org/integration/fx-team/rev/00a0c56df026 remote: https://hg.mozilla.org/integration/fx-team/rev/1fd14a76c483

https://hg.mozilla.org/mozilla-central/rev/00a0c56df026 https://hg.mozilla.org/mozilla-central/rev/1fd14a76c483

Comment on attachment 8578928 [details] MozReview Request: bz://1142196/mcomella This should be uplifted with bug 1127445 - I will do the uplift. Approval Request Comment [Feature/regressing bug #]: bug 1127445 [User impact if declined]: Users who add reading list items will have inconsistent experiences based on whether they do it in Java (browser toolbar menu) or JS (reader mode). [Describe test coverage new/current, TreeHerder]: None [Risks and why]: (Low risk) We get the original url from about:reader urls using existing methods and (moderate risk) we add some Java -> Gecko messages to express when a reading list item is added/removed using existing message Strings. This is concerning to me because I'm not as familiar with the Gecko reader mode code and don't know who might be listening for these existing messages (no one else, from what I can tell), but Margaret, who largely wrote this code, reviewed my changesets and seems okay with it. [String/UUID change made/needed]: None

remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/93781202ec1b remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/7da1870a0194