1210703 - Duplicate jar.mn entries between the windows and linux themes

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Mike Hommey [:glandium]]

On linux, both the windows and linux theme files are processed, and there are several files that are overlapping between both. We're lucky that jar.mn processing is not happening in parallel, because if it were, we'd end up with random results in the theme.

Here's a (possibly partial) list I got while working on bug 1210687 on my local build:

Warning: Item already in manifest: chrome/toolkit/skin/classic/global/autocomplete.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/button.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/checkbox.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/colorpicker.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/commonDialog.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/dropmarker.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/filepicker.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/Filepicker.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/findBar.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/global.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/groupbox.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/listbox.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/menu.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/menulist.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/netError.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/notification.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/numberbox.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/popup.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/preferences.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/printPageSetup.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/printPreview.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/radio.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/scrollbox.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/splitter.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/tabbox.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/textbox.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/toolbar.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/toolbarbutton.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/tree.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/alerts/alert.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/alerts/notification-48.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/console/console.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/console/console-toolbar.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/dirListing/remote.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/icons/autocomplete-search.svg
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/icons/autoscroll.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/icons/blacklist_favicon.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/icons/blacklist_large.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/icons/loading_16.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/icons/resizer.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/icons/sslWarning.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/icons/webapps-16.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/icons/webapps-64.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/in-content/common.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/in-content/info-pages.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/global/toolbar/spring.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/update/updates.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/downloads/downloadIcon.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/downloads/downloads.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/extensions.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/category-search.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/category-discover.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/category-plugins.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/category-service.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/category-recent.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/category-available.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/extensionGeneric.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/extensionGeneric-16.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/dictionaryGeneric.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/dictionaryGeneric-16.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/themeGeneric.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/themeGeneric-16.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/localeGeneric.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/newaddon.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/selectAddons.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/heart.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/navigation.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/extensions/utilities.svg
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/passwordmgr/key-16.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/passwordmgr/key-64.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/plugins/pluginGeneric.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/plugins/pluginBlocked.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/plugins/pluginGeneric-16.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/profile/profileicon.png
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/viewsource/viewsource.css
Warning: Item already in manifest: chrome/toolkit/skin/classic/mozapps/places/defaultFavicon.png
Warning: Item already in manifest: chrome/webapprt/skin/classic/webapprt/downloads/downloadIcon.png
Warning: Item already in manifest: chrome/webapprt/skin/classic/webapprt/downloads/downloads.css

Comment 1

[Mike Hommey [:glandium]]

And there is also the following on mac:
Warning: Item already in manifest: chrome/browser/skin/classic/browser/webRTC-indicator.css

Comment 2

[:Gijs (he/him)]

Attachment: separate out shared windows and linux files into a separate manifest,

Checked with diff whether I got all the files, otherwise only did some basic testing on Linux where things look fine. It'd be useful to doublecheck I didn't miss anything on Windows.

Comment 3

[:Gijs (he/him)]

I would have preferred to use a separate jar.mn file, but it seems multiple jar.mn files per JAR_MANIFESTS instruction in moz.build are not allowed.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8679360 [details] [diff] [review]
separate out shared windows and linux files into a separate manifest,

If I understand this bug correctly, we should just remove a bunch of stuff from toolkit/themes/linux/global/jar.mn to fix this.

Comment 5

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 8679360 [details] [diff] [review]
> separate out shared windows and linux files into a separate manifest,
> 
> If I understand this bug correctly, we should just remove a bunch of stuff
> from toolkit/themes/linux/global/jar.mn to fix this.

No, the Linux theme overwrites a bunch of files shipped as part of the Windows theme with its own files with identical names. If we stop including them in the Linux manifest, Linux will get the Windows versions, which isn't going to work very well... It should just not package those files from the Windows theme in the first place.

An alternative fix would be to add #ifdefs in the existing Windows jar.mn files, but that seems ugly and not super maintainable.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8679360 [details] [diff] [review]
separate out shared windows and linux files into a separate manifest,

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

