Closed Bug 1038562 Opened 6 years ago Closed 6 years ago

New API: Implementation of a new theme

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: Honza, Assigned: Honza)

Details

(Whiteboard: [firebug-p1])

Attachments

(1 file, 6 obsolete files)

Some extensions might want to implement a new theme for native developer tools and there should be API making this task simple.


Related use cases:

1) An extension introduces a simple stylesheet with rules that should be applied across the entire toolbox/panels. Such stylesheet should be loaded into every iframe within the toolbox.

The extensions registers such stylesheet and the framework ensures that it's loaded into every iframe automatically.

gDevTools.registerStylesheet(url);


2) An extension introduces complex new theme that is composed from a set of stylesheets (one per existing panel + one for code mirror theme, etc.) and it could have negative impact on performance to load all those stylesheets into every iframe.

Such extension needs to be able to hook into panel/iframe initialization process (e.g. handling "-ready" event fired by panels) and add custom stylesheet into iframes manually.


3) An extension wants to create an new theme-option within the Options panel. It's already possible to append a new XUL RADIO element into the existing RADIOGROUP (devtools-theme-box) and handle selection (or preference) changes, but it would be nice to have an extra API that do all this automatically.

gDevTools.addNewTheme({
  label: label,
  id: id,
  onApply: callback,
  ...
})


4) An extension needs to ensure that new theme is properly applied when the user selects it. Applying themes is done through class names (e.g. theme-light, theme-dark) created on document elements within iframes.

Creation and removal of those class names should be automated by the framework and all necessary data (e.g. the class name itself) should be provided by extensions through the addNewTheme API (see #3)

Note that extensions can built new theme on top of an existing theme and so, there might be more class names set for a custom theme (e.g. theme-light + theme-firebug).

Thoughts?

Honza
Whiteboard: [firebug-p1]
Summary: Bug 1036949 - New API: Implementation of a new theme → New API: Implementation of a new theme
add #4) There is already a theme-switching.js logic that is included into every iframe.
http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/theme-switching.js

This file might be utilized when implementing new API.

Honza
So the use case for (1) seems like it could be handy for little tweaks, but I would expect in the case for a bigger extension or extensions that are just custom themes we would want to focus on (3) and (4) - allowing the user to opt into the theme changes.

This kind of brings up a bigger question of if the entire functionality of the extension should be toggle-able when the "theme" is switched off.  So if I install an extension but switch to 'dark' theme, then should the extension politely remove itself from the running toolbox?

Perhaps 3 and 4 could be combined into a single API:

gDevTools.addNewTheme({
  label: "Firebug",
  id: "firebug",
  urls: ["chrome://browser/skin/devtools/light-theme.css", "path/to/firebug.css"], 
  className: "theme-light theme-firebug",
  onApply: callback,
  onUnapply: callback, // Maybe have this if we want to remove functionality when the theme is removed
  ...
});

Our light and dark themes could be added via:

gDevTools.addNewTheme({
  label: "Light",
  id: "light",
  urls: ["chrome://browser/skin/devtools/light-theme.css"], 
  className: "theme-light"
});

gDevTools.addNewTheme({
  label: "Dark",
  id: "dark",
  urls: ["chrome://browser/skin/devtools/dark-theme.css"], 
  className: "theme-dark"
});
(In reply to Jan Honza Odvarko [:Honza] from comment #1)
> add #4) There is already a theme-switching.js logic that is included into
> every iframe.
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/theme-
> switching.js
> 
> This file might be utilized when implementing new API.

Yes, this is where I would target any changes for use cases 1, 3, or 4 since it is reliably inserted into any frame that we add, including future ones.  I think use case 2 is too specific to fit into this file, and could be handled on a tool by tool basis.
OS: Windows 7 → All
Hardware: x86_64 → All
What I've seen from the fledgling chrome tools theme scene is:

1) defining new colors for the tools ui generally
2) including a codemirror theme for editors that matches this.

Some questions:
* can we let people provide an entire codemirror theme?
* can we instead have people define a short-list of colors and then apply those behind the scenes to codemirror?

When we've talked about this before, the discussion was mainly around using the new / almost here css variables standard that could allow users to provide a short list of colors that magically get applied. This is what I think of as 'light themeing' and I think any forays into deeper css changes might have diminishing returns?

Chrome themes seem to be quite complex, btw: https://github.com/mauricecruz/chrome-devtools-zerodarkmatrix-theme/blob/master/Custom.css

