Add a "save to pocket" item to the page action menu

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Location Bar
P1
normal
VERIFIED FIXED
3 months ago
3 days ago

People

(Reporter: adw, Assigned: adw)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
+++ This bug was initially created as a clone of Bug #1363183 +++

The page action menu should have a 'save to pocket' item at the top.
Flags: qe-verify+
(Assignee)

Comment 1

3 months ago
Aaron says that unlike the bookmark button, the Pocket button shouldn't change depending on whether the current page is pocketed.

Updated

3 months ago
QA Contact: gwimberly
(Assignee)

Comment 2

3 months ago
Bryan, Aaron, what's supposed to happen when you click Save to Pocket?  For reference, when the current Pocket button is in the toolbar, a panel opens that's anchored to the button.  When it's in the hamburger panel, a subview slides in.  At least when you're not signed in.
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
(Assignee)

Comment 3

3 months ago
And when you are signed in, you also get a panel, with Remove Page and View List links and a textbox for tags.
(Assignee)

Comment 4

3 months ago
Or a subview if the button's in the hamburger panel.

Updated

3 months ago
Iteration: 55.6 - May 29 → 55.7 - Jun 12
(Assignee)

Comment 5

3 months ago
Aaron says to do what we're currently doing.
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)

Updated

2 months ago
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
(Assignee)

Updated

2 months ago
Depends on: 1374477

Updated

2 months ago
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10

Updated

a month ago
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24

Updated

24 days ago
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
Comment hidden (mozreview-request)
(Assignee)

Comment 7

22 days ago
WIP patch built on top of bug 1374477.  Adding the page action is the easy part; the harder part is removing Pocket's CustomizableUI access point (the toolbar button), since it's kind of spread through multiple places in the tree.  This patch still needs to do that.

Comment 8

20 days ago
(In reply to Drew Willcoxon :adw from comment #7)
> WIP patch built on top of bug 1374477.  Adding the page action is the easy
> part; the harder part is removing Pocket's CustomizableUI access point (the
> toolbar button), since it's kind of spread through multiple places in the
> tree.  This patch still needs to do that.

I think we should move removing pocket into a separate bug. I'll file one shortly.
(Assignee)

Comment 9

20 days ago
OK, fair enough.  FWIW my page-action-only patch just tries to do the bare minimum to add the page action, so it co-opts and slightly modifies function(s) that were written with CustomizableUI in mind.  Removing the CustomizableUI parts may involve rewriting large parts of the extension's integration with the UI, so the bug for that may also end up rewriting/redoing a large part of the fix for this bug too, and that's why I planned to do them together in one bug.

But -- I'm OK with splitting them up.  It's probably a good idea from a project management perspective.

Updated

20 days ago
Blocks: 1385418

Comment 10

