Closed Bug 1314091 (compact-themes) Opened 8 years ago Closed 7 years ago

Change the Dev Edition theme(s) to be alternative "compact themes" and let them ride the trains

Categories

(Firefox :: Theme, defect, P3)

Product:
Firefox
Component:
Theme
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
relnote-firefox --- 53+
firefox53 + verified

People

(Reporter: canuckistani, Assigned: bgrins)

References

(Depends on 1 open bug)

Details

User Story

#MVP

* As a user with a low-resolution screen laptop or 2-in1 tablet ( eg 1366x768 ), I would appreciate a browser that used as little vertical space as possible
* I would like to be able to switch to a compact theme from about:addons as usual.
* I would like to be able to switch to a compact theme from Firefox’s customization mode because I’m on of the 5 people who knows where that is.
* I would like to choose a ‘dark’ compact theme to match all the other creative dark theme apps I use like photoshop and Logic Pro.
* I would like to choose a ‘light’ compact theme to match the light theme
* If my system has a fairly low[1] resolution screen, Firefox should suggest using a compact theme in order to save space.
* Firefox should explain clearly that compact themes do not currently support lightweight themes.


#Backlog


* As a lightweight themes enthusiast, I would love it if this new compact theme supported my lightweight favourite lightweight themes as well.
* As a lightweight theme author, I would be overjoyed to hear that I had to make no changes to my theme in order for it to work with compact themes.
* Depending on what operating system I am on, the dark and light themes should adopt as native a colour pallette as possible.
* As a developer, I want to be able to change the colors in the toolbox.
* As a developer, it would be great if common developer themes from other products like sublime text could be imported and used in the toolbox

Attachments

(9 files, 5 obsolete files)

Bug 1314091 - Backed out changeset 60d0080b95fe (bug 1308407)
58 bytes, text/x-review-board-request
Details
Bug 1314091 - Rename devedition.* to compacttheme.*;
58 bytes, text/x-review-board-request
Gijs
: review+
Details
all-the-themes.png
152.12 KB, image/png
Details
Bug 1314091 - Remove devtools-specific logic out of browser-devedition and into devtools-browser;
58 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;
58 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1314091 - TO FOLD BEFORE LANDING. Replace [devtoolstheme] with :-moz-lwtheme-darktext and :-moz-lwtheme-brighttext
58 bytes, text/x-review-board-request
Details
Bug 1314091 - TO FOLD BEFORE LANDING Move pseudo onto descendents;
59 bytes, text/x-review-board-request
Gijs
: review+
Details
theme-switching.gif
184.92 KB, image/gif
Details
Bug 1314091 - Sync the applied devtools toolbox theme and compact themes;
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
See user story
User Story: (updated)
Was this discussed with the folks who are working on the lwt/"full theme" overhaul? The kind of "meshing" between different themes suggested here seems... complex, and devedition does more than just being "compact" / dark/light.
It was discussed but I didn't see a conclusion come through. The last message I have in the email thread is from October 12th.

Since those last messages, we are now quite a bit closer to implementing the DevEdition theme through the new prototype Theme API over in bug 1306671.

I would prefer that these "compact themes" get implemented using the new API as right now the DevEdition theme touches a lot of places and the list of changes are not cataloged in one easy to find place.
I logged this bug to get it in the system. bgrins has some ideas on how to implement this and he is returning from leave tomorrow, this reminded me to add this.
Component: General → Theme
See Also: → webext-themes
(In reply to :Gijs Kruitbosch from comment #1)
> Was this discussed with the folks who are working on the lwt/"full theme"
> overhaul? The kind of "meshing" between different themes suggested here
> seems... complex, and devedition does more than just being "compact" /
> dark/light.

There could potentially be two steps here: the first is splitting the current DE theme into two different LW themes which are preinstalled on all channels, and the second is converting these LW themes into addons using the new theme API.  There'd be extra work created by doing it in two steps due to migration from one to the other, so whether it makes sense to do it in two steps or wait until the theme API is completed before rolling out 'compact themes' depends on product needs / timelines.

I have a WIP patch that does quite a bit of the first step and it might be helpful for testing, so I'll attach that.
Attached patch compact-themes-WIP.patch (obsolete) — — Splinter Review 
Just a WIP.  Here's a try push for binaries (mochitests will be failing, see commit message in the patch): https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c191b59448fac880126cf4dbcffa25bdf0beac7.

Note: if we *also* wanted to keep the devtools toolbox colors in sync with the currently applied lwt, that would require some extra code:
1) applying the correct lwt if and only if one of the two is applied when the devtools theme changes
2) applying the correct devtools theme color when one of the two lwts is applied
Updated patches are up. This is now moving all devtools-specific logic into the existing devtools-browser module and renaming all of the devedition-related files to compacttheme.  We discussed that there would be no 'linkage' between devtools toolbox color and the compact theme color, we would instead link from the toolbox to about:addons#appearance.

Here's an ongoing try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d534cc23935ce2d7597cb08e9b6fcd3145e50d37.

And talos comparison link: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=09475d266f428774a300033a1fcc37db00edace2&newProject=try&newRevision=d534cc23935ce2d7597cb08e9b6fcd3145e50d37&framework=1&filter=damp&showOnlyImportant=0
Brian: I downloaded the build from that try run and it doesn't run, macOS complains that it is damaged.
Flags: needinfo?(bgrinstead)
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #10)
> Brian: I downloaded the build from that try run and it doesn't run, macOS
> complains that it is damaged.

I suspect you need to turn off the gatekeeper checks for verified developers.
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #10)
> Brian: I downloaded the build from that try run and it doesn't run, macOS
> complains that it is damaged.

Hm, I see the same thing.  Talos tests ran successfully so the build itself should be fine, but I guess the artifact downloading is somehow not working.  I'll see if a Windows build works

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #10)
> > Brian: I downloaded the build from that try run and it doesn't run, macOS
> > complains that it is damaged.
> 
> I suspect you need to turn off the gatekeeper checks for verified developers.

That's a good thought - although I'm not even getting to the warning about verified code AFAICT.  I'm seeing "Nightly is damaged and cannot be opened", whether I try to open it from the dmg or copy to Applications
Flags: needinfo?(bgrinstead)
FYI the build works on Windows
Attachment #8807228 - Attachment is obsolete: true
Attached patch compactthemes-folded.patch (obsolete) — — Splinter Review 
Jeff, here's a folded patch file in case you want to do a build locally in the meantime
Depends on: 1321911
I got the build working by running

