Closed Bug 1210703 Opened 5 years ago Closed 5 years ago

Duplicate jar.mn entries between the windows and linux themes

Categories

(Toolkit :: Themes, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: glandium, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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
And there is also the following on mac:
Warning: Item already in manifest: chrome/browser/skin/classic/browser/webRTC-indicator.css
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.
Attachment #8679360 - Flags: review?(mh+mozilla)
Attachment #8679360 - Flags: review?(dao)
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.
Assignee: nobody → gijskruitbosch+bugs
Blocks: 1216902
Status: NEW → ASSIGNED
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.
Attachment #8679360 - Flags: review?(dao)
(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.
Attachment #8679360 - Flags: review?(dao)
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.
Attachment #8679360 - Flags: review?(mh+mozilla) → feedback+
Blocks: 1219104
(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.
(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.
(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).
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.
+ 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 on attachment 8679360 [details] [diff] [review]
separate out shared windows and linux files into a separate manifest,

This has bitrotted.
Attachment #8679360 - Flags: review?(dao)
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.
Attachment #8682585 - Flags: review?(dao)
Attachment #8679360 - Attachment is obsolete: true
(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.
Attachment #8682792 - Flags: review?(dao)
Attachment #8682585 - Attachment is obsolete: true
Attachment #8682585 - Flags: review?(dao)
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. :-\
Attachment #8682585 - Flags: review?(dao)
Attachment #8682585 - Attachment is obsolete: false
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.
Attachment #8682585 - Flags: review?(dao) → review+
Attachment #8682792 - Flags: review?(dao) → review+
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?
Depends on: 1223526
OS: Unspecified → All
Hardware: Unspecified → All
Blocks: 1279236
You need to log in before you can comment on or make changes to this bug.