Comment 37 for bug 1401454

Comment on attachment 445968
PATCH

>@@ -63,16 +63,17 @@
>+#include "prenv.h"

This is no longer needed, right?

>@@ -433,19 +434,67 @@

You may want to add "showfunc = true" to the [diff] section in your ~/.hgrc.

>+ PRUint32 permissions;
>+ rv = dir->GetPermissions(&permissions);
>+ if (permissions != PR_IRWXU) {

Right before that code, I suggest adding a comment like "Make sure that only the current user can read the filenames we end up creating".

You need to check rv there (and presumably assume the permissions are not good if it's a failure code).

>+ nsAutoString tpath;

You don't need this; more on that later.

>+ PRBool isPrivate = PR_FALSE, isExisting = PR_FALSE;

Move these declarations down to where you use them. Same for 'i' and 'lFile'. This isn't C. ;)

>+ nsILocalFile *lFile ;

nsCOMPtr<nsILocalFile>, please. That would help you avoid the lFile leaks you have all over here.

>+ user="mozillaUser";

Spaces around '=', please.

>+ nsCAutoString path;
>+ nsCAutoString tem;
>+ path.AssignWithConversion(tpath);

You just lost any non-ASCII chars in the temp dir name. That's bad.

>+ path += "/mozilla-";
>+ path += user;
>+ rv = NS_NewNativeLocalFile(path, PR_TRUE, &lFile);

You should probably just clone dir, create an nsAutoString with the concatenation of "/mozilla-" and user in it, then append that autostring to the clone. That'll work correctly with non-ASCII paths, etc. And use create() on the resulting nsIFile to create the file.

Also, do you need to worry about usernames containing path separators here?

>+ while (NS_SUCCEEDED(rv) && (!isExisting || !isPrivate)) {

I'd really prefer to replace this by something that just reuses createUnique.... but I guess reusing the existing dir if one is there is nice. So let's leave it as-is.

>+ rv = lFile->Exists(&isExisting);

This rv is never checked by anyone. If it doesn't matter, don't assign it; otherwise check it. I think you need to check it...

>+ rv = lFile->GetPermissions(&permissions);

And check that too.

>+ isPrivate = (permissions == PR_IRWXU)?PR_TRUE:PR_FALSE;

  isPrivate = (permissions == PR_IWXU);

No need for the ?: stuff.

>+ tem.Truncate(0);
>+ tem += path;

This is the same as |tem = path|, but again you want to be using append() on nsIFiles here....

I'm really sorry about the lag. :( This patch is looking much better, though!