sudo spctl --master-disable 

see http://apple.stackexchange.com/a/245029

The build works really well except for some funky preview behavior in customize mode, see bug 1321911
Target Milestone: --- → Firefox 53
Current todo list:

1) Handle migration for users currently on firefox-devedition@mozilla.org
2) Update the browser_devedition tests expecting the old behavior
3) Figure out syncing (or not) between devtools theme and this theme.  If not, somehow link to about:addons with the appearance tab selected
4) Fix the preview behavior in customize mode thing (bug 1321911)
[Tracking Requested - why for this release]: I'm changing Target Milestone to Tracking. We use Target Milestone to track when the changes actually land. It appears that Jeff wants this to land in 53, so we can use tracking for that.
status-firefox52: affected → ---
tracking-firefox53: --- → ?
Target Milestone: Firefox 53 → ---
Depends on: 1323619
Attached image all-the-themes.png — 
With the two compact themes and 'recommended' themes this popup is getting huge (and mouse events seem to get out of whack once it becomes about as tall as the browser window).  Any thoughts about removing one or two of the recommended themes to make it smaller?
Flags: needinfo?(jgriffiths)
Attachment #8816620 - Attachment is obsolete: true
Attached image theme-switching.gif (obsolete) — 

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
A nice side effect to this change is that we can set the `textcolor` and `accentcolor` properties on the two individual themes which makes them much more like normal lightweight themes, and we get "brighttitlebarforeground", ToolbarIconColor.inferFromText(), and "lwthemetextcolor"/_updateLWTBrightness (Bug 1308407) for free.  The latest patch set removes all of that from browser-devedition.

    
    

    
  
Jeff, here's a new try push for binaries: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff58b83f16c8f300b5e58ac88bceb8b20ad5204e.  Can you confirm that Bug 1321911 is resolved in it?

    
    

    
  
Comment on attachment 8818987 [details]
Bug 1314091 - Remove devtools-specific logic out of browser-devedition and into devtools-browser;

Sorry, accidentally flagged review

        Attachment #8818987 -
        Flags: review?(gijskruitbosch+bugs)

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #18)
> Created attachment 8818766 [details]
> all-the-themes.png
> 
> With the two compact themes and 'recommended' themes this popup is getting
> huge (and mouse events seem to get out of whack once it becomes about as
> tall as the browser window).  Any thoughts about removing one or two of the
> recommended themes to make it smaller?

Can you see if we have telemetry that shows how popular each one is?
Flags: needinfo?(bgrinstead)

    
    

    
  
I don't see any obvious telemetry here: https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#1421, nor do I see a match in histograms.json for anything matching 'theme' / 'selectedThemeID'.  Maybe Jeff knows if we have some kind of data on what themes people are using.
Flags: needinfo?(bgrinstead)

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #18)
> Created attachment 8818766 [details]
> all-the-themes.png
> 
> With the two compact themes and 'recommended' themes this popup is getting
> huge (and mouse events seem to get out of whack once it becomes about as
> tall as the browser window).  Any thoughts about removing one or two of the
> recommended themes to make it smaller?

I think it would be good to know 2 pieces of info:

1. doe people use customize mode to change themes?
2. I think we could cut it to a top 3, based on popularity on AMO. I'll talk to Kev about that. Logged bug 1323833
Depends on: 1323833
Flags: needinfo?(jgriffiths)

    
    

    
  

      
      
      
      

        Attached image
          
        compact-themes.png (obsolete)
        — 
      

    


  
