Tim Nguyen :ntim Reporter
	Description • 7 years ago

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.

	Tim Nguyen :ntim Reporter
	Comment 12 • 6 years ago

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

	Kris Maglione [:kmag]
	Comment 13 • 6 years ago
mozreview-review

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

	Kris Maglione [:kmag]
	Comment 14 • 6 years ago
mozreview-review

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 == "default-theme@mozilla.org");
> +               addon.id.endsWith("@mozilla.org"));

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

	:Gijs (he/him)
	Comment 15 • 6 years ago
mozreview-review

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?

	Tim Nguyen :ntim Reporter
	Comment 18 • 6 years ago

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

Not quite ready for review yet

	Code Review Bot [:reviewbot]
	Comment 19 • 6 years ago
mozreview-review

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]