This is used just to include toolboars.inc.css, whereas we could use @import, unless I miss something??

I missed the %ifdef... I thought #ifdef was enough, but no. I hope there isn't yet another preprocessing syntac I missed!! Otherwise, this time I had to add the :root hack. I hooked into theme-switching.js which is already evaluted in all our panels. I'm not using an attribute, but instead a class. Tell me if you prefer another JS location, another more verbose class names or using an attribute.

Comment on attachment 8680614 [details] [diff] [review] patch v2 Review of attachment 8680614 [details] [diff] [review]: ----------------------------------------------------------------- There's a reference to toolbars.inc.css in a comment for memory tool, can you modify that to point to the new file name? https://dxr.mozilla.org/mozilla-central/search?q=toolbars.inc.css. Also, I prefer using an attribute (and in a quick poll on irc a few people agreed) :root[platform=mac] :root[platform=win] :root[platform=linux] I think having platform as part of the string makes it a bit more clear what's going on, and since you'll only ever be on one platform at a time an attribute seems reasonable. (Of course themes are the same way and we have a class there instead of an attribute, but that's another story). ::: devtools/client/shared/theme-switching.js @@ +112,5 @@ > + if (platform.startsWith("Win")) { > + os = "win"; > + } else if (platform.startsWith("Mac")) { > + os = "mac"; > + } else if (platform.startsWith("Linux") || platform.startsWith("SunOS")) { Should this be `else {os="linux"}`, given that one of the examples here: https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/platform is "FreeBSD i386"? And I can't think of a good reason for not having a platform set. If so, then maybe this: let platform = navigator.platform; if (platform.startsWith("Win")) { documentElement.setAttribute("platform", "win"); } else if (platform.startsWith("Mac")) { documentElement.setAttribute("platform", "mac"); } else { documentElement.setAttribute("platform", "linux"); } ::: devtools/client/themes/light-theme.css @@ +3,5 @@ > * 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/. */ > > @import url(variables.css); > +@import url(toolbars.css); There's a chance we could have visual regressions from moving this up above the other rules in this file if things were relying on that ordering... But I think it's better this way so we should do some smoke tests and then go ahead and land early in the cycle so we have a chance to catch regressions.

Uses platform attribute and defaults to linux. (In reply to Brian Grinstead [:bgrins] from comment #5) > I think having platform as part of the string makes it a bit more clear > what's going on, and since you'll only ever be on one platform at a time an > attribute seems reasonable. (Of course themes are the same way and we have > a class there instead of an attribute, but that's another story). Yes, I thought about that but found class to be easier, more common in CSS and closer to what we do for themes. I went ahead with attribute, but note that we can make class more explicit by using another class names like: .platform-win, .platform-mac, .platform-linux I would be happy to change to that if that makes sense to you. > > ::: devtools/client/shared/theme-switching.js > @@ +112,5 @@ > > + if (platform.startsWith("Win")) { > > + os = "win"; > > + } else if (platform.startsWith("Mac")) { > > + os = "mac"; > > + } else if (platform.startsWith("Linux") || platform.startsWith("SunOS")) { > > Should this be `else {os="linux"}`, given that one of the examples here: > https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/platform is > "FreeBSD i386"? Yes, I default now to linux, it is easier like this. > ::: devtools/client/themes/light-theme.css > @@ +3,5 @@ > > * 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/. */ > > > > @import url(variables.css); > > +@import url(toolbars.css); > > There's a chance we could have visual regressions from moving this up above > the other rules in this file if things were relying on that ordering... But > I think it's better this way so we should do some smoke tests and then go > ahead and land early in the cycle so we have a chance to catch regressions. I had to. https://developer.mozilla.org/en/docs/Web/CSS/@import#Summary "The @import CSS at-rule is used to import style rules from other style sheets. These rules must precede all other types of rules" Hopefully this restriction won't prevent us from removing other import-like preprocessing!!

Comment on attachment 8681977 [details] [diff] [review] patch v3 Review of attachment 8681977 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, and I've tested locally and don't see any obvious regressions, but we will handle those if they come up in follow up bugs. Can you add a new test to framework/test called browser_toolbox_theme_switching.js or similar that opens up a toolbox and makes sure that the toolbox frame doc element's 'platform' attribute is set and matches what's expected? To find the expected value, I guess the test could either be preprocessed, check AppConstants, or check navigator.platform directly.

With a test, devtools/client/shared/test/browser_theme_switching.js in order to be closer to the code it test. (Note that it would be great to have such request upfront during the review in order to avoid unecessary cycles)

Comment on attachment 8682212 [details] [diff] [review] patch v4 Review of attachment 8682212 [details] [diff] [review]: ----------------------------------------------------------------- r=me with add_task ::: devtools/client/shared/test/browser_theme_switching.js @@ +3,5 @@ > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +var toolbox; > + > +function test() Please use add_task format for new tests. Something like this would work: add_task(function*() { let target = TargetFactory.forTab(gBrowser.selectedTab); let toolbox = yield gDevTools.showToolbox(target); let root = toolbox.frame.contentDocument.documentElement; let expectedPlatform = getPlatform(); let platform = root.getAttribute("platform"); is(platform, expectedPlatform, ":root[platform] is correct"); let theme = Services.prefs.getCharPref("devtools.theme"); let className = "theme-" + theme; ok(root.classList.contains(className), ":root has " + className + " class (current theme)"); yield toolbox.destroy(); }); function getPlatform() { let expectedPlatform; let {OS} = Services.appinfo; if (OS == "WINNT") { expectedPlatform = "win"; } else if (OS == "Darwin") { expectedPlatform = "mac"; } else { expectedPlatform = "linux"; } return expectedPlatform; }

Test with add_task.

https://hg.mozilla.org/integration/fx-team/rev/a081f1e22fdc054910923948099f878e73e92f37 Bug 1219613 - Stop preprocessing devtools theme css files. r=bgrins