1385418 - Remove disabled pocket code for the toolbar button now that it's been replaced by the item in the page action panel

Status: VERIFIED FIXED

(Firefox :: Pocket, defect, P1)

Description

[:Gijs (he/him)]

Splitting this off from bug 1367927 will hopefully help with iterating and landing things as soon as they're ready.

Comment 1

[:Gijs (he/him)]

Sorry, this bug should have had a summary update, so I belatedly did that now.

bug 1367927 ended up only creating the pocket button behind a pref. That pref is off by default, so users will currently not have a toolbar button anymore already. This bug exists to remove the code and fix up automated tests to deal with the removal, as the pref is flipped on automation. As such, I don't think it needs to be high-priority. Dolske, does that sound right to you? :-)

Comment 2

[Drew Willcoxon :adw]

Pocket integration on about:newtab for Activity Stream is broken; see bug 1388528.  We'll use this bug to fix that too.  The Pocket panel/doorhanger should open on the site identity icon in that case.

(In reply to Drew Willcoxon :adw from bug 1388528 comment #8)
> * The Save to Pocket context menu item is broken too:
> https://dxr.mozilla.org/mozilla-central/rev/
> a921bfb8a2cf3db4d9edebe9b35799a3f9d035da/browser/extensions/pocket/bootstrap.
> js#238-254
> 
> * The save/anchor logic is here:
> http://searchfox.org/mozilla-central/source/browser/extensions/pocket/
> content/Pocket.jsm#98

Comment 3

[Drew Willcoxon :adw]

... so this should probably be "high-priority" after all FWIW.

Comment 5

[Drew Willcoxon :adw]

Attachment: Bug 1385418 - Remove disabled pocket code for the toolbar button now that it's been replaced by the item in the page action panel.

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

Comment 6

[Drew Willcoxon :adw]

Work in progress.  This will probably end up fixing a couple of other bugs, including bug 1390048.

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 8897277 [details]
Bug 1385418 - Remove disabled pocket code for the toolbar button now that it's been replaced by the item in the page action panel.

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

Comment 9

[Drew Willcoxon :adw]

I think this is everything...  It fixes UITour's handling of Pocket too.

Comment 10

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22b87d98ab24

Comment 11

[:Gijs (he/him)]

Comment on attachment 8897277 [details]
Bug 1385418 - Remove disabled pocket code for the toolbar button now that it's been replaced by the item in the page action panel.

https://reviewboard.mozilla.org/r/168582/#review174386

This looks good to me besides the removal of the CustomizableUI listener, and would have been r+ but for the fact that the page action test is orange on try. It looks like it has something to do with the action command refactor that's part of this patch.

::: browser/base/content/theme-vars.inc.css:160
(Diff revision 2)
>  :root[lwthemeicons~="--forget-icon"] #panic-button:-moz-lwtheme,
> -:root[lwthemeicons~="--pocket-icon"] #pocket-button:-moz-lwtheme {
> -  -moz-image-region: rect(0, 16px, 16px, 0) !important;
> -}

Uh, you meant to keep the contents of the rule, and remove only the pocket selector, I'm sure. :-)

(This is causing the browser/base/content/test/static/browser_parsable_css.js orange on try, which is how I noticed - I missed it first time!)

