Allow updating a lightweight theme to a static WebExtensions theme

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: andy+bugzilla, Assigned: aswan)

Tracking

(Blocks 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

Last year
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

Last year
That should be 6% of users. Or thereabouts. More than 6 anyway.
Reporter

Updated

Last year
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

Updated

Last year
Assignee: nobody → aswan
Priority: P3 → P1
Assignee

Updated

Last year
Attachment #8984555 - Flags: review?(kmaglione+bmo)

Comment 5

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
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

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/e2c7f43bb207
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

Updated

Last year
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

Updated

Last year
Blocks: rm-lwthemes

Comment 14

Last year
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

Last year
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.