Use a single jar manifest for shared theme resources

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
Firefox 43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Posted patch Patch v.0 (PoC) (obsolete) — Splinter Review
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)
Attachment #8660238 - Flags: feedback?(MattN+bmo)
Attachment #8660238 - Flags: feedback+
Comment on attachment 8660238 [details] [diff] [review]
Patch v.0 (PoC)

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

LGTM!
Attachment #8660238 - Flags: feedback?(MattN+bmo) → feedback+
+bgrins because I'm moving a bunch of the shared devtools stuff. (Just FYI.)
Posted patch Patch v.1Splinter Review
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/notification-pluginNormal@2x.png
  skin/classic/browser/notification-pluginAlert@2x.png
  skin/classic/browser/notification-pluginBlocked@2x.png
  skin/classic/global/menu/shared-menu-check@2x.png

* 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/editor-breakpoint@2x.png

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.
Assignee: nobody → dolske
Attachment #8660238 - Attachment is obsolete: true
Attachment #8661641 - Flags: review?(gijskruitbosch+bugs)
Attachment #8661641 - Flags: review?(dao)
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.
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 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.
Attachment #8661641 - Flags: review?(gijskruitbosch+bugs)
Attachment #8661641 - Flags: review?(dao)
Attachment #8661641 - Flags: review-
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.
Attachment #8661641 - Flags: review- → review+
cc:jryans, since this could very well affect Bug 912121
https://hg.mozilla.org/mozilla-central/rev/c0dce1af1cf5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Depends on: 1206883
Blocks: 1206883
No longer depends on: 1206883
Depends on: 1208020
(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.
You need to log in before you can comment on or make changes to this bug.