1097156 - Link to customize mode from devtools theme options in devedition

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Brian Grinstead [:bgrins]]

To make sure people can find the option to enable/disable the devedition theme, we could add an entry in the devtools options panel

Comment 1

[Brian Grinstead [:bgrins]]

Here is one suggestion:

Go from this (how it currently is in both devedition and non devedition):

Choose DevTools Theme
---------------------
  

Comment 2

[Brian Grinstead [:bgrins]]

Hm, that seemed to cut off my comment starting at the unicode radio buttons?  Trying again:

Go from this (how it currently is in both devedition and non devedition):

Choose DevTools Theme
---------------------
  * Light Theme * Dark Theme

To this in DevEdition:

Themes
------
  * Light Theme * Dark Theme
  Toggle Developer Edition browser theme in customize mode (may want something a little less wordy to fit better in the options panel)

And this in non-DevEdition

Themes
------
  * Light Theme * Dark Theme

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1097156-devedition-theme-toggle.patch

Added a checkbox toggle to toggle between the dev edition browser theme.

* How's the verbiage here?
* Will need someone from the customizableUI components to review, I think

Comment 4

[:Gijs (he/him)]

Comment on attachment 8521683 [details] [diff] [review]
1097156-devedition-theme-toggle.patch

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

r=me on the browser/components bits.

::: browser/components/customizableui/CustomizeMode.jsm
@@ +1565,5 @@
>        LightweightThemeManager.currentTheme = this._lastLightweightTheme;
>      }
>    },
>  
> +  _onDevEditionThemeToggleClick: function() {

No need for this, see below.

::: browser/components/customizableui/content/customizeMode.inc.xul
@@ +56,5 @@
>        <button id="customization-devedition-theme-button"
>                class="customizationmode-button"
>                hidden="true"
>                label="&customizeMode.deveditionTheme.label;"
> +              oncommand="gCustomizeMode._onDevEditionThemeToggleClick()"

nit: oncommand="gCustomizeMode.toggleDevEditionTheme(this.hasAttribute('checked'))"

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8521683 [details] [diff] [review]
1097156-devedition-theme-toggle.patch

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

First of all, this looks awesome since it doesn't require duplicating the logic for what should happen when the checkbox is ticked.

Here is a use case that doesn't work cleanly:

1) Apply a lw theme
2) Open toolbox options panel

Notice that the checkbox is checked (that's b/c the browser.devedition.theme.enabled pref is true).  This could probably be fixed by listening to the devedition-theme-state-changed event and setting the checked value to = !!window.DevEdition.styleSheet where window is the window in browser.xul (see customizemode.jsm).  That makes things a bit more complicated, since I'm not sure if these manual value changes will mess up "pref-changed" calls (i.e. we are setting it to false even when we don't want the pref to change to false).  I guess that depends on if a 'command' event is fired from setting checkbox.value in JS.  I'd love to actually not deal with this case at all, but we should probably make this work just as well as the customize button does.

::: browser/devtools/framework/test/browser_toolbox_options_devedition.js
@@ +44,5 @@
> +      deferred.resolve(Services.prefs.getBoolPref(PREF));
> +    }
> +  });
> +
> +  // We use executeSoon here to ensure that the element is in view and

Do we actually need this executeSoon still?  What if you do el.click() instead?

::: browser/devtools/framework/toolbox-options.xul
@@ +33,5 @@
>                      class="options-groupbox"
>                      data-pref="devtools.theme"
>                      orient="horizontal">
>          </radiogroup>
> +        <vbox class="options-groupbox">

I'd say hide this if browser.devedition.theme.showCustomizeButton is false (which will make it show by default in dev edition but not in others).

::: browser/locales/en-US/chrome/browser/devtools/toolbox.dtd
@@ +119,5 @@
>    -  tools. -->
>  <!ENTITY options.selectDevToolsTheme.label   "Choose DevTools theme:">
>  
> +<!-- LOCALIZATION NOTE (options.usedeveditiontheme.*) Options under the
> +  -  toolbox for enabling and disabling the Developer Edition browser theme. -->

As discussed on IRC, these strings make sense since the theme name is separate from the branding name.

Can you please also update browser/locales/en-US/chrome/browser/browser.dtd / customizeMode.deveditionTheme.label to read "Use Developer Edition Theme"?

@@ +121,5 @@
>  
> +<!-- LOCALIZATION NOTE (options.usedeveditiontheme.*) Options under the
> +  -  toolbox for enabling and disabling the Developer Edition browser theme. -->
> +<!ENTITY options.usedeveditiontheme.label   "Use Developer Edition browser theme">
> +<!ENTITY options.usedeveditiontheme.tooltip "Toggles the Developer Edition browser theme."> 

Nit: trailing whitespace

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Thanks for the reviews! 

Brian, another reason why I chose not to use the brand name was in the event we enable this on nightly, which I heard some discussion about -- same with figuring out whether or not to display the button on nightly as well (but using that pref should do it).

Working on the revised patch.

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1097156-devedition-theme-toggle.patch

Had to duplicate some of the logic found in CustomizeButton unfortunately -- we cannot use the main pref logic in the toolbox options as it's more complicated than just checking to see if the preference is set to true.

This covers the case where dev edition is ON, and then you apply a LWT, then reopen the tools -- the checkbox should be unchecked, and removing the theme, if dev edition was enabled previously, would go back to dev edition, and check the box.

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Keep in mind, do not use e10s for applying LWT's from AMO or the ground will open up and swallow you whole

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8523117 [details] [diff] [review]
1097156-devedition-theme-toggle.patch

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

Brian is fine to review this alone, I don't have enough context anyway.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8523117 [details] [diff] [review]
1097156-devedition-theme-toggle.patch

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

This works great!  Just a few more notes

::: browser/devtools/framework/test/browser_toolbox_options_devedition.js
@@ +20,5 @@
> +  let tab = yield addTab(URL);
> +  let target = TargetFactory.forTab(tab);
> +  toolbox = yield gDevTools.showToolbox(target);
> +  let selected = toolbox.once("options-selected");
> +  toolbox.selectTool("options");

Can save a few lines here by doing:

let tool = yield toolbox.selectTool("options");

instead of waiting for options-selected and refetching the tool

::: browser/devtools/framework/toolbox-options.js
@@ +290,5 @@
> +   * special rules for the browser theme button.
> +   */
> +  setupBrowserThemeButton: function() {
> +    let checkbox = this.panelDoc.getElementById("devtools-browser-theme");
> +    checkbox.checked = this._isDevEditionThemeOn();

Is this line needed?  You also set the checked state in updateBrowserThemeButton

@@ +292,5 @@
> +  setupBrowserThemeButton: function() {
> +    let checkbox = this.panelDoc.getElementById("devtools-browser-theme");
> +    checkbox.checked = this._isDevEditionThemeOn();
> +
> +    checkbox.addEventListener("command", function() {

We should be able to refactor this.  I see 4 different event handlers in this file that do the next 6 lines with the only difference being the pref name and the new value.  We could have a setPrefAndNotify(prefName, newValue) function that each of these calls.  Don't need to necessarily do that here though, but let's file a follow up bug for that if not.

@@ +453,5 @@
> +   * Returns a boolean indicating whether or not the dev edition
> +   * browser theme is applied.
> +   */
> +  _isDevEditionThemeOn: function() {
> +    return !!this.panelWin.parent.parent.DevEdition.styleSheet;

!!this.panelWin.top.DevEdition.styleSheet

::: browser/devtools/framework/toolbox-options.xul
@@ +33,5 @@
>                      class="options-groupbox"
>                      data-pref="devtools.theme"
>                      orient="horizontal">
>          </radiogroup>
> +        <vbox class="options-groupbox">

For some reason this floats to the same line as the other theme options.  As discussed on irc, I think we need an additional class to be added to this element that sets display: block just like .option-vertical-pane label does

::: browser/locales/en-US/chrome/browser/devtools/toolbox.dtd
@@ +116,5 @@
>  
>  <!-- LOCALIZATION NOTE (options.selectDevToolsTheme.label): This is the label for
>    -  the heading of the radiobox corresponding to the theme of the developer
>    -  tools. -->
>  <!ENTITY options.selectDevToolsTheme.label   "Choose DevTools theme:">

I'd suggest to change this entity value to say "Themes".  The "DevTools" part is currently redundant, and now that we are adding the browser part in there no longer accurate.

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1097156-devedition-theme-toggle.patch

* Cleaned up the test, removed lines 
* removed redundant checking of the checkbox in setupBrowserThemeButton
* using !!this.panelWin.top.DevEdition.styleSheet now
* Fixed styling issue of the theme buttons on wider screens
* Changed label string to just "Themes", to keep in line with other sections and now with the dev edition browser theme
* For refactoring of all the pref-changed stuff in devtools, that's all the options in the toolbox and well beyond the scope of this, so yeah probably better in another bug (and someone more familiar with all of these options)

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8524034 [details] [diff] [review]
1097156-devedition-theme-toggle.patch

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

Nit: looks like there is a new line in the commit message.  Pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=07e299301036

Comment 13

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1097156-devedition-theme-toggle.patch

Updated with a better fix for when the toolbox is remote debugging, and preffed off to show the customized button when in browser toolbox

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eb854de21733

Comment 14

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/9b88cc96d75a

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b88cc96d75a