1094138 - dev edition button in customize mode should set all relevant prefs and/or be hidden if it can't be enabled

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

STR:

0. use devedition
1. switch to light devtools theme
2. open customize mode
3. click on button several times

AR:
nothing happens

ER:
button isn't there, or something happens. :-)

Background: http://logs.glob.uno/?c=mozilla%23fx-team&s=5+Nov+2014&e=5+Nov+2014#c178268 and further.

Comment 1

[Brian Grinstead [:bgrins]]

Here are the main cases and how to deal with them as I understand it:

1) If the button has been turned on on, set the pref to true and remove any applied lw theme.
2) If the button has been turned off, set the pref to false.  Do we need to previously restore the last applied lw theme (if any)?
3) If the devtools theme is light, make enabling the button switch that to dark also.  (or update the devedition theme so that it doesn't need to go back to Australis)
4) If a complete theme is applied, disable the button?  We can't instantly show any changes at that point.

Comment 2

[Brian Grinstead [:bgrins]]

Gijs, as we discussed earlier I haven't been able to get to this today.  So if have a chance tomorrow to look more into this, please do.

Comment 3

[:Gijs (he/him)]

Attachment: turn off lwt where applicable when clicking devtools button,

I'm not toggling the devtools theme in this code because we want to make the light theme work here, which is bug 1094509.

Comment 4

[:Gijs (he/him)]

The test fix means this can land independently from bug 1094509, but we'll need that change in order for this to "feel" fixed, I expect. Brian, can you also test that that works (ie, behaviour is as expected if you do apply the patch from bug 1094509 on top of this one?)

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8518275 [details] [diff] [review]
turn off lwt where applicable when clicking devtools button,

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

Very nice, works as expected with the patch from 1094509

::: browser/base/content/browser-devedition.js
@@ +85,5 @@
>      if (e.type === "load") {
>        this.styleSheet.removeEventListener("load", this);
>        gBrowser.tabContainer._positionPinnedTabs();
>        ToolbarIconColor.inferFromText();
> +      debugger;

Extra debugger statement

@@ +103,5 @@
>        this.styleSheet.remove();
>        this.styleSheet = null;
>        gBrowser.tabContainer._positionPinnedTabs();
>        ToolbarIconColor.inferFromText();
> +      debugger;

Extra debugger statement

Comment 7

[:Gijs (he/him)]

Attachment: turn off lwt where applicable when clicking devtools button,

Now with less debugger;

Comment 8

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

Comment on attachment 8519010 [details] [diff] [review]
turn off lwt where applicable when clicking devtools button,

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

::: browser/base/content/browser-devedition.js
@@ +95,5 @@
>        let styleSheetAttr = `href="${this.styleSheetLocation}" type="text/css"`;
>        this.styleSheet = document.createProcessingInstruction(
>          'xml-stylesheet', styleSheetAttr);
>        this.styleSheet.addEventListener("load", this);
>        document.insertBefore(this.styleSheet, document.documentElement);

Can you add a comment here saying that the observers are notified after the stylesheet is loaded?

::: browser/components/customizableui/CustomizeMode.jsm
@@ +472,5 @@
>        for (let toolbar of customizableToolbars)
>          toolbar.removeAttribute("customizing");
>  
>        this.window.PanelUI.endBatchUpdate();
> +      delete this._lastLightWeightTheme;

nit, please rename this to _lastLightweightTheme (no capital W)

@@ +1550,5 @@
> +    let shouldEnable = button.hasAttribute("checked");
> +
> +    Services.prefs.setBoolPref(kDeveditionThemePref, shouldEnable);
> +    if (LightweightThemeManager.currentTheme && shouldEnable) {
> +      this._lastLightWeightTheme = LightweightThemeManager.currentTheme;

LightweightThemeManager.currentTheme parses a complex JSON object each time it is called. Please store the result of the first call in a local variable and then reference that instead of calling .currentTheme twice in a row.

::: browser/components/customizableui/test/browser_970511_undo_restore_default.js
@@ +115,5 @@
>    is(undoResetButton.hidden, true, "Undo reset button should be hidden at start of test");
>    Services.prefs.setBoolPref(prefName, !defaultValue);
> +
> +  //XXXgijs this line should be removed once bug 1094509 lands
> +  Services.prefs.setCharPref("devtools.theme", "dark");

I understand that this should be removed when that patch lands, but if the plan is to land these both in the same push, then make sure to remove this (and below) before pushing.

Comment 9

[:Gijs (he/him)]

w/ nits:

https://hg.mozilla.org/integration/fx-team/rev/1afe68b5c5b0

Comment 10

[:Gijs (he/him)]

Marco, can you add this, too? Thanks!

Comment 11

[Marco Mucci [:MarcoM]]

Added to IT 36.2

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1afe68b5c5b0

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5c159fd14791