Closed Bug 1146773 Opened 6 years ago Closed 6 years ago

Unify the code paths for adding an item to the reading list (location bar + reader mode)

Categories

(Firefox Graveyard :: Reading List, defect, P3)

defect

Tracking

(firefox38 fixed, firefox39 fixed, firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Items added to the reading list via the location bar use ReadingList.addItemFromBrowser whereas the Reader Mode uses ReadingList.addItem. The first retrieves metadata using PageMetadata.jsm, whereas the latter retrieves metadata using the Readability.js parsing.
Flags: qe-verify-
Flags: firefox-backlog+
Blocks: 1132074
Attached patch Patch (obsolete) — Splinter Review
The article object is still passed in from AboutReader.jsm because Android is using it. Do you know of any plans to migrate towards ReadingList.addItemFromBrowser on Android or should desktop be migrating towards ReadingList.addItem?

It seems weird that we have two functions that should do the same thing, but can end up having different outcomes.
Attachment #8582634 - Flags: review?(margaret.leibovic)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Created attachment 8582634 [details] [diff] [review]
> Patch

Looks like you accidentally uploaded an empty patch.

> The article object is still passed in from AboutReader.jsm because Android
> is using it. Do you know of any plans to migrate towards
> ReadingList.addItemFromBrowser on Android or should desktop be migrating
> towards ReadingList.addItem?
> 
> It seems weird that we have two functions that should do the same thing, but
> can end up having different outcomes.

Nope, I don't know of any plans to use ReadingList.addItemFromBrowser. I don't know that there's much we can actually share, since we'll always need to send a message to Java with the reading list data.

However, if you see something we can improve, feel free to file a bug. I'm a fan of shared code (where it makes sense).
Attachment #8582634 - Flags: review?(margaret.leibovic)
Attached patch Patch (qref'd) (obsolete) — Splinter Review
Attachment #8582634 - Attachment is obsolete: true
Attachment #8582687 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8582687 [details] [diff] [review]
Patch (qref'd)

Ok, alternatively we could see about getting rid of addItemFromBrowser and get readability.js to use PageMetadata (after moving it to a standalone js file).

I think that's the better long-term strategy as then Reader Mode can use it.
Attachment #8582687 - Flags: feedback?(margaret.leibovic)
P5 as a code cleanup without user impact.
Priority: -- → P5
(In reply to Justin Dolske [:Dolske] from comment #5)
> P5 as a code cleanup without user impact.

bug 1146348 suggests this has user impact. Articulated another way:

<madhava> something I've noticed about reading list and view
<madhava> is that if I add to my list from the original article, it'll pick up a thumbnail image
<madhava> but if I flip into reader view, and if that view has no images, and then I add it to my list, it doesn't get a thumbnail image
<madhava> I guess because the readerified version had no images in it?

Jared, is that case affected by this too?
Flags: needinfo?(jaws)
Duplicate of this bug: 1147571
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #6)
> Jared, is that case affected by this too?

Yeah, therefore we should bump up this priority as there are already known issues due to this and potentially unknown issues.
Flags: needinfo?(jaws)
Priority: P5 → P3
Attached patch Workaround patchSplinter Review
I think to fix this the *right* way, we would alter Readability.js to use PageMetadata.jsm to retrieve the article metadata instead of Readability's _getArticleMetadata, but Readability.js says not to modify the file directly.

This patch augments what is returned by Readability by adding on the preview image that is return from PageMetadata.jsm. Note I also had to remove the "status" property of the article because it was throwing an error on Desktop saying that the column doesn't exist in the reading list database.
Attachment #8582687 - Attachment is obsolete: true
Attachment #8585807 - Flags: review?(margaret.leibovic)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Created attachment 8585807 [details] [diff] [review]
> Workaround patch
> 
> I think to fix this the *right* way, we would alter Readability.js to use
> PageMetadata.jsm to retrieve the article metadata instead of Readability's
> _getArticleMetadata, but Readability.js says not to modify the file directly.

Yeah, Readability.js should not depend on PageMetadata.jsm, since it's designed to be run in any JS context (not just Firefox).

If you want, what you could do is update the article object returned by Readability.js in ReaderMode.jsm:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/ReaderMode.jsm#248

That is the only place where we run Readability.js, so all article objects that are passed around stems from that.
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Comment on attachment 8585807 [details] [diff] [review]
Workaround patch

Review of attachment 8585807 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me. I think keeping this logic desktop-specific makes the most sense for the short-term (aka Fx38). In the long term, it could be worth incorporating this into ReaderMode.jsm, although in that case we don't always have a browser, so it may be harder to integrate with PageMetadata.jsm.

::: browser/modules/ReaderParent.jsm
@@ +44,5 @@
>    receiveMessage: function(message) {
>      switch (message.name) {
>        case "Reader:AddToList":
>          let article = message.data.article;
> +        ReadingList.getMetadataFromBrowser(message.target).then(function(metadata) {

How does this work if about:reader is currently loaded in the browser? Doesn't this get the metatdata for the about:reader document, not for the original document? Looking at PageMetadata.jsm, it looks like the preview is just an image we pull from the document, so this should work for about:reader, but it may be worth noting here that we're pulling the matadata from the about:reader document, so some of the other properties wouldn't be what we would want.
Attachment #8585807 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #11)
> Comment on attachment 8585807 [details] [diff] [review]
> Workaround patch
> 
> Review of attachment 8585807 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine to me. I think keeping this logic desktop-specific makes the
> most sense for the short-term (aka Fx38). In the long term, it could be
> worth incorporating this into ReaderMode.jsm, although in that case we don't
> always have a browser, so it may be harder to integrate with
> PageMetadata.jsm.
> 
> ::: browser/modules/ReaderParent.jsm
> @@ +44,5 @@
> >    receiveMessage: function(message) {
> >      switch (message.name) {
> >        case "Reader:AddToList":
> >          let article = message.data.article;
> > +        ReadingList.getMetadataFromBrowser(message.target).then(function(metadata) {
> 
> How does this work if about:reader is currently loaded in the browser?
> Doesn't this get the metatdata for the about:reader document, not for the
> original document? Looking at PageMetadata.jsm, it looks like the preview is
> just an image we pull from the document, so this should work for
> about:reader, but it may be worth noting here that we're pulling the
> matadata from the about:reader document, so some of the other properties
> wouldn't be what we would want.

Yeah, this will just pull the metadata from the about:reader page, so it's not perfect but it does make them closer in behavior.
https://hg.mozilla.org/mozilla-central/rev/4e58a41d5fa0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Is this getting to be uplifted?
Flags: needinfo?(jaws)
Comment on attachment 8585807 [details] [diff] [review]
Workaround patch

Approval Request Comment
[Feature/regressing bug #]: part of reader mode
[User impact if declined]: thumbnails may be different depending on which button is used
[Describe test coverage new/current, TreeHerder]: manual testing, landed on m-c
[Risks and why]: none expected
[String/UUID change made/needed]: none
Attachment #8585807 - Flags: approval-mozilla-beta?
Comment on attachment 8585807 [details] [diff] [review]
Workaround patch

Should be in 38 beta 2
Attachment #8585807 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.