> themeManifest->AppendNative(NS_LITERAL_CSTRING("{972ce4c6-7e08-4474-a285-3208198ce6fd}")); The default theme ID in all comm-central applications is the same as Firefox. Either the |#ifdef MOZ_BUILD_APP_IS_BROWSER| should be removed OR whitelist Thunderbird, SeaMonkey, and Instantbird as well.

Seems like it should use a different define altogether. What determines that theme ID and that we're shipping it?

(In reply to :Gijs Kruitbosch from comment #2) > What determines that theme ID and that we're shipping it? IIRC It is hard-coded and has been since Firefox 1.0/CVS days. And as someone pointed out recently. This GUID is all over our codebase (comm-central + mozilla-central): http://mxr.mozilla.org/comm-central/search?string=972ce4c6-7e08

> https://treeherder.mozilla.org/#/jobs?repo=try&revision=de20ccb3e09e Hey Gijs, remind me on how to download try-server builds so that I can test them locally? Also any recommended reviewers for this patch?

Comment on attachment 8644388 [details] [diff] [review] Patch v1.0 Possible fix. (In reply to Philip Chee from comment #6) > Created attachment 8644388 [details] [diff] [review] > Patch v1.0 Possible fix. > > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=de20ccb3e09e > > Hey Gijs, remind me on how to download try-server builds so that I can test > them locally? Click the "B" for the platform you want. Then on the bottom left, there's something like: Build: x86 windowsxp win in the list of job details (Job, Machine, ...), click the link and you get the right FTP dir. > Also any recommended reviewers for this patch? I can rubberstamp this assuming you can show a try-comm-central push that shows how this works. However, I don't really like the exists check because it introduces unnecessary sync io on startup. The app should know at compile-time whether this file (should) exist. You should be able to use the same kind of build-time define as you're using in the other patch instead.

(in particular, I think currently this is going to use the exists check on, say, android. Bad idea.)

Try-server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d8573b30f2a&group_state=expanded I don't think there are any tests for this so I downloaded a Firefox windows try build from the above job and tested that the overrides still worked in safe-mode. > +if CONFIG['MOZ_DEFAULT_THEME_IS_JAR']: > + DEFINES['MOZ_DEFAULT_THEME_IS_JAR'] = True SeaMonkey default theme uses a packed XPI. MOZ_DEFAULT_THEME_IS_JAR defined in SeaMonkey confvars.sh/configure.in Thunderbird default theme uses an unpacked structure. I admit this is rather sad code. > -#ifdef MOZ_BUILD_APP_IS_BROWSER > +#if defined(MOZ_LOAD_DEFAULT_THEME_MANIFEST) > } else { > // In safe mode, still load the default theme directory: > +#if defined(MOZ_BUILD_APP_IS_BROWSER) > nsCOMPtr<nsIFile> themeManifest; > mXULAppDir->Clone(getter_AddRefs(themeManifest)); Firefox uses mXULAppDir instead of mGREDir because the front end is in a /browser/ subdirectory. This was created for dual desktop/metro builds. Now that metro has been excised from mozilla-central and lives somewhere else, we can now remove the /browser/ and reunite the two omni.ja's. I'll file another bug on this if you think this is desireable. > themeManifest->AppendNative(NS_LITERAL_CSTRING("extensions")); > themeManifest->AppendNative(NS_LITERAL_CSTRING("{972ce4c6-7e08-4474-a285-3208198ce6fd}")); > themeManifest->AppendNative(NS_LITERAL_CSTRING("chrome.manifest")); > XRE_AddManifestLocation(NS_SKIN_LOCATION, themeManifest); > +#elif defined(MOZ_DEFAULT_THEME_IS_JAR) SeaMonkey default theme is in a packed XPI > + nsCOMPtr<nsIFile> jarManifest; > + mGREDir->Clone(getter_AddRefs(jarManifest)); > + jarManifest->AppendNative(NS_LITERAL_CSTRING("extensions")); > + jarManifest->AppendNative(NS_LITERAL_CSTRING("{972ce4c6-7e08-4474-a285-3208198ce6fd}.xpi")); > + XRE_AddJarManifestLocation(NS_SKIN_LOCATION, jarManifest); > +#else But Thunderbird default theme isn't. > + nsCOMPtr<nsIFile> themeManifest; > + mGREDir->Clone(getter_AddRefs(themeManifest)); > + themeManifest->AppendNative(NS_LITERAL_CSTRING("extensions")); > + themeManifest->AppendNative(NS_LITERAL_CSTRING("{972ce4c6-7e08-4474-a285-3208198ce6fd}")); > + themeManifest->AppendNative(NS_LITERAL_CSTRING("chrome.manifest")); > + XRE_AddManifestLocation(NS_SKIN_LOCATION, themeManifest); > +#endif

Comment on attachment 8655454 [details] [diff] [review] Patch v2.0 better patch Review of attachment 8655454 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/xre/moz.build @@ +153,5 @@ > > +if CONFIG['MOZ_DEFAULT_THEME_IS_JAR']: > + DEFINES['MOZ_DEFAULT_THEME_IS_JAR'] = True > + > +if CONFIG['MOZ_BUILD_APP'] == 'browser' or CONFIG['MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES']: Where is MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES defined? ::: toolkit/xre/nsXREDirProvider.cpp @@ +666,5 @@ > + themeManifest->AppendNative(NS_LITERAL_CSTRING("extensions")); > + themeManifest->AppendNative(NS_LITERAL_CSTRING("{972ce4c6-7e08-4474-a285-3208198ce6fd}")); > + themeManifest->AppendNative(NS_LITERAL_CSTRING("chrome.manifest")); > + XRE_AddManifestLocation(NS_SKIN_LOCATION, themeManifest); > +#endif Right... Yeah, ideally we should unify omni.ja for Firefox, and get seamonkey to stop using a jar file. Then we can all use the same code. Any reason seamonkey is "special" here? I expect you'll want actual review from someone like bsmedberg.

(In reply to :Gijs Kruitbosch from comment #10) > Comment on attachment 8655454 [details] [diff] [review] > Patch v2.0 better patch > > Review of attachment 8655454 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/xre/moz.build > @@ +153,5 @@ > > +if CONFIG['MOZ_DEFAULT_THEME_IS_JAR']: > + DEFINES['MOZ_DEFAULT_THEME_IS_JAR'] = True > + > +if CONFIG['MOZ_BUILD_APP'] == 'browser' or CONFIG['MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES']: > > Where is MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES defined? In SeaMonkey Bug 1022354 :P > ::: toolkit/xre/nsXREDirProvider.cpp > @@ +666,5 @@ > + themeManifest->AppendNative(NS_LITERAL_CSTRING("extensions")); > + themeManifest->AppendNative(NS_LITERAL_CSTRING("{972ce4c6-7e08-4474-a285-3208198ce6fd}")); > + themeManifest->AppendNative(NS_LITERAL_CSTRING("chrome.manifest")); > + XRE_AddManifestLocation(NS_SKIN_LOCATION, themeManifest); > +#endif > > Right... > > Yeah, ideally we should unify omni.ja for Firefox, and get seamonkey to stop > using a jar file. Then we can all use the same code. Any reason seamonkey is > "special" here? IanN decided to make the default theme packed. I've asked him in Bug 1022354 to revert it to unpacked. > I expect you'll want actual review from someone like bsmedberg. Thanks. Will do once I sort out the comm-central part of things. :P

> -#ifdef MOZ_BUILD_APP_IS_BROWSER > +#if defined(MOZ_BUILD_APP_IS_BROWSER) || defined(MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES) > Where is MOZ_SEPARATE_MANIFEST_FOR_THEME_OVERRIDES defined? I've made this an opt in. Applications like SeaMonkey will have to define this keyword if they want this behaviour. (e.g. SeaMonkey Bug 1022354) > +#if defined(MOZ_BUILD_APP_IS_BROWSER) > mXULAppDir->Clone(getter_AddRefs(themeManifest)); > +#else > + mGREDir->Clone(getter_AddRefs(themeManifest)); > +#endif mXULAppDir: In Firefox the default theme manifest is in firefox/browser/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd} mGREDir: Everybody else puts it in the GRE dir. Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa0212448b69&group_state=expanded I don't think there are any safe mode tests for this. I've downloaded a Firefox try build and manually verified that it works as expected in safe mode.

Doesn't mXULAppDir == mGREDir for seamonkey? The extra #ifdef seems really unfortunate and deserves at least an explanatory comment. I don't understand what all of the comments about omni.ja and the browser/ subdirectory are for in this bug. We're going to keep the separate browser/ directory for Firefox as far as I know.

> Doesn't mXULAppDir == mGREDir for seamonkey? Yes it does. Thanks. > The extra #ifdef seems really unfortunate and deserves at least an > explanatory comment. Added a comment. try-server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=32e282d7016e&group_state=expanded