1451402 - Display static WebExtension themes in customization mode

Status: NEW

(Firefox :: Toolbars and Customization, defect, P3)

Description

[Tim Nguyen :ntim]

At the moment, they are not displayed in the customization mode, because the CM only queries lightweight themes registered via LWTManager.jsm. WebExtension static themes are registered differently however.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1451402 - Create a synchronous internal API to get the selected theme ID.

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

Comment 2

[Tim Nguyen :ntim]

Attachment: Bug 1451402 - Fix checks for legacy themes.

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

Comment 3

[Tim Nguyen :ntim]

Attachment: Bug 1451402 - Display WebExtension static themes in the customize mode.

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

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8987897 [details]
Bug 1451402 - Display WebExtension static themes in the customize mode.

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

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8987895 [details]
Bug 1451402 - Create a synchronous internal API to get the selected theme ID.

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

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8987896 [details]
Bug 1451402 - Fix checks for legacy themes.

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

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8987897 [details]
Bug 1451402 - Display WebExtension static themes in the customize mode.

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

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8987897 [details]
Bug 1451402 - Display WebExtension static themes in the customize mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253194/diff/3-4/

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8987895 [details]
Bug 1451402 - Create a synchronous internal API to get the selected theme ID.

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

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8987896 [details]
Bug 1451402 - Fix checks for legacy themes.

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

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8987897 [details]
Bug 1451402 - Display WebExtension static themes in the customize mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253194/diff/4-5/

Comment 12

[Tim Nguyen :ntim]

Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42f270d40af0c1ab47bdb8689e6d9ef2c3e69ca4

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8987895 [details]
Bug 1451402 - Create a synchronous internal API to get the selected theme ID.

https://reviewboard.mozilla.org/r/253190/#review260242

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8987896 [details]
Bug 1451402 - Fix checks for legacy themes.

https://reviewboard.mozilla.org/r/253192/#review260244

::: toolkit/mozapps/extensions/content/extensions.js:243
(Diff revision 3)
>      // The logic here is kind of clunky but we want to mark complete
>      // themes as legacy.  There's no explicit flag for complete
>      // themes so explicitly check for new style themes (for which
>      // isWebExtension is true) or lightweight themes (which have
>      // ids that end with @personas.mozilla.org)
>      legacy = !(addon.isWebExtension || addon.id.endsWith("@personas.mozilla.org") ||
> -               addon.id == "[e-mail]");
> +               addon.id.endsWith("@mozilla.org"));

Just make this `legacy = false`. Legacy themes don't actually exist anymore.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8987897 [details]
Bug 1451402 - Display WebExtension static themes in the customize mode.

https://reviewboard.mozilla.org/r/253194/#review260328

This isn't really enough... you haven't made this work properly with reset/undoReset.

