Add GreBinD to easily differentiate between Contents/Resources (GreD) and Contents/MacOS on OSX

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
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Due to the v2 signature changes on OSX (bug 1047584), we had to move all non-code files from Contents/MacOS to Contents/Resources in the app bundle. At the same time we changed the meaning of GreD and made it point to Contents/Resources in the app bundle. Unfortunately, this left us without an easy way to access Contents/MacOS.

The idea is to introduce GreBinD which points to Contents/MacOS on OSX and will be equivalent to GreD on all other platforms.
If you haven't started I'll try to get to this tonight
Patch is well underway, thanks!
I just realized that I'm not certain if bsmedberg prefers to have this in nsDirectoryService.cpp or in nsXREDirProvider.cpp and XPCShellImpl.cpp (for xpcshell tests).
Benjamin, do you prefer to have this in nsDirectoryService.cpp or in nsXREDirProvider.cpp and XPCShellImpl.cpp (for xpcshell tests).
Flags: needinfo?(benjamin)
Posted patch Patch (wip) (obsolete) — Splinter Review
Figured it can't hurt to post my work in progress. This merely compiles but I have yet to do any testing, or change any callers to use GreBinD.
Assignee: nobody → spohl.mozilla.bugs
Posted patch Patch (wip) (obsolete) — Splinter Review
qrefresh'd for latest changes.
Attachment #8499298 - Attachment is obsolete: true
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #4)
> Benjamin, do you prefer to have this in nsDirectoryService.cpp or in
> nsXREDirProvider.cpp and XPCShellImpl.cpp (for xpcshell tests).
Meant to write
in nsDirectoryService.cpp, nsXREDirProvider.cpp, and XPCShellImpl.cpp or just in nsXREDirProvider.cpp and XPCShellImpl.cpp.

Clearing feedback request since I am going to test this out and if all is good request review.
Flags: needinfo?(benjamin)
Comment on attachment 8499300 [details] [diff] [review]
Patch (wip)

I verified this works on both Windows and Mac OS X in both Firefox and xpcshell so requesting review.
Attachment #8499300 - Flags: review?(benjamin)
Comment on attachment 8499300 [details] [diff] [review]
Patch (wip)

This works beautifully for tests, etc. but it breaks during make package which uses the -g command line arg.
Attachment #8499300 - Flags: review?(benjamin) → review-
Posted patch Patch (obsolete) — Splinter Review
Right, I hadn't tested the last patch yet. This patch seems to work for me now. Robert, before I request review, could you please confirm that this works for you as well? Thanks!
Attachment #8499300 - Attachment is obsolete: true
Attachment #8499576 - Flags: feedback?(robert.strong.bugs)
Posted patch PatchSplinter Review
Oops, forgot to clean up XPCShellDirProvider::GetFile before uploading.
Attachment #8499576 - Attachment is obsolete: true
Attachment #8499576 - Flags: feedback?(robert.strong.bugs)
Attachment #8499587 - Flags: feedback?(robert.strong.bugs)
Comment on attachment 8499587 [details] [diff] [review]
Patch

Works for me!
Attachment #8499587 - Flags: feedback?(robert.strong.bugs) → feedback+
Comment on attachment 8499587 [details] [diff] [review]
Patch

Great, thanks Robert!
Attachment #8499587 - Flags: review?(benjamin)
Attachment #8499587 - Flags: review?(benjamin) → review+
Jorge, heads up about a new dir servive provider key to use when getting the binary directory. The key is GreBinD and will be the same directory as GreD on all platforms but Mac OS X. For Mac OS X GreD points to Contents/Resources/ and GreBinD points to Contents/MacOS/
Flags: needinfo?(jorge)
https://hg.mozilla.org/mozilla-central/rev/0de83f6ba398
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
It appears that |xrePath| is now

> $OBJDIR/dist/NightlyDebug.app/Contents/Resources/

but glandium says it should be 

> $OBJDIR/dist/NightlyDebug.app/Contents/MacOS/
Blocks: 1077230
(In reply to Nicholas Nethercote [:njn] from comment #19)
> It appears that |xrePath| is now
> 
> > $OBJDIR/dist/NightlyDebug.app/Contents/Resources/
> 
> but glandium says it should be 
> 
> > $OBJDIR/dist/NightlyDebug.app/Contents/MacOS/

|xrePath| has changed to Contents/Resources. See bug 1050944 comment 28. As of the landing of this bug here, you may query GreBinD to get access to Contents/MacOS directly.
See comments 17 and 20 for documentation information. This is related to bug 1050944.
Docs to update:

https://developer.mozilla.org/en-US/docs/Using_nsIDirectoryService

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDirectoryServiceProvider/getFile

And while we're at it, https://developer.mozilla.org/en-US/docs/FileGuide/Accessing_Files should be updated so that its mention that there are lots of directories you can look up should be crosslinked to the list provided in the getFile article.
Posted patch FollowupSplinter Review
While working on bug 1077282 I noticed that there will need to be a followup to this bug. Marking as 'wip' while I'm working through the remaining test failures from the cleanup in bug 1077282, as there may need to be more client-side changes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(jorge)
Comment on attachment 8501179 [details] [diff] [review]
Followup

Previously, |nsXREDirProvider| had to be queried for NS_GRE_BIN_DIR at least once before |nsXREDirProvider::GetGREBinDir| would return the correct GREBinD (i.e. non-null). I moved the initialization of |mGREBinDir| to |nsXREDirProvider::Initialize|, where it probably should have been all along...

Also made XPCShellDirProvider GreBinD-aware.
Attachment #8501179 - Attachment description: Followup (wip) → Followup
Attachment #8501179 - Flags: review?(benjamin)
Attachment #8501179 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/fb27bd0c0084
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Landed on aurora in the Mac V2 signing combined patch in bug 1047584
You need to log in before you can comment on or make changes to this bug.