...I would not want our implementation to require developers to write ~2500 lines of css.
Attached patch bug1038562.patch (obsolete) — Splinter Review
(In reply to Brian Grinstead [:bgrins] from comment #2)
> So the use case for (1) seems like it could be handy for little tweaks, but
> I would expect in the case for a bigger extension or extensions that are
> just custom themes we would want to focus on (3) and (4) - allowing the user
> to opt into the theme changes.
Yep, agree

> This kind of brings up a bigger question of if the entire functionality of
> the extension should be toggle-able when the "theme" is switched off.  So if
> I install an extension but switch to 'dark' theme, then should the extension
> politely remove itself from the running toolbox?
Yes, I have been also thinking about this and I think that it's mostly up to
extensions what they do. We might want to publish some recommended etiquette,
but it's pretty much all we can do.

> Perhaps 3 and 4 could be combined into a single API:
> 
> gDevTools.addNewTheme({
>   label: "Firebug",
>   id: "firebug",
>   urls: ["chrome://browser/skin/devtools/light-theme.css",
> "path/to/firebug.css"], 
>   className: "theme-light theme-firebug",
>   onApply: callback,
>   onUnapply: callback, // Maybe have this if we want to remove functionality
> when the theme is removed
>   ...
> });
Yes and I like onUnapply

> Our light and dark themes could be added via:
> 
> gDevTools.addNewTheme({
>   label: "Light",
>   id: "light",
>   urls: ["chrome://browser/skin/devtools/light-theme.css"], 
>   className: "theme-light"
> });
> 
> gDevTools.addNewTheme({
>   label: "Dark",
>   id: "dark",
>   urls: ["chrome://browser/skin/devtools/dark-theme.css"], 
>   className: "theme-dark"
> });
Precisely

> I think use case 2 is too specific to fit into this file,
> and could be handled on a tool by tool basis.
OK, I think that extensions need simple access to any created iframe
1) immediately when it's created and
2) when the iframe is loaded

.. but you are right we can solve this step by step in another issues.


I am attaching a patch implementing the suggested API. Some comments are yet needed and we also need to make sure that a default theme is always set if the previous selected theme has been uninstalled and is not available any more.

I have been testing this with Firebug.

Related Firebug patch here:
https://github.com/firebug/firebug.next/commit/fc371d1ad7436551cf42f74809053d1197ae0f0d

All works just fine for Firebug!

Honza
(In reply to Jeff Griffiths (:canuckistani) from comment #4)
> What I've seen from the fledgling chrome tools theme scene is:
> 
> 1) defining new colors for the tools ui generally
> 2) including a codemirror theme for editors that matches this.
Both should be possible using suggested API (the API are basically about allowing extensions to specify stylesheets that should be automatically loaded).


> Some questions:
> * can we let people provide an entire codemirror theme?
> * can we instead have people define a short-list of colors and then apply
> those behind the scenes to codemirror?
I would personally vote for the first option. Styling CM is simple already and there are existing themes that can be directly used (I guess). Or we can support both at some point...


> When we've talked about this before, the discussion was mainly around using
> the new / almost here css variables standard that could allow users to
> provide a short list of colors that magically get applied. This is what I
> think of as 'light themeing' and I think any forays into deeper css changes
> might have diminishing returns?
I really like CSS variables and we should definitely use it. It goes inline with
the suggested API since it's more about properly specifying the variables in built-in
themes, so extensions can override it.

Honza
Comment on attachment 8456926 [details] [diff] [review]
bug1038562.patch

Review of attachment 8456926 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good! Just one main note about where we are emitting the events