::: browser/components/customizableui/CustomizeMode.jsm:166
(Diff revision 5)
> -  _updateLWThemeButtonIcon() {
> +  async _updateLWThemeButtonIcon() {
>      let lwthemeButton = this.$("customization-lwtheme-button");
>      let lwthemeIcon = this.document.getAnonymousElementByAttribute(lwthemeButton,
>                          "class", "button-icon");
> -    lwthemeIcon.style.backgroundImage = LightweightThemeManager.currentTheme ?
> +    let selectedTheme = await this.getSelectedTheme();

Please update the callsites to await this method now that it's async.

::: browser/components/customizableui/CustomizeMode.jsm:1345
(Diff revision 5)
> +    return themes.filter(a => !a.hidden);
> +  },
> +
> +  async getSelectedTheme() {
> +    let themes = await this.getAllThemes();
> +    return themes.filter(a => a.isActive)[0];

Nit: .find(a => a.isActive)

::: browser/components/customizableui/CustomizeMode.jsm:1404
(Diff revision 5)
> -    let lwts = LightweightThemeManager.usedThemes;
> -    let currentLwt = LightweightThemeManager.currentTheme;
> +    let unorderedThemes = await this.getAllThemes();
> +    let selectedTheme = await this.getSelectedTheme();

I don't think the helper functions make this better. We now request and await fetching the themes twice, for no reason. Why not do something like this:

```js
let unorderedThemes = await AddonManager.getAddonsByTypes(["theme"]));
unorderedThemes = unorderedThemes.filter(a => !a.hidden);
let selectedTheme = unorderedThemes.find(a => a.isActive);
```

This would be much shorter than the current 10 lines, more performant (because we don't request the same stuff twice), and makes it obvious where these themes are coming from.

The icon updater could just be passed the right theme id, which all the callsites seem like they should know, and call `await AddonManager.getAddonById(id)`, which also avoids filtering the same list again;

::: browser/components/customizableui/CustomizeMode.jsm:1428
(Diff revision 5)
>  
>      let footer = doc.getElementById("customization-lwtheme-menu-footer");
>      let panel = footer.parentNode;
>      let recommendedLabel = doc.getElementById("customization-lwtheme-menu-recommended");
>      for (let theme of themes) {
> -      let button = buildToolbarButton(theme);
> +      let button = buildToolbarButton(theme, LightweightThemeManager.getThemeByAddonID(theme.id));

buildToolbarButton could just get this data itself instead of having it be a parameter that depends only on data that we already have? Same thing for the other callsite, which passes the same data twice. Just once should be sufficient.

::: browser/components/customizableui/test/browser_static_themes_in_customize_mode.js:1
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +  * License, v. 2.0. If a copy of the MPL was not distributed with this
> +  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Public domain header instead please.

::: browser/components/customizableui/test/browser_static_themes_in_customize_mode.js:13
(Diff revision 5)
> +  Services.prefs.clearUserPref("lightweightThemes.usedThemes");
> +  Services.prefs.clearUserPref("lightweightThemes.recommendedThemes");

This shouldn't be necessary. If it is, fix the tests that are littering the prefs.

::: browser/components/customizableui/test/browser_static_themes_in_customize_mode.js:57
(Diff revision 5)
> +  if (activeThemes.length > 0) {
> +    is(activeThemes[0].theme.id, DEFAULT_THEME_ID, "Default theme should be selected");
> +  }
> +
> +  // Check properties of the static theme button in the popup
> +  let newThemeButton = header.nextSibling.nextSibling.nextSibling.nextSibling;

Can't you find this with a selector instead of hardcoding its position this way?

Comment 16

[Tim Nguyen :ntim]

Comment on attachment 8987895 [details]
Bug 1451402 - Create a synchronous internal API to get the selected theme ID.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253190/diff/3-4/

Comment 17

[Tim Nguyen :ntim]

Comment on attachment 8987897 [details]
Bug 1451402 - Display WebExtension static themes in the customize mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253194/diff/5-6/

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8987897 [details]
Bug 1451402 - Display WebExtension static themes in the customize mode.

Not quite ready for review yet

Comment 19

[Code Review Bot [:reviewbot]]

Comment on attachment 8987897 [details]
Bug 1451402 - Display WebExtension static themes in the customize mode.

https://reviewboard.mozilla.org/r/253194/#review269682


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python/etc)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/customizableui/CustomizableUI.jsm:2643
(Diff revision 6)
>      Services.prefs.clearUserPref(kPrefDrawInTitlebar);
>      Services.prefs.clearUserPref(kPrefExtraDragSpace);
>      Services.prefs.clearUserPref(kPrefUIDensity);
>      Services.prefs.clearUserPref(kPrefAutoTouchMode);
>      Services.prefs.clearUserPref(kPrefAutoHideDownloadsButton);
> -    LightweightThemeManager.currentTheme = null;
> +    AddonManager.getAddonByID(currentTheme.id).then(addon => addon.disable());

Error: 'currentTheme' is not defined. [eslint: no-undef]

Comment 20

[Tim Nguyen :ntim]

Comment on attachment 8987897 [details]
Bug 1451402 - Display WebExtension static themes in the customize mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253194/diff/6-7/