Closed Bug 1142196 Opened 5 years ago Closed 5 years ago

Make JS' reader mode and Java's "Add to reading list" buttons' behavior consistent

Categories

(Firefox for Android Graveyard :: Reading List, defect)

All
Android
defect
Not set

Tracking

(firefox38 fixed, firefox39 fixed, fennec38+)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
fennec 38+ ---

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
Component: General → Reading List
tracking-fennec: --- → ?
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
tracking-fennec: ? → 38+
mcomella, do you have time to work on a fix for this?
Flags: needinfo?(michael.l.comella)
Sure, I'll look into it.
Assignee: nobody → michael.l.comella
Flags: needinfo?(michael.l.comella)
/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
Attachment #8578928 - Flags: review?(margaret.leibovic)
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.
Comment on attachment 8578928 [details]
MozReview Request: bz://1142196/mcomella

https://reviewboard.mozilla.org/r/5555/#review4577

Ship It!
Attachment #8578928 - Flags: review?(margaret.leibovic) → review+
(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. :)
Blocks: 1133158
(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.
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
Attachment #8578928 - Flags: approval-mozilla-aurora?
Attachment #8578928 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8578928 - Attachment is obsolete: true
Attachment #8619731 - Flags: review+
Attachment #8619732 - Flags: review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.