1349896 - Implement devtools.panels.themeName

Status: RESOLVED FIXED

(WebExtensions :: Developer Tools, enhancement, P3)

Description

[Tim Nguyen :ntim]

Extensions should be able to get the devtools theme so they can properly integrate themselves within devtools.

browser.devtools.theme would be nice.

Comment 1

[Andy McKay]

What does "get the devtools theme" mean? Is this the same as bug 1349944 or how about bug 1349898?

Comment 2

[Tim Nguyen :ntim]

(In reply to Andy McKay [:andym] from comment #1)
> What does "get the devtools theme" mean? Is this the same as bug 1349944 or
> how about bug 1349898?

bug 1349944 is about the browser theme, not the developer tools theme.

Bug 1349898 is complementary to this bug. This bug is about a simple way to get the developer tools (without injecting any style). Bug 1349898 is about injecting stylesheets to match the devtools style.

Comment 3

[Tim Nguyen :ntim]

Attachment: devtools-theme-toolbox.png

Comment 5

[Andrew Williamson [:eviljeff]]

removing good-first-bug until design-decision-needed has been resolved (they're kinda mutually exclusive)

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

According to the Chrome docs: https://developer.chrome.com/extensions/devtools_panels#property-themeName

Not fully implemented. You must build Chromium from source to try this API.

The name of the color theme set in user's DevTools settings. Possible values: default (the default) and dark.

Comment 7

[Tim Nguyen :ntim]

(In reply to Andrew Williamson [:eviljeff] from comment #5)
> removing good-first-bug until design-decision-needed has been resolved
> (they're kinda mutually exclusive)

I wouldn't see this API rejected considering it's implemented in Chrome.

Comment 8

[Andy McKay]

I don't think there's any need to even discuss this API in a design-decision-needed meeting.

Comment 9

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1349896 - Part 1: Add theme property to Toolbox,

This uses getTheme() from devtools/client/shared/theme.js in order to abstract the code
away from the preference. devtools/client/shared/css-reload.js also uses getTheme() from
shared/theme.js, so this seems like a safe abstraction to use.

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

Comment 10

[Bob Silverberg [:bsilverberg]]

Luca, I've done a first kick at adding a theme property to Toolbox. I started working on adding the devtools.panels.themeName property, but I'm having some issues (getting confused about parent vs. child files, for one thing). Perhaps we can find some time tomorrow to discuss it?

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Bob,
I just searched for "getTheme" on dxr and it seems that while the "getTheme" helper exported from "devtools/client/shared/theme" is used in some of the modules:
- http://searchfox.org/mozilla-central/search?q=require%28%22devtools%2Fclient%2Fshared%2Ftheme%22%29%3B&path=devtools%2Fclient%2F

The performance tool uses an internal method that retrieve the current theme name directly from the preference:
- http://searchfox.org/mozilla-central/source/devtools/client/performance/performance-controller.js#195-197

If we proceed as we initially discussed ("by providing the current theme name as a property of the toolbox") we have to:

- pass the initial value of the currently selected toolbox theme from the main process to the extension child process inside the `devtoolsToolboxInfo`, that we already pass to the child process when we initialize the devtools page and devtools panels, as we are doing for the `inspectedWindowTabId`:
  - http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/browser/components/extensions/ext-devtools.js#143-147
  - http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/browser/components/extensions/ext-devtools-panels.js#150-153

- then in a API implemented in the child process, we retrieve the value from the devtoolsToolboxInfo and return it from a API getter, as we do for the devtools.inspectedWindow.tabId:
  - http://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/browser/components/extensions/ext-c-devtools-inspectedWindow.js#11-12,17-19

Nevertheless, this is not enough because, while the tabId doesn't change during the life of a developer toolbox, the toolbox theme can be changed over the time, and so we are going to also need:

- to be able to detect when the current selected toolbox theme has been changed (e.g. by registering a listener on the toolbox)
- a strategy to send the updates related to the themeName value to the extension child process where the value is cached (e.g. by exchanging an additional custom "message manager" event message)
- to provide an additional devtools.panels.onThemeNameChanged API event, so that a devtools panel implemented by a webextension is able to subscribe a listener and handle the themeName change

As an alternative strategy, at least if I recall correctly, the preferences service should be accessible also in a child process, and so it would be possible to implement both the devtools.panel.themeName property and the additional devtools.panels.onThemeNameChanged API event by interacting directly with the preferences services in ext-c-devtools-panels.js.

Comment 12

[Bob Silverberg [:bsilverberg]]

Based on what we have observed from the devtools code, it looks like it would be safe for us to read the devtools.theme preference directly from our API code, and observe that pref for changes, rather than make changes to devtools to expose the theme from the Toolbox. 

What do you think of that idea, ochameau?

Comment 13

[Alexandre Poirot [:ochameau]]

DevTools are about to be an Add-on. The pref won't be available if DevTools aren't installed yet.

From now on, it is better to go throught explicit API for every interaction between mozilla-central code and DevTools.
Using the preference is implicit, we could possibly break your code without knowing by changing its name/behavior.

Also, the theme is global and isn't specific to a toolbox. Your patch should be reviewed by someone from devtools team.

gDevTools is a better entry point. This is going to become the glue API between mozilla-central and DevTools addon.

So I would suggest using gDevTools like this file:
http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-devtools.js#12

Instead of toolbox.js, expose getTheme on gDevTools around here:
http://searchfox.org/mozilla-central/source/devtools/client/framework/devtools.js#238

Note that you would also have to specify "getTheme" in this list (to expose it on the gDevTools.jsm module):
http://searchfox.org/mozilla-central/source/devtools/client/framework/gDevTools.jsm#70

Comment 14

[Alexandre Poirot [:ochameau]]

(Note that you can add test in this test:
 http://searchfox.org/mozilla-central/source/devtools/client/framework/test/browser_toolbox_theme_registration.js)

Comment 15

[Bob Silverberg [:bsilverberg]]

Thanks for the advice, Alexandre. I will start working on that now.

Comment 16

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1349896 - Part 1: Expose devtools theme via gDevTools,

Added gDevTools.getTheme() to provide the theme name, and a gDevTools "theme-changed"
event to provide a notification when the theme is changed.

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

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8865987 [details]
Bug 1349896 - Part 1: Expose devtools theme via gDevTools,

https://reviewboard.mozilla.org/r/137588/#review141014

Hi Bob,
Follows some initial feedback on this part, I found a couple of nits and a couple of fixes to apply before moving it to the review phase by Alex.

::: devtools/client/framework/devtools.js:45
(Diff revision 1)
>    AboutDevTools.register();
>  
>    EventEmitter.decorate(this);
>  
> +  // Listen for changes to the theme pref.
> +  Services.prefs.addObserver(THEME_PREF, this._themeChanged.bind(this));

Nit: By looking at other modules from the devtoools/client/framework dir (e.g. `target.js`), it looks that conventions for the name of this kind of "private listener" is `_onSomeEventName`, I think that it could be better to follow the same convention and rename this to `_onThemeChanged`.

::: devtools/client/framework/devtools.js:45
(Diff revision 1)
>    AboutDevTools.register();
>  
>    EventEmitter.decorate(this);
>  
> +  // Listen for changes to the theme pref.
> +  Services.prefs.addObserver(THEME_PREF, this._themeChanged.bind(this));

`bind` creates a different function object instance every time we call it, and so we need to store the reference to the "binded method" to be able to correctly remove the observer later:

```
this._themeChanged = this._themeChanged.bind(this);

Servers.prefs.addObserver(...);
```

::: devtools/client/framework/devtools.js:257
(Diff revision 1)
> +  /**
> +   * Called when the developer tools theme changes.
> +   */
> +  _themeChanged() {
> +    let newTheme = Services.prefs.getCharPref(THEME_PREF);
> +    this.emit("theme-changed", newTheme);

no need for the `newTheme` identifier:

```
this.emit("theme-changed", this.getTheme());
```

::: devtools/client/framework/devtools.js:314
(Diff revision 1)
>        theme = this._themes.get(theme);
>      } else {
>        themeId = theme.id;
>      }
>  
> -    let currTheme = Services.prefs.getCharPref("devtools.theme");
> +    let currTheme = Services.prefs.getCharPref(THEME_PREF);

```
let currTheme = this.getTheme();
```

in my opinion it would be better to always refer to the the `getTheme` method instead of retrieving the value of the preference (for consistency, and to better "abstract" where the current theme is actually stored).

(it could also be a getter , but it is a minor detail and we can ask ochameau if we prefer it to be a getter during his final review).

::: devtools/client/framework/devtools.js:327
(Diff revision 1)
>      // is being removed.
>      // Ignore shutdown since addons get disabled during that time.
>      if (!Services.startup.shuttingDown &&
>          !isCoreTheme &&
>          theme.id == currTheme) {
> -      Services.prefs.setCharPref("devtools.theme", "light");
> +      Services.prefs.setCharPref(THEME_PREF, "light");

How about defining also a `setTheme` method and to use it here instead of setting the preference directly?

::: devtools/client/framework/gDevTools.jsm:71
(Diff revision 1)
> -  // Used by main.js and test
> -  "getToolDefinitionArray",
> +  // Used by WebExtensions devtools API
> +  "getTheme",
> -  "getThemeDefinitionArray",

Why the `getToolDefinitionArray` and `getThemeDefinitionArray` methods looks removed by mistake in this diff?

or is it related to some weird rendering problem of mozreview? (or maybe I'm missing something)

Comment 18

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8865987 [details]
Bug 1349896 - Part 1: Expose devtools theme via gDevTools,

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

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8865987 [details]
Bug 1349896 - Part 1: Expose devtools theme via gDevTools,

https://reviewboard.mozilla.org/r/137588/#review141082

Thanks Bob,
follows the only single comment that I still have before moving this patch to a final review from Alex.

::: devtools/client/framework/devtools.js:47
(Diff revision 2)
>  
>    EventEmitter.decorate(this);
>  
> +  // Listen for changes to the theme pref.
> +  this._onThemeChanged = this._onThemeChanged.bind(this);
> +  Services.prefs.addObserver(THEME_PREF, this._onThemeChanged);

it would be nice if we could move the `THEME_PREF` constant and the two `Services.prefs.addObserver` `removeObserver` calls into the `shared/theme` module, so that the code that uses the `shared/theme` module doesn't need to now where the preference is stored.

how that sounds to you?

Comment 20

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8865987 [details]
Bug 1349896 - Part 1: Expose devtools theme via gDevTools,

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

Comment 21

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8865987 [details]
Bug 1349896 - Part 1: Expose devtools theme via gDevTools,

Thanks Bob!
I think that this is ready for its review from Alex.

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8865987 [details]
Bug 1349896 - Part 1: Expose devtools theme via gDevTools,

https://reviewboard.mozilla.org/r/137588/#review141520

Thanks, that looks good, assuming try is green.

Comment 23

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

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

Comment 24

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

Luca, this is not ready to be reviewed, but I do need some help/feedback. My current patch has a bunch of debugging statements in it, and I seem to be able to observe the theme-changed event from ext-devtools-panels.js, but when I try to send a message to the child process that message doesn't seem to be received. See the patch for more details of what I've done and what I mean.

Comment 25

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

https://reviewboard.mozilla.org/r/138504/#review141722

Hi Bob,
while looking at this patch, I'm thinking that, given that the current devtools theme can only be changed globally and not on a per-toolbox basis, it can be reasonable to subscribe the listener using `Services.cpmm.addMessageListener` (instead of the message manager of the particular devtools extension context) and them broadcast the changed theme name from the main process using `Services.ppmm.broadcastAsyncMessage` to the processes that are listening for that event name.
(e.g. this is the approach that we already use to send some other "global" events from the webextension internals running in the main process:
- http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionTestCommon.jsm#342
to the webextensions internals running in the content and extension child processes:
- http://searchfox.org/mozilla-central/source/toolkit/components/extensions/extension-process-script.js#599
)

how that sounds to you?

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

Clearing the feedback flag on this patch, because we (Bob and I) already looked into this version of the patch and we agreed on the changes to apply for its next version.

Comment 27

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8865987 [details]
Bug 1349896 - Part 1: Expose devtools theme via gDevTools,

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

Comment 28

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

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

Comment 29

[Bob Silverberg [:bsilverberg]]

Ugh, MozReview removed the flags from part 1. It's been r+'d by both Luca and Alex, so no need for anyone to review that again.

Comment 30

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

https://reviewboard.mozilla.org/r/138504/#review142078

Hi Bob,
Thanks for these updates:
I looked into the updated version and my feeling is that patch needs some additional changes, mainly to move the implementation out of the ChildDevToolsPanel, because this API should not be restricted to the existence of a devtools panel, nor provided on the `panel` API object.

Follows some detailed comments about the changes that I think that are still needed on the API implementation part.

::: browser/components/extensions/ext-c-devtools-panels.js:36
(Diff revisions 1 - 2)
>      this._panelContext = null;
>  
>      this.mm = context.messageManager;
>      this.mm.addMessageListener("Extension:DevToolsPanelShown", this);
>      this.mm.addMessageListener("Extension:DevToolsPanelHidden", this);
> -    this.mm.addMessageListener("Extension:DevToolsThemeChanged", this);
> +    Services.cpmm.addMessageListener("Extension:DevToolsThemeChanged", this);

By subscribing the "Extension:DevToolsThemeChanged" here, the themeName value cached on the child extension process side is not going to be updated if the webextension does not create a panel.

Basically we have to keep the themeName value in sync with the current value in the main process, even if there is no `onThemeNameChanged` listeners and no panels, and so we have to always subscribe a listener to this message manager event if there is a devtools extension context around.

Also, we can subscribe just one event listener "per process" (which means that there will be just one message manager listener, because the extension will run in the main process or in a single child extension process).

To achieve it we can abstract the listener into a component that:

- ensures that the message manager event is automatically subscribed only once (e.g. when the first devtools extension context has been created) and automatically unsubscribed (e.g. when all the devtools extension contexts have been destroyed)

- provides the shared cached value of themeName
  (initialized by the first devtools extension context and updated 
  when the component handles the "Extension:DevToolsThemeChanged" messages)
  and the themeName getter implementation returns the shared cached value by
  retrieving it from this component
  
- uses EventEmitter to allow the devtools context to subscribe a listener
  (subscribed only if there is any webextension code that is subscribing
  a listener to panels.onThemeNameChanged)

- fires an EventEmitter event to all the subscribed "devtools context" 
  listeners subscribed to it when the themeName has been changed
  (so that they can fire the event to the callback subscribed by the
  webextension code)

::: browser/components/extensions/ext-devtools.js:298
(Diff revisions 1 - 2)
>        devtoolsPageDefinition.shutdownForTarget(target);
>      }
>    });
> +
> +  // Listen for changes to the devtools theme.
> +  gDevTools.on("theme-changed", (evt, themeName) => {

This should be subscribed only once (when the first devtools page is going to be created)
and unsubscribed when all the devtools pages have been destroyed.

::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js:49
(Diff revisions 1 - 2)
>          result.panelHidden++;
>  
>          browser.test.sendMessage("devtools_panel_hidden", result);
>        });
>  
> +      panel.onThemeChanged.addListener(themeName => {

Even if the API event `onThemeChanged` is not part of the API that Chrome is going to provide, we should provide it in the `devtools.panels` API namespace as `themeName`, instead of on the `panel` API object (especially given that both `themeName` and `onThemeChanged` should be accessible from the devtools_page even if no devtools_panel has been created).

Comment 31

[Julian Descottes [:jdescottes]]

Just some comments on the current approach, related to DevTools becoming an addon (& moving out of mozilla-central).

For this, we aim to migrate call sites currently using gDevTools.jsm to use a DevTools shim (developed in Bug 1356244). The shim will remain available in mozilla-central even after DevTools have been removed, and is really one of the the only DevTools dependency that should be used inside of mc.

With the current approach for this bug, to support getTheme after DevTools have been removed from mozilla-central, we have to add a getTheme method to the shim. This getTheme would throw if DevTools are not installed, and forward to gDevTools if they are installed. Looking at the part2 patch, I think you only call getTheme after receiving a toolbox, right? So I guess this could work in theory.

However I would prefer the shim to only have methods with useful implementations if DevTools are not installed. This is the case for on, off, registerTool, registerTheme etc... but not getTheme. 

Today, web-extensions only use gDevTools.on, which is supported by the shim. Once a toolbox opens and a "toolbox-created" event is fired, web-extension code then relies on the toolbox object provided with the event. Maybe we can do something similar here. Either expose getTheme on the toolbox object. Or maybe we should also send gDevTools together with the toolbox when firing toolbox-created?

Note that none of this has to be decided in the scope of this bug. I will submit new bugs to migrate web-extension's code to the DevTools shim, and we can take care of this then. I mainly wanted to write down my current thoughts about this topic (sorry for polluting this bug!).

Comment 32

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Julian, thanks for pointing it out.

I can confirm that the devtools.panels.themeName API property cannot be accessed until a toolbox has been created (because the devtools API is going to be available only in the devtools page and devtools panel extension contexts, and these contexts only exists after that their related toolbox has been created), and so the WebExtension API implementation is not going to use gDevTools.getTheme until the real devtools internals have been installed and loaded.

Our initial choice was to add getTheme (and the "theme changed" event) on the toolbox object, but Alex has pointed out that the current devtools theme is global and not specific to a particular toolbox, and so we have moved this methods into the gDevTools object (e.g. Comment 13). 

Nevertheless there is still time to re-evaluate it and so I'm going to set a needinfo assigned to Alex to hear what he thinks about it.

Comment 33

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8865987 [details]
Bug 1349896 - Part 1: Expose devtools theme via gDevTools,

re-newed my f+ on this patch.

Comment 34

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #31)
>
> Note that none of this has to be decided in the scope of this bug.

+1, this is going to work given the current assumptions we have about webext codebase
(it is triggered after toolbox-created is fired)

We can only improve this once the shim is landed, so let's just move forward with this patch.

> Today, web-extensions only use gDevTools.on, which is supported by the shim.
> Once a toolbox opens and a "toolbox-created" event is fired, web-extension
> code then relies on the toolbox object provided with the event. Maybe we can
> do something similar here. 
> Either expose getTheme on the toolbox object. 

-1 as Lucas said, this isn't toolbox specific.

> Or maybe we should also send gDevTools together with the toolbox when firing
> toolbox-created?

+1 Also, instead of passing around raw object from devtools (gDevTools, Toolbox),
it may be worth scaling down the API surface being exposed.
So expose, via the shim, only the very minimal set of API really needed.
That, to ease maintaining Firefox <=> addon compatiblity across versions.
I think at some point it will help exposing the addon version via the shim.

But again, this is unrelated to this bug/patch, unless it doesn't land shortly, in which case the shim is going to be available.

Comment 35

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

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

Comment 36

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8865987 [details]
Bug 1349896 - Part 1: Expose devtools theme via gDevTools,

https://reviewboard.mozilla.org/r/137260/#review143040

::: browser/components/extensions/ext-c-devtools-panels.js:191
(Diff revision 7)
>                                                      {cloneFunctions: true});
>                return devtoolsPanelAPI;
>              });
>            },
> +          get themeName() {
> +            return this.themeChangeObserver.themeName;

in this getter `this` is not the class instance and so this.themeChangeObserver is not going to return the ThemeChangeObserver instance created above.

We can capture the value of this to make it work in various ways, e.g. by creating an arrow function as an helper:

```
this.themeChangeObserver = new ThemeChangeObserver(context, context.devtoolsToolboxInfo.themeName);
 
const getThemeName = () => this.themeChangeObserver.themeName;
```

And then in the API getter:

```
  ...
  get themeName() {
    return getThemeName();
  },
  ...
```

Comment 37

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

https://reviewboard.mozilla.org/r/138504/#review143062

::: browser/components/extensions/ext-c-devtools-panels.js:172
(Diff revision 3)
>  }
>  
>  this.devtools_panels = class extends ExtensionAPI {
>    getAPI(context) {
> +    dump(`\n----- Creating a ThemeChangeObserver...`);
> +    this.themeChangeObserver = new ThemeChangeObserver(context, context.devtoolsToolboxInfo.themeName);

In this patch we are currently creating an instance of the `ThemeChangeObserver` in the `devtools.panels` API implementation, and I feel that this has at least two issues:

- we are creating one instance of the `ThemeChangeObserver` per extension context (while we could share a single instance with all the extension contexts that are running in the same process)

- we are going to start listening for the `"Extension:DevToolsThemeChanged"` message manager events only if the `devtools.panels` API has been accessed (because of the lazy loading of the webextensions APIs, and this can lead to a wrong behavior, e.g. if the `devtools.panels` API is accessed in a callback, then it is completely possible that the value in the `context.devtoolsToolboxInfo.themeName` property is not the current value of the devtools theme in the main process, also we were not listening to the `"Extension:DevToolsThemeChanged"` message manager events yet and so we never received the updated values)

To solve this issue we could move the ThemeChangeObserver class into a jsm module (defined at `toolkit/` level instead of `browser/` level) and initialize an instance of the ThemeChangeObserver as soon as we create the first devtools context (even if the devtools API are never been accessed yet, so that we can be sure that the ThemeChangeObserver instance has the opportunity to keep the cached themeName value updated), e.g. around here http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionPageChild.jsm#243-268

We can then export an helper function from the new jsm which returns the shared instance to the API implementation, something like:

```
XPCOMUtils.defineLazyModuleGetter(this, "ExtensionDevToolsUtils",
                                  "resource://gre/modules/ExtensionDevToolsUtils.jsm");

...
this.devtools_panels = class extends ExtensionAPI {
  getAPI(context) {
    const themeChangeObserver = ExtensionDevToolsUtils.getThemeChangeObserver();
    ...
  }
  ...
```

How that sounds to you?

Comment 38

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8865987 [details]
Bug 1349896 - Part 1: Expose devtools theme via gDevTools,

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

Comment 39

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

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

Comment 40

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

https://reviewboard.mozilla.org/r/138504/#review143520

Hi Bob,
thanks for these updates, this looks pretty near!

Follows more detailed comments related to some minor tweaks and a proposed approach for the last pending part of the patch (keeping track of the devtools extension child contexts and automatically
destroy the ThemeChangeObserver instance when all the extension child contexts has been unloaded).

::: toolkit/components/extensions/ExtensionDevToolsUtils.jsm:47
(Diff revision 4)
> +  receiveMessage({name, data}) {
> +    this.onThemeChanged(data.themeName);
> +  }
> +
> +  // TODO: I'm not sure what I need to do to make this happen when it needs to.
> +  close() {

I would name this `destroy` (in the webextensions internals `close` is more commonly used for the "extension contexts").

(More details about when it should be called in one of the following review comments).

::: toolkit/components/extensions/ExtensionPageChild.jsm:36
(Diff revision 4)
>  const CATEGORY_EXTENSION_SCRIPTS_ADDON = "webextension-scripts-addon";
>  const CATEGORY_EXTENSION_SCRIPTS_DEVTOOLS = "webextension-scripts-devtools";
>  
>  Cu.import("resource://gre/modules/ExtensionCommon.jsm");
>  Cu.import("resource://gre/modules/ExtensionChild.jsm");
> +Cu.import("resource://gre/modules/ExtensionDevToolsUtils.jsm");

I think that we can lazily import this jsm module, so that we only load it if there is at least an extension that is going to need it (e.g. at least one of the enabled webextension has a devtools page).

::: toolkit/components/extensions/ExtensionPageChild.jsm:265
(Diff revision 4)
>     */
>    constructor(extension, params) {
>      super(extension, Object.assign(params, {envType: "devtools_child"}));
>  
>      this.devtoolsToolboxInfo = params.devtoolsToolboxInfo;
> +    initThemeChangeObserver(params.devtoolsToolboxInfo.themeName);

I think that we should also pass `this` (I mean the reference to the newly created devtools context that we are creating here) to the `initThemeChangeObserver` helper, so that we can keep track of the top-level "devtools extension child context" that have been created, and then destroy the lazily created `ThemeChangeObserver` instance when these "devtools extension child contexts" are not around anymore.

e.g. something like

```
class ThemeChangeObserver extends EventEmitter {
  constructor(themeName, onDestroyed) {
    ...
    this.onDestroyed = onDestroyed;
    this.contexts = new Set();
  }
  
  watchContextClose(context) {
    if (this.contexts.has(context)) {
      return;
    }
    
    context.onClose({
      close: () => this.onContextClosed(context),
    });
    
    this.contexts.add(context);
  }
  
  onContextClosed(context) {
    this.contexts.delete(context);
    
    if (this.contexts.size === 0) {
      this.destroy();
    }
  }
  
  destroy() {
    ...
    this.onDestroyed();
  }
  
  ...
}

...
  initThemeChangeObserver(themeName, context) {
    if (!themeChangeObserver) {
      themeChangeObserver = new themeChangeObserver(
        themeName, 
        () => themeChangeObserver = null
      );
    }
    
    themeChangeObserver.watchContextClose(context);
  }
```

Comment 41

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

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

Comment 42

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

https://reviewboard.mozilla.org/r/138504/#review144106

Hi Bob,
Thanks for the new round of updates, imho it now looks pretty near to be ready for the final review stage.

Follows some additional review comments (most of them are actually small nits and/or optional, only a couple of the following review comments are related to mandatory changes or missing pieces).

::: browser/components/extensions/ext-devtools-panels.js:9
(Diff revision 5)
> +XPCOMUtils.defineLazyModuleGetter(this, "gDevTools",
> +                                  "resource://devtools/client/framework/gDevTools.jsm");

`gDevTools` is currently unused in the ext-devtools-panel.js file and we can remove it (it is a leftover of the previous version of this patch).

::: browser/components/extensions/ext-devtools.js:218
(Diff revision 5)
>      const devtoolsPage = new DevToolsPage(this.extension, {
>        toolbox, url: this.url, devToolsPageDefinition: this,
>      });
> +
> +    // If this is the first DevToolsPage, subscribe to the theme-changed event
> +    if (!this.devtoolsPageForTarget.size) {

I would prefer a more explicit `if(this.devtoolsPAgeForTarget.size === 0) {...}`
here and at line 238 (but I'm also completely ok if we decide to keep it this way and/or defer this decision to the final review from Kris).

::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js:57
(Diff revision 5)
> +        browser.test.sendMessage("devtools_theme_changed", themeName);
> +        browser.test.sendMessage("current_theme", browser.devtools.panels.themeName);
> +      });
> +
>        browser.test.sendMessage("devtools_panel_created");
> +      browser.test.sendMessage("initial_theme", browser.devtools.panels.themeName);

Currently we are only testing the `devtools.panels.themeName` and the `onThemeChanged` event from a devtools page,
we should also test it from a devtools panel (and I think that it would be good to also test it from a test webextension with a devtools page and no devtools panels, just to be sure that we don't regress on this scenario in the future).

::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js:110
(Diff revision 5)
> +  async function switchTheme(toolbox, theme) {
> +    await gDevTools.showToolbox(target, "options");
> +    let panelWin = toolbox.getCurrentPanel().panelWin;
> +    let doc = panelWin.frameElement.contentDocument;
> +    let themeBox = doc.getElementById("devtools-theme-box");
> +    let testThemeOption = themeBox.querySelector(
> +      `input[type=radio][value=${theme}]`);
> +    let onThemeSwitchComplete = new Promise(done => {
> +      let onClick = (key, event) => {
> +        panelWin.removeEventListener("theme-switch-complete", onClick);
> +        done();
> +      };
> +      panelWin.addEventListener("theme-switch-complete", onClick);
> +    });
> +
> +    // Select test theme.
> +    testThemeOption.click();
> +    await onThemeSwitchComplete;
> +  }

This helper could be smaller (and probably also a bit faster) if we use the underlying APIs here instead of switching panels and interact with the devtools UI, e.g. something like the following should work:

```
function switchDevToolsTheme(theme) {
  const waitThemeChanged = new Promise(resolve => gDevTools.once("theme-changed", resolve));
  Services.prefs.setCharPref("devtools.theme", theme);
  return waitThemeChanged;
}
```

I would also move out of the test function these test helper (only for readability reasons).

::: toolkit/components/extensions/ExtensionDevToolsUtils.jsm:7
(Diff revision 5)
> + * 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/. */
> +
> +/**
> + * @fileOverview
> + * This module contains utilities for in used for interacting with DevTools.

Nit: s/for in//

It would be probably helpful to specify (in the comment, and maybe in the jsm file name?) that this module is meant to be used only in the child process.

I'm completely ok with deferring this decision during the final review from Kris, given that I recall that you were unsure if you prefer these helpers to be defined in a new jsm module or by reusing one of the existent ones.

::: toolkit/components/extensions/ExtensionDevToolsUtils.jsm:22
(Diff revision 5)
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "EventEmitter",
> +                                  "resource://gre/modules/EventEmitter.jsm");
> +
> +let themeChangeObserver;

Nit: let's add an inline comment to explain the role of this "not inizialized" `themeChangeObserver` variable.

::: toolkit/components/extensions/ExtensionDevToolsUtils.jsm:25
(Diff revision 5)
> +                                  "resource://gre/modules/EventEmitter.jsm");
> +
> +let themeChangeObserver;
> +
> +/**
> + * An observer that watches for changes to the devtools theme and provides

Nit: we can also mention that this object also cache the updated themeName value.

::: toolkit/components/extensions/ExtensionDevToolsUtils.jsm:41
(Diff revision 5)
> +    Services.cpmm.addMessageListener("Extension:DevToolsThemeChanged", this);
> +  }
> +
> +  addContext(context) {
> +    if (this.contexts.has(context)) {
> +      return;

this is not supposed to be called on a context that has been already seen, it would be probable better to raise an exception if it happens to be called twice for the same context.

::: toolkit/components/extensions/ExtensionDevToolsUtils.jsm:66
(Diff revision 5)
> +    this.themeName = themeName;
> +    this.emit("themeChanged", themeName);
> +  }
> +
> +  receiveMessage({name, data}) {
> +    this.onThemeChanged(data.themeName);

it would not make any difference currently, but just for safety in case we start to listen to other message manager messages, we could check that `name` is the message that we expect and filter out any unexpected messages.

::: toolkit/components/extensions/ExtensionDevToolsUtils.jsm:71
(Diff revision 5)
> +    this.onThemeChanged(data.themeName);
> +  }
> +
> +  destroy() {
> +    Services.cpmm.removeMessageListener("Extension:DevToolsThemeChanged", this);
> +    this.onDestroyed();

in the destroy method it would be probably a good to also explicitly nullify `this.onDestroyed` and also "clear / nullify" the `this.contexts` Set.

::: toolkit/components/extensions/ExtensionDevToolsUtils.jsm:78
(Diff revision 5)
> +}
> +
> +this.ExtensionDevToolsUtils = {
> +  /**
> +   * Creates an cached instance of the ThemeChangeObserver class and
> +   * initializes it with the current themeName.

we can also mention that the lazily created instance is then automatically destroyed when all the "extension contexts" passed to this function have been unloaded.

::: toolkit/components/extensions/ExtensionPageChild.jsm:57
(Diff revision 5)
> +const {
> +  initThemeChangeObserver,
> +} = ExtensionDevToolsUtils;

Given that `ExtensionDevToolsUtils` has been declared as a lazy getter (to prevent it from being loaded when is not going to be used), we should not extract the `initThemeChangeObserver` method here (because it is going to always load the "ExtensionDevTools.jsm" by accessing the lazy getter).

Comment 43

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

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

Comment 44

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

https://reviewboard.mozilla.org/r/138504/#review145560

Thanks Bob, I think that this is now ready for the final review stage.

(I've added a non-blocking comment about the preference set/reset in the test and, as briefly discussed over IRC, I feel that this test should be splitted at some point, but we can evaluate if we can defer it in a follow up as part of the final review)

::: browser/components/extensions/test/browser/browser_ext_devtools_panel.js:74
(Diff revisions 5 - 6)
> +  registerCleanupFunction(function() {
> +    Preferences.reset(DEVTOOLS_THEME_PREF);
> +  });
> +
> +  // Ensure that the initial value of the devtools theme is "light".
> +  Preferences.set(DEVTOOLS_THEME_PREF, "light");

I'm wondering if it would be better to use the `SpecialPowers.pushPrefEnv` helper here (but I'm not marking it as a blocking issue because we are still ensuring that the preference is reset once the test exit using `registerCleanupFunction`).

Comment 45

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8865987 [details]
Bug 1349896 - Part 1: Expose devtools theme via gDevTools,

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

Comment 46

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

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

Comment 47

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

https://reviewboard.mozilla.org/r/138504/#review145560

> I'm wondering if it would be better to use the `SpecialPowers.pushPrefEnv` helper here (but I'm not marking it as a blocking issue because we are still ensuring that the preference is reset once the test exit using `registerCleanupFunction`).

Thanks Luca. I've fixed that in the latest commit.

Comment 48

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

(In reply to Bob Silverberg [:bsilverberg] from comment #47)
> Comment on attachment 8866889 [details]
> Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl
> 
> https://reviewboard.mozilla.org/r/138504/#review145560
> 
> > I'm wondering if it would be better to use the `SpecialPowers.pushPrefEnv` helper here (but I'm not marking it as a blocking issue because we are still ensuring that the preference is reset once the test exit using `registerCleanupFunction`).
> 
> Thanks Luca. I've fixed that in the latest commit.

Thanks Bob.

f+ re-newed

Comment 49

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

Shane, when I met with Andy today he said that he is satisfied that an r+ from Luca for a devtools patch from me is sufficient to land, but we still officially need an r+ from a peer. Would you mind taking a quick look at this (or longer, if you prefer) and let me know if you're okay giving it your r+?

Comment 50

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

https://reviewboard.mozilla.org/r/138504/#review146178

This looks cozy, but relying on rpl having reviewed details.

ExtensionChildDevToolsUtils.jsm looks generalizable enough for use in a browser.themes api that provides the same thing for the browser theme.  I supose in a followup we could change all this, but it seems we could call this ExtensionChildThemeUtils.  Have you given any thought to that overlap?

Comment 51

[Bob Silverberg [:bsilverberg]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #50)
> Comment on attachment 8866889 [details]
> Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl
> 
> https://reviewboard.mozilla.org/r/138504/#review146178
> 
> This looks cozy, but relying on rpl having reviewed details.
> 
> ExtensionChildDevToolsUtils.jsm looks generalizable enough for use in a
> browser.themes api that provides the same thing for the browser theme.  I
> supose in a followup we could change all this, but it seems we could call
> this ExtensionChildThemeUtils.  Have you given any thought to that overlap?

Thanks for the review, Shane.

This is a good point, and I think you're right that most of the code in there could be reused for observing and notifying about different types of theme changes, although I'm not sure that the contexts thing would apply. Part of the reason for all of this is that we don't need to create the observer, or subscribe to any events, until a devtools context is created. If we were to want to do something similar for an API that is available to all extensions (not just devtools contexts), I think it would look and work differently. So I'm not really sure whether renaming this now makes sense or not.

Luca, what do you think?

Comment 52

[Luca Greco [:rpl] [:luca] [:lgreco]]

I agree that, speaking theoretically, if we will end up to have a browser.themes API, it would be probably reasonable to reuse a JSM module for observing in the child process for all the theme changes events that are happening in the main process, and I think that it would be reasonable for the two APIs to share the same JSM (as long as we can keep the behavior of "not registering any listener for the devtools theme changed" event until actually needed).

I'm just not sure that it would be really useful to just rename the JSM without knowing more about what the browser.themes API would actually provide, mostly because with a clearer idea of such additional API we would be probably able to share much more of the JSM file (but having said that, I would not oppose to just rename it and keep it's implementation unchanged in the meantime).

Comment 53

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8866889 [details]
Bug 1349896 - Part 2: Implement devtools.panels.themeName API property, f?rpl

https://reviewboard.mozilla.org/r/138504/#review146178

I agree with Luca that although this might be called for in the future, it doesn't seem like a great idea to rename this now, so I'm going to land it as is.

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8e547f09534
Part 1: Expose devtools theme via gDevTools, r=ochameau
https://hg.mozilla.org/integration/autoland/rev/4ab841241d25
Part 2: Implement devtools.panels.themeName API property, f?rpl r=mixedpuppy

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8e547f09534
https://hg.mozilla.org/mozilla-central/rev/4ab841241d25

Comment 56

[Will Bamberg [:wbamberg]]

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.panels/themeName
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.panels/onThemeChanged

I wasn't sure whether theme names are part of the API and should be documented as such. I think they should be, otherwise it's hard to know how to use this API.

Please let me know if you think this looks OK.

Comment 57

[Bryan Clark (DevTools PM) [:clarkbw]]

Chrome does have the `themeName` property as well: https://developer.chrome.com/extensions/devtools_panels#property-themeName

Comment 58

[Bob Silverberg [:bsilverberg]]

Looks good, thanks Will, although I echo Bryan's comment. Chrome does support themeName, although not the event.

Comment 59

[Will Bamberg [:wbamberg]]

I did see that, but the Chrome docs say "Not fully implemented. You must build Chromium from source to try this API." - which I don't consider "support", for the purposes of add-on developers. Is this note not correct?

Comment 60

[Bryan Clark (DevTools PM) [:clarkbw]]

I don't think those docs there are correct.  You can see in PRs like this one ( https://github.com/facebook/react-devtools/pull/802 ) that Chrome does support the API without building from source.

Comment 61

[Will Bamberg [:wbamberg]]

OK, thanks Bryan, I'll update the compat information.