1372310 - Update Screenshots to version 10.3.0

Status: RESOLVED FIXED

(Firefox :: Screenshots, defect)

Description

[Ian Bicking (:ianbicking)]

Changelog: https://github.com/mozilla-services/screenshots/blob/master/CHANGELOG.md#version-1010

Comment 1

[Ian Bicking (:ianbicking)]

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

Comment 2

[Ian Bicking (:ianbicking)]

Attachment: Bug 1372310 - Exports Screenshots 10.1.0 to Firefox;

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

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8876828 [details]
Bug 1372310 - Exports Screenshots 10.1.0 to Firefox;

https://reviewboard.mozilla.org/r/148156/#review152894

Comment 4

[Ian Bicking (:ianbicking)]

One important bug found in QA: https://github.com/mozilla-services/screenshots/issues/3027

Comment 5

[Ian Bicking (:ianbicking)]

10.2.0 version coming in; Try link for 10.2.0: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c69c3cd363a51dc804ae0dbbb6a753036c20616

Comment 6

[Ian Bicking (:ianbicking)]

Attachment: Bug 1372310 - Export Screenshots 10.3.0 to Firefox;

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

Comment 7

[Ian Bicking (:ianbicking)]

Comment on attachment 8876828 [details]
Bug 1372310 - Exports Screenshots 10.1.0 to Firefox;

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

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8877678 [details]
Bug 1372310 - Export Screenshots 10.3.0 to Firefox;

https://reviewboard.mozilla.org/r/149122/#review153538

redirect to kris to vouch for the StartupCache hack

Comment 9

[Dave Townsend [:mossop] (PTO until July 20)]

Comment on attachment 8877678 [details]
Bug 1372310 - Export Screenshots 10.3.0 to Firefox;

https://reviewboard.mozilla.org/r/149122/#review154082

::: browser/extensions/screenshots/bootstrap.js:63
(Diff revision 1)
>  const APP_STARTUP = 1;
> +const APP_SHUTDOWN = 2;
> +const ADDON_ENABLE = 3;
> +const ADDON_DISABLE = 4;
> +const ADDON_INSTALL = 5;
> +const ADDON_UNINSTALL = 6;
> +const ADDON_UPGRADE = 7;
> +const ADDON_DOWNGRADE = 8;

All of these should already be defined in the scope.

Comment 10

[Ian Bicking (:ianbicking)]

Attachment: Bug 1372310 - bump ExtensionStartupCache SCHEMA_VERSION

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

Comment 11

[Dave Townsend [:mossop] (PTO until July 20)]

Comment on attachment 8878155 [details]
Bug 1372310 - bump ExtensionStartupCache SCHEMA_VERSION

https://reviewboard.mozilla.org/r/149534/#review154120

Comment 12

[Ian Bicking (:ianbicking)]

Try build with third patch included: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68edffe5036756b7dc99fcd4300743de8583521b

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8877678 [details]
Bug 1372310 - Export Screenshots 10.3.0 to Firefox;

https://reviewboard.mozilla.org/r/149122/#review154170

