Closed Bug 1372310 Opened 3 years ago Closed 3 years ago

Update Screenshots to version 10.3.0

Categories

(Firefox :: Screenshots, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox55 + fixed
firefox56 --- fixed

People

(Reporter: ianbicking, Assigned: ianbicking)

References

Details

Attachments

(3 files)

Assignee: nobody → ianb
Blocks: 1361792
Comment on attachment 8876828 [details]
Bug 1372310 - Exports Screenshots 10.1.0 to Firefox;

https://reviewboard.mozilla.org/r/148156/#review152894
Attachment #8876828 - Flags: review?(standard8) → review+
Summary: Update Screenshots to version 10.1.0 → Update Screenshots to version 10.2.0
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
Attachment #8877678 - Flags: review?(aswan) → review?(kmaglione+bmo)
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.
Attachment #8877678 - Flags: review+
Comment on attachment 8878155 [details]
Bug 1372310 - bump ExtensionStartupCache SCHEMA_VERSION

https://reviewboard.mozilla.org/r/149534/#review154120
Attachment #8878155 - Flags: review?(dtownsend) → review+
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.
Attachment #8877678 - Flags: review?(kmaglione+bmo) → review-
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?
Flags: needinfo?(kmaglione+bmo)
Depends on: 1372750
No longer depends on: 1368146
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.
Flags: needinfo?(kmaglione+bmo)
Depends on: 1373749
Looks like we may need this in 55, tracking.
Attachment #8877678 - Attachment is obsolete: true
Attachment #8878155 - Attachment is obsolete: true
Summary: Update Screenshots to version 10.2.0 → Update Screenshots to version 10.3.0
Attachment #8878155 - Flags: review?(aswan)
Attachment #8878155 - Flags: review+
Removing blocking bugs, as the "bump ExtensionStartupCache SCHEMA_VERSION" patch works around the issues that would be addressed by those bugs.
No longer depends on: 1372750, 1373749
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.
Attachment #8877678 - Flags: review?(aswan) → review+
Comment on attachment 8878155 [details]
Bug 1372310 - bump ExtensionStartupCache SCHEMA_VERSION

https://reviewboard.mozilla.org/r/149534/#review154672
Attachment #8878155 - Flags: review?(aswan) → review+
Pushed by mbanner@mozilla.com:
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 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
Attachment #8876828 - Flags: approval-mozilla-beta?
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
Attachment #8877678 - Flags: approval-mozilla-beta?
Comment on attachment 8878155 [details]
Bug 1372310 - bump ExtensionStartupCache SCHEMA_VERSION

See https://bugzilla.mozilla.org/show_bug.cgi?id=1372310#c24
Attachment #8878155 - Flags: approval-mozilla-beta?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Why was this reopened?
Flags: needinfo?(ianb)
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 :)
Attachment #8876828 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8877678 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8878155 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Yes, reopen was a mistake
Flags: needinfo?(ianb)
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Target Milestone: mozilla56 → Firefox 56
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.