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

Status: RESOLVED FIXED

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

Description

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

Comment 2

[: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).

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch (qref'd)

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 5

[Justin Dolske [:Dolske]]

P5 as a code cleanup without user impact.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(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?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gavin Sharp [email: [e-mail]] 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.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: 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.

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.

Comment 10

[: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.

Comment 11

[:Margaret Leibovic]

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.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/4e58a41d5fa0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e58a41d5fa0

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

Is this getting to be uplifted?

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a0cc9a4ec206

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8585807 [details] [diff] [review]
Workaround patch

Should be in 38 beta 2

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f7d65dc9093b