1394459 - Add support for Pocket context menu items

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement, P1)

Description

[Chenxia Liu [:liuche]]

Bug 1380808 added Pocket to new tab, but context menu items don't work (Bookmarking, Remove, etc).

From comment [1], we want the following:

Bookmark
---
Open in new tab
Open in private tab
---
Remove
---
Share
Copy address

The "Remove" item will be handled by bug 1393178.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1380808#c6

Comment 1

[Chenxia Liu [:liuche]]

Picking this up bc all other bugs in the sprint are assigned/blocked.

Comment 2

[Chenxia Liu [:liuche]]

Actually, to keep things simple, I'm just going to keep all the normal context-menu items (except Remove) so that all the items will have "pin", "add to homescreen". So this will make bookmarking and pinning work for Top Stories, and also hide the "Remove" option.

Comment 3

[Chenxia Liu [:liuche]]

Attachment: Bug 1394459 - Support bookmarking, pinning of Pocket items in context menu.

***

Review commit: https://reviewboard.mozilla.org/r/174052/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174052/

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8902470 [details]
Bug 1394459 - Support bookmarking, pinning of Pocket items in context menu.

https://reviewboard.mozilla.org/r/174052/#review179684

A little hacky but simple so it seems fine to me.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/model/TopStory.java:81
(Diff revision 1)
> +
> +    @Override
> +    public void onStateCommitted() {
> +        // Since Top Stories are not loaded from a cursor that automatically notifies of
> +        // changes to bookmark or pin state, trash this cached state once we've committed it
> +        // so it will be fetched every time.

nit: I didn't understand this comment until I found the code in `postInit` that refreshes isPinned when `isPinned == null`. Can we make the comment stand on its own, or reference postInit (to avoid duplicating information)? (maybe it's not possible)

I also only just noticed the docs in `WebpageModel.isPinned` that explains when these values are null - perhaps you should say to reference those too?

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8902470 [details]
Bug 1394459 - Support bookmarking, pinning of Pocket items in context menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/174052/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abe144dae14e
Support bookmarking, pinning of Pocket items in context menu. r=mcomella

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/abe144dae14e

Comment 8

[Bogdan Surd, QA [:bsurd, NI]]

Devices:
 - HTC 10 (Android 7.0);
 - Nexus 5 (Android 6.0.1);

Verified on the latest nightly. All menu items properly work for all 3 currently displayed pocket items. Remove item is still not available and will verify this once it is implemented in bug 1393178.