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

chrome/installer/util/shell_util.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // This file defines functions that integrate Chrome in Windows shell. These
 // functions can be used by Chrome as well as Chrome installer. All of the
 // work is done by the local functions defined in anonymous namespace in
 // this class.
 
 #include "chrome/installer/util/shell_util.h"
@@ skipping 1289 matching lines @@
 
   return ShellUtil::IS_DEFAULT;
 }
 
 // Probe the current commands registered to handle the shell "open" verb for
 // |protocols| (Windows XP); see ProbeProtocolHandlers.
 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;
   base::win::RegKey key;
   base::string16 value;
   base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
   base::string16 short_path;
 
   for (size_t i = 0; i < num_protocols; ++i) {
     // Get the command line from HKCU\<protocol>\shell\open\command.
     key_path.assign(protocols[i]).append(ShellUtil::kRegShellOpen);
     if ((key.Open(root_key, key_path.c_str(),
                   KEY_QUERY_VALUE) != ERROR_SUCCESS) ||
         (key.ReadValue(L"", &value) != ERROR_SUCCESS)) {
       return ShellUtil::NOT_DEFAULT;
     }
 
     // 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;
 }
 
 // A helper function that probes default protocol handler registration (in a
 // manner appropriate for the current version of Windows) to determine if
 // Chrome is the default handler for |protocols|.  Returns IS_DEFAULT
 // only if Chrome is the default for all specified protocols.
 ShellUtil::DefaultState ProbeProtocolHandlers(
@@ skipping 1217 matching lines @@
   base::string16 key_path(ShellUtil::kRegClasses);
   key_path.push_back(base::FilePath::kSeparators[0]);
   key_path.append(prog_id);
   return InstallUtil::DeleteRegistryKey(
       HKEY_CURRENT_USER, key_path, WorkItem::kWow64Default);
 
   // TODO(mgiuca): Remove the extension association entries. This requires that
   // the extensions associated with a particular prog_id are stored in that
   // prog_id's key.
 }