compreg.dat and xpti.dat still live in Camino.app

VERIFIED FIXED in Camino1.5

Status

VERIFIED FIXED
13 years ago
13 years ago

People

(Reporter: mark, Assigned: mark)

Tracking

({fixed1.8.0.4, fixed1.8.1})

unspecified
Camino1.5
PowerPC
macOS
fixed1.8.0.4, fixed1.8.1
Bug Flags:
camino1.0.2 +

Details

(Whiteboard: [camino-1.0.2])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

13 years ago
compreg.dat and xpti.dat still live in Camino.app/Contents/MacOS.  Other applications have moved these files into the profile, which is really a better location.

Currently, the Camino build script pregenerates compreg.dat and xpti.dat before packaging.  xpti.dat contains some paths from the Tinderbox that was used to build it and is regenerated (if writable) on first launch anyway.  Moving these files to the profile means no more pregeneration.  This carries the potential drawback of slowing the initial startup slightly.

We should think about moving these files into the profile for Camino too.  This  is probably doable entirely within AppServiceDirProvider.
(Assignee)

Comment 1

13 years ago
Posted patch nsILocalFileMac patch (obsolete) — Splinter Review
We need a function to create an nsILocalFileMac from an FSRef before xpcom has been initialized, since we want to change the location of compreg.dat to something based on an FSRef.

(I said I wouldn't add the function to nsLocalFileMac because it's obsolete, but I accidentally added it to that file first, so here ya go.)
Attachment #211399 - Flags: superreview?(dougt)
Attachment #211399 - Flags: review?(joshmoz)
(Assignee)

Comment 2

13 years ago
This moves the .dat files into the profile directory, depending on the xpcom patch for NS_NewLocalFileWithFSRef.  It also gets rid of autoRegister, since autoRegister prior to shipping is pointless now.  I'll need to update the build scripts before checking this in, or else builds will hang because Camino will no longer exit when asked to autoRegister.
Attachment #211400 - Flags: superreview?(mikepinkerton)
Attachment #211400 - Flags: review?(sfraser_bugs)
(Assignee)

Updated

13 years ago
Assignee: mikepinkerton → mark
(Assignee)

Comment 3

13 years ago
Comment on attachment 211399 [details] [diff] [review]
nsILocalFileMac patch

 void
+nsresult 
+NS_NewLocalFileWithFSRef(const FSRef* aFSRef, PRBool aFollowLinks, nsILocalFileMac* *result)

"void" does not belong!

Comment 4

13 years ago
I assume we don't want this for 1.0?
(Assignee)

Comment 5

13 years ago
There's not really enough time to test it in the wild, although there's nothing dangerous going on.  Pinkerton said we should leave it for 1.1.
(Assignee)

Comment 6

13 years ago
Actually, this might need build ID checking like during XRE startup:

http://lxr.mozilla.org/mozilla/source/toolkit/xre/nsAppRunner.cpp#2108

And, come to think of it, the compatibility check should probably look at TARGET_XPCOM_ABI to be sure the component list is regenerated when switching architectures.  (Can't rely on the build ID in a universal, it's the same for both CPUs.)

And come to think of that, that's probably the best motivator to move these files into the profile in ANY app - although for now, the only difference in components between ppc and x86 is Talkback, and what we do there is safe, so it's doesn't need to hold up 1.0.  (It'd be a different story if we supported xpxtension components.)
(Assignee)

Comment 7

13 years ago
I filed bug 326772 to handle reregistration on switching architectures in XRE.

Comment 8

13 years ago
Comment on attachment 211399 [details] [diff] [review]
nsILocalFileMac patch

+    if (file == nsnull)
+        return NS_ERROR_OUT_OF_MEMORY;

Is that kind of check worth anything on Mac OS X these days? I know it is standard protocol to do it, but is that *ever* going to be hit by a single user? I don't recall how much address space processes have...
Attachment #211399 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 9

13 years ago
Sure, you could hit a resource limit or the system could be unable to create a new swap file.  Either of those will cause a malloc failure.  Likely?  No.  But hey, they can't blame xpcom if it crashes.

Comment 10

13 years ago
i've actually had seamonkey crash on osx blaming oom. i've also had it crash in the os string land because of oom. these crashes really do happen, and i never appreciate it when they do.
(Assignee)

Comment 11

