814587 - Add "Add to Reading List" to link context menu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

It might be useful to long tap on a link on a page and add the article to your reading list. We could add pages to a queue and download/check them one by one in the background. A toast or notification could notify the user if that was successful or not.

Seems useful on pages with lists of articles and also as a way to provide a reading list/save for later functionality on low memory/power devices.

Comment 1

[Michael Kohler [:mkohler]]

Is this not more or less the same as a "Open in new background tab" feature?

Comment 2

[Michael Kohler [:mkohler]]

(In reply to Michael Kohler [:mkohler] from comment #1)
> Is this not more or less the same as a "Open in new background tab" feature?

Oh, we already have a "Reading list" we can use.. nothing at all like background tab :)

It seems like we already download the content and already inform the user about adding it when adding it from Reader Mode. We could probably use the same function.

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to Michael Kohler [:mkohler] from comment #2)
> (In reply to Michael Kohler [:mkohler] from comment #1)
> > Is this not more or less the same as a "Open in new background tab" feature?
> 
> Oh, we already have a "Reading list" we can use.. nothing at all like
> background tab :)
> 
> It seems like we already download the content and already inform the user
> about adding it when adding it from Reader Mode. We could probably use the
> same function.

It's slightly more complicated than that. We only enable reading list on pages that can actually be converted by reader mode. So, we'd have to run the readability check on the target URL before being able to tell if we can add the page to reading list or not.

The problem is that the readability check might take a few seconds depending on the size of the page. So, we'd need some sort of progress feedback while all these things are happening in background.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

We could take an alternative design: Always show the menu. When selected, try to add to reading list. If succeeded, tell the user "Page was added to Reading List". If we failed, tell the user "Unable to add page to Reading List".

I don't like needing to run the check each time the context menu is opened on a link.

What do we think?

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> We could take an alternative design: Always show the menu. When selected,
> try to add to reading list. If succeeded, tell the user "Page was added to
> Reading List". If we failed, tell the user "Unable to add page to Reading
> List".
> 
> I don't like needing to run the check each time the context menu is opened
> on a link.
> 
> What do we think?

That's actually what I meant :-) My point about the progress feedback is for when you tap on the menu option and wait until the readability check finishes.

Comment 6

[Wesley Johnston (:wesj)]

I'd be fine with a notification for that. "Adding <url>/<pages> to reading list" (with a number indicator if there's more than one?... I've been in the mood to extend nsIAlertsService for JB stuff lately).

We don't even necessarily need to load the page in a browser either. To save memory we could just xhr the url and try to parse that. If it works, yay! Otherwise we fail.

Comment 7

[Sriram [:sriramr]]

lucasr, this looks like an interesting bug to work on. Can you give me some info on where I can find the code for reader mode?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Sriram:

We initialize the context menu actions here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#326

You should be able to mimic the "Open Link in New Tab" code for most of the menu action. You'll want a different implementation of the action callback though.

You probably want to use Reader.parseDocumentFromURL, found here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7354

The callback passed to parseDocumentFromURL can be used to determine if reader mode was able to handle the link or not. You still need to add the article to the Reading List, which seems to be done with Reader.storeArticleInCache().

Lucas can fill in more details, but this might get you started.

Comment 9

[Sriram [:sriramr]]

Attachment: Proposed patch for "Add to Reading List" feature

This is just a proposed patch. I have used the existing Toast Messages to display success and failure. Please let me know if I have to use notifications. 

I know I have hard coded the string. Was too lazy to get them into browser.properties. I would take care of that in my next patch. :)

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 696612 [details] [diff] [review]
Proposed patch for "Add to Reading List" feature

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

Look generally good. Just needs to handle the error case properly.