20 days ago
(In reply to Drew Willcoxon :adw from comment #9)
> OK, fair enough.  FWIW my page-action-only patch just tries to do the bare
> minimum to add the page action, so it co-opts and slightly modifies
> function(s) that were written with CustomizableUI in mind.  Removing the
> CustomizableUI parts may involve rewriting large parts of the extension's
> integration with the UI, so the bug for that may also end up
> rewriting/redoing a large part of the fix for this bug too, and that's why I
> planned to do them together in one bug.
> 
> But -- I'm OK with splitting them up.  It's probably a good idea from a
> project management perspective.

Yeah, let's do the CUI-related rewriting in bug 1385418 then, so we get this landed + tested ASAP.
Comment hidden (mozreview-request)
(Assignee)

Comment 12

20 days ago
Shane, would you mind reviewing?  This adds a Photon page action for Pocket.  If you'd like to try it out, you'll need to apply all the mozreview commits in bug 1374477 before applying this one.  Or if you'd like, you could wait until that bug lands, which hopefully will be soon.

This will make Pocket appear in the Photon page action panel, which is the panel behind the "..." button in the urlbar when you're running a build with Photon enabled.  You can context-click the menu item to add it to the urlbar too.

The reason for the svg change -- I added the fill-opacity attribute -- is so that when Pocket is in the urlbar, its opacity is set to match the other page action icons.

One important thing is that this doesn't remove Pocket from CustomizableUI.  As Gijs says in comment 10, we filed a separate bug for that.  This patch actually breaks Pocket's CUI integration because of photonPageActionPanelFrame in main.js, which makes a few functions return early.  Maybe it would be possible to detect which panel Pocket is being viewed in so that those early returns are appropriately skipped.  But given that we want to remove the CUI integration, I'm not sure that's worth it.

Comment 13

20 days ago
mozreview-review
Comment on attachment 8890635 [details]
Bug 1367927 - Add a "save to pocket" item to the page action menu.

https://reviewboard.mozilla.org/r/161788/#review168088

This all looks reasonable to me, though I haven't dug through the new PageActions.jsm.  Given there is a followup to remove CUI (I assume very soon) I'm not concerned about the breakage on that, though I dropped a couple comments on that below.

::: browser/extensions/pocket/bootstrap.js:344
(Diff revision 2)
>      this._cachedSheet = styleSheetService.preloadSheet(gPocketStyleURI,
>                                                         this._sheetType);
>      Services.ppmm.loadProcessScript(PROCESS_SCRIPT, true);
>      PocketReader.startup();
>      CustomizableUI.addListener(this);
>      CreatePocketWidget(reason);

To avoid interim breakage it seems we could just remove these two lines, and the related shutdowns.
Attachment #8890635 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 14

19 days ago
Thanks Shane.

One other thing this needs to do is to add Pocket to the urlbar by default, according to bug 1363188.  That's a simple one-line change, setting `shownInUrlbar: true` on the action.
(Assignee)

Comment 15

17 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c26a80ac16f
(Assignee)

Comment 16

17 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27cea2b5f4cf
Comment hidden (mozreview-request)
(Assignee)

Comment 18

17 days ago
Comment on attachment 8890635 [details]
Bug 1367927 - Add a "save to pocket" item to the page action menu.

Shane, I made some non-trivial changes since you r+'ed.  Would you mind looking at this new revision?

* Removing the CUI widget's creation isn't possible without basically fixing bug 1385418, which we don't want to do here, because of tests throughout the tree that assume it exists.  So instead, I added branches for handling the two scenarios: one where the page action is used, one where the CUI widget is used.  Normally the page action is used, but to satisfy the tests, I added a pref, extensions.pocket.disablePageAction, that is set in the test environment and that falls back to the CUI widget path.

* Encapsulate all the page action stuff in a new PocketPageAction object with init, shutdown, etc. methods/properties, modeled on PocketContextMenu and the other similar objects.

* Do other things on shutdown that need doing: remove styles, remove pktUI et al. from windows.

* I wasn't handling clicks on the Pocket button in the reader view at all, so this fixes that.
Attachment #8890635 - Flags: review+ → review?(mixedpuppy)
(Assignee)

Comment 19

17 days ago
This approach is nice because now bug 1374477 is just an implementation detail.  I was thinking that Pocket would stick around in CUI until bug 1374477 is fixed, but this is much better.
(Assignee)

Comment 20

17 days ago
Bug 1385418 I mean, the one to remove CUI integration.

Comment 21

17 days ago
mozreview-review
Comment on attachment 8890635 [details]
Bug 1367927 - Add a "save to pocket" item to the page action menu.

https://reviewboard.mozilla.org/r/161788/#review168840

I like this much better, but I think the shutdown might have an issue of not removing some elements added to various menus.

::: browser/extensions/pocket/bootstrap.js:199
(Diff revision 3)
> +
> +  shutdown() {
> +    if (!this.pageAction) {
> +      return;
> +    }
> +    for (let window of browserWindows()) {

With my other comment, this is probably unecessary here.

::: browser/extensions/pocket/bootstrap.js:410
(Diff revision 3)
>  
> +    if (PocketPageAction.shouldUse) {
> +      PocketPageAction.shutdown();
> +    } else {
> -    CustomizableUI.removeListener(this);
> +      CustomizableUI.removeListener(this);
> -    for (let window of CustomizableUI.windows) {
> +      for (let window of CustomizableUI.windows) {

I think this section that removes elements needs to happen in either use case, it is removing elements added in updateWindow.
Attachment #8890635 - Flags: review?(mixedpuppy)
(Assignee)

Comment 22

17 days ago
Ah yeah.
Comment hidden (mozreview-request)
(Assignee)

Comment 24

17 days ago
This also fixes the eslint errors that appeared on try.
(Assignee)

Comment 25

17 days ago
(That's the ownerDocument.defaultView -> ownerGlobal changes.)

Comment 26

16 days ago
mozreview-review
Comment on attachment 8890635 [details]
Bug 1367927 - Add a "save to pocket" item to the page action menu.

https://reviewboard.mozilla.org/r/161788/#review168908

lgtm
Attachment #8890635 - Flags: review?(mixedpuppy) → review+

Comment 27

16 days ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f0325a1f47ee
Add a "save to pocket" item to the page action menu. r=mixedpuppy
(Assignee)

Updated

16 days ago
Depends on: 1386474

Updated

16 days ago
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15

Comment 28

15 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0325a1f47ee
Status: ASSIGNED → RESOLVED
Last Resolved: 15 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1387077
Blocks: 1387512

Updated

13 days ago
Depends on: 1387697

Comment 29

13 days ago
I can see this feature implemented on latest nightly 57.0a1  in Ubuntu 16.04, 64 bit 

Build ID 	20170805100334

User Agent      Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170802]

Comment 30

13 days ago
I can see this feature implemented on latest nightly 57.0a1 (2017-08-05) (32-bit) in windows 10 (32bit)

Build ID : 20170805100334
Mozilla/5.0 (Windows NT 10.0; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170802]

Comment 31

12 days ago
As per Comment 29 and Comment 30, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified

Updated

12 days ago
Flags: qe-verify+
(Assignee)

Updated

9 days ago
Depends on: 1388170
(Assignee)

Updated

3 days ago
No longer blocks: 1385418
You need to log in before you can comment on or make changes to this bug.