There is a massive amount of duplicate CSS between the different platform CSS for devtools. I would guess that at least 95% of the CSS is common across platforms but for most tools it is repeated. This imposes a maintenance tax because every change needs to be done three times, and verification that there isn't some platform specific subtlety being overlooked. All the common CSS should be factored out from the platform CSS and added to shared. Then each platform should %include from the shared CSS and add on any platform specifics. A good existing example of this in action is "common/devtools/webconsole.inc.css" which is imported from "<platform>/devtools/webconsole.css".

The only downside I can find to having css includes and moving all common/duplicate css to a parent file is that preprocessing them takes a build step. So, at the moment, if one was to make a change in a css file and run the build, the changes would be applied instantly (at least on OS X and Linux, some magic may need to be done on Windows to link those files instead of copying them, I don't know). With preprocessed files, an incremental build would be required first (which doesn't really take much time, but if you find yourself battling with CSS and start poking it until it works, then extra repeated build steps seem to take forever). However, I think the above issue will be alleviated once the Style Editor fully works with chrome stylesheets, so this bug has my vote.

Can we avoid preprocessing? This is also a problem with the addonsdk loader. Could we use @import instead of %include?

I was wondering that as well. I don't know if it's a factor, but %including the same thing multiple times also bloats size, so it'd be a win if we could do @import.