::: browser/extensions/screenshots/bootstrap.js:77
(Diff revision 1)
> +function maybeClearAddonCache() {
> +  let result = Promise.resolve();
> +  if ((startupReason && cacheBustReasons.includes(startupReason)) ||
> +     (shutdownReason && cacheBustReasons.includes(shutdownReason))) {
> +    result = ExtensionParent.StartupCache.clearAddonData(ADDON_ID);
> +  }
> +  return result;

This isn't meant to be a public API. I'd rather we just fix LegacyExtensionUtils to allow passing the startup reason directly.

Comment 14

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

OK. Do you think the LegacyExtensionUtils API can be updated in time to uplift the patch to Beta 55?

Is bug 1372750 the right bug to track the WebExtension work?

Comment 15

[Ian Bicking (:ianbicking)]

We going to try to make use of the fix in Bug 1372750, removing the need for the last two patches (I'll fix it up once we have something tested to work). The StartupCache-related patches don't actually seem to work, because the startup reason is 1 (APP_STARTUP). Though if we clear the cache every time (regardless of reason) then the upgrade does work.

Comment 16

[Liz Henry (:lizzard)]

Looks like we may need this in 55, tracking.

Comment 17

[Ian Bicking (:ianbicking)]

Comment on attachment 8877678 [details]
Bug 1372310 - Export Screenshots 10.3.0 to Firefox;

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

Comment 18

[Ian Bicking (:ianbicking)]

Comment on attachment 8878155 [details]
Bug 1372310 - bump ExtensionStartupCache SCHEMA_VERSION

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

Comment 19

[Ian Bicking (:ianbicking)]

Removing blocking bugs, as the "bump ExtensionStartupCache SCHEMA_VERSION" patch works around the issues that would be addressed by those bugs.

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8877678 [details]
Bug 1372310 - Export Screenshots 10.3.0 to Firefox;

https://reviewboard.mozilla.org/r/149122/#review154664

To make sure we're on the same page, the changes here are mostly meant to make screenshots run correctly once bugs 1372750 and 1373749 are fixed, but in the short term, its the next patch that actually makes everything work, right?  r=ms assuming I've got that right.

Comment 21

[Andrew Swan [:aswan]]

Comment on attachment 8878155 [details]
Bug 1372310 - bump ExtensionStartupCache SCHEMA_VERSION

https://reviewboard.mozilla.org/r/149534/#review154672

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c5c65b50258
Exports Screenshots 10.1.0 to Firefox; r=standard8
https://hg.mozilla.org/integration/autoland/rev/d356c5c2785a
Export Screenshots 10.3.0 to Firefox; r=aswan,mossop
https://hg.mozilla.org/integration/autoland/rev/33e1a6555fad
bump ExtensionStartupCache SCHEMA_VERSION r=aswan,mossop

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c5c65b50258
https://hg.mozilla.org/mozilla-central/rev/d356c5c2785a
https://hg.mozilla.org/mozilla-central/rev/33e1a6555fad

Comment 24

[Ian Bicking (:ianbicking)]

Comment on attachment 8876828 [details]
Bug 1372310 - Exports Screenshots 10.1.0 to Firefox;

Approval Request Comment
[Feature/Bug causing the regression]: Screenshots

[User impact if declined]: 
* Patch includes some performance improvement by lazily-loading the background page
* Changes to make the error reporting more helpful on our end
* Avoid seeing popup notifications from Screenshots when the user isn't actively using Screenshots
* Fixes for several small bugs found via Sentry reports
* Small improvements to onboarding

[Is this code covered by automated tests?]: Only lightly (basic smoke test)

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]: SoftVision has tested this extensively on Nightly

[List of other uplifts needed for the feature/fix]:
All three patches:
https://hg.mozilla.org/mozilla-central/rev/3c5c65b50258
https://hg.mozilla.org/mozilla-central/rev/d356c5c2785a
https://hg.mozilla.org/mozilla-central/rev/33e1a6555fad

[Is the change risky?]:
* The add-on changes are contained within Screenshots, and use public stable WebExtension APIs, making them less risky
* The bootstrap.js changes have some danger of making Screenshots not activate in case of an error. Screenshots remains pref'd off in Beta, so it wouldn't be actively noticed by users.
* The schema change (last patch: https://hg.mozilla.org/mozilla-central/rev/33e1a6555fad) causes a deliberate cache invalidation to work around some update bugs. :kmags, :aswan, or :rhelmer could say more about the risk of that than I can.

[Why is the change risky/not risky?]: (see above)

[String changes made/needed]:
Screenshots localization is handled outside of the tree

Comment 25

[Ian Bicking (:ianbicking)]

Comment on attachment 8877678 [details]
Bug 1372310 - Export Screenshots 10.3.0 to Firefox;

See https://bugzilla.mozilla.org/show_bug.cgi?id=1372310#c24

Comment 26

[Ian Bicking (:ianbicking)]

Comment on attachment 8878155 [details]
Bug 1372310 - bump ExtensionStartupCache SCHEMA_VERSION

See https://bugzilla.mozilla.org/show_bug.cgi?id=1372310#c24

Comment 27

[Julien Cristau [:jcristau]]

Why was this reopened?

Comment 28

[Julien Cristau [:jcristau]]

Comment on attachment 8876828 [details]
Bug 1372310 - Exports Screenshots 10.1.0 to Firefox;

screenshots add-on update, beta55+

I'm going to assume the reopen was a mistake and this should still land as-is.  Somebody yell if not :)

Comment 29

[Ian Bicking (:ianbicking)]

Yes, reopen was a mistake

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4d75a17437df
https://hg.mozilla.org/releases/mozilla-beta/rev/3fe646eaa40d
https://hg.mozilla.org/releases/mozilla-beta/rev/8c57a7561306