1204182 - Use a single jar manifest for shared theme resources

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.0 (PoC)

Continuing on my jar.mn cleanup, started in bug 1204154...

The /shared theme we added in bug 738491 has been hugely helpful, but it's still a bit of a pain to add new assets -- you need to add the files to /shared, and then update all the jar.mn files for Windows/Mac/Linux to package the new files.

This would be simpler and less error-prone if one only had to add the files to /shared and edit a single jar.mn file (also in /shared), which all the actual themes would just pick up automagically.

Proof of concept attached.


I also verified that a specific theme can still override a file from /shared, by simply listing it after the #include. Not sure if we actually need this anywhere, but it's possible to do. EG:

shared/jar.inc.mn
  skin/classic/canadian.svg (../shared/flappyhead.svg)

osx/jar.mn
  #include ../shared/jar.inc.mn
  skin/classic/canadian.svg (mattn.svg)

Comment 1

[Matthew N. [:MattN]]

Comment on attachment 8660238 [details] [diff] [review]
Patch v.0 (PoC)

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

LGTM!

Comment 2

[Justin Dolske [:Dolske]]

+bgrins because I'm moving a bunch of the shared devtools stuff. (Just FYI.)

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

A few notes as I went through this... I was trying to be very careful, and found a number of minor possible issues with the current manifests.

* A handful of @2x files are now shipped on Windows and Linux, but are not yet used. I don't think the trivial size overhead matters, and is a win for simplicity.

  skin/classic/browser/[e-mail]
  skin/classic/browser/[e-mail]
  skin/classic/browser/[e-mail]
  skin/classic/global/menu/[e-mail]

* Similarly, a handful of files were previously only shipped on Windows and Linux, but not OS X

  skin/classic/browser/webRTC-camera-white-16.png
  skin/classic/browser/webRTC-microphone-white-16.png
  skin/classic/browser/webRTC-screen-white-16.png

* skin/classic/browser/webRTC-indicator.css

OS X uses a non-shared version of this (because the UI is totally different). We should eventually consider if we want "shared" to be for stuff that's common everywhere, or just reused in at least 2 themes. The former seems more common, but I didn't change this here. (The alternative would be to keep it in windows, and have linux pull it from ../../windows/...)

* skin/classic/browser/devtools/[e-mail]

This wasn't packaged on Linux, but was used in shared CSS. Presumably this was broken for any unicorns using hidpi linux.

* skin/classic/browser/devtools/tool-inspector.svg

This was duplicated in the OS X and Linux manifests. Might be useful to have the jar processor complain when this happens, but then we also want to be able to deliberately override shared resources with non-shared resources, so that might be tricky.

* Similarly, a number of shared files in toolkit/themes/linux/global/jar.mn were needlessly listed, because they were already picked up through the windows theme packaging (which linux inherits). No #include of jar.inc.mn should even be needed here.

Comment 4

[Justin Dolske [:Dolske]]

Gijs/Dao: I flagged both of you, but whoever can review it first is fine. I'm a little afraid this patch will bitrot quickly, so want to get it landed reasonably quickly.

Comment 5

[Justin Dolske [:Dolske]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa7a716948cd

Oh, and I didn't clean up every usage of shared, this is like 90% of it. I just wanted to get this batch landed, and deal with the rest in a followup.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8661641 [details] [diff] [review]
Patch v.1

This seems to break the new tab button icon on linux. Not sure why, as you don't seem to touch its entry with this patch, but I applied no other patch than yours and I clobbered, so I'm fairly sure my build isn't messed up.

Comment 7

[Dão Gottwald [:dao]]

Attachment: new-tab-button.png

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8661641 [details] [diff] [review]
Patch v.1

Just unapplied your patch, built browser/themes/ and toolkit/themes/ and the new tab button icon is still broken. Probably not your fault then.

> OS X uses a non-shared version of this (because the UI is totally
> different). We should eventually consider if we want "shared" to be for
> stuff that's common everywhere, or just reused in at least 2 themes. The
> former seems more common, but I didn't change this here.

I agree we should only share what's common everywhere.

Comment 9

[Pulsebot]

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

Comment 10

[Brian Grinstead [:bgrins]]

cc:jryans, since this could very well affect Bug 912121

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/c0dce1af1cf5

Comment 12

[Justin Dolske [:Dolske]]

(In reply to Justin Dolske [:Dolske] from comment #0)
...
> I also verified that a specific theme can still override a file from
> /shared, by simply listing it after the #include. Not sure if we actually
> need this anywhere, but it's possible to do. EG:
> 
> shared/jar.inc.mn
>   skin/classic/canadian.svg (../shared/flappyhead.svg)
> 
> osx/jar.mn
>   #include ../shared/jar.inc.mn
>   skin/classic/canadian.svg (mattn.svg)

So, this doesn't actually work reliably, due to timestamps. See bug 1208020 comment 12.

Instead, the manifest should have the "+" prefix to force using the override and ignore timestamps.

Although Dao also notes that it's probably better practice to only use shared/ for things that are actually shared everywhere, and avoid this pattern entirely.