More cleanup while working on Vista integration though this is not Vista specific. Basically, we should use the full path to the exe instead of the 8dot3 path. We are in the 21st century... right? ;) See Bug 303599 for an example as to why we should do this (e.g. NtfsDisable8dot3NameCreation set to 1)

I'm going to want this for Vista integration

Sorry about that... some Vista related stuff snuck in

Comment on attachment 240359 [details] [diff] [review] patch - use long paths rev3 do you need leafName? + nsCAutoString leafName; + lf->GetNativeLeafName(leafName); + nsCAutoString exeName(leafName.get()); + ToUpperCase(exeName);

Yes, it is used for Firefox's StartMenuInternet registry key path. HKEY_LOCAL_MACHINE\SOFTWARE\Clients\StartMenuInternet\FIREFOX.EXE

(In reply to comment #5) > (From update of attachment 240359 [details] [diff] [review] [edit]) > do you need leafName? > > + nsCAutoString leafName; > + lf->GetNativeLeafName(leafName); > + nsCAutoString exeName(leafName.get()); > + ToUpperCase(exeName); My bad... I read that as do we need the value vs. can this be simplified. I've changed it locally to nsCAutoString exeName; lf->GetNativeLeafName(exeName); ToUpperCase(exeName);

Why do we need to use the long path? I'm feeling anxious about changing this unless we really need to. FWIW, nsILocalFile->Equals contains logic to compare long/shortnames correctly, though I'm not sure you want to make the roundtrip through nsILocalFile.

I got into the habit of just using long paths in the registry years ago and though many apps don't it is what the majority of my app's use. I see it as we can continue adding 8.3 paths via code which will be required in the installer if this bug isn't fixed or we can start using long paths. One reason to use long paths is if the files on the system are restored there are no actions to restore the 8.3 names of the restored files and directories. There may also be extra overhead in the OS shell when having to take a short path and convert it to a long path especially since it is possible to disable 8.3 name generation. Do we have to use long paths? Probably not today though possibly in the future. What specific concerns do you have about using long paths?

How about this just landing on trunk since we only support Win2k and above on trunk? My only concerns with this are the unknowns with Win9x and there are other scenarios besides restoring files from a backup where short paths break (e.g. copying files / directories, having NtfsDisable8dot3NameCreation set to 1 with short paths in the reg and adding / removing similar directories like Mozilla XXX).

Comment on attachment 240359 [details] [diff] [review] patch - use long paths rev3 r=me for trunk

Comment on attachment 240774 [details] [diff] [review] patch rev4 - ignore case and fix nit Doug, I moved the quoting of the path to the exe to where they are actually needed instead of appending the quotes. This makes it so when specifying a default icon or the exe without arguments quotes aren't added which is how all other apps do this.

Comment on attachment 240774 [details] [diff] [review] patch rev4 - ignore case and fix nit >- if (SetDefaultBrowserVista()) >+ if (!aForAllUsers && SetDefaultBrowserVista()) > return NS_OK; btw: the SetDefaultBrowserVista is only for current user. It is safe to add this in this patch but if you prefer I can move this change to the patch I am finishing up for setting the app as OS default.

Comment on attachment 240774 [details] [diff] [review] patch rev4 - ignore case and fix nit I like this way of quoting better. This might be the wrong bug, but what do you think of dropping aForAllUsers in the API? With this code change, doing something like: firefox -setDefaultBrowser not work on vista. See: http://lxr.mozilla.org/seamonkey/source/browser/components/shell/src/nsSetDefaultBrowser.js#63

I'm leaning towards removing -setDefaultBrowser but I'm leaving it for now since I haven't convinced myself yet that we want to do this.

The removal of -setDefaultBrowser would most likely happen in Bug 354005. Also, I'm going to modify the installer / uninstaller for Vista support to use short paths for 2.0 and long paths for 3.0 so this isn't going to be needed for when we add Vista support in 2.0.x

Comment on attachment 240774 [details] [diff] [review] patch rev4 - ignore case and fix nit r=me.

Comment on attachment 240774 [details] [diff] [review] patch rev4 - ignore case and fix nit I found a bit more logic I want to address in this patch for the trunk.

Comment on attachment 241645 [details] [diff] [review] Fixes the shortpath check for VAL_OPEN Doug, I changed the patch slightly so it properly handles shortpath comparisons for VAL_OPEN. The change from the previous patch is as follows + // Remove the quotes around %APPPATH% in VAL_OPEN for short paths + PRInt32 offsetQuoted = dataShortPath.Find("\"%APPPATH%\""); + if (offsetQuoted != -1) + dataShortPath.Replace(offsetQuoted, 11, appShortPath); + else + dataShortPath.Replace(offset, 9, appShortPath);

Comment on attachment 241645 [details] [diff] [review] Fixes the shortpath check for VAL_OPEN if that is the only change. also, do you want to also change the quoting for VAL_FILE_ICON and VAL_URL_ICON so that it is consistent with VAL_OPEN? (ooc, is mscott going to to do something similar to this for mail?)

(In reply to comment #22) > (From update of attachment 241645 [details] [diff] [review] [edit]) > if that is the only change. It is... without it the short path check would fail. > also, do you want to also change the quoting for VAL_FILE_ICON and VAL_URL_ICON > so that it is consistent with VAL_OPEN? See comment #13... the paths to the exe don't need to be quoted unless they are followed by arguments for opening the exe. > (ooc, is mscott going to to do something similar to this for mail?) I hope so... I'll run it by him at some point.

Checked in to trunk

Can this patch land on the branch to mitigate bug 371649?