[shell_util] Fall Back to Long Paths if Short Is Not Available

chrome/installer/util/shell_util.cc

Index: chrome/installer/util/shell_util.cc
diff --git a/chrome/installer/util/shell_util.cc b/chrome/installer/util/shell_util.cc
index a753f802a160ddf145e294d92f9ec2bf4aed136b..37ecbcfcd38c06cc615ba9987796ddf8d0d2670a 100644
--- a/chrome/installer/util/shell_util.cc
+++ b/chrome/installer/util/shell_util.cc
@@ -1307,10 +1307,10 @@ ShellUtil::DefaultState ProbeOpenCommandHandlers(
     const base::FilePath& chrome_exe,
     const wchar_t* const* protocols,
     size_t num_protocols) {
-  // Get its short (8.3) form.
-  base::string16 short_app_path;
-  if (!ShortNameFromPath(chrome_exe, &short_app_path))
-    return ShellUtil::UNKNOWN_DEFAULT;
+  // Get its short (8.3) form if possible.
+  base::string16 app_path;
+  if (!ShortNameFromPath(chrome_exe, &app_path))
+    app_path = chrome_exe.value();

[gab, 2015/10/15 14:21:43]

Value is only used inside if{} below, move it inline.

[Mark P, 2015/10/15 16:27:48]

I don't know what you mean.  It's used in two spots below.  And I don't see how
to inline app_path in any case unless you want me to do something awkward like 
  ShortNameFromPath(chrome_exe, &app_path) ? app_path : chrome_exe.value()

That said, this point may be moot if grt@ agrees with you.

[gab, 2015/10/15 18:33:07]

What I mean is you only need |short_app_path| in the if{} in the else{} you can
use |chrome_exe| directly.
Right.

[Mark P, 2015/10/15 19:17:42]

I don't understand.  Do you mean that new line 1338 should use
chrome_exe.value() instead of app_path?  If so, I don't necessarily agree; I
think it's possible that one ShortNameFromPath() succeeds and the other does
not.

[gab, 2015/10/16 19:02:17]

I mean you definitely don't need it if you're not using the short_path for the
command_line below. And if you are, sure getting chrome_exe's short_path can
also fail, but you can handle there (i.e. in previous code |short_app_path| is
not used until end of method -- only reason for previously having this code at
the top was the early return).

[Mark P, 2015/10/16 23:02:44]

Ah, now I understand.

 
   const HKEY root_key = HKEY_CLASSES_ROOT;
   base::string16 key_path;
@@ -1330,11 +1330,14 @@ ShellUtil::DefaultState ProbeOpenCommandHandlers(
 
     // Need to normalize path in case it's been munged.
     command_line = base::CommandLine::FromString(value);
-    if (!ShortNameFromPath(command_line.GetProgram(), &short_path))
-      return ShellUtil::UNKNOWN_DEFAULT;
-
-    if (!base::FilePath::CompareEqualIgnoreCase(short_path, short_app_path))
-      return ShellUtil::NOT_DEFAULT;
+    if (ShortNameFromPath(command_line.GetProgram(), &short_path)) {
+      if (!base::FilePath::CompareEqualIgnoreCase(short_path, app_path))

[gab, 2015/10/15 14:21:43]

Actually, @grt: shouldn't this entire method use ProgramCompare instead and stop
being bothered by doing strcmp on short/long paths?

[grt (UTC plus 2), 2015/10/19 19:30:39]

Might be a good idea, although the condition leading to the benign error message
on the bot will still be there: GetShortPathName and ProgramCompare both work
best on files that exist/are accessible. The errors on the bot indicate that a
portion of the path passed in to ShortNameFromPath are inaccessible/don't exist.
This should be investigated, as it doesn't make sense. Is there a permissions
problem on the bot?

+        return ShellUtil::NOT_DEFAULT;
+    } else {
+      if (!base::FilePath::CompareEqualIgnoreCase(
+              command_line.GetProgram().value(), app_path))
+        return ShellUtil::NOT_DEFAULT;
+    }
   }
 
   return ShellUtil::IS_DEFAULT;