Closed
Bug 1430276
Opened 7 years ago
Closed 6 years ago
Allow updating a lightweight theme to a static WebExtensions theme
Categories
(Toolkit :: Add-ons Manager, enhancement, P1)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: andy+bugzilla, Assigned: aswan)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
The eventual goal of static WebExtension themes is that they will replace the existing lightweight themes. We'll do this by changing AMO so that it accepts WebExtension themes, users who install new themes will get the new theming style.
However there will be millions (roughly 6 of users have a lightweight theme) who will be stuck on the legacy lightweight theme.
Themes have an update URL in them, but it looks like all it does is update the properties on a theme, ideally we want to turn into a WebExtension static theme.
There's probably a few checks we should do on this flow to ensure that something bad doesn't happen. Eg: check its a WebExtension static theme that is signed and maybe coming from AMO?
https://dxr.mozilla.org/mozilla-central/rev/d2edd256c3aadb1cea48da0c8f62f725bd53cb76/toolkit/mozapps/extensions/LightweightThemeManager.jsm#253
Reporter | ||
Comment 1•7 years ago
|
||
That should be 6% of users. Or thereabouts. More than 6 anyway.
Reporter | ||
Updated•7 years ago
|
Priority: -- → P3
Comment 2•7 years ago
|
||
The plan is for the theme update ping to be changed on AMO for migrated themes, returning the path of the static theme to be used for updating. More details to be sorted out on https://github.com/mozilla/addons-server/issues/7976
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → aswan
Priority: P3 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8984555 -
Flags: review?(kmaglione+bmo)
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8984555 [details]
Bug 1430276 Provide a method to update LWTs to xpi-packaged themes
https://reviewboard.mozilla.org/r/250430/#review256690
::: toolkit/mozapps/extensions/AddonManager.jsm:1280
(Diff revision 1)
> let scope = {};
> ChromeUtils.import("resource://gre/modules/LightweightThemeManager.jsm", scope);
Gross. Can we just have a ChromeUtils.defineModuleGetter at the top-level for this?
::: toolkit/mozapps/extensions/LightweightThemeManager.jsm:274
(Diff revision 1)
> req.channel.loadFlags |= Ci.nsIRequest.LOAD_BYPASS_CACHE;
> // Prevent the request from writing to the cache.
> req.channel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING;
>
> - req.addEventListener("load", () => {
> + let loadPromise = new Promise(resolve => {
> + req.addEventListener("load", resolve);
`{once: true}`
::: toolkit/mozapps/extensions/LightweightThemeManager.jsm:277
(Diff revision 1)
>
> - req.addEventListener("load", () => {
> + let loadPromise = new Promise(resolve => {
> + req.addEventListener("load", resolve);
> + });
> +
> + req.send(null);
:
await new Promise(resolve => {
...
req.send();
});
::: toolkit/mozapps/extensions/LightweightThemeManager.jsm:306
(Diff revision 1)
> + resolve(addon);
> + }
> +
> + listener = {
> + onDownloadEnded() {
> + if (install.addon && install.type != "theme") {
s/!=/&=/
::: toolkit/mozapps/extensions/LightweightThemeManager.jsm:308
(Diff revision 1)
> +
> + listener = {
> + onDownloadEnded() {
> + if (install.addon && install.type != "theme") {
> + Cu.reportError(`Refusing to update lightweight theme to a ${install.type} (from ${url})`);
> + install.cancel();
Can just return `false`
::: toolkit/mozapps/extensions/LightweightThemeManager.jsm:318
(Diff revision 1)
> + onDownloadFailed() { finish(null); },
> + onInstallFailed() { finish(null); },
> + onInstallEnded(_install, addon) { finish(addon); },
> + };
> + install.addListener(listener);
> + install.install();
Probably may as well just `updated = await install.install()` here.
No need to even remove the listener, really, or implement the other methods.
::: toolkit/mozapps/extensions/LightweightThemeManager.jsm:349
(Diff revision 1)
> + } catch (e) {
> + return;
> + }
Why try-catch?
::: toolkit/mozapps/extensions/LightweightThemeManager.jsm:363
(Diff revision 1)
> + }
> +
> + let selectedID = _prefs.getStringPref("selectedThemeID", DEFAULT_THEME_ID);
> + let newThemes = [];
> + for (let theme of allThemes) {
> + let newTheme = await this._updateOneTheme(theme, theme.id == selectedID);
Can we do these in parallel?
::: toolkit/mozapps/extensions/test/xpcshell/test_theme_update.js:1
(Diff revision 1)
> +
"use strict";
Attachment #8984555 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Comment 6•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984555 [details]
Bug 1430276 Provide a method to update LWTs to xpi-packaged themes
https://reviewboard.mozilla.org/r/250430/#review256690
> Gross. Can we just have a ChromeUtils.defineModuleGetter at the top-level for this?
of course we already have one in addition to this. i removed the import() and the scope gunk.
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984555 [details]
Bug 1430276 Provide a method to update LWTs to xpi-packaged themes
https://reviewboard.mozilla.org/r/250430/#review256690
> Can just return `false`
I believe this just postpones the install, it doesn't actually end it (ie, none of the `On*Failed` listeners are invoked so the install promise never resolve)
Comment 8•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984555 [details]
Bug 1430276 Provide a method to update LWTs to xpi-packaged themes
https://reviewboard.mozilla.org/r/250430/#review256690
> I believe this just postpones the install, it doesn't actually end it (ie, none of the `On*Failed` listeners are invoked so the install promise never resolve)
Fair point. Please *also* return false, then.
Assignee | ||
Comment 9•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984555 [details]
Bug 1430276 Provide a method to update LWTs to xpi-packaged themes
https://reviewboard.mozilla.org/r/250430/#review256690
> Why try-catch?
I dunno, I didn't change this code.
Comment 10•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984555 [details]
Bug 1430276 Provide a method to update LWTs to xpi-packaged themes
https://reviewboard.mozilla.org/r/250430/#review256690
> I dunno, I didn't change this code.
Eh, blame mozreview for not detecting the move, then. Please get rid of the try catch and pass a default pref value instead.
Comment hidden (mozreview-request) |
Comment 12•6 years ago
|
||
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2c7f43bb207
Provide a method to update LWTs to xpi-packaged themes r=kmag
Comment 13•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•6 years ago
|
Summary: Allow the updating of a lightweight theme in to a static WebExtensions theme → Allow updating a lightweight theme to a static WebExtensions theme
Updated•6 years ago
|
Blocks: rm-lwthemes
Comment 14•6 years ago
|
||
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(aswan)
Assignee | ||
Comment 15•6 years ago
|
||
Its not going to be possible to test this manually until the AMO side is done (https://github.com/mozilla/addons-server/issues/7976)
Flags: needinfo?(aswan) → qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•