Closed
Bug 1189918
Opened 9 years ago
Closed 9 years ago
Theme overrides should work in safe mode for comm-central applications too.
Categories
(Toolkit :: Themes, defect)
Toolkit
Themes
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: philip.chee, Assigned: philip.chee)
References
(Blocks 1 open bug, )
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
> 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.
Comment 2•9 years ago
|
||
Seems like it should use a different define altogether. What determines that theme ID and that we're shipping it?
Assignee | ||
Comment 3•9 years ago
|
||
(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
Assignee | ||
Comment 4•9 years ago
|
||
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
> 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?
Attachment #8644388 -
Flags: feedback?(gijskruitbosch+bugs)
Comment 7•9 years ago
|
||
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.
Attachment #8644388 -
Flags: feedback?(gijskruitbosch+bugs)
Comment 8•9 years ago
|
||
(in particular, I think currently this is going to use the exists check on, say, android. Bad idea.)
Assignee | ||
Comment 9•9 years ago
|
||
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
Attachment #8644388 -
Attachment is obsolete: true
Comment 10•9 years ago
|
||
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.
Assignee | ||
Comment 11•9 years ago
|
||
(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
Assignee | ||
Comment 12•9 years ago
|
||
> -#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.
Attachment #8655454 -
Attachment is obsolete: true
Attachment #8669341 -
Flags: review?(benjamin)
Comment 13•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8669341 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 14•9 years ago
|
||
> 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
Attachment #8669341 -
Attachment is obsolete: true
Attachment #8677630 -
Flags: review?(benjamin)
Updated•9 years ago
|
Attachment #8677630 -
Flags: review?(benjamin) → review+
Comment 15•9 years ago
|
||
Comment 16•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee | ||
Updated•9 years ago
|
status-firefox42:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•