Barry Chen Reporter
	Description • 6 years ago

Screenshots is switching from Photon pageAction to WebExtension pageAction.

There are some perf related discussion at https://github.com/mozilla-services/screenshots/pull/3967.

	Barry Chen Reporter
	Comment 1 • 6 years ago

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7fa9470f6651976130222e7e112a95e1ff9ae2a

Talos: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=f7fa9470f6651976130222e7e112a95e1ff9ae2a

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 2 • 6 years ago

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.

	Barry Chen Reporter
	Comment 3 • 6 years ago

_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. :-/

	Barry Chen Reporter
	Comment 5 • 6 years ago

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

	Barry Chen Reporter
	Comment 7 • 6 years ago

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

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 8 • 6 years ago

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.

	Kris Maglione [:kmag]
	Comment 9 • 6 years ago

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.

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 10 • 6 years ago

Cool, merging then. Thanks for the review

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 11 • 6 years ago
mozreview-review

Comment on attachment 8990760 [details]
Bug 1466575 - Replace Photon pageAction with WebExtension pageAction in Screenshots;

https://reviewboard.mozilla.org/r/255818/#review268086

	Pulsebot
	Comment 12 • 6 years ago

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

	Narcis Beleuzu [:NarcisB]
	Comment 13 • 6 years ago

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

	Barry Chen Reporter
	Comment 14 • 6 years ago

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.

	Barry Chen Reporter
	Comment 18 • 6 years ago

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

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 19 • 6 years ago

Let's see if this fixes the broken test:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aee290f9cbb465bed6c58fb48f0038e60dfd2d5

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 20 • 6 years ago

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

	Barry Chen Reporter
	Comment 23 • 6 years ago

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

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 24 • 6 years ago
mozreview-review

Comment on attachment 8997463 [details]
Bug 1466575 - Update tests to use new Screenshots pageAction id;

https://reviewboard.mozilla.org/r/261216/#review269298

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 25 • 6 years ago

LGTM, attempting a second landing

	Pulsebot
	Comment 26 • 6 years ago

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

	Andreea Pavel [:apavel]
	Comment 27 • 6 years ago
bugherder

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

	Drew Willcoxon :adw
	Comment 29 • 6 years ago

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

	Barry Chen Reporter
	Comment 30 • 6 years ago

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!

	Drew Willcoxon :adw
	Comment 31 • 6 years ago

(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

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 32 • 6 years ago

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.

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 33 • 6 years ago

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.

	:Felipe Gomes (needinfo for replies!)
	Comment 34 • 6 years ago

(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

	Ian Bicking (:ianbicking)
	Comment 35 • 6 years ago

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.

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 36 • 6 years ago

Try run for the revert:

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

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 37 • 6 years ago

Sorry for the spam. The link in comment 36 looks weird. This one seems correct:

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

	Pulsebot
	Comment 38 • 6 years ago

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

	Tiberius Oros[:tiberius_oros]
	Comment 39 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/566486a3aa57

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 40 • 6 years ago

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

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 41 • 6 years ago

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

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 42 • 6 years ago

This patch was temporarily reverted to avoid bug 1483033 for Firefox 63.

MozReview-Commit-ID: 4VaQgZQCVlE

	Ian Bicking (:ianbicking)
	Comment 43 • 6 years ago

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.

	Pulsebot
	Comment 44 • 6 years ago

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

	Pulsebot
	Comment 45 • 6 years ago

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

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please) Assignee
	Comment 46 • 6 years ago

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

	Daniel Varga [:dvarga]
	Comment 47 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1d6879ee635e

	Daniel Varga [:dvarga]
	Comment 48 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c93a8b8b0c66