(In reply to Brian Grinstead [:bgrins] from comment #30)
> Jeff, here's a new try push for binaries:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ff58b83f16c8f300b5e58ac88bceb8b20ad5204e.  Can you
> confirm that Bug 1321911 is resolved in it?

The menu button separator has a style bug in the try build on macOS.

    
    

    
  
(In reply to Sören Hentzschel from comment #35)
> Created attachment 8819038 [details]
> compact-themes.png
> 
> (In reply to Brian Grinstead [:bgrins] from comment #30)
> > Jeff, here's a new try push for binaries:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=ff58b83f16c8f300b5e58ac88bceb8b20ad5204e.  Can you
> > confirm that Bug 1321911 is resolved in it?
> 
> The menu button separator has a style bug in the try build on macOS.

Yes, I see that too - thanks for the heads up

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #36)
> (In reply to Sören Hentzschel from comment #35)
> > Created attachment 8819038 [details]
> > compact-themes.png
> > 
> > (In reply to Brian Grinstead [:bgrins] from comment #30)
> > > Jeff, here's a new try push for binaries:
> > > https://treeherder.mozilla.org/#/
> > > jobs?repo=try&revision=ff58b83f16c8f300b5e58ac88bceb8b20ad5204e.  Can you
> > > confirm that Bug 1321911 is resolved in it?
> > 
> > The menu button separator has a style bug in the try build on macOS.
> 
> Yes, I see that too - thanks for the heads up

Looks like this is already a bug with DevEdition theme in nightly.  I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1323844 for that

    
    

    
  
Comment on attachment 8816497 [details]
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme

Gijs, I'd appreciate a round of feedback on the current patch series if you have a chance (no hurry at all since I'll be out for a couple of weeks).  I hope that the series is easy enough to follow and that the end result is simpler than the current DE theme implementation (there are special cases being removed in the first and fifth patches).

I still have some changes to make, like:
* migration for old profiles
* a few more test updates in browser_devedition.js
* figure out syncing between the toolbox theme color and the corresponding lightweight theme
* use updated strings and icons

        Attachment #8816497 -
        Flags: feedback?(gijskruitbosch+bugs)

    
    

    
  
(In reply to (Unavailable until Jan 4) Brian Grinstead [:bgrins] from comment #33)
> I don't see any obvious telemetry here:
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> customizableui/CustomizeMode.jsm#1421, nor do I see a match in
> histograms.json for anything matching 'theme' / 'selectedThemeID'.  Maybe
> Jeff knows if we have some kind of data on what themes people are using.

See "persona" in the environment section of FHR.

https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/data/environment.html?highlight=persona
https://dxr.mozilla.org/mozilla-central/rev/5a536a16e33798fe7b16de35c968d5bc0cbf8448/toolkit/components/telemetry/TelemetryEnvironment.jsm#537,549

I already had a query for it here: https://sql.telemetry.mozilla.org/queries/313
Priority: -- → P3

    
    

    
  
Comment on attachment 8816497 [details]
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme

https://reviewboard.mozilla.org/r/97212/#review99808

::: browser/base/content/browser-devedition.js:78
(Diff revision 3)
>    _inferBrightness: function() {
>      ToolbarIconColor.inferFromText();
> -    // Get an inverted full screen button if the dark theme is applied.
> -    if (this.isStyleSheetEnabled &&
> -        document.documentElement.getAttribute("devtoolstheme") == "dark") {
> +
> +    if (this.isStyleSheetEnabled) {
> +      if (this.isCurrentThemeIdDark) {
> -      document.documentElement.setAttribute("brighttitlebarforeground", "true");
> +        document.documentElement.setAttribute("brighttitlebarforeground", "true");

Do we not want to do this for dark lwthemes generally? I also wonder if this is worth keeping given that the fullscreen button is now only supported on 10.9, which we do still support but only barely - given we now support 10.12, feels like 10.9 should be gone soon...

::: browser/base/content/browser-devedition.js:127
(Diff revision 3)
> +      let compactTheme = this.isCurrentThemeIdDark ? "dark" : "light";
> +      document.documentElement.setAttribute("compacttheme", compactTheme);

Feels like this should happen before enabling and/or adding the sheet to avoid flashes of partially styled content (given OMT style/compositor/rendering stuff).

::: browser/base/content/test/general/browser_devedition.js:62
(Diff revision 3)
> -add_task(function* testDevtoolsTheme() {
> -  info("Checking stylesheet and :root attributes based on devtools theme.");
> -  Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "light");
> -  is(document.documentElement.getAttribute("devtoolstheme"), "light",
> -    "The documentElement has an attribute based on devtools theme.");
> -  ok(DevEdition.isStyleSheetEnabled, "The devedition stylesheet is still there with the light devtools theme.");
> +add_task(function* testLightweightThemePreview() {
> +  if (SKIP_TEST) {
> +    ok(true, "No need to run this test since themes aren't installed");
> +    return;
> +  }
> +  info("Setting compact to current and the previewing others");

Nit: spelling

::: browser/base/content/test/general/browser_devedition.js
(Diff revision 3)
> -
> -  Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "foobar");
> -  is(document.documentElement.getAttribute("devtoolstheme"), "light",
> -    "The documentElement has 'light' as a default for the devtoolstheme attribute");
> -  ok(DevEdition.isStyleSheetEnabled, "The devedition stylesheet is still there with the foobar devtools theme.");
> -  ok(!document.documentElement.hasAttribute("brighttitlebarforeground"),
> -     "The brighttitlebarforeground attribute is not set on the window with light devtools theme.");

Why is this being removd? I thought this code still existed in this cset - maybe this removal belongs in a different cset?

::: browser/base/content/test/general/browser_devedition.js
(Diff revision 3)
> -add_task(function* testLightweightThemePreview() {
> -  info("Setting devedition to current and the previewing others");
> -  LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme("firefox-devedition@mozilla.org");
> -  ok(DevEdition.isStyleSheetEnabled, "The devedition stylesheet is enabled.");

Why are we removing this test?

    
    

    
  
Comment on attachment 8818987 [details]
Bug 1314091 - Remove devtools-specific logic out of browser-devedition and into devtools-browser;

https://reviewboard.mozilla.org/r/98868/#review99816

::: devtools/client/framework/devtools-browser.js:146
(Diff revision 1)
> +    let devtoolsTheme = Services.prefs.getCharPref("devtools.theme");
> +    if (devtoolsTheme != "dark") {
> +      devtoolsTheme = "light";
> +    }
> +
> +    win.document.documentElement.setAttribute("devtoolstheme", devtoolsTheme);

Do we actually still need to do this? And shouldn't this attribute be renamed? How do we want this syncing to work now that we're decoupling 'devtools' and thesee compact themes? I'm sorry if that was supposed to be obvious from the bug - I missed it...

        Attachment #8818987 -
        Flags: review-

    
    

    
  
General trend here looks OK, though I think I would find it easier to review if all the renaming/moving came first. Maybe that's just me. Also feels like we should be able to get rid of the light/dark attribute entirely and just use :-moz-lwtheme-brighttext.

    
  

        Attachment #8816497 -
        Flags: feedback?(gijskruitbosch+bugs) → feedback+

    
    

    
  
Tracking 53+ for this change in compact themes. Marking 53 as affected as well.

          status-firefox53:
          --- → affected

          tracking-firefox53:
          ? → +

    
    

    
  
(In reply to :Gijs from comment #42)
> General trend here looks OK, though I think I would find it easier to review
> if all the renaming/moving came first.

Sure, I'll reorder the patches

> Also feels like
> we should be able to get rid of the light/dark attribute entirely and just
> use :-moz-lwtheme-brighttext.

Hm, good point I'll look into that

    
    

    
  
(In reply to :Gijs from comment #41)
> Comment on attachment 8818987 [details]
> Bug 1314091 - Remove devtools-specific logic out of browser-devedition and
> into devtools-browser
> 
> https://reviewboard.mozilla.org/r/98868/#review99816
> 
> ::: devtools/client/framework/devtools-browser.js:146
> (Diff revision 1)
> > +    let devtoolsTheme = Services.prefs.getCharPref("devtools.theme");
> > +    if (devtoolsTheme != "dark") {
> > +      devtoolsTheme = "light";
> > +    }
> > +
> > +    win.document.documentElement.setAttribute("devtoolstheme", devtoolsTheme);
> 
> Do we actually still need to do this? And shouldn't this attribute be
> renamed? How do we want this syncing to work now that we're decoupling
> 'devtools' and thesee compact themes? I'm sorry if that was supposed to be
> obvious from the bug - I missed it...

We still need to use it for parts of devtools that live in browser.xul - specifically the splitter between the page and the toolbox and gcli  like https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/splitters.css#23 and https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/commandline.css

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed up a reordering

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #44) 
> > Also feels like
> > we should be able to get rid of the light/dark attribute entirely and just
> > use :-moz-lwtheme-brighttext.
> 
> Hm, good point I'll look into that

Good call, that works.  Pushed up a change with :-moz-lwtheme-brighttext and :-moz-lwtheme-darktext - will go through other commits later to remove any remaining references / setting of the compacttheme attribute.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to :Gijs from comment #40)
> Comment on attachment 8816497 [details]
> Bug 1314091 - WIP Expose 'compact' themes instead of the Dev Edition theme
> 
> https://reviewboard.mozilla.org/r/97212/#review99808
> 
> ::: browser/base/content/browser-devedition.js:78
> (Diff revision 3)
> >    _inferBrightness: function() {
> >      ToolbarIconColor.inferFromText();
> > -    // Get an inverted full screen button if the dark theme is applied.
> > -    if (this.isStyleSheetEnabled &&
> > -        document.documentElement.getAttribute("devtoolstheme") == "dark") {
> > +
> > +    if (this.isStyleSheetEnabled) {
> > +      if (this.isCurrentThemeIdDark) {
> > -      document.documentElement.setAttribute("brighttitlebarforeground", "true");
> > +        document.documentElement.setAttribute("brighttitlebarforeground", "true");
> 
> Do we not want to do this for dark lwthemes generally? I also wonder if this
> is worth keeping given that the fullscreen button is now only supported on
> 10.9, which we do still support but only barely - given we now support
> 10.12, feels like 10.9 should be gone soon...
> 

Sorry for any confusion - this is removed later on in the last commit in the series: https://reviewboard.mozilla.org/r/98870/diff/3.  I can fold these together if you think that makes more sense.

> ::: browser/base/content/browser-devedition.js:127
> (Diff revision 3)
> > +      let compactTheme = this.isCurrentThemeIdDark ? "dark" : "light";
> > +      document.documentElement.setAttribute("compacttheme", compactTheme);
> 
> Feels like this should happen before enabling and/or adding the sheet to
> avoid flashes of partially styled content (given OMT
> style/compositor/rendering stuff).

This isn't needed anymore since using :-moz-lwtheme-brighttext in the css, so I'll remove it

> ::: browser/base/content/test/general/browser_devedition.js:62
> (Diff revision 3)
> > -add_task(function* testDevtoolsTheme() {
> > -  info("Checking stylesheet and :root attributes based on devtools theme.");
> > -  Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "light");
> > -  is(document.documentElement.getAttribute("devtoolstheme"), "light",
> > -    "The documentElement has an attribute based on devtools theme.");
> > -  ok(DevEdition.isStyleSheetEnabled, "The devedition stylesheet is still there with the light devtools theme.");
> > +add_task(function* testLightweightThemePreview() {
> > +  if (SKIP_TEST) {
> > +    ok(true, "No need to run this test since themes aren't installed");
> > +    return;
> > +  }
> > +  info("Setting compact to current and the previewing others");
> 
> Nit: spelling

I think this is talking about the extra 'the', maybe I'm missing something else but I'll update that part

> ::: browser/base/content/test/general/browser_devedition.js
> (Diff revision 3)
> > -
> > -  Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "foobar");
> > -  is(document.documentElement.getAttribute("devtoolstheme"), "light",
> > -    "The documentElement has 'light' as a default for the devtoolstheme attribute");
> > -  ok(DevEdition.isStyleSheetEnabled, "The devedition stylesheet is still there with the foobar devtools theme.");
> > -  ok(!document.documentElement.hasAttribute("brighttitlebarforeground"),
> > -     "The brighttitlebarforeground attribute is not set on the window with light devtools theme.");
> 
> Why is this being removd? I thought this code still existed in this cset -
> maybe this removal belongs in a different cset?

Yes, this removal belongs with the last change - see comment earlier about if you want those folded in here.

> ::: browser/base/content/test/general/browser_devedition.js
> (Diff revision 3)
> > -add_task(function* testLightweightThemePreview() {
> > -  info("Setting devedition to current and the previewing others");
> > -  LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme("firefox-devedition@mozilla.org");
> > -  ok(DevEdition.isStyleSheetEnabled, "The devedition stylesheet is enabled.");
> 
> Why are we removing this test?

This test's logic has been moved above the dummyLightweightTheme function and the previous one was removed (about the devtools attribute).  I'll reorder so the diff is easier to follow.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8816497 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
  
Comment on attachment 8818988 [details]
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;

>+:root:-moz-lwtheme-brighttext .findbar-closebutton:not(:hover),

This doesn't really make sense, you should just use .findbar-closebutton:-moz-lwtheme-brighttext:not(:hover). The same goes for other parts of this patch where you used the same pattern.

        Attachment #8819038 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #79)
> Comment on attachment 8818988 [details]
> Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;
> 
> >+:root:-moz-lwtheme-brighttext .findbar-closebutton:not(:hover),
> 
> This doesn't really make sense, you should just use
> .findbar-closebutton:-moz-lwtheme-brighttext:not(:hover). The same goes for
> other parts of this patch where you used the same pattern.

Alright, thanks - I just did a find/replace so didn't consider that.  The only thing I'm worried about is changing specificity when modifying the selector

    
  
Depends on: 1329207

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #60)
> (In reply to :Gijs from comment #40)
> > Comment on attachment 8816497 [details]
> > Bug 1314091 - WIP Expose 'compact' themes instead of the Dev Edition theme
> > 
> > https://reviewboard.mozilla.org/r/97212/#review99808
> > 
> > ::: browser/base/content/browser-devedition.js:78
> > (Diff revision 3)
> > >    _inferBrightness: function() {
> > >      ToolbarIconColor.inferFromText();
> > > -    // Get an inverted full screen button if the dark theme is applied.
> > > -    if (this.isStyleSheetEnabled &&
> > > -        document.documentElement.getAttribute("devtoolstheme") == "dark") {
> > > +
> > > +    if (this.isStyleSheetEnabled) {
> > > +      if (this.isCurrentThemeIdDark) {
> > > -      document.documentElement.setAttribute("brighttitlebarforeground", "true");
> > > +        document.documentElement.setAttribute("brighttitlebarforeground", "true");
> > 
> > Do we not want to do this for dark lwthemes generally? I also wonder if this
> > is worth keeping given that the fullscreen button is now only supported on
> > 10.9, which we do still support but only barely - given we now support
> > 10.12, feels like 10.9 should be gone soon...
> > 
> 
> Sorry for any confusion - this is removed later on in the last commit in the
> series: https://reviewboard.mozilla.org/r/98870/diff/3.  I can fold these
> together if you think that makes more sense.

FYI I went ahead and folded this - I think it's easier to follow this way

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  
Depends on: 1329262

    
    

    
  

      
      
      
      

        Attached image
          
        theme-switching.gif
        — 
      

    


  
Updated screencast of switching themes in about:addons with latest code + icons + text

        Attachment #8818947 -
        Attachment is obsolete: true

    
    

    
  
Here's summary of progress / todos:

* Simplified and reordered the patch series based on feedback from Gijs
* Dropped the custom compacttheme attribute used for styling by using -moz-lwtheme-brighttext and -moz-lwtheme-darktext instead. This is one of a few code removals in the series due to more directly relying on the lightweight theming system (along with dropping special things like updateLWTBrightness and inferBrightness)
* Using updated icons and text for the themes in about:addons and customize mode

What still needs to be done:
* Handle migration for people currently using the dev edition theme (making sure the right one gets applied based on devtools theme color).  This'll be done in the main bug.
* Sync the devtools theme pref and the selected compact theme (so behavior matches what happens on dev edition have now).  This'll be done in the main bug.
* Set up compact themes with mozscreenshots.  This'll be done in Bug 1329262.
* Get an updated default theme icon as a png (see https://bugzilla.mozilla.org/show_bug.cgi?id=1323619#c7 for more info). This'll land in Bug 1329207.
* Trim recommended list of themes from 5 to 3.  This'll be done in Bug 1323833.

    
    

    
  
I'll look at this on Monday - sorry for the delay. :-(

    
    

    
  
Comment on attachment 8816499 [details]
Bug 1314091 - Rename devedition.* to compacttheme.*;

https://reviewboard.mozilla.org/r/97216/#review103774

r=me for the rename, though of course all of this will need to land together...

::: browser/base/content/test/general/browser_compacttheme.js:30
(Diff revision 9)
>  add_task(function* startTests() {
>    Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "dark");
>  
>    info("Setting the current theme to null");
>    LightweightThemeManager.currentTheme = null;
> -  ok(!DevEdition.isStyleSheetEnabled, "There is no devedition style sheet when no lw theme is applied.");
> +  ok(!CompactTheme.isStyleSheetEnabled, "There is no devedition style sheet when no lw theme is applied.");

Nit: we should update the messages here so they continue to make sense.

We should probably also ensure any intermittents filed against this bug (hopefully none exist, but...) get updated.

        Attachment #8816499 -
        Flags: review?(gijskruitbosch+bugs) → review+

    
    

    
  
Comment on attachment 8818989 [details]
Bug 1314091 - TO FOLD BEFORE LANDING.  Replace [devtoolstheme] with :-moz-lwtheme-darktext and :-moz-lwtheme-brighttext

https://reviewboard.mozilla.org/r/98872/#review103776

::: browser/themes/shared/compacttheme.inc.css:89
(Diff revision 7)
>    --pinned-tab-glow: radial-gradient(22px at center calc(100% - 2px), rgba(76,158,217,0.9) 13%, transparent 16%);
>  }
>  
>  /* Override the lwtheme-specific styling for toolbar buttons */
> -:root[devtoolstheme="light"],
> -:root[devtoolstheme="light"] toolbar:-moz-lwtheme {
> +:root:-moz-lwtheme-darktext,
> +:root:-moz-lwtheme-darktext toolbar:-moz-lwtheme {

You can use the -moz-lwtheme-bright/darktext selector directly on a descendant element, it doesn't only apply on the root, so this selector (and probably others) can be simplified further. :-)

::: browser/themes/shared/compacttheme.inc.css:200
(Diff revision 7)
> -%define selectorPrefix :root[devtoolstheme="dark"] 
> +%define selectorPrefix :root:-moz-lwtheme-brighttext 
>  %define selectorSuffix :-moz-lwtheme

So I wonder if we can omit the prefix here entirely and just have the suffix...

    
    

    
  
Comment on attachment 8816499 [details]
Bug 1314091 - Rename devedition.* to compacttheme.*;

https://reviewboard.mozilla.org/r/97216/#review103774

> Nit: we should update the messages here so they continue to make sense.
> 
> We should probably also ensure any intermittents filed against this bug (hopefully none exist, but...) get updated.

Oh, I see you updated messages in the later cset. We should probably fold before landing, maybe? Should still doublecheck intermittents, so leaving the issue for now.

    
    

    
  
Comment on attachment 8818988 [details]
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;

https://reviewboard.mozilla.org/r/98870/#review103782

Clearing review because we should have the migration in here and mozreview doesn't currently let you re-request review - but this looks good to me besides what's not here yet. :-)

::: browser/components/nsBrowserGlue.js:653
(Diff revision 7)
>  
> -    if (!AppConstants.RELEASE_OR_BETA) {
> +    if (AppConstants.INSTALL_COMPACT_THEMES) {
> -      let themeName = gBrowserBundle.GetStringFromName("deveditionTheme.name");
>        let vendorShortName = gBrandBundle.GetStringFromName("vendorShortName");
>  
> +      // XXX: Localize + Add custom icons

This "XXX" comment should probably also be addressed somewhere in this bug (at least the l10n part)?

::: browser/components/nsBrowserGlue.js:655
(Diff revision 7)
> -      let themeName = gBrowserBundle.GetStringFromName("deveditionTheme.name");
>        let vendorShortName = gBrandBundle.GetStringFromName("vendorShortName");
>  
> +      // XXX: Localize + Add custom icons
>        LightweightThemeManager.addBuiltInTheme({
> -        id: "firefox-devedition@mozilla.org",
> +        id: "firefox-compact-light@mozilla.org",

This cset should probably contain the migration as well. You should be able to just use a UI version migration in this file. That'll be a one-time thing, and we should check that the LWT manager code doesn't fail catastrophically with having the ID of a theme it doesn't know about anymore for the poor souls who do an (unsupported) backwards-migration by running the same profile in multiple versions or something.

        Attachment #8818988 -
        Flags: review?(gijskruitbosch+bugs)

    
    

    
  
Comment on attachment 8824467 [details]
Bug 1314091 - TO FOLD BEFORE LANDING Move pseudo onto descendents;

https://reviewboard.mozilla.org/r/102962/#review103786

::: browser/base/content/defaultthemes/compactdark.icon.svg:7
(Diff revision 1)
> +    .background {
> +      fill: #727780;
> +    }

All these classes are only used once here, so the SVG guide ( https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidelines ) says they should be attributes. I also believe at least one of the width/height or viewBox is unnecessary, as is the xlink namespace. Same for the other file.

::: browser/base/content/test/general/browser_compacttheme.js:56
(Diff revision 1)
>  function dummyLightweightTheme(id) {
>    return {
>      id,
>      name: id,
> -    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.header.png",
> +    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/compact.header.png",
>      iconURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.icon.png",

Shipping this icon to users just for this test seems silly, shouldn't we just get rid of it?

        Attachment #8824467 -
        Flags: review?(gijskruitbosch+bugs) → review+

    
    

    
  
Comment on attachment 8818988 [details]
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;

https://reviewboard.mozilla.org/r/98870/#review103782

> This "XXX" comment should probably also be addressed somewhere in this bug (at least the l10n part)?

Oh, I now see this is in the next commit. Sorry for missing that...

    
    

    
  
(In reply to :Gijs from comment #97)
> Comment on attachment 8818988 [details]
> Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;
> 
> https://reviewboard.mozilla.org/r/98870/#review103782
> 
> > This "XXX" comment should probably also be addressed somewhere in this bug (at least the l10n part)?
> 
> Oh, I now see this is in the next commit. Sorry for missing that...

Sorry for any confusion - I can fold these changes into the main patch

    
    

    
  
(In reply to :Gijs from comment #96)
> Comment on attachment 8824467 [details]
> Bug 1314091 - TO FOLD BEFORE LANDING Use updated icons for compact themes;
> 
> https://reviewboard.mozilla.org/r/102962/#review103786
> 
> ::: browser/base/content/defaultthemes/compactdark.icon.svg:7
> (Diff revision 1)
> > +    .background {
> > +      fill: #727780;
> > +    }
> 
> All these classes are only used once here, so the SVG guide (
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> SVG_Guidelines ) says they should be attributes. I also believe at least one
> of the width/height or viewBox is unnecessary, as is the xlink namespace.
> Same for the other file.

OK, I'll run them through svgo and make any other changes if necessary

> ::: browser/base/content/test/general/browser_compacttheme.js:56
> (Diff revision 1)
> >  function dummyLightweightTheme(id) {
> >    return {
> >      id,
> >      name: id,
> > -    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.header.png",
> > +    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/compact.header.png",
> >      iconURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.icon.png",
> 
> Shipping this icon to users just for this test seems silly, shouldn't we
> just get rid of it?

This file is also used in nsBrowserGlue as the headerURL for both compact themes

    
    

    
  
(In reply to :Gijs from comment #92)
> Comment on attachment 8816499 [details]
> Bug 1314091 - Rename devedition.* to compacttheme.*;
> 
> https://reviewboard.mozilla.org/r/97216/#review103774
> 
> r=me for the rename, though of course all of this will need to land
> together...
> 
> ::: browser/base/content/test/general/browser_compacttheme.js:30
> (Diff revision 9)
> >  add_task(function* startTests() {
> >    Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "dark");
> >  
> >    info("Setting the current theme to null");
> >    LightweightThemeManager.currentTheme = null;
> > -  ok(!DevEdition.isStyleSheetEnabled, "There is no devedition style sheet when no lw theme is applied.");
> > +  ok(!CompactTheme.isStyleSheetEnabled, "There is no devedition style sheet when no lw theme is applied.");
> 
> Nit: we should update the messages here so they continue to make sense.

Will do

> We should probably also ensure any intermittents filed against this bug
> (hopefully none exist, but...) get updated.

I don't see any here: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=browser_devedition&list_id=13385208, so I think we are good there

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #100)
> (In reply to :Gijs from comment #92)
> > Nit: we should update the messages here so they continue to make sense.
> 
> Will do

Going to make any remaining string changes in the main patch, and they'll be folded together before landing

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #99)
> > ::: browser/base/content/test/general/browser_compacttheme.js:56
> > (Diff revision 1)
> > >  function dummyLightweightTheme(id) {
> > >    return {
> > >      id,
> > >      name: id,
> > > -    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.header.png",
> > > +    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/compact.header.png",
> > >      iconURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.icon.png",
> > 
> > Shipping this icon to users just for this test seems silly, shouldn't we
> > just get rid of it?
> 
> This file is also used in nsBrowserGlue as the headerURL for both compact
> themes

I meant the devedition.icon.png file. Sorry if that was unclear. Are we using that elsewhere?
Flags: needinfo?(bgrinstead)

    
    

    
  
(In reply to :Gijs from comment #102)
> (In reply to Brian Grinstead [:bgrins] from comment #99)
> > > ::: browser/base/content/test/general/browser_compacttheme.js:56
> > > (Diff revision 1)
> > > >  function dummyLightweightTheme(id) {
> > > >    return {
> > > >      id,
> > > >      name: id,
> > > > -    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.header.png",
> > > > +    headerURL: "resource:///chrome/browser/content/browser/defaultthemes/compact.header.png",
> > > >      iconURL: "resource:///chrome/browser/content/browser/defaultthemes/devedition.icon.png",
> > > 
> > > Shipping this icon to users just for this test seems silly, shouldn't we
> > > just get rid of it?
> > 
> > This file is also used in nsBrowserGlue as the headerURL for both compact
> > themes
> 
> I meant the devedition.icon.png file. Sorry if that was unclear. Are we
> using that elsewhere?

Oh good point.  The only concerning thing I see is an entry in allowed-dupes.mn that talks about aurora branding: https://dxr.mozilla.org/mozilla-central/source/browser/installer/allowed-dupes.mn#222.  But I don't know why it's there, the branding logo should be in browser/branding/aurora.  I'll go ahead and remove it.
Flags: needinfo?(bgrinstead)

    
    

    
  
(In reply to :Gijs from comment #93)
> Comment on attachment 8818989 [details]
> Bug 1314091 - TO FOLD BEFORE LANDING.  Replace [devtoolstheme] with
> :-moz-lwtheme-darktext and :-moz-lwtheme-brighttext
> 
> https://reviewboard.mozilla.org/r/98872/#review103776
> 
> ::: browser/themes/shared/compacttheme.inc.css:89
> (Diff revision 7)
> >    --pinned-tab-glow: radial-gradient(22px at center calc(100% - 2px), rgba(76,158,217,0.9) 13%, transparent 16%);
> >  }
> >  
> >  /* Override the lwtheme-specific styling for toolbar buttons */
> > -:root[devtoolstheme="light"],
> > -:root[devtoolstheme="light"] toolbar:-moz-lwtheme {
> > +:root:-moz-lwtheme-darktext,
> > +:root:-moz-lwtheme-darktext toolbar:-moz-lwtheme {
> 
> You can use the -moz-lwtheme-bright/darktext selector directly on a
> descendant element, it doesn't only apply on the root, so this selector (and
> probably others) can be simplified further. :-)

OK, I'll do that in a new patch to make it easier to review.  Can fold that in before landing as well.

> ::: browser/themes/shared/compacttheme.inc.css:200
> (Diff revision 7)
> > -%define selectorPrefix :root[devtoolstheme="dark"] 
> > +%define selectorPrefix :root:-moz-lwtheme-brighttext 
> >  %define selectorSuffix :-moz-lwtheme
> 
> So I wonder if we can omit the prefix here entirely and just have the
> suffix...

I get a build error when omitting it entirely (browser/themes/shared/identity-block/icons.inc.css', 7, 'UNDEFINED_VAR', 'selectorPrefix') but it seems we could leave it empty and switch the suffix to :-moz-lwtheme-brighttext.

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #104)
> > ::: browser/themes/shared/compacttheme.inc.css:200
> > (Diff revision 7)
> > > -%define selectorPrefix :root[devtoolstheme="dark"] 
> > > +%define selectorPrefix :root:-moz-lwtheme-brighttext 
> > >  %define selectorSuffix :-moz-lwtheme
> > 
> > So I wonder if we can omit the prefix here entirely and just have the
> > suffix...
> 
> I get a build error when omitting it entirely
> (browser/themes/shared/identity-block/icons.inc.css', 7, 'UNDEFINED_VAR',
> 'selectorPrefix') but it seems we could leave it empty and switch the suffix
> to :-moz-lwtheme-brighttext.

This is the only place using selectorPrefix. You can remove it.

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #105)
> (In reply to Brian Grinstead [:bgrins] from comment #104)
> > > ::: browser/themes/shared/compacttheme.inc.css:200
> > > (Diff revision 7)
> > > > -%define selectorPrefix :root[devtoolstheme="dark"] 
> > > > +%define selectorPrefix :root:-moz-lwtheme-brighttext 
> > > >  %define selectorSuffix :-moz-lwtheme
> > > 
> > > So I wonder if we can omit the prefix here entirely and just have the
> > > suffix...
> > 
> > I get a build error when omitting it entirely
> > (browser/themes/shared/identity-block/icons.inc.css', 7, 'UNDEFINED_VAR',
> > 'selectorPrefix') but it seems we could leave it empty and switch the suffix
> > to :-moz-lwtheme-brighttext.
> 
> This is the only place using selectorPrefix. You can remove it.

Thanks, will do

    
    

    
  
(In reply to :Gijs from comment #95)
> ::: browser/components/nsBrowserGlue.js:655
> (Diff revision 7)
> > -      let themeName = gBrowserBundle.GetStringFromName("deveditionTheme.name");
> >        let vendorShortName = gBrandBundle.GetStringFromName("vendorShortName");
> >  
> > +      // XXX: Localize + Add custom icons
> >        LightweightThemeManager.addBuiltInTheme({
> > -        id: "firefox-devedition@mozilla.org",
> > +        id: "firefox-compact-light@mozilla.org",
> 
> This cset should probably contain the migration as well. You should be able
> to just use a UI version migration in this file. That'll be a one-time
> thing, and we should check that the LWT manager code doesn't fail
> catastrophically with having the ID of a theme it doesn't know about anymore
> for the poor souls who do an (unsupported) backwards-migration by running
> the same profile in multiple versions or something.

Just confirmed it's OK to have an uninstalled lw theme set in the pref as would be the case in this backwards-migration - you see  the default theme.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8818987 [details]
Bug 1314091 - Remove devtools-specific logic out of browser-devedition and into devtools-browser;

https://reviewboard.mozilla.org/r/98868/#review104136

Thanks a ton for migrating that to /devtools/!

        Attachment #8818987 -
        Flags: review?(poirot.alex) → review+

    
    

    
  
Comment on attachment 8818989 [details]
Bug 1314091 - TO FOLD BEFORE LANDING.  Replace [devtoolstheme] with :-moz-lwtheme-darktext and :-moz-lwtheme-brighttext

https://reviewboard.mozilla.org/r/98872/#review104138

This patch seems to make "Bug 1314091 - Remove devtools-specific logic out of browser-devedition" useless?
(also it is not obvious to me how -moz-lwtheme-darktext/-moz-lwtheme-brighttext are set in this patch, or what that already done in existing code?)

    
    

    
  
Comment on attachment 8818988 [details]
Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;

https://reviewboard.mozilla.org/r/98870/#review104216

::: browser/base/content/defaultthemes/compactdark.icon.svg:5
(Diff revision 8)
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - 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/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32">

I would not expect the width/height to be necessary if you're already listing a viewBox, and vice versa.

Same for the light icon.

        Attachment #8818988 -
        Flags: review?(gijskruitbosch+bugs) → review+

    
    

    
  
(In reply to Alexandre Poirot [:ochameau] from comment #115)
> Comment on attachment 8818989 [details]
> Bug 1314091 - TO FOLD BEFORE LANDING.  Replace [devtoolstheme] with
> :-moz-lwtheme-darktext and :-moz-lwtheme-brighttext
> 
> https://reviewboard.mozilla.org/r/98872/#review104138
> 
> This patch seems to make "Bug 1314091 - Remove devtools-specific logic out
> of browser-devedition" useless?
> (also it is not obvious to me how
> -moz-lwtheme-darktext/-moz-lwtheme-brighttext are set in this patch, or what
> that already done in existing code?)

I know it's confusing but the attribute was serving two purposes before:
1. Change colors for dev edition theme
2. Change colors for devtools stuff in browser.xul (gcli, toolbox splitter)

Now, that attribute is only doing (2) so it makes more sense in /devtools/

    
    

    
  
(In reply to :Gijs from comment #116)
> Comment on attachment 8818988 [details]
> Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;
> 
> https://reviewboard.mozilla.org/r/98870/#review104216
> 
> ::: browser/base/content/defaultthemes/compactdark.icon.svg:5
> (Diff revision 8)
> > +<?xml version="1.0" encoding="UTF-8"?>
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - 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/. -->
> > +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32">
> 
> I would not expect the width/height to be necessary if you're already
> listing a viewBox, and vice versa.
> 
> Same for the light icon.

Alright, I'll update that.  Do you have a preference for width/height vs viewBox?
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
(In reply to Brian Grinstead [:bgrins] from comment #118)
> (In reply to :Gijs from comment #116)
> > Comment on attachment 8818988 [details]
> > Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;
> > 
> > https://reviewboard.mozilla.org/r/98870/#review104216
> > 
> > ::: browser/base/content/defaultthemes/compactdark.icon.svg:5
> > (Diff revision 8)
> > > +<?xml version="1.0" encoding="UTF-8"?>
> > > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > > +   - 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/. -->
> > > +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32">
> > 
> > I would not expect the width/height to be necessary if you're already
> > listing a viewBox, and vice versa.
> > 
> > Same for the light icon.
> 
> Alright, I'll update that.  Do you have a preference for width/height vs
> viewBox?

Not really. :ntim might if you run into him.
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
(In reply to :Gijs from comment #119)
> (In reply to Brian Grinstead [:bgrins] from comment #118)
> > (In reply to :Gijs from comment #116)
> > > Comment on attachment 8818988 [details]
> > > Bug 1314091 - Expose 'compact' themes instead of the Dev Edition theme;
> > > 
> > > https://reviewboard.mozilla.org/r/98870/#review104216
> > > 
> > > ::: browser/base/content/defaultthemes/compactdark.icon.svg:5
> > > (Diff revision 8)
> > > > +<?xml version="1.0" encoding="UTF-8"?>
> > > > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > > > +   - 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/. -->
> > > > +<svg xmlns="http://www.w3.org/2000/svg" width="32" height="32" viewBox="0 0 32 32">
> > > 
> > > I would not expect the width/height to be necessary if you're already
> > > listing a viewBox, and vice versa.
> > > 
> > > Same for the light icon.
> > 
> > Alright, I'll update that.  Do you have a preference for width/height vs
> > viewBox?
> 
> Not really. :ntim might if you run into him.
I like width/height over viewBox, when viewBox is redundant (this is the case here). width/height actually constrains the dimensions when you open an SVG in a tab, or when you set an SVG as background-image, while viewBox doesn't.

    
    

    
  
Comment on attachment 8824467 [details]
Bug 1314091 - TO FOLD BEFORE LANDING Move pseudo onto descendents;

Sorry - I think I confused mozreview with this patch.  This one does need a review but it looks like it somehow carried over an r+ from a different patch in the series.

        Attachment #8824467 -
        Flags: review+ → review?(gijskruitbosch+bugs)

    
  
Flags: qe-verify+

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Heads-up: this bug is going to conflict/race with bug 1306561.

    
    

    
  
Comment on attachment 8824467 [details]
Bug 1314091 - TO FOLD BEFORE LANDING Move pseudo onto descendents;

https://reviewboard.mozilla.org/r/102962/#review104552

Looks OK. Thanks!

        Attachment #8824467 -
        Flags: review?(gijskruitbosch+bugs) → review+

    
    

    
  
Comment on attachment 8825580 [details]
Bug 1314091 - Sync the applied devtools toolbox theme and compact themes;

https://reviewboard.mozilla.org/r/103710/#review104612

Are you also doing the opposite operation somewhere?
Changing devtools theme when you install light/dark lightweight theme?

::: devtools/client/framework/devtools-browser.js:165
(Diff revision 1)
> +    if (currentLighweightThemeID == COMPACT_LIGHT_ID && devtoolsTheme == "dark") {
> +      LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme(COMPACT_DARK_ID);
> +    }
> +    if (currentLighweightThemeID == COMPACT_DARK_ID && devtoolsTheme == "light") {
> +      LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme(COMPACT_LIGHT_ID);
> +    }

You may use shorter variable name to ease reading.
(currentTheme, currentThemeID)

        Attachment #8825580 -
        Flags: review?(poirot.alex) → review+

    
    

    
  
(In reply to Alexandre Poirot [:ochameau] from comment #132)
> Comment on attachment 8825580 [details]
> Bug 1314091 - Sync the applied devtools toolbox theme and compact themes;
> 
> https://reviewboard.mozilla.org/r/103710/#review104612
> 
> Are you also doing the opposite operation somewhere?
> Changing devtools theme when you install light/dark lightweight theme?

Yes that's done in this patch in the "lightweight-theme-changed" observer.

> ::: devtools/client/framework/devtools-browser.js:165
> (Diff revision 1)
> > +    if (currentLighweightThemeID == COMPACT_LIGHT_ID && devtoolsTheme == "dark") {
> > +      LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme(COMPACT_DARK_ID);
> > +    }
> > +    if (currentLighweightThemeID == COMPACT_DARK_ID && devtoolsTheme == "light") {
> > +      LightweightThemeManager.currentTheme = LightweightThemeManager.getUsedTheme(COMPACT_LIGHT_ID);
> > +    }
> 
> You may use shorter variable name to ease reading.
> (currentTheme, currentThemeID)

Done

    
    

    
  
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05090352ae09
Backed out changeset 60d0080b95fe (bug 1308407)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cef98a724789
Rename devedition.* to compacttheme.*;r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/4333fc0c05eb
Expose 'compact' themes instead of the Dev Edition theme;r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/b17adc402c82
Remove devtools-specific logic out of browser-devedition and into devtools-browser;r=ochameau
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bbe0c7e6489
Sync the applied devtools toolbox theme and compact themes;r=ochameau

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/05090352ae09
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox53:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1331276
Depends on: 1331369
Depends on: 1331449

    
  
Blocks: 1331679

    
    

    
  
Release Note Request (optional, but appreciated)
[Why is this notable]: Two new built-in themes available
[Affects Firefox for Android]: No
[Suggested wording]: There are two new 'compact' themes available in Firefox, a dark and light version based on the Developer Edition theme 
[Links (documentation, blog post, etc)]: https://groups.google.com/forum/#!msg/firefox-dev/BKJQ8V8pd0o/jVFWD_EOEQAJ

          relnote-firefox:
          --- → ?

    
  
Depends on: 1332201
No longer depends on: 1332201

    
  
Depends on: 1332375
Depends on: 1339754
Depends on: 1339859

    
    

    
  
[Test Plan]:
https://wiki.mozilla.org/QA/Compact Themes
QA Contact: ciprian.georgiu

    
    

    
  
This feature is verified fixed per our pre-Release sign off on 53 beta.
Status: RESOLVED → VERIFIED

          status-firefox53:
          fixed → verified
Flags: qe-verify+

    
  
Depends on: 1354332

    
  
Depends on: 1356863

    
  
Depends on: 1358468

    
  
Depends on: 1416822

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: