Replace chrome copy of files by overrides

RESOLVED FIXED in Firefox 40

Status

()

Firefox
Theme
RESOLVED FIXED
7 years ago
3 months ago

People

(Reporter: dao, Assigned: dao)

Tracking

({addon-compat, memory-footprint})

Trunk
Firefox 40
addon-compat, memory-footprint
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
+++ 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.
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.
(Assignee)

Comment 2

7 years ago
(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.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 967739
Duplicate of this bug: 974372
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

4 years ago
(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?
Flags: needinfo?(dao)

Comment 7

4 years ago
Dão, ping re the needinfo in comment #6 ?
As I said before, if you need any help to fix this using the "os_target" approach, please let me know.
(Assignee)

Comment 9

4 years ago
Talked to Gijs in person, he's going to try to put together a proof-of-concept patch.
Flags: needinfo?(dao)
(Assignee)

Comment 10

4 years ago
I'll work on this when I'm back from vacation.
Assignee: nobody → dao
(Assignee)

Comment 11

3 years ago
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...
Iteration: --- → 39.2 - 23 Mar
Points: --- → 8
Flags: qe-verify-
Flags: needinfo?(dtownsend)
Flags: firefox-backlog+

Updated

3 years ago
Status: NEW → ASSIGNED

Comment 12

3 years ago
(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

3 years ago
(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?)
(Assignee)

Comment 14

3 years ago
(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.
(Assignee)

Comment 15

3 years ago
(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?
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(gps)
Flags: needinfo?(dtownsend)
(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.
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.
Flags: needinfo?(mh+mozilla)
(Assignee)

Updated

3 years ago
Depends on: 910660
Flags: needinfo?(gps)

Updated

3 years ago
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
(Assignee)

Comment 18

3 years ago
Created attachment 8583546 [details] [diff] [review]
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).
Attachment #8583546 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 19

3 years ago
Created attachment 8583547 [details] [diff] [review]
replace chrome copy of files by overrides in browser/themes/

Forgot to recreate the patch after I made some last changes...
Attachment #8583546 - Attachment is obsolete: true
Attachment #8583546 - Flags: review?(gijskruitbosch+bugs)
Attachment #8583547 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 20

3 years ago
Created attachment 8583789 [details] [diff] [review]
replace chrome copy of files by overrides in toolkit/themes/

This one is slightly more straightforward
Attachment #8583789 - Flags: review?(gijskruitbosch+bugs)

Comment 21

3 years ago
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)
(Assignee)

Comment 22

3 years ago
(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.
(Assignee)

Comment 23

3 years ago
Created attachment 8583915 [details] [diff] [review]
replace chrome copy of files by overrides in toolkit/themes/, v2

Fixed the notifyPluginGeneric.png mistake
Attachment #8583789 - Attachment is obsolete: true
Attachment #8583789 - Flags: review?(gijskruitbosch+bugs)
Attachment #8583915 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 24

3 years ago
Created attachment 8583919 [details] [diff] [review]
replace chrome copy of files by overrides in toolkit/themes/, v2

wrong file
Attachment #8583915 - Attachment is obsolete: true
Attachment #8583915 - Flags: review?(gijskruitbosch+bugs)
Attachment #8583919 - Flags: review?(gijskruitbosch+bugs)

Comment 25

3 years ago
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?
Attachment #8583919 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 26

3 years ago
(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.
(Assignee)

Comment 27

3 years ago
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
Attachment #8583919 - Flags: checkin+
(Assignee)

Updated

3 years ago
Keywords: leave-open

Updated

3 years ago
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr

Comment 29

3 years ago
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. :-)
Attachment #8583547 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 30

3 years ago
(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.
(Assignee)

Comment 31

3 years ago
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
Attachment #8583547 - Flags: checkin+
(Assignee)

Updated

3 years ago
Keywords: leave-open

Comment 32

3 years ago
Dão, can you clarify where (ie here or another bug?) we'll move the overrides into the theme dir?
Flags: needinfo?(dao)
(Assignee)

Comment 33

3 years ago
I'll file a new bug.
Flags: needinfo?(dao)
(Assignee)

Updated

3 years ago
Blocks: 1150006
https://hg.mozilla.org/mozilla-central/rev/3a3ffe5df28e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
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)
(a big part of the remainder is d3.js, which we have twice...)

Comment 38

3 years ago
Is there data on how much this has actually saved in the size of omni.ja / startup IO / installer size?
Flags: needinfo?(mh+mozilla)

Comment 39

3 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

3 years ago
Blocks: 1150417
(Assignee)

Comment 40

3 years ago
Alfred, please read the bug before commenting, and don't reopen bugs you don't own.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Blocks: 1150843
(Assignee)

Updated

3 years ago
Blocks: 1150867
(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.
Flags: needinfo?(mh+mozilla)
Keywords: addon-compat
(Assignee)

Updated

3 years ago
Depends on: 1153209

Updated

3 years ago
Depends on: 1154218
(Assignee)

Updated

3 years ago
No longer depends on: 1154218
Depends on: 1216902

Updated

3 years ago
Depends on: 1232647
(Assignee)

Updated

3 months ago
Duplicate of this bug: 597678
You need to log in before you can comment on or make changes to this bug.