::: browser/devtools/framework/gDevTools.jsm
@@ +236,5 @@
> +   */
> +  registerTheme: function DT_registerTheme(themeDefinition) {
> +    let themeId = themeDefinition.id;
> +
> +    if (!themeId) {

Should also check to make sure the id is not already in this._themes

::: browser/devtools/framework/toolbox-options.xul
@@ -32,5 @@
>          <radiogroup id="devtools-theme-box"
>                      class="options-groupbox"
>                      data-pref="devtools.theme"
>                      orient="horizontal">
> -          <radio value="light" label="&options.lightTheme.label;"/>

Can remove the labels from browser/locales/en-US/chrome/browser/devtools/toolbox.dtd

::: browser/devtools/shared/theme-switching.js
@@ +76,5 @@
> +      for (let name of oldThemeDef.classList) {
> +        documentElement.classList.remove(name);
> +      }
> +
> +      if (oldThemeDef.onUnapply)

This will cause the onUnapply / onApply to be fired many times (one for each frame).  It would be nice if we could actually only send these events once.

Maybe we could get rid of the handlePrefChange listener here and instead have gDevTools emit the theme-switched event.  gDevTools would also fire off onApply/onUnapply accordingly in this case.

We would still want this to call switchTheme on load and on gDevTools.on("theme-switched"), but this file would not be in charge of emitting anything
Does it make sense to change the theme selection radio options into a menulist (like the one in default color unit) ?
(In reply to Girish Sharma [:Optimizer] from comment #8)
> Does it make sense to change the theme selection radio options into a
> menulist (like the one in default color unit) ?

Maybe at some point, but I do like having only one click to switch themes, plus you can see all of the options at one glance with radios.  I think if we make sure that it wraps well it would be good enough until if/when custom themes became popular or we expanded the default choices.
(In reply to Brian Grinstead [:bgrins] from comment #9)
> (In reply to Girish Sharma [:Optimizer] from comment #8)
> > Does it make sense to change the theme selection radio options into a
> > menulist (like the one in default color unit) ?
> 
> Maybe at some point, but I do like having only one click to switch themes,
> plus you can see all of the options at one glance with radios.  I think if
> we make sure that it wraps well it would be good enough until if/when custom
> themes became popular or we expanded the default choices.

Agreed, for 3-4 theme options, it does not make sense, but if in future we have like 20+ options on web, and people start creating addons to add more and more, then radios will start looking ugly.
(In reply to Girish Sharma [:Optimizer] from comment #10)
...
> Agreed, for 3-4 theme options, it does not make sense, but if in future we
> have like 20+ options on web, and people start creating addons to add more
> and more, then radios will start looking ugly.

I'm not going to write requirements for a UI assuming 20+ theme options, that's pathological. IMO people will install at most 1-2 themes.
(In reply to Jeff Griffiths (:canuckistani) from comment #11)
> (In reply to Girish Sharma [:Optimizer] from comment #10)
> ...
> > Agreed, for 3-4 theme options, it does not make sense, but if in future we
> > have like 20+ options on web, and people start creating addons to add more
> > and more, then radios will start looking ugly.
> 
> I'm not going to write requirements for a UI assuming 20+ theme options,
> that's pathological. IMO people will install at most 1-2 themes.

I am not saying that we should definitely do this. It was just a thought.

But, looking at this again, if we think from a consistency point of view in the whole of the options panel:
 - the default color unit options has just 4 choices
 - the key bindings options for editor (bug 964356) also has 4 choices
 - the tab size options in the same bug has just 3 options

All of them are menulist. IMO, after bug 964356, the theme thing would be odd one out.

And to be frank, theme changing is generally not a thing which someone would require to be super handy to not have to do an additional click.
Attached patch bug1038562-2.patch (obsolete) — Splinter Review
(In reply to Brian Grinstead [:bgrins] from comment #7)

New patch attached:

1) Checking whether there is an existing theme with the same ID during registration
2) Remove obsolete DTD theme labels + also using the same string keys for the new labels
3) Ensure that a theme is always selected - when the one in preferences is not available any more.
4) More JSDoc comments.

> This will cause the onUnapply / onApply to be fired many times (one for each
> frame).  It would be nice if we could actually only send these events once.
Yes, but it's actually what extensions need - being able to customize the
theme applying process in every iframe.

Note that:
1) These methods are not set for built-in themes and so there is no firing by default.
2) The methods will be necessary only for extension that want to do special stuff in every iframe.
3) Iframes are created dynamically, so there isn't usually so many existing iframes at the moment.


Honza
(In reply to Girish Sharma [:Optimizer] from comment #12)
> I am not saying that we should definitely do this. It was just a thought.
This is great example of a feature that might be implemented
in an extension.

Honza
Attachment #8457838 - Flags: review?(bgrinstead)
Attachment #8456926 - Attachment is obsolete: true
(In reply to Jan Honza Odvarko [:Honza] from comment #13)
> > This will cause the onUnapply / onApply to be fired many times (one for each
> > frame).  It would be nice if we could actually only send these events once.
> Yes, but it's actually what extensions need - being able to customize the
> theme applying process in every iframe.
> 
> Note that:
> 1) These methods are not set for built-in themes and so there is no firing
> by default.
> 2) The methods will be necessary only for extension that want to do special
> stuff in every iframe.
> 3) Iframes are created dynamically, so there isn't usually so many existing
> iframes at the moment.
> 
> 
> Honza

What kinds of changes would an extension need to do after the theme was applied/unapplied on each frame?  I was thinking onApply/onUnapply would be used for giving an extension information on when it should enable/disable itself based on recommended etiquette.

So, how do we make it easy to allow an extension to handle use case 2 (custom sheets per panel) while also respecting 3/4 (user selectable global theme)?  When the theme is applied, we'd like it to be very easy for an extension to add any other custom sheets or logic, and when unapplied it should be easy to remove them.  Note: If an extension *just* exists for use case 3/4 (it's just a theme) then the point is moot because they probably don't even need the callbacks.

I was thinking a single event would be better because it would allow a very easy way for the extension to keep track of whether it is applied (it only fires once, so a simple bit will do).  Here is some pseudocode of how I would think it could work in the case of a single event:

  /// extension.js

  let isExtensionActive = false;

  gDevTools.registerTheme({
    id: "mytheme",
    label: "My awesome theme"
    stylesheets: ["chrome://browser/skin/devtools/dark-theme.css"],
    classList: ["theme-dark"],
    onApply: () => {
      isExtensionActive = true;
      for (let overlay of overlays)
        this.extensionActivated(overlay);
    },
    onUnapply: () => {
      isExtensionActive = false;
      for (let overlay of overlays)
        this.extensionDeactivated(overlay);
    }
  });

  /// debuggerOverlay.js

  onReady: function() {
    if (!isExtensionActive) {
      return;
    }
    this.extensionActivated();
  },
  extensionActivated: function() {
    let win = this.panel.panelWin;
    loadSheet(win, "jsdebugger.css", "author");
    // Other initialization steps
  },
  extensionDeactivated: function() {
    let win = this.panel.panelWin;
    removeSheet(win, "jsdebugger.css", "author");
    // Other deinitialization steps
  }

I suppose one of the benefits of onApplied firing on every panel is that you don't also need to wait for a 'ready' event before calling loadSheet (it is implied to be ready by the time the theme has switched).  But there are some weird situations with theme switching like force-theme that make me hesitant to use it as a replacement for a more core tool-ready event.
(In reply to Brian Grinstead [:bgrins] from comment #15)
> What kinds of changes would an extension need to do after the theme was
> applied/unapplied on each frame?
The main advantage of this event is that it allows simple access to every
iframe window that is themed and at the right moment. This is something,
which is almost impossible to do now. It's quite powerful.

Extension can start modifying anything within the iframe DOM tree, changing
existing class names or appending new class names, changing the tree,
appending elements, loading custom style-sheets for particular URLs (point #2),
etc.


> I was thinking onApply/onUnapply would be
> used for giving an extension information on when it should enable/disable
> itself based on recommended etiquette.
I think this doesn't bring much additional power since the same can be
achieved by listening to "pref-changed" and watching "devtools.theme" I guess.

> So, how do we make it easy to allow an extension to handle use case 2
> (custom sheets per panel) while also respecting 3/4 (user selectable global
> theme)?
I think that by using onApply callback fired for every iframe.


> When the theme is applied, we'd like it to be very easy for an
> extension to add any other custom sheets or logic, and when unapplied it
> should be easy to remove them.

> Note: If an extension *just* exists for use
> case 3/4 (it's just a theme) then the point is moot because they probably
> don't even need the callbacks.
yep, agree


> I was thinking a single event would be better because it would allow a very
> easy way for the extension to keep track of whether it is applied (it only
> fires once, so a simple bit will do).

Could extensions use just simple "pref-changed" event to implement
the same? If yes, we can perhaps postpone onApply and onUnapply for now
(yet think more through it) and extensions can use global in the meanwhile:
gDevTools.emit("theme-switched", window, newTheme, oldTheme);

...for the things I mentioned above.


Here is some pseudocode of how I
> would think it could work in the case of a single event:
> 
>   /// extension.js
> 
>   let isExtensionActive = false;
> 
>   gDevTools.registerTheme({
>     id: "mytheme",
>     label: "My awesome theme"
>     stylesheets: ["chrome://browser/skin/devtools/dark-theme.css"],
>     classList: ["theme-dark"],
>     onApply: () => {
>       isExtensionActive = true;
>       for (let overlay of overlays)
>         this.extensionActivated(overlay);
>     },
>     onUnapply: () => {
>       isExtensionActive = false;
>       for (let overlay of overlays)
>         this.extensionDeactivated(overlay);
>     }
>   });
> 
>   /// debuggerOverlay.js
> 
>   onReady: function() {
>     if (!isExtensionActive) {
>       return;
>     }
>     this.extensionActivated();
>   },
>   extensionActivated: function() {
>     let win = this.panel.panelWin;
>     loadSheet(win, "jsdebugger.css", "author");
>     // Other initialization steps
>   },
>   extensionDeactivated: function() {
>     let win = this.panel.panelWin;
>     removeSheet(win, "jsdebugger.css", "author");
>     // Other deinitialization steps
>   }


> I suppose one of the benefits of onApplied firing on every panel is that you
> don't also need to wait for a 'ready' event before calling loadSheet (it is
> implied to be ready by the time the theme has switched).
Yep, it's necessary to loadSheet before as soon as possible otherwise
the defalt theme appears for a little time and then the new theme is
applied (I am having the problem now).


> But there are some
> weird situations with theme switching like force-theme that make me hesitant
> to use it as a replacement for a more core tool-ready event.
I don't understand what is the weird case.


Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #16)
> > But there are some
> > weird situations with theme switching like force-theme that make me hesitant
> > to use it as a replacement for a more core tool-ready event.
> I don't understand what is the weird case.

The Web IDE uses the theme switcher for its editor, but we set it to always use the light theme by setting a force-theme attribute on the body.  In this case it is probably best that it doesn't apply a firebug theme - I'm just pointing out that we can't rely on this in place of a more generic ready event because

1) This can fire on non-panel windows (like editors)
2) There is a chance that it will not fire on a panel (currently only does this in the case of an editor in Web IDE, but the mechanism is in place).

After looking at the branch, neither of these are a factor in the way you're using it so it is fine.  It's more an argument that we need a more robust tool-ready event in addition to these changes.
Comment on attachment 8457838 [details] [diff] [review]
bug1038562-2.patch

Review of attachment 8457838 [details] [diff] [review]:
-----------------------------------------------------------------

Please rebase toolbox-options.js on latest fx-team.  We will also need a test in browser/devtools/framework.  We've got a test called browser_toolbox_dynamic_registration.js that tests gDevTools.registerTool so it could be kind of like that.

::: browser/devtools/framework/gDevTools.jsm
@@ +272,5 @@
> +   * Needed so that add-ons can remove themselves when they are deactivated
> +   *
> +   * @param {string|object} theme
> +   *        Definition or the id of the theme to unregister.
> +   * @param {boolean} isQuitApplication

I know this came from registerTool, but I don't think we need this optional isQuitApplication here (it just fires a single event when unregistering a theme)

::: browser/devtools/framework/toolbox-options.js
@@ +270,5 @@
>    },
>  
> +  updateDefaultTheme: function() {
> +    // Make sure a theme is set in case the previous one coming from
> +    // an extension insn't available anymore.

typo: isn't

@@ +272,5 @@
> +  updateDefaultTheme: function() {
> +    // Make sure a theme is set in case the previous one coming from
> +    // an extension insn't available anymore.
> +    let themeBox = this.panelDoc.getElementById("devtools-theme-box");
> +    if (themeBox.selectedIndex == -1) {

Could just do themeBox.selectedItem = themeBox.querySelector("[value=light]") instead of looping.
Attachment #8457838 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #18)
> Comment on attachment 8457838 [details] [diff] [review]
> bug1038562-2.patch
> 
> Review of attachment 8457838 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please rebase toolbox-options.js on latest fx-team.  We will also need a
> test in browser/devtools/framework.  We've got a test called
> browser_toolbox_dynamic_registration.js that tests gDevTools.registerTool so
> it could be kind of like that.
> 
> ::: browser/devtools/framework/gDevTools.jsm
> @@ +272,5 @@
> > +   * Needed so that add-ons can remove themselves when they are deactivated
> > +   *
> > +   * @param {string|object} theme
> > +   *        Definition or the id of the theme to unregister.
> > +   * @param {boolean} isQuitApplication
> 
> I know this came from registerTool, but I don't think we need this optional
> isQuitApplication here (it just fires a single event when unregistering a
> theme)

I searched for the place where unregisterTool actually uses the argument and found:
https://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/gDevTools.jsm#351
I guess we should also call unregisterTheme with no-firing-events on destroy, no?
(I think this is related to what Paul mentioned at the meeting yesterday)

> @@ +272,5 @@
> > +  updateDefaultTheme: function() {
> > +    // Make sure a theme is set in case the previous one coming from
> > +    // an extension insn't available anymore.
> > +    let themeBox = this.panelDoc.getElementById("devtools-theme-box");
> > +    if (themeBox.selectedIndex == -1) {
> 
> Could just do themeBox.selectedItem =
> themeBox.querySelector("[value=light]") instead of looping.
Nice, I like that.

Thanks for the review!
Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #19)
> (In reply to Brian Grinstead [:bgrins] from comment #18)
> > Comment on attachment 8457838 [details] [diff] [review]
> > bug1038562-2.patch
> > 
> > Review of attachment 8457838 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please rebase toolbox-options.js on latest fx-team.  We will also need a
> > test in browser/devtools/framework.  We've got a test called
> > browser_toolbox_dynamic_registration.js that tests gDevTools.registerTool so
> > it could be kind of like that.
> > 
> > ::: browser/devtools/framework/gDevTools.jsm
> > @@ +272,5 @@
> > > +   * Needed so that add-ons can remove themselves when they are deactivated
> > > +   *
> > > +   * @param {string|object} theme
> > > +   *        Definition or the id of the theme to unregister.
> > > +   * @param {boolean} isQuitApplication
> > 
> > I know this came from registerTool, but I don't think we need this optional
> > isQuitApplication here (it just fires a single event when unregistering a
> > theme)
> 
> I searched for the place where unregisterTool actually uses the argument and
> found:
> https://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/
> gDevTools.jsm#351
> I guess we should also call unregisterTheme with no-firing-events on
> destroy, no?
> (I think this is related to what Paul mentioned at the meeting yesterday)

Looking at this, I don't think we really need to unregisterTheme() / unregisterTool() in the destroy method - we could just call `this._themes.clear();` and `this._tools.clear()` instead (all that function does is remove an object from the map if you pass in isQuitApplication).  And it's probably be best anyway to *not* expose this isQuitApplication functionality to an extension where it could pass in a special second parameter that stops internal events from firing.

And yes, it'd be best to not set up the expectation that an event will fire on toolbox close (as was mentioned in the meeting - sounds like there may be times when we don't go through the full destroy() logic when the browser process is killed with an opened toolbox).
Attached patch bug1038562-3.patch (obsolete) — Splinter Review
New patch attached, changes:

* New test: browser/devtools/framework/test/browser_toolbox_theme_registration.js
* Removed: isQuitApplication
* One typo fixed
* Avoid unnecessary looping (in updateDefaultTheme)

Honza
Attachment #8457838 - Attachment is obsolete: true
Attachment #8459524 - Flags: review?(bgrinstead)
We might want to have couple of follow-ups:

1) Call call `this._themes.clear();` and `this._tools.clear()` in DevTools.destroy instead of looping and using isQuitApplication to avoid events in destroy sequence.
2) Update the Options panel when a tool or theme is unregistered.

Honza
Comment on attachment 8459524 [details] [diff] [review]
bug1038562-3.patch

Review of attachment 8459524 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a couple more notes.  Also, please add a commit message to the patch

::: browser/devtools/framework/test/browser_toolbox_theme_registration.js
@@ +42,5 @@
> +    let panel = toolbox.getCurrentPanel();
> +    let doc = panel.panelWin.frameElement.contentDocument;
> +    let themeOption = doc.querySelector("#devtools-theme-box > radio[value=test-theme]");
> +
> +    ok(themeOption, "new theme exists in the Options panel");

We should also test the following cases:
1) Select it in options panel and making sure it gets applied.
2) Select a different theme and make sure the new theme gets unapplied.
3) Select it again, then unregister it while it is applied, making sure that the default theme gets applied.

::: browser/devtools/framework/toolbox-options.js
@@ +245,5 @@
> +    // Populating the default theme list
> +    let themes = gDevTools.getThemeDefinitionArray();
> +    for (let theme of themes) {
> +      themeBox.appendChild(createThemeOption(theme));
> +    }

Should this also be setting the selectedItem on themeBox and then calling updateDefaultTheme?  Additionally, unless if I'm missing something, this function seems that it should be called when a new theme is registered / unregistered to make sure that a theme is always selected.

::: browser/devtools/main.js
@@ +383,5 @@
>    observe: function(subject, topic, data) {
>      if (subject.wrappedJSObject === require("@loader/unload")) {
>        Services.obs.removeObserver(unloadObserver, "sdk:loader:destroy");
>        for (let definition of gDevTools.getToolDefinitionArray()) {
>          gDevTools.unregisterTool(definition.id);

Weird, this existing looping and unregistering all tools happens also in destroy() of gDevTools.  I don't see why we couldn't remove this unloadObserver and move this functionality into the destroy function of gDevTools, but I'm not positive when "sdk:loader:destroy" fires vs "quit-application" in gDevTools.  We could do that as a follow up

::: browser/devtools/shared/theme-switching.js
@@ +39,5 @@
>  
> +    // The theme might not be available anymore (e.g. uninstalled)
> +    // Use the default one.
> +    if (!newThemeDef)
> +      newThemeDef = gDevTools.getThemeDefinition("light");

Please use brackets around the new one line if statements in this file
Attachment #8459524 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #23)
> ::: browser/devtools/framework/test/browser_toolbox_theme_registration.js
> @@ +42,5 @@
> > +    let panel = toolbox.getCurrentPanel();
> > +    let doc = panel.panelWin.frameElement.contentDocument;
> > +    let themeOption = doc.querySelector("#devtools-theme-box > radio[value=test-theme]");
> > +
> > +    ok(themeOption, "new theme exists in the Options panel");
> 
> We should also test the following cases:
> 1) Select it in options panel and making sure it gets applied.
> 2) Select a different theme and make sure the new theme gets unapplied.
Done

> 3) Select it again, then unregister it while it is applied, making sure that
> the default theme gets applied.
See below

> ::: browser/devtools/framework/toolbox-options.js
> @@ +245,5 @@
> > +    // Populating the default theme list
> > +    let themes = gDevTools.getThemeDefinitionArray();
> > +    for (let theme of themes) {
> > +      themeBox.appendChild(createThemeOption(theme));
> > +    }
> 
> Should this also be setting the selectedItem on themeBox and then calling
> updateDefaultTheme?  Additionally, unless if I'm missing something, this
> function seems that it should be called when a new theme is registered /
> unregistered to make sure that a theme is always selected.
I think that all this should be done in a follow up bug (including the test).
Currently the list of registered tools (coming from extensions) is also
not updated and support for dynamic Options panel updates will require
more changes (e.g. the way how <radiogroup> elements are treated must
be also changed).


> ::: browser/devtools/main.js
> @@ +383,5 @@
> >    observe: function(subject, topic, data) {
> >      if (subject.wrappedJSObject === require("@loader/unload")) {
> >        Services.obs.removeObserver(unloadObserver, "sdk:loader:destroy");
> >        for (let definition of gDevTools.getToolDefinitionArray()) {
> >          gDevTools.unregisterTool(definition.id);
> 
> Weird, this existing looping and unregistering all tools happens also in
> destroy() of gDevTools.  I don't see why we couldn't remove this
> unloadObserver and move this functionality into the destroy function of
> gDevTools, but I'm not positive when "sdk:loader:destroy" fires vs
> "quit-application" in gDevTools.  We could do that as a follow up
OK


> ::: browser/devtools/shared/theme-switching.js
> @@ +39,5 @@
> >  
> > +    // The theme might not be available anymore (e.g. uninstalled)
> > +    // Use the default one.
> > +    if (!newThemeDef)
> > +      newThemeDef = gDevTools.getThemeDefinition("light");
> 
> Please use brackets around the new one line if statements in this file
Done

Honza
Attachment #8459524 - Attachment is obsolete: true
Attachment #8472290 - Flags: review?(bgrinstead)
Yet one improvement. When a theme is changed in a toolbox instance all existing Options panels should be updated accordingly.

The following lines are appended:

else if (data.pref === "devtools.theme") {
  let themeBox = this.panelDoc.getElementById("devtools-theme-box");
  let themeOption = themeBox.querySelector("[value=" + data.newValue + "]");

  if (themeBox.selectedItem != themeOption)
    themeBox.selectedItem = themeOption;
}

Honza
Attachment #8472337 - Flags: review?(bgrinstead)
Attachment #8472290 - Flags: review?(bgrinstead)
Attachment #8472290 - Attachment is obsolete: true
Comment on attachment 8472337 [details] [diff] [review]
bug1038562-api-for-theme-registration2.patch

Review of attachment 8472337 [details] [diff] [review]:
-----------------------------------------------------------------

I've applied this and played with it, along with the issue39 branch for firebug.next - it is coming together very nicely!  Please add a commit message to the patch.

One thing I noticed with unregistering a theme - enable firebug.next, open devtools, switch to firebug theme, disable firebug.next from the addons menu, switch to dark theme.  I end up seeing error messages, and I believe that even clicking on the close toolbox button starts throwing errors (see stack trace below).  I understand wanting to follow up with the dynamic rebuilding / reapplying of themes on unregister, but we should make sure that there at least are not errors when it happens.

TypeError: can't access dead object: EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/event-emitter.js:137:11
OptionsPanel.prototype.populatePreferences/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox-options.js:290:9
set_selectedItem@chrome://global/content/bindings/radio.xml:147:11
OptionsPanel.prototype._prefChanged@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox-options.js:142:9
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/event-emitter.js:137:11
OptionsPanel.prototype.populatePreferences/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox-options.js:290:9
set_selectedItem@chrome://global/content/bindings/radio.xml:147:11
onxblclick@chrome://global/content/bindings/radio.xml:512:13

TypeError: can't access dead object: EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/event-emitter.js:137:11
OptionsPanel.prototype.populatePreferences/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox-options.js:290:9
set_selectedItem@chrome://global/content/bindings/radio.xml:147:11
onxblclick@chrome://global/content/bindings/radio.xml:512:13

::: browser/devtools/framework/test/browser.ini
@@ +4,5 @@
>    browser_toolbox_options_disable_js.html
>    browser_toolbox_options_disable_js_iframe.html
>    browser_toolbox_options_disable_cache.sjs
>    head.js
> +  test-theme.css

Please rename this file doc_theme.css (that's what most of the browser/devtools directories use as a prefix for non-test helper files).
Attachment #8472337 - Flags: review?(bgrinstead)
Attached patch bug1038562-6.patch (obsolete) — Splinter Review
(In reply to Brian Grinstead [:bgrins] from comment #26)
> I've applied this and played with it, along with the issue39 branch for
> firebug.next - it is coming together very nicely!  Please add a commit
> message to the patch.
Commit message appended.

If the current theme is dynamically removed (through bootstrapped extension)
the light theme is automatically selected.

> One thing I noticed with unregistering a theme - enable firebug.next, open
> devtools, switch to firebug theme, disable firebug.next from the addons
> menu, switch to dark theme.  I end up seeing error messages, and I believe
> that even clicking on the close toolbox button starts throwing errors (see
> stack trace below).  I understand wanting to follow up with the dynamic
> rebuilding / reapplying of themes on unregister, but we should make sure
> that there at least are not errors when it happens.
The errors were coming from Firebug and I think I fixed them all.
All fixes are committed into issue39 branch
https://github.com/firebug/firebug.next/commits/issue39

> Please rename this file doc_theme.css (that's what most of the
> browser/devtools directories use as a prefix for non-test helper files).
Done

I also created a simple extension that shows how to use the API:
https://github.com/firebug/extension-examples/tree/master/CustomTheme

Honza
Attachment #8472337 - Attachment is obsolete: true
Comment on attachment 8473707 [details] [diff] [review]
bug1038562-6.patch

Review of attachment 8473707 [details] [diff] [review]:
-----------------------------------------------------------------

I've applied this alongside the latest firebug.next, and it works very well.  Just a couple of more notes:

::: browser/devtools/framework/gDevTools.jsm
@@ +291,5 @@
> +    // But, do not change it if the application is just shutting down.
> +    if (!Services.startup.shuttingDown && theme.id == currTheme) {
> +      Services.prefs.setCharPref("devtools.theme", "light");
> +
> +      let data = {

We really should make gDevTools have a preference observer on the "devtools." branch and automatically emit pref-changed events, instead of having it rely on the options panel to do so.  This would clean up this code quite a bit.  But, not in this bug

::: browser/devtools/framework/test/browser_toolbox_theme_registration.js
@@ +74,5 @@
> +  let color = panelWin.getComputedStyle(testThemeOption).color;
> +  isnot(color, "rgb(255, 0, 0)", "style unapplied");
> +
> +  // Then unregister it.
> +  testUnregister();

I believe that you've now pretty much added support for part (3) in Comment 23 in the code, so could you add a case in this test where you apply the custom theme before unregistering, then ensure that the light theme is reapplied after unregistering?

::: browser/devtools/framework/toolbox-options.js
@@ +140,5 @@
> +    else if (data.pref === "devtools.theme") {
> +      let themeBox = this.panelDoc.getElementById("devtools-theme-box");
> +      let themeOption = themeBox.querySelector("[value=" + data.newValue + "]");
> +
> +      if (themeBox.selectedItem != themeOption)

Nit: brackets around block.  Also, do we actually need to check to make sure it isn't selected before setting selectedItem?  Seems like the (data.newValue != data.oldValue) check in the select listener would prevent any harm from just selecting the themeOption node.

In fact, the condition we may want to use is `if (themeOption) {}` since I could come in and set my theme as "nonexistent-theme", in which case it would probably just be best to ignore it.

@@ +149,5 @@
> +  _themeUnregistered: function(event, theme) {
> +    // Update the theme box if the current theme is dynamically removed.
> +    let themeBox = this.panelDoc.getElementById("devtools-theme-box");
> +    if (theme && themeBox.selectedItem.value == theme.id) {
> +      themeBox.selectedItem = themeBox.querySelector("[value=light]");

It'd be great to do something like this to remove the unavailable option and to reuse the light-theme-as-default logic already in the file (assuming this doesn't break anything):

if (theme && themeBox.selectedItem.value == theme.id) {
  themeBox.selectedItem.remove();
  this.updateDefaultTheme();
}

@@ +300,4 @@
>          data.oldValue = GetPref(data.pref);
>          SetPref(data.pref, data.newValue);
> +
> +        if (data.newValue != data.oldValue)

Nit: brackets around block
Attachment #8473707 - Flags: review?(bgrinstead)
Comment on attachment 8473707 [details] [diff] [review]
bug1038562-6.patch

See comment 28 for review comments
Attachment #8473707 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #28)
> I believe that you've now pretty much added support for part (3) in Comment
> 23 in the code, so could you add a case in this test where you apply the
> custom theme before unregistering, then ensure that the light theme is
> reapplied after unregistering?
Done

> Nit: brackets around block.  Also, do we actually need to check to make sure
> In fact, the condition we may want to use is `if (themeOption) {}` since I
> could come in and set my theme as "nonexistent-theme", in which case it
> would probably just be best to ignore it.
Agree, done.

> It'd be great to do something like this to remove the unavailable option and
> to reuse the light-theme-as-default logic already in the file (assuming this
> doesn't break anything):
OK, also done + support for appending again (but I think we should stop including additional UI related features in this bug).

Thanks for the review!

Honza
Attachment #8473707 - Attachment is obsolete: true
Attachment #8474510 - Flags: review?(bgrinstead)
Comment on attachment 8474510 [details] [diff] [review]
bug1038562-7.patch

Review of attachment 8474510 [details] [diff] [review]:
-----------------------------------------------------------------

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9dd217c216cd
Attachment #8474510 - Flags: review?(bgrinstead) → review+
Looks like that was a bad try push - here is another: https://tbpl.mozilla.org/?tree=Try&rev=dca307d22d04
https://hg.mozilla.org/integration/fx-team/rev/254ebb32beb7
Assignee: nobody → odvarko
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [firebug-p1] → [firebug-p1][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/254ebb32beb7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [firebug-p1][fixed-in-fx-team] → [firebug-p1]
Target Milestone: --- → Firefox 34
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.