Closed Bug 1430276 Opened 2 years ago Closed 2 years ago

Allow updating a lightweight theme to a static WebExtensions theme

Categories

(Toolkit :: Add-ons Manager, enhancement, P1)

enhancement

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
That should be 6% of users. Or thereabouts. More than 6 anyway.
Priority: -- → P3
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
Duplicate of this bug: 1449583
Assignee: nobody → aswan
Priority: P3 → P1
Attachment #8984555 - Flags: review?(kmaglione+bmo)
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+
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.
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 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.
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 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.
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
https://hg.mozilla.org/mozilla-central/rev/e2c7f43bb207
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1468018
Summary: Allow the updating of a lightweight theme in to a static WebExtensions theme → Allow updating a lightweight theme to a static WebExtensions theme
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)
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.