13 years ago
dougt, is the nsILocalFileMac patch OK with you?
(Assignee)

Comment 12

13 years ago
Comment on attachment 211400 [details] [diff] [review]
Camino patch (will be updated)

Cancelling r? on the Camino patch.  I'll want to force rereg when the buildid or other things change (like compatibility.ini) and it won't apply after bug 323780 anyway.
Attachment #211400 - Attachment description: Camino patch → Camino patch (will be updated)
Attachment #211400 - Flags: superreview?(mikepinkerton)
Attachment #211400 - Flags: review?(sfraser_bugs)
Comment on attachment 211399 [details] [diff] [review]
nsILocalFileMac patch

Why do you have to change the uuid for this?

I would rather have you just say something in a comment about what version it was included with.
(Assignee)

Comment 14

13 years ago
I know the uuid change isn't strictly necessary code-wise, but there was no guidance about using the uuid to track non-interface changes like this, so I was playing it safe.  I've got no problem leaving it alone.
this is exactly why we try to keep c++ cruft out of idl's. ;-)

leave the uuid alone.  Since this is a public function, you should add a comment as I suggested indicated where this function will be availabe.
(Assignee)

Comment 16

13 years ago
*** Bug 329138 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 17

13 years ago
Updated per comments 13-15.

Requesting approval1.8.1 because this is targeted at Camino 1.1, which will come from that branch.
Attachment #211399 - Attachment is obsolete: true
Attachment #214255 - Flags: superreview?(dougt)
Attachment #214255 - Flags: approval-branch-1.8.1?(dougt)
Attachment #211399 - Flags: superreview?(dougt)
Comment on attachment 214255 [details] [diff] [review]
nsILocalFileMac patch, v2 (on trunk)

if Josh is happy, I am happy.
Attachment #214255 - Flags: superreview?(dougt) → superreview+
(Assignee)

Comment 19

13 years ago
Comment on attachment 214255 [details] [diff] [review]
nsILocalFileMac patch, v2 (on trunk)

Doug is happy!  Checked in on trunk.
Attachment #214255 - Attachment description: nsILocalFileMac patch, v2 → nsILocalFileMac patch, v2 (on trunk)
(Assignee)

Updated

13 years ago
Depends on: 323780
(Assignee)

Comment 20

13 years ago
Doug, is this branchable yet?  We'll need this in 1.8.1 for Camino 1.1.
No longer depends on: 323780
Comment on attachment 214255 [details] [diff] [review]
nsILocalFileMac patch, v2 (on trunk)

sorry.  feel free.
Attachment #214255 - Flags: approval-branch-1.8.1?(dougt) → approval-branch-1.8.1+

Comment 22

13 years ago
Why is NS_NewLocalFileWithFSRef declared in nsILocalFileMac instead of in nsXPCOM.h? (Or, why aren't we holding the line against declaring exports in IDL?) In any case it really needs to use the NS_COM macro instead of NS_EXPORT.

You really didn't have to invent this new API; it's possible to do the same thing with:

nsCOMPtr<nsILocalFile> lf;
NS_NewNativeLocalFile(EmptyString(), PR_FALSE, getter_AddRefs(lf));

nsCOMPtr<nsILocalFileMac> lfm(do_QueryInterface(lf));
lfm->InitWithFSRef(theFSRef);

I don't think this patch should land on 1.8, and should be fixed in nsXPCOM.h or backed out on the trunk.
(Assignee)

Comment 23

13 years ago
We've got 15 or so calls to NS_NewLocalFileWithFSSpec throughout the codebase already.  Should those be removed too?

Comment 24

13 years ago
I don't care nearly as much about alreay-bad code, just avoiding new-bad code. But if you want to remove those usages or move NS_NewLocalFileWithFSSpec into nsXPCOM.h I certainly don't mind.
(Assignee)

Updated

13 years ago
Blocks: 336447
(Assignee)

Comment 25

13 years ago
Moving the target up on this for bug 336447.
No longer blocks: 336447
Flags: camino1.0.2+

Comment 26

13 years ago
*** Bug 337022 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 27

13 years ago
I have a fully functional 1.8.0-branch patch ready to go, as soon as I clean it up to edit the nasty words out of the comments.
(Assignee)

Comment 28

13 years ago
NS_NewLocalFileWithFSSpec/FSRef removal in bug 337367.
*** Bug 337488 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 30

13 years ago
Attachment #211400 - Attachment is obsolete: true
Attachment #222281 - Flags: review?(hwaara)
(Assignee)

Comment 31

13 years ago
These two patches are identical except for the AppDirServiceProvider sections: hwaara, there's no need to review the duplicated parts more than once.
Attachment #222282 - Flags: review?(hwaara)

Comment 32

13 years ago
Comment on attachment 222282 [details] [diff] [review]
v2 trunk: move compreg.dat and xpti.dat into the profile

>+static PRBool
>+CheckCompatibility(nsIFile* aProfileDir, const nsCString& aVersion,
>+                   const nsCString& aOSABI, nsIFile* aAppDir)
>+{

Maybe use abstract string type nsACString& instead as params in these two functions?

>+
>+static void
>+WriteVersion(nsIFile* aProfileDir, const nsCString& aVersion,
>+             const nsCString& aOSABI, nsIFile* aAppDir)

These two functions are some good stuff. Will we get them for free when we're on XULRunner? I'm thinking it's something a regular JohnDoe embeddor shouldn't need to do all by himself.

>+
> @interface PreferenceManager(PreferenceManagerPrivate)
> 
> - (void)registerNotificationListener;
> 
> - (void)termEmbedding: (NSNotification*)aNotification;
> - (void)xpcomTerminate: (NSNotification*)aNotification;
> 
> - (void)showLaunchFailureAndQuitWithErrorTitle:(NSString*)inTitleFormat errorMessage:(NSString*)inMessageFormat;
>@@ -363,17 +468,17 @@ static BOOL gMadePrefManager;
> 
>     NSString *path = [[[NSBundle mainBundle] executablePath] stringByDeletingLastPathComponent];
>     const char *binDirPath = [path fileSystemRepresentation];
>     nsCOMPtr<nsILocalFile> binDir;
>     rv = NS_NewNativeLocalFile(nsDependentCString(binDirPath), PR_TRUE, getter_AddRefs(binDir));
>     if (NS_FAILED(rv))
>     {
>       [self showLaunchFailureAndQuitWithErrorTitle:NSLocalizedString(@"StartupFailureAlert", @"")
>-                                      errorMessage:NSLocalizedString(@"StartupFailureBinPathMsg", @"")];
>+                                      errorMessage:NSLocalizedString(@"StartupFailureMsg", @"")];

You remember to edit that in the plist too, if it doesn't exist, right?
Attachment #222282 - Flags: review?(hwaara) → review+

Comment 33

13 years ago
Comment on attachment 222281 [details] [diff] [review]
v2 branch: move compreg.dat and xpti.dat into the profile

Same comments as trunk patch apply.
Attachment #222281 - Flags: review?(hwaara) → review+
(Assignee)

Comment 34

13 years ago
Comment on attachment 222281 [details] [diff] [review]
v2 branch: move compreg.dat and xpti.dat into the profile

> Maybe use abstract string type nsACString& instead as params in these two
> functions?

'k.

> These two functions are some good stuff. Will we get them for free when we're
> on XULRunner? I'm thinking it's something a regular JohnDoe embeddor shouldn't
> need to do all by himself.

Yeah, I think we'll get that with the toolkit profile stuff.

> You remember to edit that in the plist too, if it doesn't exist, right?

Actually, I didn't mean to change that one, I'll fix it.  I did only use keys that already exist in the strings file.
Attachment #222281 - Flags: superreview?(mikepinkerton)
(Assignee)

Updated

13 years ago
Attachment #222282 - Flags: superreview?(mikepinkerton)
Comment on attachment 222281 [details] [diff] [review]
v2 branch: move compreg.dat and xpti.dat into the profile

sr=pink
Attachment #222281 - Flags: superreview?(mikepinkerton) → superreview+
Comment on attachment 222282 [details] [diff] [review]
v2 trunk: move compreg.dat and xpti.dat into the profile

sr=pink
Attachment #222282 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 37

13 years ago
Checked in on trunk, MOZILLA_1_8_BRANCH (1.8.1), MOZILLA_1_8_0_BRANCH (1.8.0.4), and CAMINO_1_0_1_MINIBRANCH in case 1.0.2 comes from there.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed1.8.0.4, fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [camino-1.0.2]
Verified on 1.0.2, 1.8.0, 1.8 branch and trunk
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.