Closed
Bug 1077282
Opened 10 years ago
Closed 10 years ago
General cleanup for the Mac v2 signing changes
Categories
(Firefox :: General, defect)
Tracking
()
RESOLVED
FIXED
Firefox 35
People
(Reporter: robert.strong.bugs, Assigned: spohl)
References
Details
Attachments
(3 files, 7 obsolete files)
3.64 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
15.08 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
1.55 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
Things can be cleaaned up some with the new GreBinD directory key and using SetNativeLeafName instead of GetParent then AppendNative.
Reporter | ||
Comment 1•10 years ago
|
||
Hey Stephen, this cleans up most of the use cases and I was hoping you'd be willing to drive this to landing. I think I got most everything and the areas I didn't change are * xpcom/build/XPCOMInit.cpp NS_InitXPCOM2 and specifically how Mac sets xpcomLib since it isn't clear that the check for existence isn't needed. * toolkit/xre/nsUpdateDriver.cpp GetInstallDirPath cause I got tired. :)
Attachment #8499362 -
Flags: feedback?(spohl.mozilla.bugs)
Reporter | ||
Updated•10 years ago
|
OS: Windows 8.1 → Mac OS X
Hardware: x86_64 → All
Reporter | ||
Comment 2•10 years ago
|
||
Try was locked up so I pushed to oak https://hg.mozilla.org/projects/oak/rev/3df4eb39cf4a
Reporter | ||
Comment 3•10 years ago
|
||
Meh, failures so I backed this one out
Reporter | ||
Comment 4•10 years ago
|
||
This one compiles on Mac. The problem which I haven't investigated is in the changes to nsExceptionHandler.cpp
Attachment #8499362 -
Attachment is obsolete: true
Attachment #8499362 -
Flags: feedback?(spohl.mozilla.bugs)
Attachment #8499407 -
Flags: review?(spohl.mozilla.bugs)
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #4) > The problem which I haven't investigated is in the changes to nsExceptionHandler.cpp NS_LITERAL_CSTRING will work (the previous patch used NS_LITERAL_STRING).
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8499407 [details] [diff] [review] patch in progress Review of attachment 8499407 [details] [diff] [review]: ----------------------------------------------------------------- This will make things much cleaner, thanks! Just a few comments below. Please feel free to also add the change to nsExceptionHandler.cpp back in (after making it use NS_LITERAL_CSTRING). ::: 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), false, I would prefer if we could introduce a new global gGREBinPath and use this instead. ::: js/xpconnect/src/XPCShellImpl.cpp @@ +1360,5 @@ > XRE_GetFileFromPath(argv[0], getter_AddRefs(greDir)); > nsCOMPtr<nsIFile> parentDir; > greDir->GetParent(getter_AddRefs(parentDir)); > + greDir = parentDir.forget(); > + greDir->SetNativeLeafName(NS_LITERAL_CSTRING("Resources")); Could you rewrite this to something like this (saves one line): nsCOMPtr<nsIFile> tmpDir; XRE_GetFileFromPath(argv[0], getter_AddRefs(tmpDir)); tmpDir->GetParent(getter_AddRefs(greDir)); greDir->SetNativeLeafName(NS_LITERAL_CSTRING("Resources")); ::: security/manager/ssl/tests/unit/head_psm.js @@ +359,5 @@ > let directoryService = Cc["@mozilla.org/file/directory_service;1"] > .getService(Ci.nsIProperties); > let envSvc = Cc["@mozilla.org/process/environment;1"] > .getService(Ci.nsIEnvironment); > + let greDir = directoryService.get("GreBinD", Ci.nsIFile); nit: let's change the variable name to greBinDir ::: startupcache/test/TestStartupCache.cpp @@ +411,5 @@ > nsresult scrv; > > // Register TestStartupCacheTelemetry > nsCOMPtr<nsIFile> manifest; > scrv = NS_GetSpecialDirectory(NS_GRE_DIR, We should be able to query NS_GRE_BIN_DIR now and do away with the entire #ifdef XP_MACOSX below, no? ::: xpcom/build/XPCOMInit.cpp @@ +600,5 @@ > parent->AppendNative(NS_LITERAL_CSTRING("MacOS")); > bool pathExists = false; > parent->Exists(&pathExists); > if (pathExists) { > + xpcomLib = parent.forget(); We should be able to query for NS_GRE_BIN_DIR here as well and do away with the entire #ifdef XP_MACOSX block here. ::: xpcom/tests/unit/head_xpcom.js @@ +14,5 @@ > var envSvc = Components.classes["@mozilla.org/process/environment;1"]. > getService(Components.interfaces.nsIEnvironment); > var dirSvc = Components.classes["@mozilla.org/file/directory_service;1"]. > getService(Components.interfaces.nsIProperties); > + var greDir = dirSvc.get("GreBinD", Components.interfaces.nsIFile); nit: change variable name to greBinDir
Attachment #8499407 -
Flags: review?(spohl.mozilla.bugs) → review-
Assignee | ||
Comment 7•10 years ago
|
||
If you'd like me to drive this from here, just let me know.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•10 years ago
|
||
in progress
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8501180 -
Attachment is obsolete: true
Attachment #8502242 -
Flags: review?(benjamin)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8501181 -
Attachment is obsolete: true
Attachment #8502243 -
Flags: review?(benjamin)
Assignee | ||
Comment 12•10 years ago
|
||
These patches are blocked from landing by bug 1080338, but should be reviewable nevertheless. Kicked off a try build with the patches here and bug 1077099 (followup) applied. The cpp unit tests on OSX are expected to fail due to bug 1080338: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0476c5006628
Assignee | ||
Comment 13•10 years ago
|
||
Now with commit message.
Attachment #8502242 -
Attachment is obsolete: true
Attachment #8502242 -
Flags: review?(benjamin)
Attachment #8502245 -
Flags: review?(benjamin)
Assignee | ||
Comment 14•10 years ago
|
||
Missed the 'X' at the end of |#ifdef XP_MACOSX| in TestHarness.h.
Attachment #8502245 -
Attachment is obsolete: true
Attachment #8502245 -
Flags: review?(benjamin)
Attachment #8502499 -
Flags: review?(benjamin)
Assignee | ||
Comment 15•10 years ago
|
||
This workaround for bug 1080338 would allow us to land the patches here and then transition without hiccups to a proper fix in bug 1080338. This patch could simply be reverted once bug 1080338 lands. I could not test this on try yet since we're seemingly having problems with our jobs db. Releng is looking into it. Will wait to have a green try run before asking for review.
Assignee | ||
Comment 16•10 years ago
|
||
Corrected #ifdef here as well.
Attachment #8502505 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8502509 [details] [diff] [review] Workaround for bug 1080338 This workaround turns things green on try and I've also confirmed that running ./mach cppunittest locally still works: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7930021c75fd
Attachment #8502509 -
Flags: review?(benjamin)
Updated•10 years ago
|
Attachment #8502243 -
Flags: review?(benjamin) → review+
Updated•10 years ago
|
Attachment #8502499 -
Flags: review?(benjamin) → review+
Updated•10 years ago
|
Attachment #8502509 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c87dcd65628 https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a359d3faec https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a3560432b0
https://hg.mozilla.org/mozilla-central/rev/9c87dcd65628 https://hg.mozilla.org/mozilla-central/rev/a2a359d3faec https://hg.mozilla.org/mozilla-central/rev/d3a3560432b0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Reporter | ||
Comment 20•10 years ago
|
||
Landed on aurora in the Mac V2 signing combined patch in bug 1047584
status-firefox34:
--- → fixed
status-firefox35:
--- → fixed
Updated•10 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•