896733 - Move remaining platform specific devtools theme files into themes/shared/

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Brandon Benvie [:benvie]]

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".

Comment 1

[Victor Porof [:vporof][:vp]]

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.

Comment 2

[Mihai Sucan [:msucan]]

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

Comment 3

[Brandon Benvie [:benvie]]

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.

Comment 4

[Paul Rouget [:paul]]

(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.

Comment 5

[Mihai Sucan [:msucan]]

(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.

Comment 6

[Paul Rouget [:paul]]

Brian, can you dupe this?

Comment 7

[Brian Grinstead [:bgrins]]

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.

Comment 8

[Brian Grinstead [:bgrins]]

(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.

Comment 9

[Brian Grinstead [:bgrins]]

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.

Comment 10

[Brian Grinstead [:bgrins]]

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

Comment 11

[Brian Grinstead [:bgrins]]

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

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

Comment 12

[Brian Grinstead [:bgrins]]

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

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

Comment 13

[Brian Grinstead [:bgrins]]

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 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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.

Comment 16

[Brian Grinstead [:bgrins]]

(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

Comment 17

[Brian Grinstead [:bgrins]]

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

Comment 18

[Pulsebot]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Tim Nguyen :ntim]

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.

Comment 21

[Brian Grinstead [:bgrins]]

(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