Closed Bug 659407 Opened 13 years ago Closed 13 years ago

Remove duplicate theme files

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 11

People

(Reporter: glandium, Assigned: dao)

References

Details

(Keywords: memory-footprint)

Attachments

(7 files, 11 obsolete files)

11.39 KB, patch
dao
: review-
Details | Diff | Splinter Review
101.77 KB, patch
Details | Diff | Splinter Review
498.81 KB, patch
Details | Diff | Splinter Review
4.00 KB, patch
Details | Diff | Splinter Review
45.15 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
10.51 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
10.77 KB, patch
Dolske
: review+
Details | Diff | Splinter Review
Assignee: nobody → mh+mozilla
Keywords: footprint
This removes duplicated files from the source tree. This does not affect the footprint. Further patches will.
I tested that the resulting omni.jar has the same content.
This removes duplicated files from the source tree. This does not affect the footprint. Further patches will.
This removes duplicated files from the source tree. This does not affect the footprint. Further patches will.
This removes duplicated files from the source tree. This does not affect the footprint. Further patches will.
Note that this one is for files that are in both gnomestripe and winstripe (since gnomestrip is actually an overlay)
OS: Windows 7 → All
Summary: Duplicate files in aero skin make omnijar bigger than it should be → Duplicate files make omni.jar bigger than it should be
Attachment #535030 - Attachment is obsolete: true
Attachment #535031 - Attachment is obsolete: true
Attachment #535032 - Attachment is obsolete: true
Attachment #535033 - Attachment is obsolete: true
While previous patches are meant not to change the resulting omni.jar, this one replaces duplicate files with chrome overrides. That's a lot of them, but it's safer and faster than trying to find all places using these urls (considering they could be relative or constructed). From my local builds, it looks like this saves around 400K of omni.jar on windows builds. I still need to double check that all urls still work as before, though. I'll also most actual size differences from try builds.
chrome/browser/content/browser/places/bookmarkProperties.xul is also duplicated as chrome/browser/content/browser/places/bookmarkProperties2.xul, because of bug 380232.

There are also these:
res/table-remove-row-active.gif res/table-remove-column-active.gif
res/table-remove-row.gif res/table-remove-column.gif
res/table-remove-row-hover.gif res/table-remove-column-hover.gif

But they require changes to places where they are used (which may also be out of the tree).

There are also more files that are duplicated across winstripe, pinstripe and gnomestripe, that could be picked from a single location from the jar.mn, but that's unrelated to the binary footprint.
(In reply to comment #10)
> chrome/browser/content/browser/places/bookmarkProperties.xul is also
> duplicated as chrome/browser/content/browser/places/bookmarkProperties2.xul,
> because of bug 380232.

We started evaluating the removal of the duplicate, but finally was a larger change than we could do at that time (was near FF4 release). We plan to remove that though, when resources allow, you're free to file a bug against Bookmarks & History and I'll then evaluate dependencies and such, but doubt can be fixed early.
Couple of general comments:

* Not sure we should be pulling stuff from /toolkit in /browser's jar.mn.

* There are a few cases of different contexts currently happening to use the same icon. Should consider if we want to leave some of these duplicated for easier theme maintenance?

* Dao (or gavin?) should sign off on the chrome override approach. Dunno if that was explicitly considered/rejected when the Windows Aero stuff was originally done. At the very least it might be nice to see if we can add some preprocessing/manifest-fu to make the syntax nicer.
Component: General → Theme
QA Contact: general → theme
(In reply to comment #12)
> Couple of general comments:
> 
> * Not sure we should be pulling stuff from /toolkit in /browser's jar.mn.

Note this goes away in part 5.
So, patches up to part 4 are working properly on windows and mac, but miss some files on linux, so I'll have to work that out.
Part 5 has an unexpected carriage return in one file that breaks mac build, but other than that looks to be working ok.

As for sizes:
          Before       After      Difference
Windows   5369184      4950972    418212 (7.78%)
Mac       4689823      4667164    22659 (0.48%)

Definitively a massive win on Windows. I still need to validate that no chrome:// url was harmed in the process.
Component: Theme → General
OS: All → Windows 7
Assignee: mh+mozilla → nobody
Component: General → Theme
OS: Windows 7 → All
(In reply to comment #4)
> Created attachment 535033 [details] [diff] [review] [review]
> part 4 - Deduplicate skin files shared between gnomestripe and winstripe
> themes
> 
> This removes duplicated files from the source tree. This does not affect the
> footprint. Further patches will.
> Note that this one is for files that are in both gnomestripe and winstripe
> (since gnomestrip is actually an overlay)

The browser part of gnomestripe isn't an overlay.
Attachment #535093 - Flags: review?(gavin.sharp)
Attachment #535094 - Flags: review?(gavin.sharp)
Attachment #537085 - Flags: review?(gavin.sharp)
Attachment #535095 - Attachment is obsolete: true
Attachment #535097 - Attachment is obsolete: true
Attachment #535100 - Attachment is obsolete: true
This is the first part of what I used to validate nothing breaks(*) with the 5 parts applied.

* at least, that the chrome urls have the same content as before.
This is second part, containing a mochitest that checks whether all chrome:// urls with the duplicated tree from validation part 1 match the corresponding chrome:// urls with the 5 patches applied.

Note validation part 1 needs to be applied before the 5 patches. This one can be applied any time.

This is a try build with an earlier iteration of the patch that has one error on linux, one on mac, and none on windows:
http://tbpl.mozilla.org/?tree=Try&rev=1e45f098a4d9

This is the try build I'm currently running with the patch queue I just sent, which should not fail on mac, and still have the error on linux, but it's actually not important.
http://tbpl.mozilla.org/?tree=Try&rev=596af34a60d5

The reason why that linux error is not important is that the failing url, chrome://global-orig/skin/console/bullet-warning.png, is defined as an override in winstripe and is inherited from there, but the url is actually not used in the gnomestripe theme so we don't care it's different from what it used to be. The reason it's different from the original case is that in the original case, we were copying icons/warning-16.png, and with this patch queue, we just override console/bullet-warning.png to be icons/warning-16.png. But, icons/warnings-16.png is overridden in browser/themes/gnomestripe/browser/jar.mn (which it really shouldn't, btw, that should be done in toolkit) to be the corresponding moz-icon:// gtk native warning icon. The console uses the gtk native warning icon urls directly in the gnomestripe stylesheet anyways.
I will file separate bugs for the remaining (non theme) duplicate files.
Comment on attachment 537085 [details] [diff] [review]
part 3 - Deduplicate skin files in the pinstripe theme

>-  skin/classic/browser/places/tag.png                       (places/tag.png)
>+  skin/classic/browser/places/tag.png                       (../../../../toolkit/themes/pinstripe/mozapps/places/tagContainerIcon.png)

Please remove tag.png and use chrome://mozapps/skin/places/tagContainerIcon.png instead of chrome://browser/skin/places/tag.png.

>-  skin/classic/global/dirListing/local.png                           (dirListing/local.png)
>+  skin/classic/global/dirListing/local.png                           (dirListing/folder.png)

Note that local.png actually isn't being used...

>   skin/classic/mozapps/extensions/category-languages.png          (extensions/category-languages.png)
>   skin/classic/mozapps/extensions/category-searchengines.png      (extensions/category-searchengines.png)
>   skin/classic/mozapps/extensions/category-extensions.png         (extensions/category-extensions.png)
>   skin/classic/mozapps/extensions/category-themes.png             (extensions/category-themes.png)
>   skin/classic/mozapps/extensions/category-plugins.png            (extensions/category-plugins.png)
>   skin/classic/mozapps/extensions/category-recent.png             (extensions/category-recent.png)
>   skin/classic/mozapps/extensions/category-available.png          (extensions/category-available.png)
>   skin/classic/mozapps/extensions/discover-logo.png               (extensions/discover-logo.png)
>-  skin/classic/mozapps/extensions/extensionGeneric.png            (extensions/extensionGeneric.png)
>+  skin/classic/mozapps/extensions/extensionGeneric.png            (extensions/category-extensions.png)
>   skin/classic/mozapps/extensions/extensionGeneric-16.png         (extensions/extensionGeneric-16.png)
>-  skin/classic/mozapps/extensions/themeGeneric.png                (extensions/themeGeneric.png)
>+  skin/classic/mozapps/extensions/themeGeneric.png                (extensions/category-themes.png)
>   skin/classic/mozapps/extensions/themeGeneric-16.png             (extensions/themeGeneric-16.png)
>-  skin/classic/mozapps/extensions/localeGeneric.png               (extensions/localeGeneric.png)
>+  skin/classic/mozapps/extensions/localeGeneric.png               (extensions/category-languages.png)

Please do the opposite, e.g. let category-extensions.png copy extensionGeneric.png.

>-  skin/classic/mozapps/extensions/background-texture.png          (extensions/background-texture.png)
>+  skin/classic/mozapps/extensions/background-texture.png          (../global/icons/tabprompts-bgtexture.png)

Not cool.

>-  skin/classic/mozapps/update/buttons.png                         (update/buttons.png)
>+  skin/classic/mozapps/update/buttons.png                         (downloads/buttons.png)

Also not cool. There's no strong link between these files, they won't necessarily stay in sync in the future.

>-  skin/classic/mozapps/places/defaultFavicon.png                  (places/defaultFavicon.png)
>+  skin/classic/mozapps/places/defaultFavicon.png                  (../global/tree/item.png)

We should remove item.png and use chrome://mozapps/skin/places/defaultFavicon.png instead.
Attachment #537085 - Flags: review?(gavin.sharp) → review-
Comment on attachment 535094 [details] [diff] [review]
part 2 - Deduplicate skin files in the gnomestripe theme

a bunch of the pinstripe comments apply here as well
Attachment #535094 - Flags: review?(gavin.sharp) → review-
Comment on attachment 535093 [details] [diff] [review]
part 1 - Deduplicate skin files in the winstripe theme

>-        skin/classic/aero/browser/preferences/plugin.png             (preferences/plugin-aero.png)
>+        skin/classic/aero/browser/preferences/plugin.png             (../../../../toolkit/themes/winstripe/mozapps/plugins/pluginGeneric-16-aero.png)

This appears to be unused.

>-        skin/classic/global/console/bullet-error.png             (console/bullet-error.png)
>-        skin/classic/global/console/bullet-question.png          (console/bullet-question.png)
>-        skin/classic/global/console/bullet-warning.png           (console/bullet-warning.png)
>+        skin/classic/global/console/bullet-error.png             (icons/error-16.png)
>+        skin/classic/global/console/bullet-question.png          (icons/information-16.png)
>+        skin/classic/global/console/bullet-warning.png           (icons/warning-16.png)

We can remove these in favor of chrome://global/skin/icons/error-16.png etc.

>-        skin/classic/global/icons/notfound.png                   (icons/notfound.png)
>+        skin/classic/global/icons/notfound.png                   (icons/information-16.png)

We can remove this in favor of chrome://global/skin/icons/information-16.png
Attachment #535093 - Flags: review?(gavin.sharp) → review-
Comment on attachment 537086 [details] [diff] [review]
part 4 - Deduplicate skin files shared between gnomestripe and winstripe themes

>-+ skin/classic/mozapps/extensions/category-search.png      (extensions/category-search.png)

This is actually a tango icon that winstripe happens to steal, therefore it should remain in gnomestripe.
Attachment #537086 - Flags: review?(gavin.sharp) → review-
(In reply to comment #22)
> Comment on attachment 537085 [details] [diff] [review] [review]
> part 3 - Deduplicate skin files in the pinstripe theme
> >-  skin/classic/mozapps/extensions/background-texture.png          (extensions/background-texture.png)
> >+  skin/classic/mozapps/extensions/background-texture.png          (../global/icons/tabprompts-bgtexture.png)
> 
> Not cool.

What is not cool ? to use tabprompts-bgtexture.png or to use something from ../global ?
Using tabprompts-bgtexture.png. The idea that touching tabprompts-bgtexture.png would affect the add-on manager seems weird.
(In reply to comment #27)
> Using tabprompts-bgtexture.png. The idea that touching
> tabprompts-bgtexture.png would affect the add-on manager seems weird.

Well, that could be said of many of the changes from this bug... but then, we could also expect people modifying or reviewing a modification to the main theme to be careful about the consequences of their changes (which is also true now, since some non duplicated images in the tree are copied several times in chrome, so modifying the only file will lead to all these to be modified)
> Well, that could be said of many of the changes from this bug

To varying extents. extensions/background-texture.png is just background noise, whereas tabprompts-bgtexture.png overlays web content (together with a color overlay). This is quite a different context. Changing one has no consequence whatsoever for the other.
Attachment #537087 - Flags: review?(gavin.sharp) → review?(dao)
Comment on attachment 537087 [details] [diff] [review]
part 5 - Replace chrome copy of files by overrides

Needs updates for the requested changes on the other patches.

>+% override chrome://browser/skin/feeds/audioFeedIcon.png chrome://browser/skin/feeds/feedIcon.png
>+% override chrome://browser/skin/feeds/audioFeedIcon16.png chrome://browser/skin/feeds/feedIcon16.png
>+% override chrome://browser/skin/feeds/videoFeedIcon.png chrome://browser/skin/feeds/feedIcon.png
>+% override chrome://browser/skin/feeds/videoFeedIcon16.png chrome://browser/skin/feeds/feedIcon16.png

Could you add spaces so that the overriding sources line up in the same column? Exceptions for overly long file names are fine.
Attachment #537087 - Flags: review?(dao)
Blocks: 597678
Assignee: nobody → mh+mozilla
(In reply to comment #29)
> > Well, that could be said of many of the changes from this bug
> 
> To varying extents. extensions/background-texture.png is just background
> noise, whereas tabprompts-bgtexture.png overlays web content (together with
> a color overlay). This is quite a different context. Changing one has no
> consequence whatsoever for the other.

How about completely changing the file name so that it is explicit that it's tied to neither the addon manager nor tabs? I'm insisting on deduplicating this file because since bug 653143, we output duplicates from omni.jar during build. We don't error-out because we have so many duplicates right now but I guess we should in the future. And even if we don't, we still don't want a few known files to add noise.
Warning is fine, but I don't think we want two images that happen to have the same pixel data to be an error. Given this motivation that I don't share, I'd prefer if you left these two images alone.
Target Milestone: --- → Firefox 7
Version: unspecified → 8 Branch
This bug seems to be taking about removing *some* files from the aero/ directories, but can't we go further than this?  As I said in bug #696565, we should be able to use CSS selectors for everything we're using separate Aero files for now (@media all and (-moz-windows-theme: aero) {...}), and no aero/ directories need to exist; Aero stuff can all be in the main theme files.  This would reduce all the unnecessary duplicate CSS files there, and make life easier for theme developers (checking changes to Aero files as well as non-Aero files is more work).
Assignee: mh+mozilla → dao
Target Milestone: Firefox 7 → ---
Version: 8 Branch → Trunk
Attachment #535093 - Attachment is obsolete: true
Attachment #573572 - Flags: review?(gavin.sharp)
Attachment #535094 - Attachment is obsolete: true
Attachment #573577 - Flags: review?(gavin.sharp)
forgot to hg rm tag.png
Attachment #573577 - Attachment is obsolete: true
Attachment #573577 - Flags: review?(gavin.sharp)
Attachment #573578 - Flags: review?(gavin.sharp)
Attachment #537085 - Attachment is obsolete: true
Attachment #573583 - Flags: review?(gavin.sharp)
Attachment #573572 - Flags: review?(dolske)
Attachment #573578 - Flags: review?(dolske)
Attachment #573583 - Flags: review?(dolske)
Hm, I honestly don't understand why tagContainerIcon.png exists in mozapps at all, it's not even used in toolkit code... I'd prefer us doing the opposite (keep tag.png in browser and kill the useless entry in mozapps). Do you think that'd break anything?
Ah, I found why it's there, the original Places toolkit code was using it, but that code has been removed long long time ago, still the icon is still in the wrong place, can be removed (and switch to using the browser's theme one)
Comment on attachment 573572 [details] [diff] [review]
part 1 - Remove duplicate winstripe files, v2

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

I didn't go through and verify correctness of all the changes, I'll rely on Dao's usual thoroughness for that. :) Reflag for review if you want a second set of nitpicky eyes on things, though.

All the change here make sense to me, though.
Attachment #573572 - Flags: review?(gavin.sharp)
Attachment #573572 - Flags: review?(dolske)
Attachment #573572 - Flags: review+
Attachment #573578 - Flags: review?(gavin.sharp)
Attachment #573578 - Flags: review?(dolske)
Attachment #573578 - Flags: review+
Attachment #573583 - Flags: review?(gavin.sharp)
Attachment #573583 - Flags: review?(dolske)
Attachment #573583 - Flags: review+
Just please invert the tag.png <-> tagContainerIcon.png replacement, we should keep the former and remove the latter. Otherwise we'll have to do the same in a follow-up inverting part of this patch, that would make things a bit harder to follow.
Yeah, I assumed that would be sorted out one way or the other. :-)

Oh, and for the other work in this bug from Glandium... If there are parts that make sense to keep, can we spin off other bugs for that? I don't want to lose that work if so, but I also don't want to have this bug drag on further.
Blocks: 706100
Blocks: 706103
I added tag.png back and removed tagContainerIcon.png instead.

https://hg.mozilla.org/mozilla-central/rev/68ba25ded632
https://hg.mozilla.org/mozilla-central/rev/1bb37562781c
https://hg.mozilla.org/mozilla-central/rev/45311e361fd1

Filed bug 706100 and bug 706103 for the missing parts.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Summary: Duplicate files make omni.jar bigger than it should be → Remove duplicate theme files
Target Milestone: --- → Firefox 11
Oh please!! We can't override files that are inside the skin provider! We don't know nothing about it contents, since the skin provider is fully replaced according to our theme system. Overriding files that are "supposed" to be inside the skin provider can cause every kind of problems and brake badly third-party themes.
(In reply to Pardal Freudenthal (:ShareBird) from comment #45)
You're commenting in the wrong bug.
While I don't see overrides in attachments parts 1, 2 & 3, which have 'review +' flags, but there other attachments with no flags that do have overrides. Hopefully these attachments will not be approved.

The practice of trying to "fix" things using overrides in the skin provider is very troubling and should be prohibited. As we found with the OS X Lion bugs, skin overrides can and do break lots of AMO themes.
Depends on: 1305748
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: