Closed Bug 396209 Opened 15 years ago Closed 15 years ago

Allow applications to specify a profile directory from application.ini

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 4 obsolete files)

Attached patch Patch, v1 (obsolete) — Splinter Review
Right now the profile directory is determined on a per-platform basis using the vendor name and application name only. This patch adds the optional key 'Profile' to application.ini to allow apps to specify an alternate directory structure within the user's application data directory.
Attachment #280927 - Flags: review?(benjamin)
Attached patch Patch, v2 (obsolete) — Splinter Review
Bah, I've been in frozen string land for too long. This makes much better use of the internal strings.
Assignee: nobody → bent.mozilla
Attachment #280927 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #280960 - Flags: review?(benjamin)
Attachment #280927 - Flags: review?(benjamin)
Attached patch Patch, v3 (obsolete) — Splinter Review
And this fixes the (questionably useless) behavior if someone added a '.' to their profile path to make it hidden.
Attachment #280960 - Attachment is obsolete: true
Attachment #280968 - Flags: review?(benjamin)
Attachment #280960 - Flags: review?(benjamin)
> +nsXREDirProvider::AppendProfileString(nsIFile* aFile,
> ...
> +  FindInReadable

That's just a stupid patch artifact. I removed it.
Seth, Rob, I think that these changes will work just fine with the Vista update stuff... what do you think?
Comment on attachment 280968 [details] [diff] [review]
Patch, v3

I'm going to delegate this review to Ted.
Attachment #280968 - Flags: review?(benjamin) → review?(ted.mielczarek)
> Seth, Rob, I think that these changes will work just fine with the Vista update
> stuff... what do you think?

as long as not specifying profile doesn't change anything, and since this only affects ProfD (and not UAppData), this should not affect the Vista update stuff.
Attached patch Patch, v4 (obsolete) — Splinter Review
Ted, this patch fixes this bug as well as bug 396486 and bug 396199. Phew.
Attachment #280968 - Attachment is obsolete: true
Attachment #281582 - Flags: review?(ted.mielczarek)
Attachment #280968 - Flags: review?(ted.mielczarek)
Comment on attachment 281582 [details] [diff] [review]
Patch, v4

>Index: toolkit/xre/nsAppRunner.cpp
>-    if (appData.vendor)
>-      CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("Vendor"),
>-                                     nsDependentCString(appData.vendor));
>-    if (appData.name)
>-      CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("ProductName"),
>-                                     nsDependentCString(appData.name));
>+    CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("Vendor"),
>+                                       NS_LITERAL_CSTRING("Songbird"));
>+    CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("ProductName"),
>+                                       NS_LITERAL_CSTRING("Songbird"));

Yeah, uh, make sure you don't check that in.

>Index: toolkit/xre/nsXREDirProvider.cpp

>-      SUCCEEDED(SHGetPathFromIDList(pItemIDList, result))) {
>+      SHGetPathFromIDListW(pItemIDList, buf)) 

I don't understand why that was using SUCCEEDED in the first place, but good catch.

> #error dont_know_how_to_get_product_dir_on_your_platform

Could you change this to a string while you're here?  That's just silly looking.  Down below as well.

>Index: toolkit/xre/nsXULAppAPI.h
>+   * ($UAppData is defined below and $vendor is optional):
>+   *
>+   *   ProfD = $UAppData[/$vendor]/$app

This isn't really correct since the definition of UAppData below includes $vendor and $app.  I don't know if you can really make it clearer or not.  Maybe editing the comment for UAppData will help?

>+  const char *profile;

I don't think you're allowed to stick a field in the middle of this struct.  It should go at the end.  We do length checking in parts of the code to stay binary compatible with previous versions.  (For old stubs, presumably.)

>Index: toolkit/crashreporter/nsExceptionHandler.cpp
>+dataDirEnv.Append(NS_ConvertUTF16toUTF8(dataDirectoryPath));

Have you tested this with a unicode path?  I'm just worried that the unicode translation code might not work properly at this point, since that other bug I fixed for Linux seemed to be based on the fact that nsIFile::Append was screwing up in Linux in startup.

Ok, I ran out of steam for the night, I'll finish up tomorrow.
(I really did type that last night, but the WiFi here was uncooperative.)
(In reply to comment #13)
> Yeah, uh, make sure you don't check that in.

Haha. /blush.

> Could you change this to a string while you're here?

Sure

> Maybe editing the comment for UAppData will help?

I did a complete overhaul of both comments. Much better now.

> I don't think you're allowed to stick a field in the middle of this struct.

Yep, my bad. Thanks for catching that, moved.

> Have you tested this with a unicode path?

Hm. I tested and it works great on Windows when my username is non-ASCII where it utterly failed before. I guess I assumed that Linux would handle it as well if not better than Windows. I don't have a linux test box atm but I can try it out on OS X.
Ok, so I tested this on OS X with a non-ascii home path and everything worked fine.
(In reply to comment #13)
> fixed for Linux seemed to be based on the fact that nsIFile::Append was
> screwing up in Linux in startup.

Ted was talking about bug 390127.

I will test this, but I think we're ok. The file stuff invokes iconv and other fun stuff in nsLocalFileUnix.cpp to get "native" strings whereas the string code uses a homemade UTF8 converter in nsUTF8Utils.h.
Attached patch Patch, v4.1Splinter Review
Great, so, yeah.

Attachment 281582 [details] [diff] did actually cause that regression. Turns out that the native charset utils aren't initialized yet (setlocale hasn't been called) so iconv was failing to convert on a GetPath call.

This patch fixes that by only using GetPath on windows and relying on GetNativePath on OS X and Linux.

We could fix the charset initialization to happen earlier, I guess, but I think that should be a different bug. It could mess up lots of things.
Attachment #281582 - Attachment is obsolete: true
Attachment #282138 - Flags: review?(ted.mielczarek)
Attachment #281582 - Flags: review?(ted.mielczarek)
Comment on attachment 282138 [details] [diff] [review]
Patch, v4.1

FWIW, I'd appreciate a |diff -w| in the future, what with your editor waging the war on whitespace.  Not a big deal right now since I've already reviewed most of the patch.

>+#error Don't know how to get product dir on your platform

That may actually need to be in quotes.

>Index: toolkit/crashreporter/nsExceptionHandler.cpp

>+#if defined(XP_WIN32)
>+  nsAutoString dataDirectoryPath;
>+  rv = dataDirectory->GetPath(dataDirectoryPath);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  dataDirEnv.Append(NS_ConvertUTF16toUTF8(dataDirectoryPath));

I think you want AppendUTF16toUTF8.

Looks good otherwise.  Thanks for all the other fixes you rolled into this!
Attachment #282138 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 282138 [details] [diff] [review]
Patch, v4.1

This patch fixes a number of bugs (see dup list) with the profile manager and crash reporter.
Attachment #282138 - Flags: approval1.9?
(In reply to comment #18)
> That may actually need to be in quotes.

It doesn't, but I could make it so if you'd like.

> I think you want AppendUTF16toUTF8.

I'll fix that on checkin.
Benjamin, could you take a look?  I'm the wrong person to approve this...
Attachment #282138 - Flags: approval1.9? → approval1.9+
Landed. Thanks guys!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Component: XRE Startup → Startup and Profile System
QA Contact: xre.startup → startup
You need to log in before you can comment on or make changes to this bug.