Closed Bug 1134927 Opened 5 years ago Closed 5 years ago

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

Categories

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

35 Branch
All
Android
defect
Not set

Tracking

(firefox38 verified, firefox39 verified, fennec38+)

VERIFIED FIXED
Firefox 39
Tracking Status
firefox38 --- verified
firefox39 --- verified
fennec 38+ ---

People

(Reporter: Margaret, Assigned: dropkick, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 5 obsolete files)

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.
Hello,

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

Thanks,
Apoorv
Attached patch bug1134927.patch (obsolete) — Splinter Review
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.
Attachment #8567659 - Flags: review?(margaret.leibovic)
I was able to finally get everything working right and have verified the patch.
Attached patch real bug1134927.patch (obsolete) — Splinter Review
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.
Attachment #8567659 - Attachment is obsolete: true
Attachment #8567659 - Flags: review?(margaret.leibovic)
Attachment #8567667 - Flags: review?(margaret.leibovic)
Assignee: nobody → tylertstonge
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 :)
Attachment #8567667 - Flags: review?(margaret.leibovic) → feedback+
Attached patch Bug1134927.patch (obsolete) — Splinter Review
Sorry about that! I think this is the right format?
Attachment #8567667 - Attachment is obsolete: true
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
Attached patch Bug1134927.patch (obsolete) — Splinter Review
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.
Attachment #8568237 - Attachment is obsolete: true
Flags: needinfo?(margaret.leibovic)
Attached patch Bug1134927.patch (obsolete) — Splinter Review
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?
Attachment #8568643 - Flags: review?(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.
Attachment #8568643 - Flags: review?(margaret.leibovic) → feedback+
Attached patch Bug1134927.patchSplinter Review
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!
Attachment #8568357 - Attachment is obsolete: true
Attachment #8568643 - Attachment is obsolete: true
Attachment #8568929 - Flags: review?(margaret.leibovic)
Comment on attachment 8568929 [details] [diff] [review]
Bug1134927.patch

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

Excellent, thanks!
Attachment #8568929 - Flags: review?(margaret.leibovic) → review+
tracking-fennec: --- → ?
Flags: needinfo?(margaret.leibovic)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/331a66399399
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
tracking-fennec: ? → 38+
https://hg.mozilla.org/mozilla-central/rev/331a66399399
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 39
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)
Status: RESOLVED → VERIFIED
Depends on: 1138188
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)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.