Closed Bug 989058 Opened 9 years ago Closed 9 years ago

Some cumulative theme fixes

Categories

(DevTools :: General, defect, P1)

x86
macOS
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 31

People

(Reporter: rcampbell, Assigned: rcampbell)

References

Details

Attachments

(4 files, 1 obsolete file)

Devtools should have more chalupuiles in its chrome.
Group: mozilla-employee-confidential
Chrome is Lithophile, not Chalcophile.

I suspect that WAT, WONTFIX is a better response, but I'll start with WAT.

WAT?
Attached patch brushed-metal (obsolete) — Splinter Review
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Dave and I were joking about landing a Brushed Metal theme for April 1st.

I'll be the first to admit that April Fool's gags on the internet are cheap, annoying and a source of unpleasantness for a lot of people.

But a Brushed Metal Theme...

http://cl.ly/UhAG

If we agree to do this, we land this on Sunday, back it out on Monday after the nightly's been built.

I'm pretty on-the-fence about this. I don't want to hurt our reputation by letting our users think we might land something frivolous at any time. Then again, it's "just" nightly.

What do you all think?
oh, and in case anyone's wondering what a "chalupuiles" is, it is a fictional breakfast food from Taco Bell that is part Chilaquiles, part Chalupa. I made it up.
I'm just gonna wontfix this. It's a bad idea, though I had a giggle making it.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
(In reply to Rob Campbell [:rc] (:robcee) from comment #3)
> Dave and I were joking about landing a Brushed Metal theme for April 1st.
> 
> I'll be the first to admit that April Fool's gags on the internet are cheap,
> annoying and a source of unpleasantness for a lot of people.
> 
> But a Brushed Metal Theme...
> 
> http://cl.ly/UhAG
> 
> If we agree to do this, we land this on Sunday, back it out on Monday after
> the nightly's been built.
> 
> I'm pretty on-the-fence about this. I don't want to hurt our reputation by
> letting our users think we might land something frivolous at any time. Then
> again, it's "just" nightly.
> 
> What do you all think?

Is that comic sans? +1 if it's comic sans.
Think we shouldn't be so brash and give this some more thought.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
I think this would actually be fun if it landed. People using Nightly *must* have a sense of humor, otherwise they wouldn't be on Nightly.
yeah, it's comic sans. (Purisa as a fallback for linux)

I dunno...
Will it be the default theme? Will it be a separate theme from light and dark? Will there be a doge-themed background-image for the options panel?

Doesn't really matter, just ship it (and then un-ship it or optionally hide it behind a pref)
It should probably be the only and default theme :)
(In reply to Victor Porof [:vporof][:vp] from comment #11)
> It should probably be the only and default theme :)

+1!
This really has to happen.
I've never been so delighted to read bugmail at 2am on a weekend.
seems like we have some semblance of consensus. I am suitably terrified.

This patch forcibly switches to the light theme and wedges the brushed metal into that theme. Comic Sans on the toolbars (purisa as fallback for linux).

One issue is the funny background edges in the breadcrumbs. Not sure I care? Anybody want to take a crack at it?
oh, and the options panel has only one theme option available: "Metal".
I'm also +1 on this!

Having a second theme in the options panel would be more "safe" and less likely to stir a firestorm, but in light of recent events I think even so a change in context would be actually welcome, and what we are mostly craving for right now is "fun".
Now I want to file a bug like "devtools should be more user friendly: adds more puppies in its chrome should do the job".
Comment on attachment 8398511 [details] [diff] [review]
brushed-metal

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

::: browser/devtools/framework/gDevTools.jsm
@@ +34,4 @@
>  
>    this._testing = false;
>  
> +  Services.prefs.setCharPref("devtools.theme", "light");

