Get Firefox to launch with the new .app bundle structure

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: spohl, Assigned: spohl)

Tracking

({addon-compat, dev-doc-needed})

Trunk
Firefox 35
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed)

Details

Attachments

(2 attachments, 10 obsolete attachments)

Get Firefox to launch with the new bundle structure, mostly by changing the GRE and XRE directory to point to the Resources directory instead of MacOS.

Note that this also requires the patch in bug 1048687.
Posted patch Patch (obsolete) — Splinter Review
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Comment on attachment 8470212 [details] [diff] [review]
Patch

Argh, cleanup broke the patch. Will upload a fixed version shortly.
Attachment #8470212 - Attachment is obsolete: true
Comment on attachment 8470212 [details] [diff] [review]
Patch

False alarm. Patch is okay, I just forgot to apply it locally before building. :-)
Attachment #8470212 - Attachment is obsolete: false
Comment on attachment 8470212 [details] [diff] [review]
Patch

Steven, I'm asking for your review of the Cocoa changes in nsAppShell.mm since you're going to be out next week. I still have major testing to do before I feel comfortable requesting reviews for the other parts, but I don't expect any more changes to Cocoa code.
Attachment #8470212 - Flags: review?(smichaud)
Comment on attachment 8470212 [details] [diff] [review]
Patch

Forwarding the review request for nsAppShell.mm to Markus. Markus, we used to have a structure of:

Contents/MacOS/res/MainMenu.nib and
Contents/MacOS/res/cursors/

Since we had to move all the data files from MacOS to Resources, it didn't make sense to have a structure of Resources/res anymore. Also, it seems like MainMenu.nib traditionally lives in the root of Resources anyway. The change here just allows us to find these files under Resources. Note that GRE now points at Resources, not MacOS. Here is the new structure:

