1134927 - Add back "Add to Reading List" link context menu item

Status: VERIFIED FIXED

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

Description

[:Margaret Leibovic]

This feature was originally added in bug 814587, then removed in bug 872005. However, now that we support a reading list on all devices, we can add it back.

To fix this bug, I would just try backing out the patch that landed in bug 872005:
https://hg.mozilla.org/mozilla-central/rev/ee75d69fe879

Luckily we failed to remove the string originally, so we don't need to worry about adding a new string here.

Comment 1

[Apoorv]

Hello,

I wish to work on this bug. How do I proceed?

Thanks,
Apoorv

Comment 2

[Tyler St. Onge[:dropkick]]

Attachment: bug1134927.patch

How does this patch look? I've been messing with my build environment and I'm having trouble getting any changes to carry over into the new build.

Comment 3

[Tyler St. Onge[:dropkick]]

I was able to finally get everything working right and have verified the patch.

Comment 4

[Tyler St. Onge[:dropkick]]

Attachment: real bug1134927.patch

Sorry, Im making a real mess of this thread. I attatched the wrong patch file to begin with. But now I believe everything should be correct.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8567667 [details] [diff] [review]
real bug1134927.patch

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

This is looking good! However, it looks like your patch isn't quite in the format we expect. We like to use patches generated in the git style, you can follow directions here for more information about that:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

If you upload a new version that's generated that way, I can test it out an I'll give you an r+ if everything seems to be in working order :)

Comment 6

[Tyler St. Onge[:dropkick]]

Attachment: Bug1134927.patch

Sorry about that! I think this is the right format?

Comment 7

[:Margaret Leibovic]

Comment on attachment 8568237 [details] [diff] [review]
Bug1134927.patch

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

Patch format is looking good, but this doesn't appear to work locally. Did you test it?

Looking into the code, I suspect that the "Reader:Add" message just needs to be updated to "Reader:AddToList".

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ReadingListHelper.java#67

Comment 8

[Tyler St. Onge[:dropkick]]

Attachment: Bug1134927.patch

I tried what you said, I was running it locally but I was mostly just looking to see if it showed up in the context menu, I didnt realize all the functionality was already implemented.

The only problem is that it still isnt getting the message to the Java even with the right message. I've been reading the Reader.js because I know that is able to successfully send the message, but I'm still pretty confused.

Comment 9

[Tyler St. Onge[:dropkick]]

Attachment: Bug1134927.patch

Well, I've been messing with different things and arrived at this method which works, but I'm not sure if this is an intended use of the Reader.js class. It seems better to access the method there than to copy the logic and have it in two places?

Comment 10

[:Margaret Leibovic]

Comment on attachment 8568643 [details] [diff] [review]
Bug1134927.patch

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

On the right track, nice work looking into the different APIs! However, I think this can be simpler.

::: mobile/android/chrome/content/browser.js
@@ +634,5 @@
> +      function(aTarget) {
> +        let url = NativeWindow.contextmenus._getLinkURL(aTarget);
> +        ReaderMode.downloadAndParseDocument(url).then(function(article) {
> +            Reader.receiveMessage({ name: "Reader:AddToList", data: { article: article } });
> +        });

Whenever an item is added to the reading list, our reading list content provider will send a message to Java to automatically download the content in the background, so you don't need to download it here. However, if you were to do that, you would need to account for the case where we don't find an article for this URL, since we'd still want to add the URL to the reading list. I'm sorry that this automatic downloading isn't better documented, it's not obvious that we do that.

You're correct that this use of the Reader API isn't really an intended use, but you should be able to just directly call Messaging.sendRequestForResult to send a message to Java directly. I'm sorry this is so confusing, we have two "Reader:AddToList" messsages, one of which is a content->parent process message (used to send a message from AboutReader.jsm to Reader.js), and the other is a JS->Java message (used to send a message from Reader.js to ReadingListHelper.java.

I believe all you should need to do here is something like:

let url = NativeWindow.contextmenus._getLinkURL(aTarget);
Messaging.sendRequestForResult({
  type: "Reader:AddToList",
  url: truncate(url, MAX_URI_LENGTH),
}).catch(Cu.reportError);

To be more complete, we could also try passing the link text as a title, but when we download the content in the background, we will be updating the title anyway.

Comment 11

[Tyler St. Onge[:dropkick]]

Attachment: Bug1134927.patch

That is a lot more straightforward. I did learn a lot from poking around at any file that may be related in any way to the reading list, so it wasn't for nothing.

This patch should be good now, I've tested it and everything seems to be running fine. Thanks for all the guidance!

Comment 12

[:Margaret Leibovic]

Comment on attachment 8568929 [details] [diff] [review]
Bug1134927.patch

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

Excellent, thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/331a66399399

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/331a66399399

Comment 15

[Teodora Vermesan (:TeoVermesan)]

Go to news.google.com, long tap on a link => a context menu will be displayed with the "Add to Reading List" option. Choosing that option, a notification will be displayed: "Page added to your reading list". Repeating the steps mentioned, a notification will be displayed, saying that the page is already added to the reading list.
Verified as fixed on;
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 39.0a1 (2015-03-01)

Comment 16

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4d847c044609

Comment 17

[Teodora Vermesan (:TeoVermesan)]

Choosing "Add to Reading List" option from context menu for a link will trigger a notification: "Page added to your reading list". Repeating the action mentioned, a new notification will be displayed, saying that the page is already added to the reading list.
Verified as fixed on;
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 38.0a2 (2015-03-06)