Instead of setting the pref here, I would suggest to modify theme-switching.js line 82 to just hardcode "light".  This will have the bonus of it automatically switching back to their default afterwards.
Attachment #8398511 - Flags: feedback+
(In reply to Brian Grinstead [:bgrins] from comment #19)
> Comment on attachment 8398511 [details] [diff] [review]
> brushed-metal
> 
> Review of attachment 8398511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/framework/gDevTools.jsm
> @@ +34,4 @@
> >  
> >    this._testing = false;
> >  
> > +  Services.prefs.setCharPref("devtools.theme", "light");
> 
> Instead of setting the pref here, I would suggest to modify
> theme-switching.js line 82 to just hardcode "light".  This will have the
> bonus of it automatically switching back to their default afterwards.

great suggestion. Was wondering if we could cache the previous setting somehow.
comic sans might be a *bit* much.
Comment on attachment 8398511 [details] [diff] [review]
brushed-metal

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

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

With my previous selection, the metal theme wouldn't be selected if your pref was dark.  I would suggest to remove the data-pref from radio group, and set checked="true" on the metal theme.  Also, it could be fun to include light and dark theme as disabled options: 

        <radiogroup id="devtools-theme-box"
                    class="options-groupbox"
                    orient="horizontal">
          <radio checked="true" value="light" label="Metal"/>
          <radio checked="false" disabled="true" value="light" label="&options.lightTheme.label;"/>
          <radio checked="false" disabled="true" value="dark" label="&options.darkTheme.label;"/>
        </radiogroup>
(In reply to Dave Camp (:dcamp) from comment #21)
> comic sans might be a *bit* much.

Comic Sans ties everything together.  How can you look at this and not love it? https://www.dropbox.com/s/gk7p2s5goa34bsw/Screenshot%202014-03-31%2010.10.58.png
Priority: -- → P1
(In reply to Brian Grinstead [:bgrins] from comment #23)

Not enough Comic Sans. Needs more monospaced Comic Sans. Why aren't we funding a monospaced version of Comic Sans?
Comment on attachment 8398511 [details] [diff] [review]
brushed-metal

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

Oh, and if you want to fix the breadcrumb issue, widgets.inc.css line 162: 

.theme-light #breadcrumb-separator-after,
.theme-light #breadcrumb-separator-before:after {
  background: url("chrome://browser/skin/devtools/metal.jpg"); /* Toolbars */
}
Comment on attachment 8398511 [details] [diff] [review]
brushed-metal

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

::: browser/devtools/framework/toolbox-options.xul
@@ +33,4 @@
>                      class="options-groupbox"
>                      data-pref="devtools.theme"
>                      orient="horizontal">
> +          <radio value="light" label="Metal"/>

A minor niggle perhaps but you're going to destroy blame for this line by doing this
(In reply to Dave Townsend (:Mossop) from comment #26)
> Comment on attachment 8398511 [details] [diff] [review]
> brushed-metal
> 
> Review of attachment 8398511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/framework/toolbox-options.xul
> @@ +33,4 @@
> >                      class="options-groupbox"
> >                      data-pref="devtools.theme"
> >                      orient="horizontal">
> > +          <radio value="light" label="Metal"/>
> 
> A minor niggle perhaps but you're going to destroy blame for this line by
> doing this

even after we back this out?
Attached patch brushed-metalSplinter Review
Attachment #8399486 - Flags: review?(dcamp)
Attached image Screen Shot.png
Attached image options
(In reply to Rob Campbell [:rc] (:robcee) from comment #27)
> (In reply to Dave Townsend (:Mossop) from comment #26)
> > Comment on attachment 8398511 [details] [diff] [review]
> > brushed-metal
> > 
> > Review of attachment 8398511 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/devtools/framework/toolbox-options.xul
> > @@ +33,4 @@
> > >                      class="options-groupbox"
> > >                      data-pref="devtools.theme"
> > >                      orient="horizontal">
> > > +          <radio value="light" label="Metal"/>
> > 
> > A minor niggle perhaps but you're going to destroy blame for this line by
> > doing this
> 
> even after we back this out?

Yes, the backout will cause the blame for that line to just be the backout changeset.
Comment on attachment 8399486 [details] [diff] [review]
brushed-metal

r+ and preemptive r+ for the backout.
Attachment #8399486 - Flags: review?(dcamp) → review+
Comment on attachment 8399486 [details] [diff] [review]
brushed-metal

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

8 lines of context Rob, geez!

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: browser/devtools/framework/toolbox-options.xul
@@ +34,3 @@
>                      orient="horizontal">
> +          <radio checked="true" value="light" label="Metal"/>
> +          <radio checked="false" disabled="true" value="light" label="&options.lightTheme.label;"/>

If you're just modifying the light theme, maybe it makes sense to remove the light theme option?

::: browser/devtools/shared/theme-switching.js
@@ +79,4 @@
>    const StylesheetUtils = devtools.require("sdk/stylesheet/utils");
>  
>    let theme = Services.prefs.getCharPref("devtools.theme");
> +  switchTheme("light");

Can we add a date check for April first and only do the metal theme that day?

This would require some refactoring, I suppose.
Attachment #8399486 - Flags: review?(dcamp)
Attachment #8399486 - Flags: review+
Comment on attachment 8399486 [details] [diff] [review]
brushed-metal

Woops, didn't mean to undo dcamp's r+
Attachment #8399486 - Flags: review?(dcamp) → review+
(In reply to Nick Fitzgerald [:fitzgen] from comment #33)
> Comment on attachment 8399486 [details] [diff] [review]
> brushed-metal
> 
> Review of attachment 8399486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 8 lines of context Rob, geez!
> 
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F
> 
> ::: browser/devtools/framework/toolbox-options.xul
> @@ +34,3 @@
> >                      orient="horizontal">
> > +          <radio checked="true" value="light" label="Metal"/>
> > +          <radio checked="false" disabled="true" value="light" label="&options.lightTheme.label;"/>
> 
> If you're just modifying the light theme, maybe it makes sense to remove the
> light theme option?
> 
> ::: browser/devtools/shared/theme-switching.js
> @@ +79,4 @@
> >    const StylesheetUtils = devtools.require("sdk/stylesheet/utils");
> >  
> >    let theme = Services.prefs.getCharPref("devtools.theme");
> > +  switchTheme("light");
> 
> Can we add a date check for April first and only do the metal theme that day?
> 
> This would require some refactoring, I suppose.

sadly no. It'd require the addition of a full brushed-metal theme and we just don't have that kinda time.
Summary: Deploy the Chalupuiles → Some cumulative theme fixes
Attachment #8398511 - Attachment is obsolete: true
This is so metal. Looks good on Windows.
(In reply to Brandon Benvie [:benvie] from comment #36)
> This is so metal. Looks good on Windows.

Define "good"
"metal"
Whiteboard: [fixed-in-fx-team] → [fixed-in-fx-team][backout on apr 1st!]
(In reply to Brian Grinstead [:bgrins] from comment #37)
> (In reply to Brandon Benvie [:benvie] from comment #36)
> > This is so metal. Looks good on Windows.
> 
> Define "good"

Looks metal with an excellent new typography choice.
*realises that this will ruin getting a screencast of the new add-on debugger done tomorrow*
(In reply to Dave Townsend (:Mossop) from comment #41)
> *realises that this will ruin getting a screencast of the new add-on
> debugger done tomorrow*

I don't see why.
Whiteboard: [fixed-in-fx-team][backout on apr 1st!] → [fixed-in-fx-team][backout on apr 1st!][leave open]
https://hg.mozilla.org/mozilla-central/rev/8d720133605f
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][backout on apr 1st!][leave open] → [backout on apr 1st!][leave open]
Target Milestone: --- → Firefox 31
(leave open)
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Whiteboard: [backout on apr 1st!][leave open] → [backout on apr 1st!][leave-open]
This is really good april's fool joke. Funny enough that I had to back it out today on my local repo. :) it's THAT good.
Group: mozilla-employee-confidential
Duplicate of this bug: 990508
Sorry to be that guy, but not allowing to change theme at all was to take this a bit too far. It's funny until you try to use it.
Attached patch backout.patchSplinter Review
backed-out

https://hg.mozilla.org/integration/fx-team/rev/a4fbdc403e0f
Whiteboard: [backout on apr 1st!][leave-open] → [backed-out-in-fx-team]
Nice joke :)
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: [backed-out-in-fx-team]
Status: RESOLVED → VERIFIED
The following changeset is now in Firefox Nightly:

> a4fbdc403e0f Bug 989058 - Some cumulative theme fixes - BACKOUTOMG; r=dcamp,#developers

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.