Status

()

For bugs in Firefox Desktop, the Mozilla Foundation's web browser. For Firefox user interface issues in menus, developer tools, bookmarks, location bar, and preferences. Many Firefox bugs will either be filed here or in the Core product. (more info)

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.

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.

(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.

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.