Closed Bug 1466575 Opened Last year Closed Last year

Switch Screenshots to WebExtension pageAction

Categories

(Firefox :: Screenshots, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: bchen, Assigned: _6a68)

References

Details

Attachments

(4 files)

Screenshots is switching from Photon pageAction to WebExtension pageAction.

There are some perf related discussion at https://github.com/mozilla-services/screenshots/pull/3967.
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.
Flags: needinfo?(bchen)
_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. :-/
Attachment #8990760 - Flags: review?(jhirsch)
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.
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
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
Attachment #8990760 - Flags: review?(jhirsch) → review+
Pushed by jhirsch@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/412b5e9b8525
Replace Photon pageAction with WebExtension pageAction in Screenshots; r=_6a68
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.
Attachment #8997463 - Flags: review?(jhirsch)
_6a68: Added a commit to update the Screenshots pageAction id in tests. r?
Flags: needinfo?(bchen)
Looks like there are some test failures in the try run. Mind taking a look?
Flags: needinfo?(bchen)
Flags: needinfo?(bchen)
Comment on attachment 8997463 [details]
Bug 1466575 - Update tests to use new Screenshots pageAction id;

https://reviewboard.mozilla.org/r/261216/#review269298
Attachment #8997463 - Flags: review?(jhirsch) → review+
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
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Duplicate of this bug: 1412377
(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.
Flags: needinfo?(bchen)
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!
Flags: needinfo?(bchen)
Depends on: 1483085
Depends on: 1483088
Blocks: 1422437
(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.
Flags: needinfo?(felipc)
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
Flags: needinfo?(felipc)
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.
Attachment #9003630 - Flags: review+
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This patch was temporarily reverted to avoid bug 1483033 for Firefox 63.

MozReview-Commit-ID: 4VaQgZQCVlE
Assignee: bchen → jhirsch
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.
Attachment #9008255 - Flags: review+
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.
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Depends on: 1494135
You need to log in before you can comment on or make changes to this bug.