(In reply to Mihai Sucan [:msucan] from comment #2) > Can we avoid preprocessing? This is also a problem with the addonsdk loader. > Could we use @import instead of %include? We use %include everywhere in the Firefox code base. We started using %include for the devtools as well. On Linux and Windows, the incremental build is very fast (only the preprocessing is needed). On Mac, you have to repackage `browser/app` (this is the slow part). I don't think it's a good reason to slow down devtools startup.

(In reply to Paul Rouget [:paul] from comment #4) > I don't think it's a good reason to slow down devtools startup. Incremental builds are very fast, indeed. 5 seconds, at most, on my system. My point wasn't about this aspect, really. It was about the problem we have with the jetpack loader which cannot handle preprocessing. We would like to allow contributors (and even us) to easily and quickly prototype changes in devtools code, without having to restart/rebuild Firefox. I have patches in-flight for switching the webconsole to the jetpack loader and missing the CSS reload is rather important for most of the console UI hacking.

Brian, can you dupe this?

I'm not sure that there is another bug already opened for this, but it is something we have talked about in Bug 916766, and I have started on with the toolbox in Bug 941673.

(In reply to Mihai Sucan [:msucan] from comment #5) > (In reply to Paul Rouget [:paul] from comment #4) > > I don't think it's a good reason to slow down devtools startup. > > Incremental builds are very fast, indeed. 5 seconds, at most, on my system. > My point wasn't about this aspect, really. It was about the problem we have > with the jetpack loader which cannot handle preprocessing. We would like to > allow contributors (and even us) to easily and quickly prototype changes in > devtools code, without having to restart/rebuild Firefox. I have patches > in-flight for switching the webconsole to the jetpack loader and missing the > CSS reload is rather important for most of the console UI hacking. Are you still switching over to the jetpack loader for the webconsole? I've considered following the pattern of themes/shared/devtools/webconsole.inc.css for the other panels, and want to make sure this makes sense and doesn't cause any problems.

We should definitely move the icons out of the individual theme folders and into the shared theme. We have an alternate color on the toolbox icons for the light theme in Bug 916766, and it would be great to not to add three copies of these new icons.

Going to wrap this up by moving the final instances of browser/themes/[linux|osx|windows]/devtools/* into browser/themes/shared/devtools/*

Bug 896733 - Move remaining platform-specific devtools styling into shared themes folder;r=jryans

Bug 896733 - Import the important platform-specific CSS changes into the shared devtools theme folder;r=jryans

Note: there were a few places where I didn't completely copy the windows/linux/osx styles if I checked and they weren't needed. I think there's some places that they were just unintentionally different. Specifically, I unified the floating-scrollbar width to 10px across all platforms (osx used to be 8px) and there was a rule in linux/webconsole.css for `.jsterm-input-node { width: 98%; }` but after checking in a local build I didn't think it was necessary. Same for some linux widget.css changes to do with the checkboxes seen in debuggers 'events' pane. The netmonitor changes I wasn't sure about so I just directly copied in the linux/windows changes with a preprocessor. Ideally we can unify the markup/css there as time goes on (I've filed Bug 1183280 for that), but in the meantime I'm happy to get all of our theme styles in browser/themes/shared/devtools.

Comment on attachment 8648299 [details] MozReview Request: Bug 896733 - Move remaining platform-specific devtools styling into shared themes folder;r=jryans https://reviewboard.mozilla.org/r/16171/#review14545 r+ assuming you revert the non-DevTools changes. ::: browser/themes/windows/jar.mn:43 (Diff revision 1) > - skin/classic/browser/fullscreen/insecure.svg (../shared/fullscreen/insecure.svg) > + skin/classic/browser/fullscreen-darknoise.png What is this about? It seems unrelated to DevTools. ::: browser/themes/windows/jar.mn:202 (Diff revision 1) > - skin/classic/browser/controlcenter/tracking-protection.svg (../shared/controlcenter/tracking-protection.svg) > + skin/classic/browser/controlcenter/tracking-protection.svg (../shared/controlcenter/tracking-protection.svg) What is this about? It seems unrelated to DevTools.

Comment on attachment 8648300 [details] MozReview Request: Bug 896733 - Import the important platform-specific CSS changes into the shared devtools theme folder;r=jryans https://reviewboard.mozilla.org/r/16173/#review14561 ::: browser/themes/shared/devtools/netmonitor.css:857 (Diff revision 1) > +/* Nothing needed for OSX, move on to Linux*/ It's potentially simpler to just `%endif` here and open a new a block with `%ifdef XP_LINUX` and skip Mac.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14) > Comment on attachment 8648299 [details] > MozReview Request: Bug 896733 - Move remaining platform-specific devtools > styling into shared themes folder;r=jryans > > https://reviewboard.mozilla.org/r/16171/#review14545 > > r+ assuming you revert the non-DevTools changes. > > ::: browser/themes/windows/jar.mn:43 > (Diff revision 1) > > - skin/classic/browser/fullscreen/insecure.svg (../shared/fullscreen/insecure.svg) > > + skin/classic/browser/fullscreen-darknoise.png > > What is this about? It seems unrelated to DevTools. > > ::: browser/themes/windows/jar.mn:202 > (Diff revision 1) > > - skin/classic/browser/controlcenter/tracking-protection.svg (../shared/controlcenter/tracking-protection.svg) > > + skin/classic/browser/controlcenter/tracking-protection.svg (../shared/controlcenter/tracking-protection.svg) > > What is this about? It seems unrelated to DevTools. Thanks, good catches. Just errors when rebasing

Alright, got a try push with the updates included: https://treeherder.mozilla.org/#/jobs?repo=try&revision=133a697331b4

https://hg.mozilla.org/integration/fx-team/rev/58c0cb4ec593 https://hg.mozilla.org/integration/fx-team/rev/5d1eab75ad1e

https://hg.mozilla.org/mozilla-central/rev/58c0cb4ec593 https://hg.mozilla.org/mozilla-central/rev/5d1eab75ad1e

https://reviewboard.mozilla.org/r/16173/#review14895 ::: browser/themes/shared/devtools/floating-scrollbars.css:10 (Diff revision 1) > +%ifdef XP_MACOSX The ifdef looks unnecessary as none of our platforms have a border in the floating scrollbars styling. Using border: none; shouldn't hurt too. ::: browser/themes/shared/devtools/floating-scrollbars.css:29 (Diff revision 1) > +%ifdef XP_MACOSX The ifdef here also looks unneccessary to me since most of the rules below are similar. ::: browser/themes/shared/devtools/widgets.css:223 (Diff revision 1) > +%ifdef XP_WIN I think it's safe to remove the ifdef here, since all platforms hide the border all the time. We can even remove this rule altogether and add !important to the border: none; declaration above.

(In reply to Tim Nguyen [:ntim] (mostly away until 26 August) from comment #20) > https://reviewboard.mozilla.org/r/16173/#review14895 > > ::: browser/themes/shared/devtools/floating-scrollbars.css:10 > (Diff revision 1) > > +%ifdef XP_MACOSX > > The ifdef looks unnecessary as none of our platforms have a border in the > floating scrollbars styling. > Using border: none; shouldn't hurt too. > > ::: browser/themes/shared/devtools/floating-scrollbars.css:29 > (Diff revision 1) > > +%ifdef XP_MACOSX > > The ifdef here also looks unneccessary to me since most of the rules below > are similar. > > ::: browser/themes/shared/devtools/widgets.css:223 > (Diff revision 1) > > +%ifdef XP_WIN > > I think it's safe to remove the ifdef here, since all platforms hide the > border all the time. We can even remove this rule altogether and add > !important to the border: none; declaration above. Let's move that work into a follow up. We have Bug 1183280 already, but that's about removing all preprocessor usage. I'll file a new bug