Closed Bug 1367927 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Address Bar, enhancement, P1)

Product:
Firefox
Component:
Address Bar
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Iteration:
57.1 - Aug 15
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: adw, Assigned: adw)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-structure])

Attachments

(1 file)

Bug 1367927 - Add a "save to pocket" item to the page action menu.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
+++ 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+
Aaron says that unlike the bookmark button, the Pocket button shouldn't change depending on whether the current page is pocketed.
QA Contact: gwimberly
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)
And when you are signed in, you also get a panel, with Remove Page and View List links and a textbox for tags.
Or a subview if the button's in the hamburger panel.
Iteration: 55.6 - May 29 → 55.7 - Jun 12
Aaron says to do what we're currently doing.
Flags: needinfo?(bbell)
Flags: needinfo?(abenson)
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
Depends on: 1374477
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
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.
(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.
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.
Blocks: 1385418
(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.
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 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+
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.
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)
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.
Bug 1385418 I mean, the one to remove CUI integration.
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)
Ah yeah.
This also fixes the eslint errors that appeared on try.
(That's the ownerDocument.defaultView -> ownerGlobal changes.)
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+
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
Depends on: 1386474
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
https://hg.mozilla.org/mozilla-central/rev/f0325a1f47ee
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Blocks: 1387512
Depends on: 1387697
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]
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]
As per Comment 29 and Comment 30, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
Depends on: 1388170
No longer blocks: 1385418
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: