If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Use long paths in registry when setting default browser

RESOLVED FIXED

Status

()

Firefox
Shell Integration
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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
Blocks: 352420
Created attachment 240357 [details] [diff] [review]
patty patch patch
Created attachment 240358 [details] [diff] [review]
patch - use full paths
Attachment #240357 - Attachment is obsolete: true
Attachment #240358 - Flags: superreview?(benjamin)
Attachment #240358 - Flags: review?(dougt)
Created attachment 240359 [details] [diff] [review]
patch - use long paths rev3

Sorry about that... some Vista related stuff snuck in
Attachment #240358 - Attachment is obsolete: true
Attachment #240359 - Flags: superreview?(benjamin)
Attachment #240359 - Flags: review?(dougt)
Attachment #240358 - Flags: superreview?(benjamin)
Attachment #240358 - Flags: review?(dougt)

Comment 5

11 years ago
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);
Attachment #240359 - Flags: review?(dougt) → review+
Yes, it is used for Firefox's StartMenuInternet registry key path.
HKEY_LOCAL_MACHINE\SOFTWARE\Clients\StartMenuInternet\FIREFOX.EXE
Summary: Use full paths in registry when setting default browser → Use long paths in registry when setting default browser
(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

11 years ago
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 11

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

r=me for trunk
Attachment #240359 - Flags: superreview?(benjamin) → superreview+
Created attachment 240774 [details] [diff] [review]
patch rev4 - ignore case and fix nit
Attachment #240359 - Attachment is obsolete: true
Attachment #240774 - Flags: review?(dougt)
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 15

11 years ago
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
No longer blocks: 352420

Comment 18

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

r=me.
Attachment #240774 - Flags: review?(dougt) → review+
Version: 2.0 Branch → Trunk
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.
Attachment #240774 - Attachment is obsolete: true
Created attachment 241645 [details] [diff] [review]
Fixes the shortpath check for VAL_OPEN
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);
Attachment #241645 - Flags: review?(dougt)

Comment 22

11 years ago
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?)
Attachment #241645 - Flags: review?(dougt) → review+
(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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Can this patch land on the branch to mitigate bug 371649?
You need to log in before you can comment on or make changes to this bug.