323780 - Portable Camino - allow the profile root directory to be specified in the environment

Status: VERIFIED FIXED

(Camino Graveyard :: General, enhancement)

Description

[Carlo Gandolfi]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

allow the profile root directory to be specified in the environment as to allow to a script to start camino from external device.

Allow variance for NS_APP_USER_PROFILES_ROOT_DIR

Reproducible: Always

Actual Results:  
Camino opened from external device use local HD profile folder

Expected Results:  
Camino use as profile folder the one on the device where it start.

Portable Camino folder dir:
Start Camino.command
-profile
-app
--camino.app

double click on command set the profile user dir to "profile" with a defaults write... and then start /app/Camino.app

Comment 1

[Chris Lawson (gone)]

Confirming per Mark on IRC.

cl

Comment 2

[Mark Mentovai]

I wanted a way to specify a path to the profile in the environment anyway, ever since a camino tinderbox clobbered my profile because we're not flexible enough with where it's allowed to be.

Comment 3

[Håkan Waara]

What behavior is wanted here? Arguments to Camino.app, and an env var? This is very much dogfood, as I can't stand using Safari while developing Camino...

Comment 4

[Mark Mentovai]

I had planned on doing this as environment variable.  This will mostly involve changes in AppDirServiceProvider and PreferenceManager.

The thing that complicates this slightly is that (I think) the cache should be inside the profile when you're running from an alternate profile.  We've got some code that looks for the cache if it's in the profile and moves it - this will need to be adjusted.

Comment 5

[Håkan Waara]

Attachment: Make Camino portable

So, this makes Camino portable. If $CAMINO_PROFILE_DIR is specified with a path at runtime, Camino will use that for all its data.

The cache will also be stored there, and the reason is that major problems would occur if two Camini were running at the same time using the system cache.

So this patch also makes it possible to run two Camino at the same time, if you specify the env var for at least one of them.

Code-wise, I've removed some Chimera-migration code because those heurestics wouldn't be fun to maintain if we want this feature, and if someone - against all odds would be running Chimera 0.7 they should be able to just specify $CAMINO_PROFILE_DIR to his old profile and get this working.

When running the second Camino, CF barks at us and tells us a BOOTSTRAP_SERVICE_ACTIVE warning, because our other instance is running with the same app ID - but that seems harmless.

I've been testing and debugging this, and everything seems to work fine.

Comment 6

[Håkan Waara]

An "#import"-statement that belong in the Camino update-bug sneaked in.  I'll make sure to clean my tree of other patches before next time.

Comment 7

[Håkan Waara]

One more thing, if you're want, I could at the same time remove the old mozNewProfileDir-pref which now seems redundant.

Comment 8

[Mark Mentovai]

Comment on attachment 212673 [details] [diff] [review]
Make Camino portable

I'm not sure we want to drop the profile migration stuff yet.  Could we leave it in and only migrate profiles when an alternate hasn't been selected?

Comment 9

[Mark Mentovai]

Håkan points out that the Chimera folder hasn't been used since 0.7, and 0.8 saw the switch to Camino.  I didn't realize that (because it's been so long since I've even used 0.8.)  With that in mind, I don't really have a problem losing the migration stuff.

Comment 10

[Simon Fraser [no longer active]]

Comment on attachment 212673 [details] [diff] [review]
Make Camino portable

>Index: src/application/AppDirServiceProvider.cpp
>===================================================================

>   {
>-    rv = GetCacheDirectory(getter_AddRefs(localFile));
>+    // if we're using a custom profile directory, store the cache
>+    // inside the profile - since there could be major problems
>+    // if two Camini would simultaneously be using the same cache!

That would only happen if you have 2 builds on different machines, and using
different profiles, sharing the same cache dir.

>+    if (getenv(CUSTOM_PROFILE_DIR) != nsnull)
>+      rv = GetProfileDirectory (getter_AddRefs(localFile));

I don't like the getenv() in two places. Perhaps make a HasCustomProfile()
method that does the getenv() call.

>+    else
>+      rv = GetSystemDirectory(kCachedDataFolderType, getter_AddRefs(localFile));

>+nsresult
>+AppDirServiceProvider::GetProfileDirectory(nsILocalFile** outFolder)

>+  NS_ENSURE_ARG_POINTER(outFolder);
>+  *outFolder = nsnull;
> 
>+  nsresult rv;
>+  nsCOMPtr<nsILocalFile> profileDir;
>+  rv = GetCustomProfileDirectory (getter_AddRefs (profileDir));
>+  
>+  // If the custom profile folder can't be found, we silently fall back to using
>+  // the Application Support folder.
>+  if (!profileDir || NS_FAILED(rv))
>+  {
>+    rv = GetSystemDirectory (kApplicationSupportFolderType, getter_AddRefs (profileDir));
>+    NS_ENSURE_SUCCESS (rv, rv);
>+    
>+    rv = profileDir->AppendNative(mProductDirName);
>+    if (NS_FAILED(rv))
> {
>+      NS_WARNING ("Couldn't get the profile directory!");

AppendNative() is unlikely to fail (it doesn't check the disk or anything), so
this warning seems redundant.

> nsresult
>+AppDirServiceProvider::GetCustomProfileDirectory(nsILocalFile **outFolder)
> {
>   *outFolder = nsnull;
>   
>+  nsCOMPtr<nsILocalFile> out = do_CreateInstance (NS_LOCAL_FILE_CONTRACTID);

I'm not keen on "out" as the variable name.

>+  // If the user has this env variable set, we will use the specified path for our Camino folder.
>+  const char *customProfilePath = getenv (CUSTOM_PROFILE_DIR);
>+  if (!customProfilePath)
>+    return NS_ERROR_FAILURE;
>+
>+  nsresult rv = NS_OK;
>+  nsCOMPtr<nsILocalFileMac> macFolder = do_CreateInstance (NS_LOCAL_FILE_CONTRACTID, &rv);

No space before ( please.

>+  rv = macFolder->InitWithNativePath (nsDependentCString (customProfilePath));
>+  if (NS_FAILED (rv))
>+    return rv;

Do we need to ensure that the "CUSTOM_PROFILE_DIR" contains something that looks like
a valid path? What if it's garbage? And what if it's not UTF8? Can you specify a UTF8
string in an environment variable?

>+  // always return a copy of our cached object, since the caller might play with it.
>+  out->InitWithFile (mCustomProfileDir);

Remove the space please.

>+  NS_ADDREF (*outFolder = out);
>+  
>+  return NS_OK;
>+}

>+nsresult
>+AppDirServiceProvider::GetSystemDirectory (OSType inFolderType, nsILocalFile** outFolder)

No space before ( please.

>+{
>   FSRef   foundRef;
>   
>   OSErr err = ::FSFindFolder(kUserDomain, inFolderType, kCreateFolder, &foundRef);
>   if (err != noErr)
>     return NS_ERROR_FAILURE;
>-  nsCOMPtr<nsILocalFileMac> localDir(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));
>-  if (!localDir)
>+  
>+  nsCOMPtr<nsILocalFileMac> macFolder = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
>+  if (!macFolder)
>     return NS_ERROR_FAILURE;
>-  rv = localDir->InitWithFSRef(&foundRef);
>-  if (NS_FAILED(rv))
>-    return rv;
>-  rv = localDir->AppendNative(mProductDirName);
>+  
>+  nsresult rv = macFolder->InitWithFSRef(&foundRef);
>   if (NS_FAILED(rv))
>     return rv;
>   
>+  nsCOMPtr<nsILocalFile> temp (do_QueryInterface (macFolder));
>+  NS_ADDREF (*outFolder = temp);
>+  
>+  return NS_OK;
>+}
>+
>+nsresult
>+AppDirServiceProvider::EnsureExists (nsILocalFile* inFolder)

No space before ( please.

Comment 11

[Mark Mentovai]

> That would only happen if you have 2 builds on different machines, and using
> different profiles, sharing the same cache dir.

Why do they have to be on different machines?  You can launch two Caminoes on a single machine with different profiles using this patch.  In fact, that's why Håkan told me he got interested in this: the ability to run one Camino for real work while debugging another.

Comment 12

[Simon Fraser [no longer active]]

(In reply to comment #11)
> > That would only happen if you have 2 builds on different machines, and using
> > different profiles, sharing the same cache dir.
> 
> Why do they have to be on different machines?  You can launch two Caminoes on a
> single machine with different profiles using this patch.

Oh right, i forgot we just use the profile lock.

Comment 13

[Mark Mentovai]

Comment on attachment 212673 [details] [diff] [review]
Make Camino portable

I don't like all the calls to getenv() at all.  Let's get the profile path once (in PreferenceManager) and then call into the dirprovider to set it.  Right now, we set productDirName in the dirprovider ctor.  We should avoid doing that when we're not using the standard profile.

Comment 14

[Håkan Waara]

Attachment: Make Camino portable v2

Changes:

* Addressed review comments.
* Fixed movable prefpanes to use the right profile dir, removing the old mozNewProfileDir pref in the process.
* Implemented a simple -profile argument for Camino that is mapped to the env var.

As for the UTF8-question: nsILocalFile's InitWithNativePath() expects an nsACString, and we get the env var as a c-string; I'm not a string expert, but that gives you problems as far as conversion goes, no?

All other nsILocalFile APIs use nsAString instead of nsACString -- maybe this should be fixed? If that's the case, perhaps we can file a follow-up bug.

Comment 15

[Simon Fraser [no longer active]]

Comment on attachment 212813 [details] [diff] [review]
Make Camino portable v2

>Index: Info-Camino.plist
>===================================================================

>-	<key>mozNewProfileDirName</key>
>-	<string>Camino</string>

One reason we had the profile name in the Info.plist was so embedding clients could reuse our code, and be able to specify their own profile dir. I think this is nicer than hardcoding "Camino".

Comment 16

[Mark Mentovai]

Comment on attachment 212813 [details] [diff] [review]
Make Camino portable v2

This doesn't address my comments fully: you're still calling getenv every time you want to get the profile or cache directory.  I suggested moving getenv to PreferenceManager, and calling into AppDirServiceProvider with the custom profile location (or doing it in the constructor).

Comment 17

[Mark Mentovai]

Comment on attachment 212813 [details] [diff] [review]
Make Camino portable v2

>Index: src/application/AppDirServiceProvider.cpp
>   if (strcmp(prop, NS_APP_APPLICATION_REGISTRY_DIR) == 0)
>   {
>-    rv = GetProductDirectory(getter_AddRefs(localFile));
>+    rv = GetProfileDirectory(getter_AddRefs(localFile));

Lots of these keys return the profile directory (formerly the product directory).  Especially if you're going to be adding more, but even if you're not, I think some of these cases should be combined.

>   }
>   else if (strcmp(prop, NS_APP_APPLICATION_REGISTRY_FILE) == 0)
>   {
>-    rv = GetProductDirectory(getter_AddRefs(localFile));
>+    rv = GetProfileDirectory(getter_AddRefs(localFile));
>     if (NS_SUCCEEDED(rv))
>       rv = localFile->AppendNative(APP_REGISTRY_NAME);
>   }
>   else if (strcmp(prop, NS_APP_USER_PROFILES_ROOT_DIR) == 0)
>   {
>-    rv = GetProductDirectory(getter_AddRefs(localFile));
>+    rv = GetProfileDirectory(getter_AddRefs(localFile));
>+  }
>+  else if (strcmp(prop, NS_APP_USER_PROFILE_50_DIR) == 0)

Is this strictly necessary?

>+  {
>+    rv = GetProfileDirectory(getter_AddRefs(localFile));
>   }
>   else if (strcmp(prop, NS_APP_CACHE_PARENT_DIR) == 0)
>   {
>-    rv = GetCacheDirectory(getter_AddRefs(localFile));
>+    // if we're using a custom profile directory, store the cache
>+    // inside the profile - since there could be major problems
>+    // if two Camini would simultaneously be using the same cache!
>+    if (!HasCustomProfileDirectory(getter_AddRefs(localFile)))
>+      rv = GetSystemDirectory(kCachedDataFolderType, getter_AddRefs(localFile));

And what if there is a custom profile directory?

>+nsresult
>+AppDirServiceProvider::GetProfileDirectory(nsILocalFile** outFolder)
> {
>-  return EnsureFolder(kApplicationSupportFolderType, outLocalFile);
>-}
>+  NS_ENSURE_ARG_POINTER(outFolder);
>+  *outFolder = nsnull;
>+  nsresult rv;
>+  nsCOMPtr<nsILocalFile> profileDir;
>+  PRBool isCustomDir = HasCustomProfileDirectory(getter_AddRefs(profileDir));
>+  
>+  // If the custom profile folder can't be found, we silently fall back to using
>+  // the Application Support folder.
>+  if (!isCustomDir || !profileDir)

That doesn't seem right.  If, for some reason, isCustomDir is true but profileDir is false, we don't want to fall back, we want to return an error.  This could happen because of an out-of-memory condition, but if something else were able to cause it, it would really suck to just start using some other profile in the middle of a session just for the hell of it.

>+PRBool
>+AppDirServiceProvider::HasCustomProfileDirectory(nsILocalFile **outFolder)
> {
>   *outFolder = nsnull;
>+  nsCOMPtr<nsILocalFile> profileDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
>   
>+  if (mCustomProfileDir) {
>+    profileDir->InitWithFile(mCustomProfileDir);
>+    NS_ADDREF(*outFolder = profileDir);
>+    return PR_TRUE;
>+  }
>+  
>+  // If the user has this env variable set, we will use the specified path for our Camino folder.
>+  const char *customProfilePath = getenv(CUSTOM_PROFILE_DIR);

This is what I keep complaining about.  You're probably going to say "but look above, we only call this once, and after that, we return a new file copied from mCustomProfileDir!"  But then I'd say "only when you're using a custom profile directory!"

Is the mCustomProfileDir cache really helping out much?  I don't see why you'd want to cache when using a custom profile and not cache when not.

>Index: src/application/AppDirServiceProvider.h
>-#include "nsILocalFile.h"
>+#include "nsILocalFileMac.h"

Why?  You don't use any nsILocalFileMac extensions in the header.

> #include "nsString.h"
> 
> #include <Carbon/Carbon.h>
> 
> class nsIFile;
> 
> //*****************************************************************************
> // class AppDirServiceProvider
> //*****************************************************************************   
> 
> class AppDirServiceProvider : public nsIDirectoryServiceProvider
> {
> public:
>+                              AppDirServiceProvider ();
>
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIDIRECTORYSERVICEPROVIDER
> 
> protected:
>    virtual              ~AppDirServiceProvider();

What's going on with the spacing here?

>+   nsresult                   GetProfileDirectory (nsILocalFile **outFolder);
>+   nsresult                   EnsureExists (nsILocalFile* inFolder);
>+   nsresult                   GetSystemDirectory (OSType inFolderType, nsILocalFile** outFolder);
>+   PRBool                     HasCustomProfileDirectory (nsILocalFile **outFolder);
> 
>    nsCString            mProductDirName;
>+   nsCOMPtr<nsILocalFile>     mCustomProfileDir;
> };

>+  
>+  // simple hack to implement "-profile" argument support.
>+  // if -profile is specified, overwrite/set the $CAMINO_PROFILE_DIR env var
>+  if (argc >= 3) {
>+    if (argv && strcmp(argv[1], "-profile") == 0)
>+      setenv("CAMINO_PROFILE_DIR", argv[2], 1);
>+  }

This isn't gonna fly.  :)

>Index: src/preferences/PreferenceManager.mm
>-    AppDirServiceProvider *provider = new AppDirServiceProvider(nsDependentCString(profileDirName));
>+    AppDirServiceProvider *provider = new AppDirServiceProvider();

Aha, this is where the getenv belongs!

Comment 18

[Håkan Waara]

Attachment: Satisfy embeddors too

Ok, so this patch takes the approach to make AppDirServiceProvider agnostic about its user.  An embeddor can pass it its name, and use the Application Support/<product name> folder (instead of the old mozProfileNewDir plist-change).

Other notes:

* Some components in core are not happy about using non-ascii chars right now; this is for follow-up bug(s). In general, this feature will need some QA and should be considered experimental for now.

* Changed it so we don't create a history_BAD file if there's no earlier history file.

Comment 19

[Mark Mentovai]

Comment on attachment 213658 [details] [diff] [review]
Satisfy embeddors too

>Index: camino/src/application/AppDirServiceProvider.cpp
>===================================================================
>+AppDirServiceProvider::AppDirServiceProvider()
>+{
>+}

Why a default constructor AND a SetProfilePath method?  Why not just have a constructor for setting mProductName and one for setting mProfilePath?  It's not like these paths will be changing during the lifetime of the dirprovider.

>+// Use this constructor if you want to use the Application Support/<inProductName> folder, 
>+// and the 'Caches' folder.
>+AppDirServiceProvider::AppDirServiceProvider(const nsACString &inProductName)
> {
>+  mProductName.Assign(inProductName);
> }
> 
> AppDirServiceProvider::~AppDirServiceProvider()
> {
> }
> 
>+nsresult
>+AppDirServiceProvider::SetProfilePath(const nsACString &inPath)
>+{
>+  mProfilePath.Assign(inPath);
>+  return NS_OK;
>+}
> 
> NS_IMPL_ISUPPORTS1(AppDirServiceProvider, nsIDirectoryServiceProvider)

If you do have a good reason to keep SetProfilePath, move it after NS_IMPL_ISUPPORTS.  This is a good rule of thumb anywhere: NS_IMPL_ISUPPORTS comes first, and then methods that implement each interface should be grouped together.

> NS_IMETHODIMP
> AppDirServiceProvider::GetFile(const char *prop, PRBool *persistent, nsIFile **_retval)
> {    
>   nsCOMPtr<nsILocalFile>  localFile;
>   nsresult rv = NS_ERROR_FAILURE;
>-  nsCAutoString strBuf;
> 
>   *_retval = nsnull;
>   *persistent = PR_TRUE;
>   
>-  if (strcmp(prop, NS_APP_APPLICATION_REGISTRY_DIR) == 0)
>+  if (strcmp(prop, NS_APP_APPLICATION_REGISTRY_DIR) == 0 ||
>+      strcmp(prop, NS_APP_USER_PROFILES_ROOT_DIR)   == 0 /*||
>+      strcmp(prop, NS_APP_USER_PROFILE_50_DIR)      == 0*/)

How's it working with this commented out?  I still maintain that we don't need it.  You shouldn't check in with the comment in place, anyway.

>   {
>-    rv = GetProductDirectory(getter_AddRefs(localFile));
>+    rv = GetProfileDirectory(getter_AddRefs(localFile));
>   }
>   else if (strcmp(prop, NS_APP_APPLICATION_REGISTRY_FILE) == 0)
>   {
>-    rv = GetProductDirectory(getter_AddRefs(localFile));
>+    rv = GetProfileDirectory(getter_AddRefs(localFile));
>     if (NS_SUCCEEDED(rv))
>       rv = localFile->AppendNative(APP_REGISTRY_NAME);
>   }
>-  else if (strcmp(prop, NS_APP_USER_PROFILES_ROOT_DIR) == 0)
>-  {
>-    rv = GetProductDirectory(getter_AddRefs(localFile));
>-  }
>   else if (strcmp(prop, NS_APP_CACHE_PARENT_DIR) == 0)
>   {
>-    rv = GetCacheDirectory(getter_AddRefs(localFile));
>+    // if we're using a custom profile directory, store the cache
>+    // inside the profile - since there could be major problems
>+    // if two Camini would simultaneously be using the same cache!
>+    if (!mProfilePath.IsEmpty())
>+      rv = GetProfileDirectory(getter_AddRefs(localFile));
>+    else
>+      rv = GetSystemDirectory(kCachedDataFolderType, getter_AddRefs(localFile));

Since you have GetProfileDirectory, why not keep GetCacheDirectory and do the dirty work there?

Don't you need to append something here?  Append some literal in the first case and append the product name in the second?  Be sure when we're putting the cache in the profile that necko's not detecting it as an old-style in-profile cache and obliterating it.

>   }
>     
>   if (localFile && NS_SUCCEEDED(rv))
>     return localFile->QueryInterface(NS_GET_IID(nsIFile), (void**)_retval);
>     
>   return rv;
> }
> 
>+// Private functions

Pedantery: methods, not functions.

>+nsresult
>+AppDirServiceProvider::GetProfileDirectory(nsILocalFile** outFolder)
>+{
>+  NS_ENSURE_ARG_POINTER(outFolder);
>+  *outFolder = nsnull;
>+  nsresult rv = NS_OK;
>+  
>+  // We init the profile directory lazily, since the component manager
>+  // is not around when this class is first instantiated.

This is what NS_NewNativeLocalFile is for.  You don't need to do that here yet, but I'll need to fix it in bug 326668.

>+  if (!mProfileDir)
> {
>-  return EnsureFolder(kApplicationSupportFolderType, outLocalFile);
>+    mProfileDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS (rv, rv);
>+    
>+    // if a custom profile path is specified, let's try using that.
>+    if (!mProfilePath.IsEmpty())
>+    {
>+      nsCOMPtr<nsILocalFileMac> macFolder = do_QueryInterface(mProfileDir);
>+      rv = macFolder->InitWithNativePath(mProfilePath);
>+      NS_ENSURE_SUCCESS(rv, rv);
> }

What's up with the weird (nonexistent) indentation?  (Above, too.)

>+    // if a product name is specified, we append that to the profile path,
>+    // be it the Application Support folder, or a custom path.
>+    if (!mProductName.IsEmpty())
> {

And again with the indentation?

Seems like this should be |else if|, letting you get rid of this next conditional:

>+      if (mProfilePath.IsEmpty())
>+      {
>+        // get the application support folder
>+        rv = GetSystemDirectory(kApplicationSupportFolderType, getter_AddRefs(mProfileDir));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+      }
>+      
>+      mProfileDir->AppendNative(mProductName);

...since I don't see any reason you'd want to append mProductName to a custom path.

>+    }
>+    
>+    EnsureExists(mProfileDir);
>+  } // end lazy init
>+  
>+  nsCOMPtr<nsILocalFile> profileDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
>+  profileDir->InitWithFile(mProfileDir);
>+  
>+  NS_ADDREF(*outFolder = profileDir);
>+  return NS_OK;
> }
> 
> nsresult
>+AppDirServiceProvider::GetSystemDirectory(OSType inFolderType, nsILocalFile** outFolder)
> {
>   FSRef   foundRef;
>   
>   OSErr err = ::FSFindFolder(kUserDomain, inFolderType, kCreateFolder, &foundRef);
>   if (err != noErr)
>     return NS_ERROR_FAILURE;
>+  
>+  nsCOMPtr<nsILocalFileMac> macFolder = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
>+  if (!macFolder)
>     return NS_ERROR_FAILURE;
>+  
>+  nsresult rv = macFolder->InitWithFSRef(&foundRef);
>   if (NS_FAILED(rv))
>     return rv;
>   
>+  nsCOMPtr<nsILocalFile> temp(do_QueryInterface(macFolder));
>+  NS_ADDREF(*outFolder = temp);
>+  
>+  return NS_OK;
>+}
>+
>+nsresult
>+AppDirServiceProvider::EnsureExists(nsILocalFile* inFolder)
>+{
>   PRBool exists;
>+  nsresult rv = inFolder->Exists(&exists);
>   if (NS_SUCCEEDED(rv) && !exists)
>+    rv = inFolder->Create(nsIFile::DIRECTORY_TYPE, 0775);
>   if (NS_FAILED(rv))
>     return rv;
>+  return NS_OK;

Can't the three lines above be replaced with |return rv;|?

> }

>Index: camino/src/application/AppDirServiceProvider.h
>===================================================================
>@@ -49,25 +49,30 @@ class nsIFile;
> 
> //*****************************************************************************
> // class AppDirServiceProvider
> //*****************************************************************************   
> 
> class AppDirServiceProvider : public nsIDirectoryServiceProvider
> {
> public:
>+                            AppDirServiceProvider ();
>+                            AppDirServiceProvider (const nsACString &inProductName);
>+  
>+    nsresult                SetProfilePath (const nsACString &inPath);
> 
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIDIRECTORYSERVICEPROVIDER
> 
> protected:
>    virtual              ~AppDirServiceProvider();

As long as you're reindenting the whole file, you might as well do the destructor too.

> 
>+    nsresult                GetProfileDirectory (nsILocalFile **outFolder);
>+    nsresult                GetSystemDirectory (OSType inFolderType, nsILocalFile** outFolder);
>+    nsresult                EnsureExists (nsILocalFile* inFolder);
>+  
>+    nsCOMPtr<nsILocalFile>  mProfileDir;
>+    nsCString               mProductName;
>+    nsCString               mProfilePath;
> };
> 
> #endif // __AppDirServiceProvider_h__

>Index: camino/src/history/nsSimpleGlobalHistory.cpp
>===================================================================
>@@ -1737,38 +1737,39 @@ nsSimpleGlobalHistory::OpenDB()
> 
>   // MDB requires native file paths
> 
>   nsCAutoString filePath;
>   rv = historyFile->GetNativePath(filePath);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   PRBool exists = PR_TRUE;
>-
>   historyFile->Exists(&exists);
> 
>-  if (!exists || NS_FAILED(rv = OpenExistingFile(gMdbFactory, filePath.get())))
>+  if (NS_FAILED(rv = OpenExistingFile(gMdbFactory, filePath.get())) && exists)
>   {
>-    // we couldn't open the file, so it's either corrupt or doesn't exist.
>+    // we couldn't open the file, so it might be corrupt.
>     // attempt to delete the file, but ignore the error
>     NS_ASSERTION(0, "Failed to open history.dat file");
> #if DEBUG
>     nsCOMPtr<nsIFile> fileCopy;
>     historyFile->Clone(getter_AddRefs(fileCopy));
>     fileCopy->SetLeafName(NS_LITERAL_STRING("history_BAD.dat"));
>     fileCopy->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
>     nsString uniqueFileName;
>     fileCopy->GetLeafName(uniqueFileName);
>     historyFile->MoveTo(nsnull, uniqueFileName);
> #else
>     historyFile->Remove(PR_FALSE);
> #endif
>-    rv = OpenNewFile(gMdbFactory, filePath.get());
>   }
> 
>+  if (!exists)
>+    rv = OpenNewFile(gMdbFactory, filePath.get());
>+

OK, this is getting weird and you're introducing duplication.  I think this is better:

if (NS_FAILED(rv = OpenExistingFile(gMdbFactory, filePath.get()))) {
  PRBool exists = PR_TRUE;
  historyFile->Exists(&exists);
  if (exists) {
    NS_ASSERTION(0, "Failed to open history.dat file");
#if DEBUG
    // copy
#else
    historyFile->Remove(PR_FALSE);
#endif
  }
  rv = OpenNewFile(gMdbFactory, filePath.get());
}

Do you agree?

You also need to do something about camino/browser/SiteIconCache.mm:

- (NSString*)cacheDirectory
{
  return [@"~/Library/Caches/Camino/IconCache" stringByStandardizingPath];
}

Probably you would handle this by adding a cacheDirPath method to the prefmanager.

Getting closer!

Comment 20

[Mark Mentovai]

Oh, yuck, this was a -w diff.  At least that explains the goofy indentation, but you shouldn't have needed -w on this.

Comment 21

[Håkan Waara]

Filed bug 329283 for the issue that necko is hardcoded to remove <profile>/Cache, if it doesn't find the cache dir at first.

Comment 22

[Håkan Waara]

Dunno if this already happens on trunk, but my patched build is getting a lot of warnings on startup due to bug 326837.  This is harmless, but a little annoying.

New patch coming up. The biggest change is that I made the SiteIconCache work correctly. For local profiles, it'll live in <local profile>/IconCache, and for a normal profile, it'll live where it always has, i.e. Caches/Camino/IconCache.

I've gone through all other comments (except for the NS_* instantiation of the local file, which Mark will do in another bug).

Comment 23

[Håkan Waara]

Attachment: Patch v4

Let's see if it passes Mark this time, before setting the sr?-flag. ;-)

Comment 24

[Håkan Waara]

If you try to start up another Camino, using the same profile, as of today you get a "Camino is already running..."-error dialog.  

Should we somehow modify that to point out that the profile is already used, and hint about this feature?

Comment 25

[Mark Mentovai]

Comment on attachment 214102 [details] [diff] [review]
Patch v4

+// If no profile path is specified, we'll use the Application Support/<product name> and Caches/<product name> folders.
+AppDirServiceProvider::AppDirServiceProvider(const char *inProductName, const char *inProfilePath)
 {
-  mProductDirName.Assign(productDirName);
+  if (inProductName)
+    mProductName.Assign(nsDependentCString(inProductName));
+  
+  if (inProfilePath)
+    mProfilePath.Assign(nsDependentCString(inProfilePath));

Make this |else if| (and eliminate the newline).

   else if (strcmp(prop, NS_APP_CACHE_PARENT_DIR) == 0)
   {
+    // if we're using a custom profile directory, store the cache
+    // inside the profile - since there could be major problems
+    // if two Camini would simultaneously be using the same cache!
     rv = GetCacheDirectory(getter_AddRefs(localFile));


This comment belongs in GetCacheDirectory, not GetFile - but the comments in GetCacheDirectory are already sufficient, so you can just get rid of this.  (But I know you want to use "Camini," so maybe you want to rephrase the comment in the other method anyway!)

+// Private methods


But they're not, they're protected!

+      nsCOMPtr<nsILocalFileMac> macFolder = do_QueryInterface(mProfileDir);
+      rv = macFolder->InitWithNativePath(mProfilePath);


Why QueryInterface?  InitWithNativePath is part of nsILocalFile.

+  nsCOMPtr<nsILocalFile> profileDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
+  profileDir->InitWithFile(mProfileDir);


What if you couldn't create profileDir?  Use the two-argument form of do_CreateInstance (preferred for consistency, since you did it above), or null-check profileDir.

-  *outFolder = localDir;
-  NS_ADDREF(*outFolder);
+  nsCOMPtr<nsILocalFile> temp(do_QueryInterface(macFolder));
+  NS_ADDREF(*outFolder = temp);


Why the QI?  Seems better the old way (with the new variable name).

 - (void)loadCache
 {
   if ([self readCacheFile])
     return;
-
+  

This change will turn the tinderboxen red!

+  if (exists && NS_FAILED(rv = OpenExistingFile(gMdbFactory, filePath.get())))
[...remove corrupt history file but don't create a new one...]
+  else if (!exists)

     rv = OpenNewFile(gMdbFactory, filePath.get());

What happens if the history file is corrupt?  You can set |exists| to PR_FALSE if the file is corrupt, and turn the |else if| into an |if|.

-    // get the 'mozNewProfileDirName' key from our Info.plist file
-    NSString *dirString = [[[NSBundle mainBundle] infoDictionary] objectForKey:@"mozNewProfileDirName"];
-    const char* profileDirName;
-    if (dirString)
-      profileDirName = [dirString UTF8String];
-    else {
-      NSLog(@"mozNewProfileDirName key missing from Info.plist file. Using default profile directory");
-      profileDirName = "Camino";
-    }


I'm more ambivalent, but I think Simon didn't like taking the key out of Info.plist - see comment 15.  In any event, like in earlier versions of your patch, you should supply a diff against Info-Camino.plist and Info-CaminoStatic.plist to get rid of mozOldProfileDirName, and to remove, if it's OK with Simon, or possibly rename mozNewProfileDirName.

Getting close, I can feel it!  I bet the next one will be gold.

(In reply to comment #24)
> If you try to start up another Camino, using the same profile, as of today you
> get a "Camino is already running..."-error dialog.  
>
> Should we somehow modify that to point out that the profile is already used,
> and hint about this feature?

This is a power-user feature, and I don't want to make the error message overly inaccessible to the average Ellen Feiss.  I'm not opposed to changing the message, but I don't know if it's possible to do this without filling it with scary-sounding terms like "environment variable."  Worse, I don't want to fill the heads of people who really don't want to run more than one Camino with more ideas and information than they need.  Regardless, don't let it hold up your work here.  We can easily change the strings later.

Comment 26

[Håkan Waara]

(In reply to comment #25)
>  - (void)loadCache
>  {
>    if ([self readCacheFile])
>      return;
> -
> +  
> 
> This change will turn the tinderboxen red!

Why?

> I'm more ambivalent, but I think Simon didn't like taking the key out of
> Info.plist - see comment 15.  In any event, like in earlier versions of your
> patch, you should supply a diff against Info-Camino.plist and
> Info-CaminoStatic.plist to get rid of mozOldProfileDirName, and to remove, if
> it's OK with Simon, or possibly rename mozNewProfileDirName.

Oh, the thought is that embeddors can use the AppDirServiceProvider() constructor directly for providing their product name. I don't really see a reason for keeping around moz*ProfileDirName with this change... unless someone insists?

> 
> Getting close, I can feel it!  I bet the next one will be gold.
> 
> (In reply to comment #24)
> > If you try to start up another Camino, using the same profile, as of today you
> > get a "Camino is already running..."-error dialog.  
> >
> > Should we somehow modify that to point out that the profile is already used,
> > and hint about this feature?
> 
> This is a power-user feature, and I don't want to make the error message overly
> inaccessible to the average Ellen Feiss.  I'm not opposed to changing the
> message, but I don't know if it's possible to do this without filling it with
> scary-sounding terms like "environment variable."  Worse, I don't want to fill
> the heads of people who really don't want to run more than one Camino with more
> ideas and information than they need.  Regardless, don't let it hold up your
> work here.  We can easily change the strings later.

Ok, let's wait with that for now.

Comment 27

[Mark Mentovai]

> > -
> > +  
> > 
> > This change will turn the tinderboxen red!
> 
> Why?

It was a joke!  :)

(But you shouldn't add blank lines with nothing but whitespace.)

> Oh, the thought is that embeddors can use the AppDirServiceProvider()
> constructor directly for providing their product name. I don't really see a
> reason for keeping around moz*ProfileDirName with this change... unless
> someone insists?

They can use the constructor today, too, so I imagine he had something else in mind.  Possibly it was as simple as eliminating as much hard-coded stuff as possible, or maybe it's about sharing our PreferenceManager implementation.  Simon?

Comment 28

[Håkan Waara]

Attachment: Patch v5

Fifth time's the charm?

Comment 29

[Mark Mentovai]

Comment on attachment 214137 [details] [diff] [review]
Patch v5

In GetSystemDirectory:

+  nsCOMPtr<nsILocalFile> temp(do_QueryInterface(macFolder));
+  NS_ADDREF(*outFolder = temp);
   
-  *outFolder = localDir;
-  NS_ADDREF(*outFolder);
+  return QueryInterface(NS_GET_IID(nsILocalFile), (void**)outFolder);

I think you may have misinterpreted my comment.  What's wrong with this?:

*outFolder = macFolder;
NS_ADDREF(*macFolder);
return NS_OK;

This is equivalent to what you already have in GetProfileDirectory, and to what is already in EnsureFolder now.  Why do you need to QI to something more general?  Why do you need to do it twice?

Comment 30

[Mark Mentovai]

Plenty's wrong with it: I mean to addref *outFolder.  But as long as I'm here, I'll reassign the bug to where it belongs.

Comment 31

[Håkan Waara]

(In reply to comment #29)
> (From update of attachment 214137 [details] [diff] [review] [edit])
> In GetSystemDirectory:
> 
> +  nsCOMPtr<nsILocalFile> temp(do_QueryInterface(macFolder));
> +  NS_ADDREF(*outFolder = temp);
> 
> -  *outFolder = localDir;
> -  NS_ADDREF(*outFolder);
> +  return QueryInterface(NS_GET_IID(nsILocalFile), (void**)outFolder);
> 
> I think you may have misinterpreted my comment.  What's wrong with this?:
> 
> *outFolder = macFolder;
> NS_ADDREF(*macFolder);
> return NS_OK;
> 
> This is equivalent to what you already have in GetProfileDirectory, and to what
> is already in EnsureFolder now.  Why do you need to QI to something more
> general?  Why do you need to do it twice?
> 

Oh, that double QI in GetSystemDir() is just an error, made while addressing your comment.

Simply NS_ADDREF(*outFolder = macFolder) isn't possible since |macFolder| is a nsILocalFileMac, and |outFolder| is a nsILocalFile; that's why you need the QI first.

Comment 32

[Håkan Waara]

Hm, now that I look closer:  indeed it seems as the old EnsureFolder() did that. I thought you had to QI before changing return type too.

So, at the end of GetSystemDirectory() I've removed both QIs and replaced with:

NS_ADDREF(*outFolder = macFolder);
return NS_OK;

Comment 33

[Mark Mentovai]

Comment on attachment 214137 [details] [diff] [review]
Patch v5

r=me with the QI change discussed above.  Also, two more minor changes:
- Initialize *outFolder to nsnull in GetSystemDirectory
- Rename GetCacheDirectory to GetCacheParentDirectory for clarity

You can make these changes on checkin, pending sr.  I'd also like smfr to clarify comment 15 (see 26 and 27).

Comment 34

[Simon Fraser [no longer active]]

We took the profile name from the plist to reduce the number of hardcoded paths. It also allows savvy users to run 2 copies of Camino if they modify that key in one of them. I'm fine with taking the "new" out of the name.

Comment 35

[Simon Fraser [no longer active]]

Comment on attachment 214137 [details] [diff] [review]
Patch v5

>Index: src/application/AppDirServiceProvider.cpp
>===================================================================

>-AppDirServiceProvider::AppDirServiceProvider(const nsACString& productDirName)
>+// If no profile path is specified, we'll use the Application Support/<product name> and Caches/<product name> folders.
>+AppDirServiceProvider::AppDirServiceProvider(const char *inProductName, const char *inProfilePath)
> {
>-  mProductDirName.Assign(productDirName);
>+  if (inProfilePath)
>+    mProfilePath.Assign(nsDependentCString(inProfilePath));
>+  else if (inProductName)
>+    mProductName.Assign(nsDependentCString(inProductName));

What if both are null? What if both are set?

> nsresult
>-AppDirServiceProvider::GetCacheDirectory(nsILocalFile** outCacheFolder)
>+AppDirServiceProvider::GetProfileDirectory(nsILocalFile** outFolder)
> {
>-  return EnsureFolder(kCachedDataFolderType, outCacheFolder);
>+  NS_ENSURE_ARG_POINTER(outFolder);
>+  *outFolder = nsnull;
>+  nsresult rv = NS_OK;
>+  
>+  // We init the profile directory lazily, since the component manager
>+  // is not around when this class is first instantiated.
>+  if (!mProfileDir)
>+  {
>+    mProfileDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);

So if this can be called before XPCOM is initialized, how can you expect
CreateInstance to work? And, anyway, why not just use NS_NewLocalFile here?

>+    
>+    // if a custom profile path is specified, let's try using that.
>+    if (!mProfilePath.IsEmpty())
>+    {
>+      rv = mProfileDir->InitWithNativePath(mProfilePath);
>+      NS_ENSURE_SUCCESS(rv, rv);

>+    EnsureExists(mProfileDir);

You should NSLog here if this fails, I think, and say why. I'd imagine
that people using env vars are savvy enough to look in the console for
errors.

>+  } // end lazy init
>+  
>+  nsCOMPtr<nsILocalFile> profileDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  profileDir->InitWithFile(mProfileDir);
>+  NS_ADDREF(*outFolder = profileDir);

Why not just Clone it?

> nsresult
>+AppDirServiceProvider::GetSystemDirectory(OSType inFolderType, nsILocalFile** outFolder)
> {
>-  NS_ENSURE_ARG_POINTER(outFolder);
>-  *outFolder = nsnull;
>-  
>-  nsresult rv;
>-  FSRef   foundRef;
>+  FSRef foundRef;
>   
>   OSErr err = ::FSFindFolder(kUserDomain, inFolderType, kCreateFolder, &foundRef);
>   if (err != noErr)
>     return NS_ERROR_FAILURE;
>-  nsCOMPtr<nsILocalFileMac> localDir(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));
>-  if (!localDir)
>+  
>+  nsCOMPtr<nsILocalFileMac> macFolder = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);

NS_NewLocalFile?

>+// Gets the parent directory to the Cache folder.
>+nsresult
>+AppDirServiceProvider::GetCacheDirectory(nsILocalFile** outFolder)
>+{
>+  if (!mProfilePath.IsEmpty())
>+    return GetProfileDirectory(outFolder);

I think I'd prefer a more specific flag that says "hey, we're using
a special profile dir" than a non-empty mProfilePath. See also
the ctor.

>+nsresult
>+AppDirServiceProvider::EnsureExists(nsILocalFile* inFolder)

This can be a static method, right?

>Index: src/application/AppDirServiceProvider.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/application/AppDirServiceProvider.h,v
>retrieving revision 1.3
>diff -u -8 -p -r1.3 AppDirServiceProvider.h
>--- src/application/AppDirServiceProvider.h	6 May 2005 03:46:06 -0000	1.3
>+++ src/application/AppDirServiceProvider.h	6 Mar 2006 00:47:07 -0000
>@@ -49,25 +49,29 @@ class nsIFile;
> 
> //*****************************************************************************
> // class AppDirServiceProvider
> //*****************************************************************************   
> 
> class AppDirServiceProvider : public nsIDirectoryServiceProvider
> {
> public:
>-                        AppDirServiceProvider(const nsACString& productDirName);
>+                            AppDirServiceProvider (const char *inProductName, const char *inProfilePath);

No space before (

> protected:
>-   virtual              ~AppDirServiceProvider();
>+    virtual                 ~AppDirServiceProvider ();

Icky space.

> 
>-   NS_METHOD            GetProductDirectory(nsILocalFile **aLocalFile);
>-   nsresult             GetCacheDirectory(nsILocalFile** outCacheFolder);
>-   nsresult             EnsureFolder(OSType inFolderType, nsILocalFile** outFolder);
>-
>-   nsCString            mProductDirName;
>+    nsresult                GetProfileDirectory (nsILocalFile **outFolder);
>+    nsresult                GetCacheDirectory(nsILocalFile** outFolder);
>+      
>+    nsresult                GetSystemDirectory (OSType inFolderType, nsILocalFile** outFolder);
>+    nsresult                EnsureExists (nsILocalFile* inFolder);

I like to add another "protected:" to separate methods from member variables here.

>+  
>+    nsCOMPtr<nsILocalFile>  mProfileDir;
>+    nsCString               mProductName;
>+    nsCString               mProfilePath;

Add comments to describe the significance and special rules concerting these member varaibles, since they do exist, and you haven't been very explicit about them.

>Index: src/browser/SiteIconCache.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/SiteIconCache.mm,v
>retrieving revision 1.1
>diff -u -8 -p -r1.1 SiteIconCache.mm
>--- src/browser/SiteIconCache.mm	13 Nov 2005 00:07:27 -0000	1.1
>+++ src/browser/SiteIconCache.mm	6 Mar 2006 00:47:12 -0000
>@@ -32,16 +32,17 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #import "NSString+Utils.h"
> #import "NSFileManager+Utils.h"
>+#import "PreferenceManager.h"

I don't like this new dependency. I'd prefer that the site icon cache
path is pushed in from the outside.

>Index: src/preferences/PreferenceManager.mm
>===================================================================

>+#define PRODUCT_NAME        "Camino"

I still want this to come from the plist.


>     // Supply our own directory service provider so we can control where
>     // the registry and profiles are located.
>-    AppDirServiceProvider *provider = new AppDirServiceProvider(nsDependentCString(profileDirName));
>-    if (!provider)
>+    AppDirServiceProvider *provider;
>+    const char* customProfilePath = getenv(CUSTOM_PROFILE_DIR);
>+    if (customProfilePath)
>+      provider = new AppDirServiceProvider(NULL, customProfilePath);
>+    else
>+      provider = new AppDirServiceProvider(PRODUCT_NAME, NULL);

General coding cleanliness rule: separate what is significant, share what is not.
In this case, the if tests should set the params, but there should be just one
"new AppDirServiceProvider" call:

const char* customProfilePath = NULL;
const char* productName = NULL;
... init them

provider = new AppDirServiceProvider(productName, customProfilePath);

This either/or thing is weakly documented even here.


>-- (NSString*) newProfilePath
>+- (NSString*) profilePath
> {
>-  nsCOMPtr<nsIFile> appSupportDir;
>+  nsCOMPtr<nsIFile> profileDir;
>   nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILES_ROOT_DIR,
>-                                       getter_AddRefs(appSupportDir));
>+                                       getter_AddRefs(profileDir));
>   if (NS_FAILED(rv))
>     return nil;
>   nsCAutoString nativePath;
>-  rv = appSupportDir->GetNativePath(nativePath);
>+  rv = profileDir->GetNativePath(nativePath);
>   if (NS_FAILED(rv))
>     return nil;
>   return [NSString stringWithUTF8String:nativePath.get()];
> }

Hm, I bet we call -profilePath quite a bit on startup. Maybe we should cache
the result?

Comment 36

[Mark Mentovai]

> const char* customProfilePath = NULL;
> const char* productName = NULL;
> ... init them
>
> provider = new AppDirServiceProvider(productName, customProfilePath);

One thing we talked about was having a ctor with a bool (aIsCustomProfile) and a single string that would carry either the product name or profile path.  This also lends itself to addressing your concern about using mProfilePath.IsEmpty() as the switch.

Comment 37

[Håkan Waara]

(In reply to comment #35)
> >+  // We init the profile directory lazily, since the component manager
> >+  // is not around when this class is first instantiated.
> >+  if (!mProfileDir)
> >+  {
> >+    mProfileDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> 
> So if this can be called before XPCOM is initialized, how can you expect
> CreateInstance to work? And, anyway, why not just use NS_NewLocalFile here?

Note that the appdirserviceprovider-object is created before startup of xpcom, but the actual calls to get the profile directory are post-xpcom startup.

Working on a new patch with all other comments. Regarding the ctor-switch-thing; we found no real good solution for that. Either it was either-or, or the bool, or setters (which is probably the worst).  So I'll go ahead and make the bool-version for the new patch.

Comment 38

[Håkan Waara]

I can't find a sensible way to not let SiteIconCache depend on the prefsmanager. 

SiteIconProvider owns SiteIconCache, and in itself also seems kinda independent -- would it be okay to let that ask the prefsmanager for the cache dir path?

Comment 39

[Håkan Waara]

Attachment: Patch v6

Here's a patch with all comments incorporated sans the site-icon-cache change.

Comment 40

[Håkan Waara]

Comment on attachment 214564 [details] [diff] [review]
Patch v6

No idea when 1.1 is planned, but I'd like to see this land on the trunk soon, in order to get as much testing as possible before that.

Comment 41

[Mark Mentovai]

This blocks bug 326668, which in turn blocks building nightlies straight out of tinderbox and moving away from the old nightly build scripts.

Comment 42

[Mark Mentovai]

Simon, are you OK with SiteIconCache fetching the cache dir path by calling in to PreferenceManager?  The SiteIconCache path is hardcoded right now anyway.

I'd like to get this bad boy moving so I can get the new builds in shape - 326668 blocks that, and I'll need to re-touch this code in 326668.  I don't want to cause bitrot if it can be avoided.

Comment 43

[Simon Fraser [no longer active]]

(In reply to comment #42)
> Simon, are you OK with SiteIconCache fetching the cache dir path by calling in
> to PreferenceManager?  The SiteIconCache path is hardcoded right now anyway.S

Sure, I guess.

Comment 44

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 214564 [details] [diff] [review]
Patch v6

+ *  HŒkan Waara <[e-mail]>

we should probably keep high-byte characters out of CVS files as they won't be correct on other platforms.

-  // previous versions would keep the cache in the profile folder. If we find it there, remove it so
-  // that backup apps can more easily back up our profile. This will mean if anyone goes back to 
-  // 0.8.x, they'll lose their favicons and cache, but that's ok.

we're ok removing this code? 

sr=pink

Comment 45

[Håkan Waara]

This is finally checked in on trunk and branch.

Special thanks to Mark for all the help!

Comment 46

[Håkan Waara]

With Mark's help, I did a last-minute change in AppDirServiceProvider.mm:

From:

return mProfileDir->Clone(getter_AddRefs(profileDir));

to:

nsCOMPtr<nsIFile> profileDir;
rv = mProfileDir->Clone(getter_AddRefs(profileDir));
NS_ENSURE_SUCCESS(rv, rv);
  
return CallQueryInterface(profileDir, outFolder);

This is because Clone() returns a nsIFile, not nsILocalFile, and Bad Things may happen if you just ignore this.

Comment 47

[Karlheinz Dobler]

How can third-party apps or scripts recognize what's the current profile?
For example you've opened 3 Caminos: C1 has the default profile, C2 gets its profile from the variable and C3 gets the profile from a changed variable. Now you quit C3. How can you get the profile of C2? After quitting C2 there still exists the env variable but C1 doesn't use it at all.
Do I miss something essential?

Comment 48

[Mark Mentovai]

The default profile location hasn't moved.  What's your "third-party app or script" trying to do?

Comment 49

[Håkan Waara]

(In reply to comment #47)
> How can third-party apps or scripts recognize what's the current profile?
> For example you've opened 3 Caminos: C1 has the default profile, C2 gets its
> profile from the variable and C3 gets the profile from a changed variable. Now
> you quit C3. How can you get the profile of C2? After quitting C2 there still
> exists the env variable but C1 doesn't use it at all.
> Do I miss something essential?

I'm not sure what you mean here.

If this is your setup:

C1: runs like default (Application Support-folder)
C2: runs from a shell/script that sets CAMINO_PROFILE_DIR to something
C3: runs frmo another script/shell that sets CAMINO_PROFILE_DIR to something else

Well, then that is all there is to it.
Camino will look on startup if the env var is set, and if it is, use that instead of the default.

Comment 50

[Karlheinz Dobler]

(In reply to comment #48)
> The default profile location hasn't moved.  What's your "third-party app or
> script" trying to do?
Examples: 
- install CamiTools in the adequate profile
- do a backup of the profile
- download the bookmarks from a server to the profile
- etc.

Comment 51

[Karlheinz Dobler]

> Well, then that is all there is to it.
> Camino will look on startup if the env var is set, and if it is, use that
> instead of the default.
I'm sure Camino does know its profile path but how can a different app know what's the profile path?

Comment 52

[Håkan Waara]

(In reply to comment #50)
> (In reply to comment #48)
> > The default profile location hasn't moved.  What's your "third-party app or
> > script" trying to do?
> Examples: 
> - install CamiTools in the adequate profile
> - do a backup of the profile
> - download the bookmarks from a server to the profile
> - etc.

All third parties should continue to assume that the default profile is always in the Application Support folder.

Other profiles are special-cased; you may not be supposed to know that they exist.  

Custom profiles might be used for all kinds of purposes, for example for having a special profile for testing trunk builds, while keeping the default stable.

Comment 53

[Mark Mentovai]

The environment variable is considered a power-user feature.  We used to have a different way of using alternate profiles, and if your users never cared about that, they probably won't care about this either.  At the very most, it would be sufficient for your app to have a nondescript "choose alternate profile" button.

Comment 54

[[e-mail]]

Verified on 1.8 branch and trunk