Theme overrides should work in safe mode for comm-central applications too.

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: philip.chee, Assigned: philip.chee)

Tracking

(Blocks 1 bug)

Trunk
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

()

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

4 years ago
No description provided.
Assignee

Comment 1

4 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.
Seems like it should use a different define altogether. What determines that theme ID and that we're shipping it?
Assignee

Comment 3

4 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

4 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41f2fbac476e
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Assignee

Comment 6

4 years ago
Posted patch Patch v1.0 Possible fix. (obsolete) — Splinter Review
> 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 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)
(in particular, I think currently this is going to use the exists check on, say, android. Bad idea.)
Assignee

Comment 9

4 years ago
Posted patch Patch v2.0 better patch (obsolete) — Splinter Review
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 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

4 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

4 years ago
Posted patch Patch v3.0 smaller/better. (obsolete) — Splinter Review
> -#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)
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

4 years ago
Attachment #8669341 - Flags: review?(benjamin) → review-
Assignee

Updated

4 years ago
Blocks: 1133743
Assignee

Comment 14

4 years ago
Posted patch Patch v4 useSplinter Review
> 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

4 years ago
Attachment #8677630 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/430fb98d2900
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee

Updated

4 years ago
You need to log in before you can comment on or make changes to this bug.