With this patch applied, there are still a few remaining duplicates:

Warning: Item already in manifest: chrome/toolkit/skin/classic/global/Filepicker.png
Warning: Item already in manifest: chrome/webapprt/skin/classic/webapprt/downloads/downloadIcon.png
Warning: Item already in manifest: chrome/webapprt/skin/classic/webapprt/downloads/downloads.css

I was about to say the one from comment 1 is not fixed either, but it looks like it was removed by bug 1208020.

::: toolkit/themes/linux/global/jar.mn
@@ +4,5 @@
>  
>  toolkit.jar:
> +#include ../../shared/jar.inc.mn
> +
> +#include ../../shared/linux-windows-shared.jar.inc.mn

non-mac would be shorter than linux-windows. Also, shared is already in the directory name, so it's not really necessary in the file name.
I'd also just include jar.inc.mn from that file instead of having the includers include both files.

@@ +5,5 @@
>  toolkit.jar:
> +#include ../../shared/jar.inc.mn
> +
> +#include ../../shared/linux-windows-shared.jar.inc.mn
> +# This file changes the destination jar for some files, so we have to reset it:

It would probably be clearer if each of those include files just had the toolkit.jar: line.

Comment 7

[:Gijs (he/him)]

(In reply to Mike Hommey [:glandium] from comment #6)
> Comment on attachment 8679360 [details] [diff] [review]
> separate out shared windows and linux files into a separate manifest,
> 
> Review of attachment 8679360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> With this patch applied, there are still a few remaining duplicates:
> 
> Warning: Item already in manifest:
> chrome/toolkit/skin/classic/global/Filepicker.png

Ugh, will fix.

> Warning: Item already in manifest:
> chrome/webapprt/skin/classic/webapprt/downloads/downloadIcon.png
> Warning: Item already in manifest:
> chrome/webapprt/skin/classic/webapprt/downloads/downloads.css

These aren't in these manifest files, so I'd prefer to fix them in a separate patch, and maybe a separate bug.

Comment 8

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #4)
> > Comment on attachment 8679360 [details] [diff] [review]
> > separate out shared windows and linux files into a separate manifest,
> > 
> > If I understand this bug correctly, we should just remove a bunch of stuff
> > from toolkit/themes/linux/global/jar.mn to fix this.
> 
> No, the Linux theme overwrites a bunch of files shipped as part of the
> Windows theme with its own files with identical names.

I see, although I don't understand why that's necessarily a problem. I can't quite make sense of comment 0.

Comment 9

[Mike Hommey [:glandium]]

(In reply to Dão Gottwald [:dao] from comment #8)
> I see, although I don't understand why that's necessarily a problem. I can't
> quite make sense of comment 0.

We're currently relying on the build system processing the linux and windows themes in the right order such that the files that end up in Firefox are the expected ones. This prevents parallelizing jar manifest processing in the recursive make backend. mach build faster, however doesn't have the problem, but it also has a workaround to replicate this behavior, otherwise it would throw an error because of the duplicate files. And I want to remove this workaround (bug 1219104).

Comment 10

[Dão Gottwald [:dao]]

We use the + marker in the jar manifest on Linux, so I don't think the order should matter, + should take precedence over entries no matter if they're processed earlier or later.

Comment 11

[Mike Hommey [:glandium]]

+ doesn't guarantee much of anything. If the windows jar manifest is treated after the linux one, there's still a chance that the file would be overwritten. BTW, I think the + thing is just useless and confusing.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8679360 [details] [diff] [review]
separate out shared windows and linux files into a separate manifest,

This has bitrotted.

Comment 13

[:Gijs (he/him)]

Attachment: create shared windows and linux toolkit theme jar manifests,

This addresses comment #6 (besides the webapprt) and all the conflicts. If possible, it would be nice if we can iterate quickly so I don't have to play fix-the-bitrot repeatedly, as it's draining and not super productive.

Comment 14

[Mike Hommey [:glandium]]

(In reply to :Gijs Kruitbosch from comment #13)
> Created attachment 8682585 [details] [diff] [review]
> create shared windows and linux toolkit theme jar manifests,
> 
> This addresses comment #6 (besides the webapprt)

Why not do the webapprt files at the same time? There are only two of them, they are _also_ theme files, like the others, and fixing them now would allow to land bug 1219104 at the same time as this bug, so that no regression occurs.

Comment 15

[:Gijs (he/him)]

Attachment: fix webapprt theme overlap,

Comment 16

[:Gijs (he/him)]

Comment on attachment 8682585 [details] [diff] [review]
create shared windows and linux toolkit theme jar manifests,

Why not? Because bzexport is stupid and then obsoletes the earlier patch, apparently. :-\

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8682585 [details] [diff] [review]
create shared windows and linux toolkit theme jar manifests,

>--- a/toolkit/themes/linux/mozapps/jar.mn
>+++ b/toolkit/themes/linux/mozapps/jar.mn
>@@ -1,15 +1,14 @@
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> toolkit.jar:
> #include ../../shared/mozapps.inc.mn
>-+ skin/classic/mozapps/update/updates.css                  (update/updates.css)
> + skin/classic/mozapps/downloads/downloadIcon.png          (downloads/downloadIcon.png)
> + skin/classic/mozapps/downloads/downloads.css             (downloads/downloads.css)
> * skin/classic/mozapps/extensions/extensions.css           (extensions/extensions.css)
> + skin/classic/mozapps/extensions/category-search.png      (extensions/category-search.png)
> + skin/classic/mozapps/extensions/category-discover.png    (extensions/category-discover.png)
> + skin/classic/mozapps/extensions/category-plugins.png     (extensions/category-plugins.png)
> + skin/classic/mozapps/extensions/category-service.png     (extensions/category-service.png)
> + skin/classic/mozapps/extensions/category-recent.png      (extensions/category-recent.png)
>@@ -25,16 +24,17 @@ toolkit.jar:
> + skin/classic/mozapps/extensions/heart.png                (extensions/heart.png)
> + skin/classic/mozapps/passwordmgr/key-16.png              (passwordmgr/key-16.png)
> + skin/classic/mozapps/passwordmgr/key-64.png              (passwordmgr/key-64.png)
> + skin/classic/mozapps/plugins/pluginGeneric.png           (plugins/pluginGeneric.png)
> + skin/classic/mozapps/plugins/pluginBlocked.png           (plugins/pluginBlocked.png)
> + skin/classic/mozapps/plugins/pluginGeneric-16.png        (plugins/pluginGeneric-16.png)
> + skin/classic/mozapps/profile/profileicon.png             (profile/profileicon.png)
> + skin/classic/mozapps/viewsource/viewsource.css           (viewsource/viewsource.css)
>++ skin/classic/mozapps/update/updates.css                  (update/updates.css)

skin/classic/mozapps/update/updates.css should be before skin/classic/mozapps/viewsource/viewsource.css.

I can't say I like this patch. People will now have to look for images in three places -- the actual platform they care about, shared and shared-but-not-quite-everywhere. I'd almost prefer duplicating stuff on Linux and Windows. But let's take this for now and go from there.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/88b8fa914138
https://hg.mozilla.org/integration/fx-team/rev/98befa2ee036

Comment 19

[Pulsebot]

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

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/67fbffadaa47

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/88b8fa914138
https://hg.mozilla.org/mozilla-central/rev/98befa2ee036
https://hg.mozilla.org/mozilla-central/rev/ad65b8d8f442
https://hg.mozilla.org/mozilla-central/rev/67fbffadaa47

Comment 22

[Richard Newman [:rnewman]]

This caused a 100KB APK size regression on Android, because a bunch of what look like Windows XP icons ended up in toolkit/skin/classic.

Is there a better solution for us all than guarding toolkit/skin with a legacy XUL-only build conditional to avoid it shipping with non-XUL-UI applications?