354555 - Use long paths in registry when setting default browser

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Robert Strong (they/them - no direct email)]

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)

Comment 1

[Robert Strong (they/them - no direct email)]

I'm going to want this for Vista integration

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patty patch patch

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: patch - use full paths

Comment 4

[Robert Strong (they/them - no direct email)]

Attachment: patch - use long paths rev3

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

Comment 5

[Doug Turner (:dougt)]

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);

Comment 6

[Robert Strong (they/them - no direct email)]

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

Comment 7

[Robert Strong (they/them - no direct email)]

(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);

Comment 8

[Benjamin Smedberg]

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.

Comment 9

[Robert Strong (they/them - no direct email)]

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?

Comment 10

[Robert Strong (they/them - no direct email)]

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 11

[Benjamin Smedberg]

Comment on attachment 240359 [details] [diff] [review]
patch - use long paths rev3

r=me for trunk

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: patch rev4 - ignore case and fix nit

Comment 13

[Robert Strong (they/them - no direct email)]

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 14

[Robert Strong (they/them - no direct email)]

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 15

[Doug Turner (:dougt)]

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

Comment 16

[Robert Strong (they/them - no direct email)]

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.

Comment 17

[Robert Strong (they/them - no direct email)]

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 18

[Doug Turner (:dougt)]

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

r=me.

Comment 19

[Robert Strong (they/them - no direct email)]

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 20

[Robert Strong (they/them - no direct email)]

Attachment: Fixes the shortpath check for VAL_OPEN

Comment 21

[Robert Strong (they/them - no direct email)]

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 22

[Doug Turner (:dougt)]

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?)

Comment 23

[Robert Strong (they/them - no direct email)]

(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.

Comment 24

[Robert Strong (they/them - no direct email)]

Checked in to trunk

Comment 25

[Masatoshi Kimura [:emk]]

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