::: mobile/android/chrome/content/browser.js
@@ +343,5 @@
> +      NativeWindow.contextmenus.linkOpenableContext,
> +      function(aTarget) {
> +        let url = NativeWindow.contextmenus._getLinkURL(aTarget);
> +        Reader.parseDocumentFromURL(url,
> +          function(article) {

nit: move the "function(article) {" to the same line as Reader.parseDocument...

@@ +346,5 @@
> +        Reader.parseDocumentFromURL(url,
> +          function(article) {
> +            if(article) {
> +              Reader.storeArticleInCache(article,
> +                function(result) {

nit: move the "function (resullt) {" to the same line as Reader.storeArticleInCache...

Also, rename "result" to "success" for clarity.

@@ +357,5 @@
> +                    }
> +                  });
> +                });
> +            } else {
> +              NativeWindow.toast.show("Cannot add article", "short");

You should just send a "Reader:Added" message with "success: false" here. A toast notification will be shown for you.

Comment 11

[Shriram (irc: Mavericks)]

Attachment: Patch that adds "Add to reading list" to link context menu

Contacted sriram and taking over this bug with his consent.
Builds on previous patch, and made changes as per last comment.

I'm not sure if this would suffice

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 725562 [details] [diff] [review]
Patch that adds "Add to reading list" to link context menu

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

Looks good overall but still needs work. Let me know if you have any questions.

::: mobile/android/chrome/content/browser.js
@@ +358,5 @@
>        });
>  
> +    NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.addToReadingList"),
> +      NativeWindow.contextmenus.linkOpenableContext,
> +      function(aTarget) {

You'll have to check if the article is already stored in cache before doing anything here. Otherwise the storeArticleInCache() will fail when when the article is already in the reading list. You can use Reader.getArticleFromCache() to do it.

We should replace the current 'success' boolean property (sent with the Reader:Added message) with a more fine-grained result id. See my comment on bug 784387 about this same case in another context:

https://bugzilla.mozilla.org/show_bug.cgi?id=784387#c21

@@ +361,5 @@
> +      NativeWindow.contextmenus.linkOpenableContext,
> +      function(aTarget) {
> +        let url = NativeWindow.contextmenus._getLinkURL(aTarget);
> +        Reader.parseDocumentFromURL(url,
> +        function(article) {

"function(article) {" should be on the same line than the parseDocumentFromURL method call.

@@ +364,5 @@
> +        Reader.parseDocumentFromURL(url,
> +        function(article) {
> +          if(article) {
> +            Reader.storeArticleInCache(article,
> +            function(success) {

Same here. Move "function(success) {" to the same line than the storeArticleInCache call.

Comment 13

[Lucas Rocha (:lucasr)]

Shriram, I'm going to take over this bug for now just make to sure it lands asap.

Comment 14

[Lucas Rocha (:lucasr)]

Attachment: Add "Add to Reading List" to link context menu

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Add "Add to Reading List" context menu

Comment 16

[Lucas Rocha (:lucasr)]

(In reply to Lucas Rocha (:lucasr) from comment #14)
> Created attachment 738078 [details] [diff] [review]
> Add "Add to Reading List" to link context menu

The summary is actually: "Change Reader:Add to handle tabID and URL as input". This builds a bit on top of Mark Capella's patch on bug 800899 btw.

Comment 17

[Brian Nicholson (:bnicholson)]

Comment on attachment 738078 [details] [diff] [review]
Add "Add to Reading List" to link context menu

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

::: mobile/android/chrome/content/browser.js
@@ +6518,5 @@
> +          url = uri.spec;
> +          urlWithoutRef = uri.specIgnoringRef;
> +        } else {
> +          // Nothing can be done here.
> +          return;

This shouldn't happen, so I'd log an error in case we have any bugs so it's easier to track down.

Comment 18

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #17)
> Comment on attachment 738078 [details] [diff] [review]
> Add "Add to Reading List" to link context menu
> 
> Review of attachment 738078 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +6518,5 @@
> > +          url = uri.spec;
> > +          urlWithoutRef = uri.specIgnoringRef;
> > +        } else {
> > +          // Nothing can be done here.
> > +          return;
> 
> This shouldn't happen, so I'd log an error in case we have any bugs so it's
> easier to track down.

Done.

Comment 19

[Lucas Rocha (:lucasr)]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3573bf02fda8
https://hg.mozilla.org/integration/mozilla-inbound/rev/478ec73ea931

Comment 20

[Nick Alexander :nalexander [he/him]]

Hey everybody, just wanted to thank Shriram for picking this up and doing the hard work to get it moving again, and to Lucas and Brian for landing it.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3573bf02fda8
https://hg.mozilla.org/mozilla-central/rev/478ec73ea931

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Note: Bug 872005 has removed this feature. Well, it removed the context menu, but left the rest of the changes.