::: browser/extensions/pocket/bootstrap.js
(Diff revision 2)
>      Services.ppmm.loadProcessScript(PROCESS_SCRIPT, true);
>      PocketReader.startup();
> -    if (PocketPageAction.shouldUse) {
> -      PocketPageAction.init();
> +    PocketPageAction.init();
> -    } else {
> -      CustomizableUI.addListener(this);

The listener is in use for onWindowOpened/onWindowClosed listeners still, I think, to add/remove the rest of the UI? We should probably keep that for now.

::: browser/extensions/pocket/bootstrap.js
(Diff revision 2)
>      AboutPocket.aboutSignup.unregister();
>  
> -    if (PocketPageAction.shouldUse) {
> -      PocketPageAction.shutdown();
> +    PocketPageAction.shutdown();
> -    } else {
> -      CustomizableUI.removeListener(this);

Which also means we should keep the removeListener here.

Comment 12

[Drew Willcoxon :adw]

Comment on attachment 8897277 [details]
Bug 1385418 - Remove disabled pocket code for the toolbar button now that it's been replaced by the item in the page action panel.

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

Comment 13

[Drew Willcoxon :adw]

(In reply to :Gijs from comment #11)
> and would have been r+ but for the fact that the page action test is orange
> on try. It looks like it has something to do with the action command
> refactor that's part of this patch.

It's because I renamed tempPanel to activatedActionPanel I think.

> ::: browser/base/content/theme-vars.inc.css:160
> (Diff revision 2)
> >  :root[lwthemeicons~="--forget-icon"] #panic-button:-moz-lwtheme,
> > -:root[lwthemeicons~="--pocket-icon"] #pocket-button:-moz-lwtheme {
> > -  -moz-image-region: rect(0, 16px, 16px, 0) !important;
> > -}
> 
> Uh, you meant to keep the contents of the rule, and remove only the pocket
> selector, I'm sure. :-)

Yikes, thanks.

browser_startup_images.js was also failing because the Pocket svg was loaded but not shown, so I fixed that too.  It's the same problem that the bookmark star svg had when I made that a page action.

> ::: browser/extensions/pocket/bootstrap.js
> (Diff revision 2)
> >      Services.ppmm.loadProcessScript(PROCESS_SCRIPT, true);
> >      PocketReader.startup();
> > -    if (PocketPageAction.shouldUse) {
> > -      PocketPageAction.init();
> > +    PocketPageAction.init();
> > -    } else {
> > -      CustomizableUI.addListener(this);
> 
> The listener is in use for onWindowOpened/onWindowClosed listeners still, I
> think, to add/remove the rest of the UI? We should probably keep that for
> now.

PocketOverlay.onWindowOpened is now called by the page action's onPlacedInPanel, so that part of the CUI listener is covered.

There's no PocketOverlay.onWindowClosed, but there is a PocketOverlay.onWidgetAfterDOMChange, which your comment made me take look at.  All it does is show/hide non-primary Pocket UI depending on whether Pocket is "enabled" according to isPocketEnabled.  In addition to checking the pref, isPocketEnabled returns false if Pocket doesn't have a CUI placement.  In the new page action world, pref aside, Pocket is always "enabled/placed," right?  So it's not necessary anymore to show/hide non-primary Pocket UI except on window open.  And that's covered by PocketOverlay.onWindowOpened.

So I removed the onWidgetAfterDOMChange callback and the updateWindowAfterWidgetPlaced method that it delegated to.  The only thing in updateWindowAfterWidgetPlaced that still needs to happen is showing/hiding the reader-view button.

Comment 14

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1be82a6975a8

Comment 15

[:Gijs (he/him)]

Comment on attachment 8897277 [details]
Bug 1385418 - Remove disabled pocket code for the toolbar button now that it's been replaced by the item in the page action panel.

https://reviewboard.mozilla.org/r/168582/#review174614

Nice! Sorry for missing the onWindowOpened thing. And yes, removing the other callbacks was the right thing.

Comment 16

[Drew Willcoxon :adw]

Try looks good so far, landing.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/397fe94c67f1
Remove disabled pocket code for the toolbar button now that it's been replaced by the item in the page action panel. r=Gijs

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/397fe94c67f1

Comment 20

[u554753]

This seems back-end, but asking as my name is attached to this bug: Any steps needed from me to verify?

Comment 21

[:Gijs (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #20)
> This seems back-end, but asking as my name is attached to this bug: Any
> steps needed from me to verify?

Probably good to check that pocket items work correctly, both in the page action menu, the library, the bookmarks menu and the page context menu, both with pocket in the URL bar and when it's been removed (you can use the context menu on the item in the url bar for that).

Comment 22

[Robert Wood [:rwood]]

Hi :adw, I am seeing a talos performance regression in canvasmark, that *looks* like it might be coming from this patch, however I am not certain. Do you think your patch could have caused the perf regression below? Seems odd though when your patch is removing disabled code - that's why I am hesitant to file a perf regression bug against this patch. Please verify, thanks!

== Change summary for alert #8841 (as of August 16 2017 19:12 UTC) ==

Regressions:

  3%  tcanvasmark summary windows7-32 pgo e10s     9,754.57 -> 9,466.46

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8841

https://wiki.mozilla.org/Buildbot/Talos/Tests#CanvasMark

Comment 23

[Drew Willcoxon :adw]

It seems impossible for this bug to have regressed *canvas* performance.  This is purely a front-end bug and there's no way it could have impacted any part of the core web platform, like canvas.

Comment 24

[Abe - QA (:Abe_LV)]

I have tested this based on the suggestion in comment 21 and it is verified as fixed.