706103 - Replace chrome copy of files by overrides

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

+++ This bug was initially created as a clone of Bug #659407 +++

https://bugzilla.mozilla.org/show_bug.cgi?id=647453#c14

This bug is for what was originally part 5 of bug 659407 (attachment 537087 [details] [diff] [review]).
In order to do this, we need a way to avoid running into bug 702558.

Comment 1

[Pardal Freudenthal (:ShareBird)]

I've already posted this at the google groups (http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/86d6adcd1d567f64#)

I have an alternative approach instead of using overrides. I've described and discussed this solution at MozillaZine: http://forums.mozillazine.org/viewtopic.php?f=18&t=906535 

The solution I'm proposing for the original issue consists in registering a skin-only package (which I called os_target) containing directories that host images and different codes for the various operating systems. Through manifest flags is possible to register these directories differently according to the operating system. Via @import rules we can insert everything that is different into files that contain the common code.

This could solve also the problems with different images for different OSs. For example we could just use something like:

myButton {
  list-style-image: url(chrome://os_target/skin/myImage.png);
}

This approach has been used by me and several other developers for nearly three  years with great success.

I do not have exact measurements, but I believe the Winstripe could be decreased by approximately 40%, and if we think that this method allows us to develop a single theme for all operating systems, we can have an idea what this mean in terms of maintenance and size of the source.

Comment 2

[Dão Gottwald [:dao]]

(In reply to Pardal Freudenthal (:ShareBird) from comment #1)
We should experiment with this in bug 702558, as there are way fewer affected images in pinstripe than in winstripe.

Comment 5

[Pardal Freudenthal (:ShareBird)]

Unfortunately I don't have the time to work in some Bug to demonstrate the approach I'm suggesting on comment 1.
This approach could solve several issues:
1. The use of overrides like at Bug 960517, that causes several breakages for third-party full themes.
2. The unnecessary duplicate of files (code and images) overall at the theme.
3. The possibility to make a simple theme that works on all platforms, smaller as the winstripe and serving as basis for third-party themes that want to support all platforms.

The approach is really very, very simple. For sure I could help if necessary, but I'm unable for now to be the assigned to some Bug

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #2)
> (In reply to Pardal Freudenthal (:ShareBird) from comment #1)
> We should experiment with this in bug 702558, as there are way fewer
> affected images in pinstripe than in winstripe.

Dão, I have some questions here, as I think bug 702558 is not a viable target to fix this (AIUI we want to get rid of any lion-specificness that's leftover on OS X anyway).

- what (if any) downsides do you expect to this solution?
- can we start converting our aero stuff piece by piece (I think so?)
- I think fixing this should be in our backlog (possibly the prioritized backlog as well), because the aero/non-aero distinction and having to duplicate all our manifest entries on Windows has repeatedly caused bugs (in one case, something we had to back out post-last-beta on 29). Would you agree or do you see issues with going forward with this?

Comment 7

[:Gijs (he/him)]

Dão, ping re the needinfo in comment #6 ?

Comment 8

[Pardal Freudenthal (:ShareBird)]

As I said before, if you need any help to fix this using the "os_target" approach, please let me know.

Comment 9

[Dão Gottwald [:dao]]

Talked to Gijs in person, he's going to try to put together a proof-of-concept patch.

Comment 10

[Dão Gottwald [:dao]]

I'll work on this when I'm back from vacation.

Comment 11

[Dão Gottwald [:dao]]

So, I think introducing a new package would work but also make the overall theme structure more complicated, with some files living in the browser package and some in the other one. I'd like to avoid that if possible, as it feels like replacing one hack with different one.

In bug 702558, Dave wrote:

> An alternative would be to put the overrides (indeed all the default theme
> manifest directives) into a manifest file in the default theme's directory.
> That's an additional file to process on startup though so perhaps not what
> we want. It would provide better separation between the themes though.

I'd like to try that and see how much, if at all, it affects startup performance. How would I make the build system spit out that separate manifest file? I just spent some time looking at the build system magic under browser/themes/ and reading some documentation at https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/build/buildsystem/index.html, but it didn't seem to enlighten me...

Comment 12

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #11)
> So, I think introducing a new package would work but also make the overall
> theme structure more complicated, with some files living in the browser
> package and some in the other one. I'd like to avoid that if possible, as it
> feels like replacing one hack with different one.
> 
> In bug 702558, Dave wrote:
> 
> > An alternative would be to put the overrides (indeed all the default theme
> > manifest directives) into a manifest file in the default theme's directory.
> > That's an additional file to process on startup though so perhaps not what
> > we want. It would provide better separation between the themes though.
> 
> I'd like to try that and see how much, if at all, it affects startup
> performance. How would I make the build system spit out that separate
> manifest file? I just spent some time looking at the build system magic
> under browser/themes/ and reading some documentation at
> https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/build/
> buildsystem/index.html, but it didn't seem to enlighten me...

I'm not sure I understand why this would help...

However, AFAICT everything in jar.mn things that goes into JAR_MANIFESTS goes into chrome.manifest, and so you can't really use that bit of the build architecture.

If we need things that are in jar.mn (like preprocessing) then we would need to write some makefile logic that invokes the jar manifest processor on our file and spits out the manifest we need.

If we don't need the preprocessor it should be possible to just have a .manifest file that we indicate needs to be shipped the way that we would indicate other files that need to be shipped (and removed on uninstall) ?

Does this help? If not, can you elaborate on what exactly is the bit that we need here?

Comment 13

[:Gijs (he/him)]

(the hardcoded manifest would probably mean adding stuff to http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/Makefile.in#9 et al. - which does seem to invoke the preprocessor already, so maybe it's possible to use some bits?)

Comment 14

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #12)
> > > An alternative would be to put the overrides (indeed all the default theme
> > > manifest directives) into a manifest file in the default theme's directory.
> > > That's an additional file to process on startup though so perhaps not what
> > > we want. It would provide better separation between the themes though.
> 
> I'm not sure I understand why this would help...

It would allow us to use overrides without affecting third-party themes.

> However, AFAICT everything in jar.mn things that goes into JAR_MANIFESTS goes into chrome.manifest,
> and so you can't really use that bit of the build architecture.

That's indeed the impression I got from the documentation, but I kind of refused to believe it...

(In reply to :Gijs Kruitbosch from comment #13)
> (the hardcoded manifest would probably mean adding stuff to
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/
> %7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/Makefile.in#9 et al. - which does
> seem to invoke the preprocessor already, so maybe it's possible to use some
> bits?)

Thanks, this seems to be the part of the code base I was looking for. Didn't even think of this since it's kind of an obscure corner I rarely ever had to look at over the years.

Comment 15

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #12)
> > > An alternative would be to put the overrides (indeed all the default theme
> > > manifest directives) into a manifest file in the default theme's directory.
> > > That's an additional file to process on startup though so perhaps not what
> > > we want. It would provide better separation between the themes though.
> 
> [...]
> If we need things that are in jar.mn (like preprocessing) then we would need
> to write some makefile logic that invokes the jar manifest processor on our
> file and spits out the manifest we need.

Mike or Gregory, does this sound about right to you?

Comment 16

[Dave Townsend [:mossop] (PTO until 6/3)]

(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > (the hardcoded manifest would probably mean adding stuff to
> > http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/
> > %7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/Makefile.in#9 et al. - which does
> > seem to invoke the preprocessor already, so maybe it's possible to use some
> > bits?)
> 
> Thanks, this seems to be the part of the code base I was looking for. Didn't
> even think of this since it's kind of an obscure corner I rarely ever had to
> look at over the years.

Yeah sticking a chrome.manifest into http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/extensions/%7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D/ is what I was talking about. The app already attempts to read it on startup (see bug 586610) so it should be relatively straightforward. Those manifest directives would only take effect when the default theme was applied.

Comment 17

[Mike Hommey [:glandium]]

I'm not quite sure what the question is, but I'll give some info that I think is relevant:
- % entries from jar.mn end up in a chrome.manifest in the relevant directory, the relevant directory depending on e.g. XPINAME and such.
- a chrome.manifest can also be "manually" installed in the relevant directory.
- a chrome.manifest in an extensions directory under dist/bin currently is going to do bad things within make package (IOW, the resulting firefox package won't contain what you'd expect it to), but you're lucky, it's going to be fixed soon in bug 910660.

Comment 18

[Dão Gottwald [:dao]]

Attachment: replace chrome copy of files by overrides in browser/themes/

Asking for review on this part while I work on the equivalent toolkit patch. Moving the manifest files will be yet another patch (maybe in a separate bug).

Comment 19

[Dão Gottwald [:dao]]

Attachment: replace chrome copy of files by overrides in browser/themes/

Forgot to recreate the patch after I made some last changes...

Comment 20

[Dão Gottwald [:dao]]

Attachment: replace chrome copy of files by overrides in toolkit/themes/

This one is slightly more straightforward

Comment 21

[:Gijs (he/him)]

Comment on attachment 8583789 [details] [diff] [review]
replace chrome copy of files by overrides in toolkit/themes/

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

These overrides are going to be in place even on non-aero themes on Win>=6, how do you intend to deal with this?

::: toolkit/themes/osx/mozapps/jar.mn
@@ +85,5 @@
> +% override chrome://mozapps/skin/extensions/category-extensions.png       chrome://mozapps/skin/extensions/extensionGeneric.png
> +% override chrome://mozapps/skin/extensions/category-languages.png        chrome://mozapps/skin/extensions/localeGeneric.png
> +% override chrome://mozapps/skin/extensions/category-themes.png           chrome://mozapps/skin/extensions/themeGeneric.png
> +% override chrome://mozapps/skin/plugins/notifyPluginCrashed.png          chrome://mozapps/skin/plugins/pluginGeneric-16.png
> +% override chrome://mozapps/skin/plugins/notifyPluginGeneric.png          chrome://mozapps/skin/plugins/pluginGeneric-16.png

This seems to be a different file than before? (used to be plugins/notifyPluginGeneric.png)

(I've not checked if the files' contents are the same, but if they are, I'd image you would have hg rm'd the duplicate, too, rather than "just" removing references)

Comment 22

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch (away 26-30/3) from comment #21)
> Comment on attachment 8583789 [details] [diff] [review]
> replace chrome copy of files by overrides in toolkit/themes/
> 
> Review of attachment 8583789 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> These overrides are going to be in place even on non-aero themes on Win>=6,
> how do you intend to deal with this?

The "-aero" prefixes are a lie. These files have always been used on Win>=6.

There's certainly room for cleanup, renaming files, particularly in browser/themes/ where there's more inconsistency compared to toolkit/themes/ in this regard.

> ::: toolkit/themes/osx/mozapps/jar.mn
> @@ +85,5 @@
> > +% override chrome://mozapps/skin/extensions/category-extensions.png       chrome://mozapps/skin/extensions/extensionGeneric.png
> > +% override chrome://mozapps/skin/extensions/category-languages.png        chrome://mozapps/skin/extensions/localeGeneric.png
> > +% override chrome://mozapps/skin/extensions/category-themes.png           chrome://mozapps/skin/extensions/themeGeneric.png
> > +% override chrome://mozapps/skin/plugins/notifyPluginCrashed.png          chrome://mozapps/skin/plugins/pluginGeneric-16.png
> > +% override chrome://mozapps/skin/plugins/notifyPluginGeneric.png          chrome://mozapps/skin/plugins/pluginGeneric-16.png
> 
> This seems to be a different file than before? (used to be
> plugins/notifyPluginGeneric.png)

This might just be on oversight on my side, let me double-check. I haven't intentionally used a different file.

Comment 23

[Dão Gottwald [:dao]]

Attachment: replace chrome copy of files by overrides in toolkit/themes/, v2

Fixed the notifyPluginGeneric.png mistake

Comment 24

[Dão Gottwald [:dao]]

Attachment: replace chrome copy of files by overrides in toolkit/themes/, v2

wrong file

Comment 25

[:Gijs (he/him)]

Comment on attachment 8583919 [details] [diff] [review]
replace chrome copy of files by overrides in toolkit/themes/, v2

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

r=me with the unused item below addressed.

(apologies for the delays, this is proving to be tedious to review (I'm guessing it's similar to write... hope the delays have not bitrotted you yet))

Regarding the naming, might be worth doing a separate bug when we're done here to update naming to "vistaAndLater" or whatever... but that can wait if we do decide it's a good idea.

::: toolkit/themes/windows/global/jar.mn
@@ -308,5 @@
> -        skin/classic/aero/global/Filepicker.png                          (filepicker/Filepicker.png)
> -        skin/classic/aero/global/icons/autoscroll.png                    (icons/autoscroll-aero.png)
> -        skin/classic/aero/global/icons/autocomplete-search.svg           (icons/autocomplete-search.svg)
> -        skin/classic/aero/global/icons/blacklist_favicon.png             (icons/blacklist_favicon-aero.png)
> -        skin/classic/aero/global/icons/blacklist_large.png               (icons/blacklist_large-aero.png)

Filed bug 1149485 on updating these names.

::: toolkit/themes/windows/mozapps/jar.mn
@@ +82,5 @@
> +*       skin/classic/mozapps/downloads/downloads-aero.css          (downloads/downloads-aero.css)
> +*       skin/classic/mozapps/extensions/extensions-aero.css        (extensions/extensions-aero.css)
> +*       skin/classic/mozapps/extensions/selectAddons-aero.css      (extensions/selectAddons-aero.css)
> +        skin/classic/mozapps/extensions/category-discover-aero.png (extensions/category-discover-aero.png)
> +        skin/classic/mozapps/extensions/category-extensions-aero.png (extensions/extensionGeneric-aero.png)

This seems to be unused?

Comment 26

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #25)
> > +        skin/classic/mozapps/extensions/category-extensions-aero.png (extensions/extensionGeneric-aero.png)
> 
> This seems to be unused?

Indeed, this is covered by the category-extensions.png -> extensionGeneric.png -> extensionGeneric-aero.png overrides. Thanks.

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 8583919 [details] [diff] [review]
replace chrome copy of files by overrides in toolkit/themes/, v2

https://hg.mozilla.org/integration/fx-team/rev/0692b35349e1

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0692b35349e1

Comment 29

[:Gijs (he/him)]

Comment on attachment 8583547 [details] [diff] [review]
replace chrome copy of files by overrides in browser/themes/

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

Again, apologies for slow review... been doing this in parts during the day while being interrupted by other stuff. :-\

::: browser/themes/windows/jar.mn
@@ -3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  browser.jar:
> -% skin browser classic/1.0 %skin/classic/browser/ os=WINNT osversion<6
> -% skin browser classic/1.0 %skin/classic/browser/ os!=WINNT

Huh. Is this the fallback theme for non-Windows/Linux/OSX or something? Why did you remove this, out of curiosity? :-)

@@ +64,5 @@
>          skin/classic/browser/menuPanel-exit.png
>          skin/classic/browser/menuPanel-help.png
>          skin/classic/browser/menuPanel-small.png
> +        skin/classic/browser/menuPanel-small-aero.png
> +        skin/classic/browser/Metro_Glyph.png

Ugh, it doesn't help that splinter messes up the display of the renames here...

@@ +109,5 @@
>          skin/classic/browser/slowStartup-16.png
>          skin/classic/browser/theme-switcher-icon.png
> +        skin/classic/browser/theme-switcher-icon-aero.png
> +        skin/classic/browser/Toolbar.png
> +        skin/classic/browser/Toolbar-inverted.png

This line is now duplicated 3 lines further. Please ensure these (ie /Toolbar*.png) are alphabetically sorted. :-)

Comment 30

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #29)
> ::: browser/themes/windows/jar.mn
> @@ -3,5 @@
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> >  browser.jar:
> > -% skin browser classic/1.0 %skin/classic/browser/ os=WINNT osversion<6
> > -% skin browser classic/1.0 %skin/classic/browser/ os!=WINNT
> 
> Huh. Is this the fallback theme for non-Windows/Linux/OSX or something?

Yes, it is or was used on OS/2 or something like that.

> Why did you remove this, out of curiosity? :-)

I replaced it with an unconditional "% skin browser classic/1.0 %skin/classic/browser/". The osversion flag being gone makes the os distinction pointless.

> > +        skin/classic/browser/Toolbar-inverted.png
> 
> This line is now duplicated 3 lines further. Please ensure these (ie
> /Toolbar*.png) are alphabetically sorted. :-)

Oops.

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 8583547 [details] [diff] [review]
replace chrome copy of files by overrides in browser/themes/

https://hg.mozilla.org/integration/fx-team/rev/3a3ffe5df28e

Comment 32

[:Gijs (he/him)]

Dão, can you clarify where (ie here or another bug?) we'll move the overrides into the theme dir?

Comment 33

[Dão Gottwald [:dao]]

I'll file a new bug.

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3a3ffe5df28e

Comment 35

[Mike Hommey [:glandium]]

FWIW, before the first landing:
- Linux32: 44 duplicated files taking 438170 bytes (uncompressed)
- OSX: 46 duplicated files taking 527902 bytes (uncompressed)
- Windows: 616 duplicated files taking 2018536 bytes (uncompressed)

before the second landing:
- Linux32: 37 duplicated files taking 428902 bytes (uncompressed)
- OSX: 41 duplicated files taking 520065 bytes (uncompressed)
- Windows: 427 duplicated files taking 1671015 bytes (uncompressed)

after both:
- Linux32: 35 duplicated files taking 423716 bytes (uncompressed)
- OSX: 39 duplicated files taking 514729 bytes (uncompressed)
- Windows: 32 duplicated files taking 412963 bytes (uncompressed)

(those messages are in build logs, there is also a full list of what's duplicated)

Comment 36

[Mike Hommey [:glandium]]

(a big part of the remainder is d3.js, which we have twice...)

Comment 37

[Mike Hommey [:glandium]]

Filed bug 1150366

Comment 38

[:Gijs (he/him)]

Is there data on how much this has actually saved in the size of omni.ja / startup IO / installer size?

Comment 39

[Alfred Kayser]

This is a "ridiculous" change: 
In the chrome.manifest there are now many override's like this:
override chrome://global/skin/popup.css chrome://global/skin/popup-aero.css osversion>=6

For custom themes these override are a nightmare.
Custom themes are not allowed to do overrides, but now we have to include files such as popup-aero-css.css in our theme importing the normal popup.css just to be able to restore normal order again.

This impacts ALL customs themes enormously.

This needs to be reverted and another solution is to be found for this than using overrides in this way.

Comment 40

[Dão Gottwald [:dao]]

Alfred, please read the bug before commenting, and don't reopen bugs you don't own.

Comment 41

[Mike Hommey [:glandium]]

(In reply to :Gijs Kruitbosch from comment #38)
> Is there data on how much this has actually saved in the size of omni.ja /
> startup IO / installer size?

Not that I know of.