Screenshots is switching from Photon pageAction to WebExtension pageAction. There are some perf related discussion at https://github.com/mozilla-services/screenshots/pull/3967.

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7fa9470f6651976130222e7e112a95e1ff9ae2a Talos: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=f7fa9470f6651976130222e7e112a95e1ff9ae2a

Hey Barry, could you attach the patch to this bug and r? me? Since this has been sitting for a while, would probably be a good idea to re-push to try and re-run the Talos tests against the current baseline. Once that's done, we can ping desktop peers for feedback on the perf regressions.

_6a68: I will also cherry-pick https://github.com/mozilla-services/screenshots/pull/4594 into the patch. Let me know if I shouldn't. Oh sorry, I thought I already pushed to mozreview previously on this bug. :-/

Build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a249a349e6821fca5edc0f87e924ebecd23a34df Talos: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=a249a349e6821fca5edc0f87e924ebecd23a34df

Latest try build and talos: - https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c6a630e0157b5793584cfb1960bb4cf2ecc5b2e - https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=9c6a630e0157b5793584cfb1960bb4cf2ecc5b2e

Are the Talos regressions in comment 7 ok for this patch to land? Note that we talked about this same patch a while ago in https://github.com/mozilla-services/screenshots/pull/3967, but the Screenshots team thought it'd be better to land this separately from our periodic github exports, in case it gets bounced due to perf regressions.

I'm kind of surprised by the Linux tp5o measurements given that we have OOP extensions enabled on Linux now. But, given that it's Linux-only, and that the regressions are small, I don't think they should block this landing. The tps numbers are a bit more worrying, but given that it's only a difference of about a half a millisecond, it also probably shouldn't block this landing, but we probably should do some profiling after it lands to see what we can win back.

Cool, merging then. Thanks for the review

Comment on attachment 8990760 [details] Bug 1466575 - Replace Photon pageAction with WebExtension pageAction in Screenshots; https://reviewboard.mozilla.org/r/255818/#review268086

Pushed by jhirsch@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/412b5e9b8525 Replace Photon pageAction with WebExtension pageAction in Screenshots; r=_6a68

Backed out for bc failures on browser_screenshots_ui_check.js and browser_UITour_availableTargets.js. Backout link: https://hg.mozilla.org/integration/autoland/rev/3873025d2fa85ea891919586ebced0873c8997ce Push link: https://hg.mozilla.org/integration/autoland/rev/412b5e9b8525c717eb8941ab67ac5525b2ac62bb Log link for browser_screenshots_ui_check.js: https://treeherder.mozilla.org/logviewer.html#?job_id=191652915&repo=autoland&lineNumber=3012 Log link for browser_UITour_availableTargets.js: https://treeherder.mozilla.org/logviewer.html#?job_id=191658761&repo=autoland&lineNumber=3510

The button id's changed now that it's a WebExtension pageAction. That's causing those test failures. I don't know who own(?) those tests, but I could update them.

_6a68: Added a commit to update the Screenshots pageAction id in tests. r?

Let's see if this fixes the broken test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aee290f9cbb465bed6c58fb48f0038e60dfd2d5

Looks like there are some test failures in the try run. Mind taking a look?

Updated the test updating commit. Tests are green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67e561ed65b416bd49303c7e83bf01ce7bcdd189

Comment on attachment 8997463 [details] Bug 1466575 - Update tests to use new Screenshots pageAction id; https://reviewboard.mozilla.org/r/261216/#review269298

LGTM, attempting a second landing

Pushed by jhirsch@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ea0593b7eb05 Replace Photon pageAction with WebExtension pageAction in Screenshots; r=_6a68 https://hg.mozilla.org/integration/autoland/rev/b17bf2e7a761 Update tests to use new Screenshots pageAction id; r=_6a68

https://hg.mozilla.org/mozilla-central/rev/ea0593b7eb05 https://hg.mozilla.org/mozilla-central/rev/b17bf2e7a761

(In reply to Barry Chen from bug 1412377 comment #10) > Bug 1466575 has landed. Now the Screenshots button is in the alpha-sorted > list of WebExtension page actions in the "..." panel (and appears in the > address bar by default). > > Is there a way to place the Screenshots button back among the built-in > actions? > > :adw or :ntim? There are a couple of things here: PageActions.jsm still recognizes "screenshots" as a built-in action: https://dxr.mozilla.org/mozilla-central/source/browser/modules/PageActions.jsm#1049 But that _isBuiltIn getter is only used in one place, when logging telemetry: https://dxr.mozilla.org/mozilla-central/source/browser/modules/PageActions.jsm#333 Did 1466575 break that? We should either fix it or remove screenshots from the array in _isBuiltIn. Barry, could you please file a bug for that? The way that screenshots got added to the built-in section of the menu was by using the private _insertBeforeActionID property: https://dxr.mozilla.org/mozilla-central/source/browser/modules/PageActions.jsm#562 WebExtension page actions don't have access to that property. You would need to coordinate with the WebExtensions people to specifically allow screenshots to use it, or something.

I filed two follow-up bugs: - Bug 1483085 to update Screenshots' action id in pageActions.jsm - Bug 1483088 to allow system addons to set `_insertBeforeActionID` Thanks, :adw!

(In reply to Drew Willcoxon :adw from comment #29) > But that _isBuiltIn getter is only used in one place, when logging > telemetry: > https://dxr.mozilla.org/mozilla-central/source/browser/modules/PageActions. > jsm#333 For the record there's one other place _isBuiltIn is used, when setting the state of the context menu: https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/browser/base/content/browser-pageActions.js#839

There's no rush to make this change in 63, and since there are a number of open related bugs, I'm going to revert this until 63 branches, then re-land it for 64. Might take a little bit, as I'm doing the phabricator transition now, but wanted to give you a heads-up.

Screenshots moved from a Photon page action to a pure WebExtension page action, but this move introduced some UI bugs where the two page actions behave differently. Since this change isn't needed for 63, seems better to just temporarily revert.

(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #32) > There's no rush to make this change in 63, and since there are a number of > open related bugs, I'm going to revert this until 63 branches, then re-land > it for 64. > > Might take a little bit, as I'm doing the phabricator transition now, but > wanted to give you a heads-up. ok cool

Comment on attachment 9003630 [details] Temporarily revert Screenshots back to Photon page action until 63 branches (Bug 1466575). Ian Bicking (:ianbicking) has approved the revision.

Try run for the revert: https://treeherder.mozilla.org/#/jobs?repo=try&author=jhirsch@mozilla.com&selectedJob=195639081

Sorry for the spam. The link in comment 36 looks weird. This one seems correct: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7f834e5bad801f02392a94958150c916c7f0e7a

Pushed by jhirsch@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/566486a3aa57 Temporarily revert Screenshots back to Photon page action until 63 branches . r=ianbicking

Re-applied the patch to move the Screenshots page action from Photon to WebExtension. Reopening until this new patch lands. Try run with Talos x5: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccf9b5d9c5cd7fac5eaacc322aae02c87202078e

OK, the Talos regressions seem to be the same as what we decided was an acceptable performance hit back in comment 9. https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=ccf9b5d9c5cd7fac5eaacc322aae02c87202078e&framework=1&showOnlyImportant=1&selectedTimeRange=172800

This patch was temporarily reverted to avoid bug 1483033 for Firefox 63. MozReview-Commit-ID: 4VaQgZQCVlE

Comment on attachment 9008255 [details] Bug 1466575 - Re-enable Screenshots WebExtension page action for Firefox 64; r?ianbicking Ian Bicking (:ianbicking) has approved the revision.

Pushed by jhirsch@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1d6879ee635e Re-enable Screenshots WebExtension page action for Firefox 64; r=ianbicking

Pushed by dluca@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c93a8b8b0c66 Re-enable Screenshots WebExtension page action for Firefox 64; r=ianbicking

Closing as fixed, now that the patch has re-landed.