1414512 - browser.theme.getCurrent() always returns an empty theme object if theme doesn't originate from the extension

Status: VERIFIED FIXED

(WebExtensions :: Frontend, enhancement, P3)

Description

[Tim Nguyen :ntim]

If you create an extension that queries the current theme using `theme.getCurrent()`, the query will always return an empty object if the theme applied doesn't originate from that extension.

`theme.onUpdated` works well from the extension though.

Comment 1

[Shane Caraveo (:mixedpuppy)]

ntim and I have been discussing this, it needs to be addressed since there are a couple of things wrong with the theme api right now.

Comment 2

[Tim Nguyen :ntim]

Attachment: Bug 1414512 - Refactor ext-theme.js to have per-theme Theme objects and global tracking variables.

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

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8925745 [details]
Bug 1414512 - Refactor ext-theme.js to have per-theme Theme objects and global tracking variables.

https://reviewboard.mozilla.org/r/196896/#review202068

Looking better and I think closer to what the original intention was, thanks!

Still need some tests as well.  A couple captured from our irc discussion:

test setting global theme where there are window overrides
addon1 theme.update(theme-1)
addon2 theme.update(window-2, theme-2)
addon1 theme.update(theme-1)

test resetting a window
addon1 theme.update(theme-1)
addon2 theme.reset(window-2)

::: toolkit/components/extensions/ext-theme.js:20
(Diff revision 1)
>    getWinUtils,
>  } = ExtensionUtils;
>  
>  const ICONS = Services.prefs.getStringPref("extensions.webextensions.themes.icons.buttons", "").split(",");
>  
> +const emptyTheme = {

Lets add a comment about why emptyTheme is necessary.

::: toolkit/components/extensions/ext-theme.js:25
(Diff revision 1)
> +const emptyTheme = {
> +  details: {},
> +};
> +
> +let defaultTheme = emptyTheme;
> +let windowOverrides = new Map();

comment

// Map[windowId -> Theme instance]

::: toolkit/components/extensions/ext-theme.js:27
(Diff revision 1)
> +};
> +
> +let defaultTheme = emptyTheme;
> +let windowOverrides = new Map();
> +
>  /** Class representing a theme. */

Lets document this a bit better.  Maybe something like:

Theme class represents either a global theme affecting all windows or an override on a specific window.  Only one global theme (defaultTheme) is allowed.  Any extension updating the theme with a new global theme will replace the singleton defaultTheme.