Contents/Resources/MainMenu.nib and
Contents/Resources/cursors/
Attachment #8470212 - Flags: review?(smichaud) → review?(mstange)
(In reply to Stephen Pohl [:spohl] from comment #5)
> The change here just allows us to find these files under Resources.

Actually, the change allows us to find MainMenu.nib. I'm actually not sure if or where we load the contents of cursors. Looking into it.
Comment on attachment 8470212 [details] [diff] [review]
Patch

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

I haven't double-checked the folder traversing code with the structure you showed me, but I expect that the browser would look completely broken if you had gotten something wrong, so I trust you on that.

::: toolkit/xre/nsXREDirProvider.cpp
@@ +300,5 @@
> +      rv = file->GetParent(getter_AddRefs(parent));
> +      if (NS_SUCCEEDED(rv)) {
> +        rv = parent->GetParent(getter_AddRefs(file));
> +        if (NS_SUCCEEDED(rv)) {
> +          rv = file->AppendNative(NS_LITERAL_CSTRING("MozResources"));

What does this code do? What's "MozResources"?

@@ +1414,5 @@
>    rv = GetAppDir()->Clone(getter_AddRefs(defaultsDir));
> +  NS_WARN_IF(NS_FAILED(rv));
> +  if(NS_FAILED(rv)) {
> +    return rv;
> +  }

I think NS_WARN_IF is supposed to be used like so:
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}
(also note the space after "if")
Attachment #8470212 - Flags: review?(mstange) → review+
Posted patch Patch (obsolete) — Splinter Review
Addressed review feedback. Carrying over Markus' r+.

(In reply to Markus Stange [:mstange] from comment #7)
> ::: toolkit/xre/nsXREDirProvider.cpp
> @@ +300,5 @@
> > +      rv = file->GetParent(getter_AddRefs(parent));
> > +      if (NS_SUCCEEDED(rv)) {
> > +        rv = parent->GetParent(getter_AddRefs(file));
> > +        if (NS_SUCCEEDED(rv)) {
> > +          rv = file->AppendNative(NS_LITERAL_CSTRING("MozResources"));
> 
> What does this code do? What's "MozResources"?

This traverses back to the root of the .app bundle, where we will have a "MozResources" directory sitting next to "Contents" (see bug 1047584 comment 4 and bug 1047728 comment 9). This is because everything inside of "Contents" will be signed with Apple's new v2 signatures and we can no longer add or modify files inside of it after the bundle was signed. "MozResources" is our solution for things like channel-prefs.js and files for our partner builds of Firefox.

> I think NS_WARN_IF is supposed to be used like so:
> if (NS_WARN_IF(NS_FAILED(rv))) {
>   return rv;
> }

Ah, that definitely makes it less cumbersome to use.

> (also note the space after "if")

Oops, that was a typo and not intentional. Thanks for the catch!
Attachment #8470212 - Attachment is obsolete: true
Attachment #8470254 - Flags: review+
(In reply to Stephen Pohl [:spohl] from comment #8)
> This traverses back to the root of the .app bundle, where we will have a
> "MozResources" directory sitting next to "Contents" (see bug 1047584 comment
> 4 and bug 1047728 comment 9). This is because everything inside of
> "Contents" will be signed with Apple's new v2 signatures and we can no
> longer add or modify files inside of it after the bundle was signed.
> "MozResources" is our solution for things like channel-prefs.js and files
> for our partner builds of Firefox.

Alright, thanks for the explanation.
Group: mozilla-employee-confidential
Posted patch Patch (obsolete) — Splinter Review
Fixed lookup for XUL library when running xpcshell-tests from obj-dir.

Carrying over mstange's r+ for the Cocoa parts.
Attachment #8470254 - Attachment is obsolete: true
Attachment #8471765 - Flags: review+
Attachment #8471765 - Flags: feedback?(benjamin)
Posted patch Patch (obsolete) — Splinter Review
Updated GeckoChildProcessHost::GetPathToBinary to work with the new GRE path.
Attachment #8471765 - Attachment is obsolete: true
Attachment #8471765 - Flags: feedback?(benjamin)
Attachment #8473868 - Flags: review+
Attachment #8473868 - Flags: feedback?(benjamin)
Comment on attachment 8473868 [details] [diff] [review]
Patch

This needs some docs. Especially I'm pretty sure we're changing which directory is passed to XRE_main and probably also NS_InitXPCOM, and the headers for those methods should contain a note about this structural issue. The doc comments on nsXREDirProvider should also mention what directories mGREDir/mXULAppDir point to on mac.

In nsBrowserApp.cpp you have exePath = exePath.AppendASCII("../MacOS"). In the case of symlinks, this may not be the same path as a direct path manipulation. It probably doesn't matter in this case, but is there a reason we can't use real path manipulation here as we do elsewhere?
Attachment #8473868 - Flags: feedback?(benjamin) → feedback-
Posted patch Patch (wip) (obsolete) — Splinter Review
This is just updated for current trunk and does not yet address any feedback.
Attachment #8473868 - Attachment is obsolete: true
Attachment #8476657 - Flags: review+
Attachment #8476657 - Flags: feedback-
Keywords: dev-doc-needed
Posted patch Patch (obsolete) — Splinter Review
Addressed feedback plus some cleanup. Still waiting for test harnesses to be updated and start passing before requesting review.
Attachment #8476657 - Attachment is obsolete: true
Posted patch Patch (obsolete) — Splinter Review
Minor fixup.
Attachment #8480058 - Attachment is obsolete: true
Posted patch Patch (obsolete) — Splinter Review
More fixup.
Attachment #8480292 - Attachment is obsolete: true
Comment on attachment 8486547 [details] [diff] [review]
Patch

Try push is almost completely green, so requesting review (updater xpcshell test failures and gaia python integration test suite failures are handled in separate bugs):
https://tbpl.mozilla.org/?tree=Try&rev=7362867ad903
Attachment #8486547 - Flags: superreview?(benjamin)
Attachment #8486547 - Flags: review?(ted)
Attachment #8486547 - Flags: review?(smichaud)
Comment on attachment 8486547 [details] [diff] [review]
Patch

This looks fine to me.
Attachment #8486547 - Flags: review?(smichaud) → review+
Comment on attachment 8486547 [details] [diff] [review]
Patch

>diff --git a/ipc/glue/GeckoChildProcessHost.cpp b/ipc/glue/GeckoChildProcessHost.cpp

>@@ -131,16 +132,34 @@ GeckoChildProcessHost::GetPathToBinary(F
>     nsCString path;
>     NS_CopyUnicodeToNative(nsDependentString(gGREPath), path);
>     exePath = FilePath(path.get());
> #endif
> #ifdef MOZ_WIDGET_COCOA
>     // We need to use an App Bundle on OS X so that we can hide
>     // the dock icon. See Bug 557225.
>     exePath = exePath.AppendASCII(MOZ_CHILD_PROCESS_BUNDLE);
>+
>+    const char* cPath = exePath.value().c_str();
>+    nsCOMPtr<nsIFile> tempPath;
>+    XRE_GetFileFromPath(cPath, getter_AddRefs(tempPath));

Is "exePath" an absolute path? If so, this should not be using XRE_GetFileFromPath, but NS_NewNativeLocalFile. XRE_GetFileFromPath is only for figuring out the binary path from argv[0]! It happens again below.
Attachment #8486547 - Flags: superreview?(benjamin) → superreview-
Posted patch Patch (obsolete) — Splinter Review
Sent this to Oak with all the other patches. Waiting for the results before requesting review again:
https://tbpl.mozilla.org/?tree=Oak&rev=45d91d73dbf5
Attachment #8486547 - Attachment is obsolete: true
Attachment #8486547 - Flags: review?(ted)
Attachment #8488817 - Flags: superreview?(benjamin)
Attachment #8488817 - Flags: review?(ted)
Attachment #8488817 - Flags: superreview?(benjamin) → superreview+
Comment on attachment 8488817 [details] [diff] [review]
Patch

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

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +136,5 @@
>  #ifdef OS_WIN
>      exePath = FilePath(char16ptr_t(gGREPath));
> +#elif MOZ_WIDGET_COCOA
> +    nsCOMPtr<nsIFile> childProcPath;
> +    NS_NewLocalFile(nsDependentString(gGREPath), true,

I'm assuming I should change followLinks to false here as well?
Attachment #8488817 - Flags: feedback?(benjamin)
Posted patch Patch (obsolete) — Splinter Review
Went ahead and changed followLinks to false. Let me know if I shouldn't have.

Carrying over sr=bsmedberg and r=smichaud.
Attachment #8488817 - Attachment is obsolete: true
Attachment #8488817 - Flags: review?(ted)
Attachment #8488817 - Flags: feedback?(benjamin)
Attachment #8489522 - Flags: superreview+
Attachment #8489522 - Flags: review?(ted)
Yes.
Comment on attachment 8489522 [details] [diff] [review]
Patch

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

I don't see anything here that concerns me.
Attachment #8489522 - Flags: review?(ted) → review+
Posted patch PatchSplinter Review
Updated for current trunk. Carrying over r=smichaud,ted and sr=bsmedberg.
Attachment #8489522 - Attachment is obsolete: true
Attachment #8496236 - Flags: superreview+
Attachment #8496236 - Flags: review+
Flagging this bug for addon-compat since we're changing the GRE and XRE directory to point to Contents/Resources (instead of Contents/MacOS).
Keywords: addon-compat
Comment on attachment 8496236 [details] [diff] [review]
Patch

>+#ifdef XP_MACOSX
>+    nsCOMPtr<nsIFile> parent;
>+    greDir->GetParent(getter_AddRefs(parent));
>+    greDir = parent.forget();
>+    greDir->AppendNative(NS_LITERAL_CSTRING(kOSXResourcesFolder));
>+#endif
Out of interest, would SetNativeLeafName have worked for you here?
(In reply to neil@parkwaycc.co.uk from comment #31)
> Comment on attachment 8496236 [details] [diff] [review]
> Patch
> 
> >+#ifdef XP_MACOSX
> >+    nsCOMPtr<nsIFile> parent;
> >+    greDir->GetParent(getter_AddRefs(parent));
> >+    greDir = parent.forget();
> >+    greDir->AppendNative(NS_LITERAL_CSTRING(kOSXResourcesFolder));
> >+#endif
> Out of interest, would SetNativeLeafName have worked for you here?

It does seem like it would have.
https://hg.mozilla.org/mozilla-central/rev/11d48c719ec9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
I'll document this change. See c#28, tag #dev-doc-info.
Blocks: 1078640
(In reply to Stephen Pohl [:spohl] from comment #28)
> Flagging this bug for addon-compat since we're changing the GRE and XRE
> directory to point to Contents/Resources (instead of Contents/MacOS).

OK, I found NS_GRE_DIR easily enough, but what is XRE? That abbreviation isn't used anywhere that I could find, so I figured I'd better ask.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #37)
> https://developer.mozilla.org/en-US/docs/XRE

That answers what the abbreviation stands for, but doesn't clarify for me which of the NS_*_DIR constants it refers to. (I'm sure it's obvious to you guys, but I'm afraid it's not to me)
If a constant is used to query it, it would be the same one, i.e. NS_GRE_DIR. In our test infrastructure, XRE directories are often passed via command line instead (via --xre-path command line flag for example).
Landed on aurora in the Mac V2 signing combined patch in bug 1047584
I saw this note from the addons blog. I was wondering does this affect me? In my addon I use App Bundle to create a shortcut to firefox:

https://gist.github.com/Noitidart/6a2cbe0b4c74499765be

This snippet creates a desktop icon that launches the profile manager.

You can copy paste that code and run from scratchpad. It seems to work to me, but I just wanted to check.
(In reply to noitidart from comment #41)
> It seems to work to me, but I just wanted to check.

After briefly looking over the code I think you should be in the clear. You're using XREExeF to find the target for your shortcut, which is the ideal way to do it.

Just fyi: What would no longer work is to request the GreD and then append the name of the target, since GreD is now pointing at Contents/Resources instead of Contents/MacOS. If you need to get the path to Contents/MacOS, use GreBinD instead. Again, this does not appear to apply to you since you're already using XREExeF.
(In reply to Stephen Pohl [:spohl] from comment #42)
> (In reply to noitidart from comment #41)
> > It seems to work to me, but I just wanted to check.
> 
> After briefly looking over the code I think you should be in the clear.
> You're using XREExeF to find the target for your shortcut, which is the
> ideal way to do it.
> 
> Just fyi: What would no longer work is to request the GreD and then append
> the name of the target, since GreD is now pointing at Contents/Resources
> instead of Contents/MacOS. If you need to get the path to Contents/MacOS,
> use GreBinD instead. Again, this does not appear to apply to you since
> you're already using XREExeF.

Thanks Stephen! :) That FYI is real nice. I saw mentions of XRE in topic so got confused.
You need to log in before you can comment on or make changes to this bug.