::: toolkit/components/extensions/ext-theme.js:32
(Diff revision 1)
>  /** Class representing a theme. */
>  class Theme {
>    /**
>     * Creates a theme instance.
>     *
> -   * @param {string} baseURI The base URI of the extension, used to
> +   * @param {string} extension Extension behind the theme.

s/behind/that created/

::: toolkit/components/extensions/ext-theme.js:259
(Diff revision 1)
>        }
>      }
>    }
> +}
>  
> -  /**
> +Theme.unload = (windowId) => {

move this into the class declaration.  static unload(windowId) {...}

::: toolkit/components/extensions/ext-theme.js:305
(Diff revision 1)
>        // Return early if themes are disabled.
>        return;
>      }
>  
> -    this.theme = new Theme(extension.baseURI, extension.logger);
> -    this.theme.load(manifest.theme);
> +    defaultTheme = new Theme(extension);
> +    defaultTheme.load(manifest.theme);

We should also clear the overrides here since this can happen at any time during the session.

::: toolkit/components/extensions/ext-theme.js:311
(Diff revision 1)
> +
>      onUpdatedEmitter.emit("theme-updated", manifest.theme);
>    }
>  
>    onShutdown() {
> -    if (this.theme) {
> +    Theme.unload();

This should be unloading the defaultTheme only if it belongs to the extension.  See onManifestEntry for how to get the extension.

Also need to remove any window overrides the theme has done (if not unloading the defaultTheme).

You should also check the reason, if it is an app shutdown no need to bother unloading anything.

::: toolkit/components/extensions/ext-theme.js:326
(Diff revision 1)
> -          // Return theme applied on last focused window when no ID is supplied.
>            if (!windowId) {
> -            return Promise.resolve(this.theme.getWindowTheme(windowTracker.topWindow));
> +            windowId = windowTracker.getId(windowTracker.topWindow);
>            }
>  
>            const browserWindow = windowTracker.getWindow(windowId, context);

I'm not certain this is necessary any longer.  An invalid id wont get stored into the overrides.  Im not too concerned about returning the default theme if they send an invalid id.  I'm more concerned about this in the other api calls.

::: toolkit/components/extensions/ext-theme.js:342
(Diff revision 1)
>            if (!gThemesEnabled) {
>              // Return early if themes are disabled.
>              return;
>            }
>  
> -          if (!this.theme) {
> +          let theme = new Theme(extension, windowId);

We should validate the windowId here.

::: toolkit/components/extensions/ext-theme.js:358
(Diff revision 1)
> -          if (!this.theme) {
> +          if (!defaultTheme && !windowOverrides.has(windowId)) {
>              // If no theme has been initialized, nothing to do.
>              return;
>            }
>  
> -          let browserWindow;
> +          Theme.unload(windowId);

I think we may also want to validate windowId here, since sending a bogus id would result in unloading the default?  Actually, looks like it could just throw an error in unload.

Comment 4

[Tim Nguyen :ntim]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> Comment on attachment 8925745 [details]
> Bug 1414512 - Refactor ext-theme.js to have per-theme Theme objects and
> global tracking variables.
> 
> https://reviewboard.mozilla.org/r/196896/#review202068
> 
> Looking better and I think closer to what the original intention was, thanks!
> 
> Still need some tests as well.  A couple captured from our irc discussion:
> 
> test setting global theme where there are window overrides
> addon1 theme.update(theme-1)
> addon2 theme.update(window-2, theme-2)
> addon1 theme.update(theme-1)
> 
> test resetting a window
> addon1 theme.update(theme-1)
> addon2 theme.reset(window-2)

These are already covered by the tests in bug 1349944.

I plan to add another test that tests that getCurrent() works correctly across 2 different extensions though (which is what this patch is mainly fixing).

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8925745 [details]
Bug 1414512 - Refactor ext-theme.js to have per-theme Theme objects and global tracking variables.

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

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8925745 [details]
Bug 1414512 - Refactor ext-theme.js to have per-theme Theme objects and global tracking variables.

https://reviewboard.mozilla.org/r/196896/#review202068

> We should also clear the overrides here since this can happen at any time during the session.

This is already done in load().

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8925745 [details]
Bug 1414512 - Refactor ext-theme.js to have per-theme Theme objects and global tracking variables.

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

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8925745 [details]
Bug 1414512 - Refactor ext-theme.js to have per-theme Theme objects and global tracking variables.

https://reviewboard.mozilla.org/r/196896/#review202420

Assuming try finishes out clean, I'm happy with this, thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/858a5f9806c3
Refactor ext-theme.js to have per-theme Theme objects and global tracking variables. r=mixedpuppy

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/858a5f9806c3

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for fixing this so promptly! I started doing a post-review of the code (for my own knowledge) but will need to come back to finish. Flagging myself for needinfo.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Looks good!

Comment 13

[marius.santa]

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.

Comment 14

[Tim Nguyen :ntim]

STR:
- Install this extension: https://github.com/mdn/webextensions-examples/tree/master/theme-integrated-sidebar
- Install a WebExtension theme (make sure you only have one installed at a time):
  https://addons.mozilla.org/en-US/firefox/addon/purple-private-windows/
  https://addons.mozilla.org/en-US/firefox/addon/vivaldifox/
  https://addons.mozilla.org/en-US/firefox/addon/containers-theme/

The sidebar should always match the window theme, no matter which theme is installed.

Comment 15

[Victor Carciu]

Verified as fixed using Build identifier: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0. Please see attached postfix screenshot.

Comment 16

[Victor Carciu]

Attachment: Postfix screenshot (FF57 vs FF58)

Comment 17

[Filippo]

Attachment: Test.xpi

I'm trying to use browser.theme.getCurrent() with Firefox 61.0a1, but I get always the same result (an empty object).
You can try to load this extension as a temporary extension and debug it. You will see always an empty object in the console log.
Am I missing something? Thanks.

Comment 18

[Tim Nguyen :ntim]

(In reply to Filippo from comment #17)
> Created attachment 8964602 [details]
> Test.xpi
> 
> I'm trying to use browser.theme.getCurrent() with Firefox 61.0a1, but I get
> always the same result (an empty object).
> You can try to load this extension as a temporary extension and debug it.
> You will see always an empty object in the console log.
> Am I missing something? Thanks.

Which theme are you testing with ?

Comment 19

[Filippo]

(In reply to Tim Nguyen :ntim from comment #18)
> (In reply to Filippo from comment #17)
> > Created attachment 8964602 [details]
> > Test.xpi
> > 
> > I'm trying to use browser.theme.getCurrent() with Firefox 61.0a1, but I get
> > always the same result (an empty object).
> > You can try to load this extension as a temporary extension and debug it.
> > You will see always an empty object in the console log.
> > Am I missing something? Thanks.
> 
> Which theme are you testing with ?

Default theme, dark theme, lightweight theme.

Comment 20

[Tim Nguyen :ntim]

(In reply to Filippo from comment #19)
> (In reply to Tim Nguyen :ntim from comment #18)
> > (In reply to Filippo from comment #17)
> > > Created attachment 8964602 [details]
> > > Test.xpi
> > > 
> > > I'm trying to use browser.theme.getCurrent() with Firefox 61.0a1, but I get
> > > always the same result (an empty object).
> > > You can try to load this extension as a temporary extension and debug it.
> > > You will see always an empty object in the console log.
> > > Am I missing something? Thanks.
> > 
> > Which theme are you testing with ?
> 
> Default theme, dark theme, lightweight theme.

Yeah, it currently doesn't work for those themes. You need to try a WebExtension theme: https://addons.mozilla.org/en-US/firefox/collections/ntim/theming-extensions/

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Filippo from comment #19)
> (In reply to Tim Nguyen :ntim from comment #18)
> > (In reply to Filippo from comment #17)
> > > Created attachment 8964602 [details]
> > > Test.xpi
> > > 
> > > I'm trying to use browser.theme.getCurrent() with Firefox 61.0a1, but I get
> > > always the same result (an empty object).
> > > You can try to load this extension as a temporary extension and debug it.
> > > You will see always an empty object in the console log.
> > > Am I missing something? Thanks.
> > 
> > Which theme are you testing with ?
> 
> Default theme, dark theme, lightweight theme.

This will be fixed for Light and Dark themes by bug 1386004.

Comment 22

[Filippo]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> (In reply to Filippo from comment #19)
> > (In reply to Tim Nguyen :ntim from comment #18)
> > > (In reply to Filippo from comment #17)
> > > > Created attachment 8964602 [details]
> > > > Test.xpi
> > > > 
> > > > I'm trying to use browser.theme.getCurrent() with Firefox 61.0a1, but I get
> > > > always the same result (an empty object).
> > > > You can try to load this extension as a temporary extension and debug it.
> > > > You will see always an empty object in the console log.
> > > > Am I missing something? Thanks.
> > > 
> > > Which theme are you testing with ?
> > 
> > Default theme, dark theme, lightweight theme.
> 
> This will be fixed for Light and Dark themes by bug 1386004.

OK, thanks.
Another little question: current API doesn't allow you to detect if a LWT is applied to Firefox. Do you plan to change this point or LWT will "die" in favour of theming API?
Ciao!

Comment 23

[Tim Nguyen :ntim]

(In reply to Filippo from comment #22)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #21)
> > (In reply to Filippo from comment #19)
> OK, thanks.
> Another little question: current API doesn't allow you to detect if a LWT is
> applied to Firefox. Do you plan to change this point or LWT will "die" in
> favour of theming API?
> Ciao!

Yeah, LWT are expected to be replaced by the static WebExtension themes. Same for the light/dark built-in themes.

Comment 24

[depiction]

This doesn't appear to be fixed. I'm using Firefox 61.0.1 and the getCurrent method always returns an empty object.

The following code gets all windows:

browser.windows.getAll().then(r => console.log(r))

The first window in the array returned is the main browser window. 

I then pass the id for the first window to the following:

browser.theme.getCurrent(id).then(t => console.log(t))

I'm using this code in an extension. The extension temporarily changes the theme. After a few seconds I'm calling reset() to reset the theme. It resets the theme to the default theme, not the theme the user enabled. I thought getting the current theme before changing it would help resolve my bug, but it won't help because getCurrent always returns an empty object.

I tried this with the Light, Quantum, and Running Foxes themes.

Comment 25

[Tim Nguyen :ntim]

As mentioned in this bug, theme.getCurrent() only works with WebExtension themes, not lightweight or built-in themes.