Refactoring installer shortcut deletion; adding dedicated shortcut update feature.

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"
 
 #include <windows.h>
 #include <shlobj.h>
 
 #include <limits>
 #include <string>
 
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/md5.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/path_service.h"
 #include "base/string16.h"
@@ skipping 1124 matching lines @@
   const base::win::Version windows_version = base::win::GetVersion();
 
   if (windows_version >= base::win::VERSION_WIN8)
     return ProbeCurrentDefaultHandlers(protocols, num_protocols);
   else if (windows_version >= base::win::VERSION_VISTA)
     return ProbeAppIsDefaultHandlers(protocols, num_protocols);
 
   return ProbeOpenCommandHandlers(protocols, num_protocols);
 }
 
-// Removes shortcut at |shortcut_path| if it is a shortcut that points to
-// |target_exe|. If |delete_folder| is true, deletes the parent folder of
-// the shortcut completely. Returns true if either the shortcut was deleted
-// successfully or if the shortcut did not point to |target_exe|.
-bool MaybeRemoveShortcutAtPath(const base::FilePath& shortcut_path,
-                               const base::FilePath& target_exe,
-                               bool delete_folder) {
-  base::FilePath target_path;
-  if (!base::win::ResolveShortcut(shortcut_path, &target_path, NULL))
+// (Windows 8+) Finds the folder for app shortcuts, store it in *|path|.
+// Returns true on success.
+bool GetAppShortcutsFolder(BrowserDistribution* dist,
+                                     bool is_user_level,

[grt (UTC plus 2), 2013/04/30 14:38:07]

indentation

[grt (UTC plus 2), 2013/04/30 14:38:07]

bool -> ShellChange for consistency

[huangs, 2013/04/30 15:58:38]

Done.

[huangs, 2013/04/30 15:58:38]

Done.

+                                     base::FilePath *path) {
+  DCHECK(path);
+  if (base::win::GetVersion() < base::win::VERSION_WIN8)
     return false;
 
-  if (InstallUtil::ProgramCompare(target_exe).EvaluatePath(target_path)) {
-    // Unpin the shortcut if it was ever pinned by the user or the installer.
-    VLOG(1) << "Trying to unpin " << shortcut_path.value();
-    if (!base::win::TaskbarUnpinShortcutLink(shortcut_path.value().c_str())) {
-      VLOG(1) << shortcut_path.value()
-              << " wasn't pinned (or the unpin failed).";
-    }
-    if (delete_folder)
-      return file_util::Delete(shortcut_path.DirName(), true);
-    else
-      return file_util::Delete(shortcut_path, false);
+  base::FilePath folder;
+  if (!PathService::Get(base::DIR_APP_SHORTCUTS, &folder)) {
+    LOG(ERROR) << "Could not get application shortcuts location.";
+    return false;
   }
 
-  // The shortcut at |shortcut_path| doesn't point to |target_exe|, act as if
-  // our shortcut had been deleted.
+  folder = folder.Append(ShellUtil::GetBrowserModelId(dist, is_user_level));
+  if (!file_util::DirectoryExists(folder)) {
+    VLOG(1) << "No start screen shortcuts.";
+    return false;
+  }
+
+  *path = folder;
   return true;
 }
 
+const wchar_t kAllShortcuts[] = L"*.lnk";
+
+typedef base::Callback<bool(const base::FilePath&)> FileOperationCallback;
+
+FileOperationCallback kNullFileOperation;

[grt (UTC plus 2), 2013/04/30 14:38:07]

remove this (globals are banned).

[huangs, 2013/04/30 15:58:38]

Done. Replacing uses with FileOperationCallback().

+
+// Shortcut operations for BatchShortcutAction().
+
+bool ShortcutOpUnpin(const base::FilePath& shortcut_path) {
+  VLOG(1) << "Trying to unpin " << shortcut_path.value();
+  if (!base::win::TaskbarUnpinShortcutLink(shortcut_path.value().c_str())) {
+    VLOG(1) << shortcut_path.value() << " wasn't pinned (or the unpin failed).";
+    // No error, since shortcut might not be pinned.
+  }
+  return true;
+}
+
+bool ShortcutOpDelete(const base::FilePath& shortcut_path) {
+  return file_util::Delete(shortcut_path, false);
+}
+
+bool ShortcutOpUnpinAndDelete(const base::FilePath& shortcut_path) {
+  // Want to try both, without short-circuiting.
+  bool ret1 = ShortcutOpUnpin(shortcut_path);
+  bool ret2 = ShortcutOpDelete(shortcut_path);
+  return ret1 && ret2;
+}
+
+bool ShortcutOpSetTarget(const base::FilePath& target_path,
+                         const base::FilePath& shortcut_path) {
+  base::win::ShortcutProperties shortcut_properties;
+  shortcut_properties.set_target(target_path);
+  shortcut_properties.set_working_dir(target_path.DirName());
+  return base::win::CreateOrUpdateShortcutLink(
+      shortcut_path, shortcut_properties, base::win::SHORTCUT_REPLACE_EXISTING);
+}
+
+// Folder operations for BatchShortcutAction().
+
+bool FolderOpDelete(const base::FilePath& folder_path) {
+  return file_util::Delete(folder_path, true);
+}
+
+// {|location|, |dist|, |level|} determine |shortcut_folder| to operate in.
+// In |shortcut_folder|, iterate over shortcuts with names matching
+// |name_filter| and targets pointing to |target_exe|.
+// Applies |shortcut_operation| on each matching shortcuts and then applies
+// |folder_operation| on |shortcut_folder|.
+// If |name_filter| is empty, then ignores |shortcut_operation|; otherwise
+// iterate with implicitly ".lnk" suffix. |kAllShortcuts| is used to specify
+// all shortcuts.
+// Returns true iff all operations are successful.
+bool BatchShortcutAction(const FileOperationCallback& shortcut_operation,
+                         const FileOperationCallback& folder_operation,

[grt (UTC plus 2), 2013/04/30 14:38:07]

folder_operation seems overly abstract. it's only either null or delete, so how
about a delete_folder boolean? since it does a recursive delete, is there any
point in operating on any shortcuts in the folder since they will just be
deleted anyway?

[huangs, 2013/04/30 15:58:38]

Decided to extract RemoveShortcutFolder(). Before it was munged here to reuse
shortcut_folder. But the GetShortcutPath() cleanup makes it less expensive to
duplicate code to get |shortcut_folder|.

+                         ShellUtil::ShortcutLocation location,
+                         BrowserDistribution* dist,
+                         ShellUtil::ShellChange level,
+                         const base::FilePath& target_exe,
+                         const string16& name_filter) {

[grt (UTC plus 2), 2013/04/30 14:38:07]

simplify by removing name_filter and kAllShortcuts and always use * + kLnkExt as
the enumerator pattern.

[huangs, 2013/04/30 15:58:38]

Done.

+  base::FilePath shortcut_folder;
+  if (!ShellUtil::GetShortcutPath(location, dist, level, &shortcut_folder) ||
+      !file_util::PathExists(shortcut_folder)) {

[gab, 2013/04/30 19:20:33]

Why did you remove this PathExists() check? This is why I had suggested making
it a WARNING (i.e. the folder may simply not exist). GetShortcutPath() itself
should never fail.

The Create methods you might have taken this from are different in that they do
not need to verify that the folder exists because they will create it if it
doesn't (or silently fail in the case of an update).

In your case you are reading/deleting something and should make sure it exists
first imo (unless you can rely on FileEnumerator to successfully generate an
empty list if |shortcut_folder| doesn't exist?).

(Either way GetShortcutPath() failing by itself is a NOTREACHED() case).

+    LOG(WARNING) << "Cannot find path at location " << location;
+    return false;
+  }
+
+  // Operate on specified shortcuts in the folder.
+  InstallUtil::ProgramCompare target_compare(target_exe);
+  bool success = true;
+  if (!shortcut_operation.is_null() && !name_filter.empty()) {
+    // Ensure ".lnk" extension.
+    string16 pattern = name_filter;
+    if (!EndsWith (pattern, installer::kLnkExt, false))

[grt (UTC plus 2), 2013/04/30 14:38:07]

remove space before open paren

[huangs, 2013/04/30 15:58:38]

Moot; deleted code.

+      pattern.append(installer::kLnkExt);
+    file_util::FileEnumerator enumerator(
+        shortcut_folder, false, file_util::FileEnumerator::FILES, pattern);
+    for (base::FilePath shortcut_path = enumerator.Next();
+         !shortcut_path.empty();
+         shortcut_path = enumerator.Next()) {
+      base::FilePath target_path;
+      if (base::win::ResolveShortcut(shortcut_path, &target_path, NULL)) {
+        if (target_compare.EvaluatePath(target_path) &&
+            !shortcut_operation.Run(shortcut_path)) {
+          LOG(ERROR) << "Failed shortcut operation on "

[grt (UTC plus 2), 2013/04/30 14:38:07]

suggestion: move the logging into the operations so that it can be more
meaningful.

[huangs, 2013/04/30 15:58:38]

Done.

+                     << shortcut_path.value();
+          success = false;
+        }
+      } else {
+        LOG(ERROR) << "Cannot resolve shortcut at " << shortcut_path.value();
+        success = false;
+      }
+    }
+  }
+
+  // Operate on shortcut folder itself.
+  if (!folder_operation.is_null() && !folder_operation.Run(shortcut_folder)) {

[grt (UTC plus 2), 2013/04/30 14:38:07]

document that folder_operation (or delete_folder boolean if you follow my advice
above) takes place regardless of whether shortcut_operation succeeds or fails.
(and be sure that this is the right thing to do!)

[huangs, 2013/04/30 15:58:38]

Moved to RemoveShortcutFolder(). Right thing to do or not, it was the previous
thing -- and I'm just refactoring!

Added comment on BatchShortcutAction() to say that operations are performed even
if failures occur.

+    LOG(ERROR) << "Failed folder operation on " << shortcut_folder.value();
+    success = false;
+  }
+  return success;
+}
+
 }  // namespace
 
 const wchar_t* ShellUtil::kRegDefaultIcon = L"\\DefaultIcon";
 const wchar_t* ShellUtil::kRegShellPath = L"\\shell";
 const wchar_t* ShellUtil::kRegShellOpen = L"\\shell\\open\\command";
 const wchar_t* ShellUtil::kRegStartMenuInternet =
     L"Software\\Clients\\StartMenuInternet";
 const wchar_t* ShellUtil::kRegClasses = L"Software\\Classes";
 const wchar_t* ShellUtil::kRegRegisteredApplications =
     L"Software\\RegisteredApplications";
@@ skipping 46 matching lines @@
                                               const string16& chrome_exe,
                                               const string16& suffix) {
   return QuickIsChromeRegistered(dist, chrome_exe, suffix,
                                  CONFIRM_SHELL_REGISTRATION_IN_HKLM);
 }
 
 bool ShellUtil::GetShortcutPath(ShellUtil::ShortcutLocation location,
                                 BrowserDistribution* dist,
                                 ShellChange level,
                                 base::FilePath* path) {
+  DCHECK(path);
   int dir_key = -1;
   bool add_folder_for_dist = false;
   switch (location) {
     case SHORTCUT_LOCATION_DESKTOP:
       dir_key = (level == CURRENT_USER) ? base::DIR_USER_DESKTOP :
                                           base::DIR_COMMON_DESKTOP;
       break;
     case SHORTCUT_LOCATION_QUICK_LAUNCH:
       dir_key = (level == CURRENT_USER) ? base::DIR_USER_QUICK_LAUNCH :
                                           base::DIR_DEFAULT_USER_QUICK_LAUNCH;
       break;
     case SHORTCUT_LOCATION_START_MENU:
       dir_key = (level == CURRENT_USER) ? base::DIR_START_MENU :
                                           base::DIR_COMMON_START_MENU;
       add_folder_for_dist = true;
       break;
+    case SHORTCUT_LOCATION_TASKBAR_PINS:
+      if (base::win::GetVersion() < base::win::VERSION_WIN7)
+        return false;
+
+      dir_key = base::DIR_TASKBAR_PINS;
+      break;
+    case SHORTCUT_LOCATION_APP_SHORTCUTS:
+      return GetAppShortcutsFolder(dist, level == CURRENT_USER, path);
+
     default:
       NOTREACHED();
       return false;
   }
 
   if (!PathService::Get(dir_key, path) || path->empty()) {
     NOTREACHED() << dir_key;
     return false;
   }
 
   if (add_folder_for_dist)
     *path = path->Append(dist->GetAppShortCutName());
 
   return true;
 }
 
 bool ShellUtil::CreateOrUpdateShortcut(
     ShellUtil::ShortcutLocation location,
     BrowserDistribution* dist,
     const ShellUtil::ShortcutProperties& properties,
     ShellUtil::ShortcutOperation operation) {
   DCHECK(dist);
   // |pin_to_taskbar| is only acknowledged when first creating the shortcut.
   DCHECK(!properties.pin_to_taskbar ||
          operation == SHELL_SHORTCUT_CREATE_ALWAYS ||
          operation == SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL);
 
   base::FilePath user_shortcut_path;
   base::FilePath system_shortcut_path;
-  if (!GetShortcutPath(location, dist, SYSTEM_LEVEL, &system_shortcut_path) ||
-      system_shortcut_path.empty()) {

[grt (UTC plus 2), 2013/04/30 14:38:07]

why remove this? it seems like a reasonable safeguard.

[huangs, 2013/04/30 15:58:38]

GetShortcutPath() already checks for this.

Removed instances in BatchShortcutAction() as well -- but will put it back if
you insist!

+  if (!GetShortcutPath(location, dist, SYSTEM_LEVEL, &system_shortcut_path)) {
     NOTREACHED();
     return false;
   }
 
   string16 shortcut_name(ExtractShortcutNameFromProperties(dist, properties));
   system_shortcut_path = system_shortcut_path.Append(shortcut_name);
 
   base::FilePath* chosen_path;
   bool should_install_shortcut = true;
   if (properties.level == SYSTEM_LEVEL) {
     // Install the system-level shortcut if requested.
     chosen_path = &system_shortcut_path;
   } else if (operation != SHELL_SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL ||
              !file_util::PathExists(system_shortcut_path)) {
     // Otherwise install the user-level shortcut, unless the system-level
     // variant of this shortcut is present on the machine and |operation| states
     // not to create a user-level shortcut in that case.
-    if (!GetShortcutPath(location, dist, CURRENT_USER, &user_shortcut_path) ||
-        user_shortcut_path.empty()) {
+    if (!GetShortcutPath(location, dist, CURRENT_USER, &user_shortcut_path)) {
       NOTREACHED();
       return false;
     }
     user_shortcut_path = user_shortcut_path.Append(shortcut_name);
     chosen_path = &user_shortcut_path;
   } else {
     // Do not install any shortcut if we are told to install a user-level
     // shortcut, but the system-level variant of that shortcut is present.
     // Other actions (e.g., pinning) can still happen with respect to the
     // existing system-level shortcut however.
@@ skipping 518 matching lines @@
   } else if (elevate_if_not_admin &&
              base::win::GetVersion() >= base::win::VERSION_VISTA) {
     // Elevate to do the whole job
     return ElevateAndRegisterChrome(dist, chrome_exe, suffix, protocol);
   } else {
     // Admin rights are required to register capabilities before Windows 8.
     return false;
   }
 }
 
+// static
 bool ShellUtil::RemoveShortcut(ShellUtil::ShortcutLocation location,
                                BrowserDistribution* dist,
-                               const base::FilePath& target_exe,
                                ShellChange level,
-                               const string16* shortcut_name) {
-  const bool delete_folder = (location == SHORTCUT_LOCATION_START_MENU);
-
-  base::FilePath shortcut_folder;
-  if (!GetShortcutPath(location, dist, level, &shortcut_folder) ||
-      shortcut_folder.empty()) {
-    NOTREACHED();
-    return false;
-  }
-
-  if (!delete_folder && !shortcut_name) {
-    file_util::FileEnumerator enumerator(shortcut_folder, false,
-        file_util::FileEnumerator::FILES);
-    bool had_failures = false;
-    for (base::FilePath path = enumerator.Next(); !path.empty();
-         path = enumerator.Next()) {
-      if (path.Extension() != installer::kLnkExt)
-        continue;
-
-      if (!MaybeRemoveShortcutAtPath(path, target_exe, delete_folder))
-        had_failures = true;
-    }
-    return !had_failures;
-  }
-
-  const string16 shortcut_base_name(
-      (shortcut_name ? *shortcut_name : dist->GetAppShortCutName()) +
-      installer::kLnkExt);
-  const base::FilePath shortcut_path(
-      shortcut_folder.Append(shortcut_base_name));
-  if (!file_util::PathExists(shortcut_path))
-    return true;
-
-  return MaybeRemoveShortcutAtPath(shortcut_path, target_exe, delete_folder);
+                               const base::FilePath& target_exe) {
+  bool delete_file = (location != SHORTCUT_LOCATION_TASKBAR_PINS);
+  bool delete_folder = (location == SHORTCUT_LOCATION_START_MENU);
+  return BatchShortcutAction(
+      base::Bind(delete_file ? &ShortcutOpUnpinAndDelete : &ShortcutOpUnpin),
+      delete_folder ? base::Bind(&FolderOpDelete) : kNullFileOperation,
+      location, dist, level, target_exe, kAllShortcuts);
 }
 
-void ShellUtil::RemoveTaskbarShortcuts(const string16& target_exe) {
-  if (base::win::GetVersion() < base::win::VERSION_WIN7)
+// static
+void ShellUtil::RemoveStartScreenShortcuts(BrowserDistribution* dist,
+                                           const base::FilePath& target_exe) {
+  if (base::win::GetVersion() < base::win::VERSION_WIN8)
     return;
 
-  base::FilePath taskbar_pins_path;
-  if (!PathService::Get(base::DIR_TASKBAR_PINS, &taskbar_pins_path) ||
-      !file_util::PathExists(taskbar_pins_path)) {
-    LOG(ERROR) << "Couldn't find path to taskbar pins.";
-    return;
-  }
-
-  file_util::FileEnumerator shortcuts_enum(
-      taskbar_pins_path, false,
-      file_util::FileEnumerator::FILES, FILE_PATH_LITERAL("*.lnk"));
-
-  base::FilePath target_path(target_exe);
-  InstallUtil::ProgramCompare target_compare(target_path);
-  for (base::FilePath shortcut_path = shortcuts_enum.Next();
-       !shortcut_path.empty();
-       shortcut_path = shortcuts_enum.Next()) {
-    base::FilePath read_target;
-    if (!base::win::ResolveShortcut(shortcut_path, &read_target, NULL)) {
-      LOG(ERROR) << "Couldn't resolve shortcut at " << shortcut_path.value();
-      continue;
-    }
-    if (target_compare.EvaluatePath(read_target)) {
-      // Unpin this shortcut if it points to |target_exe|.
-      base::win::TaskbarUnpinShortcutLink(shortcut_path.value().c_str());
-    }
+  ShortcutLocation location = SHORTCUT_LOCATION_APP_SHORTCUTS;

[grt (UTC plus 2), 2013/04/30 14:38:07]

nit: remove this local var and put the constant inline in the function call
below.

[huangs, 2013/04/30 15:58:38]

Done.

+  ShellChange level =
+      InstallUtil::IsPerUserInstall(target_exe.value().c_str()) ?
+      ShellUtil::CURRENT_USER : ShellUtil::SYSTEM_LEVEL;
+  VLOG(1) << "Removing start screen shortcuts.";
+  if (!BatchShortcutAction(kNullFileOperation,
+                           base::Bind(&FolderOpDelete),
+                           location, dist, level, base::FilePath(target_exe),

[grt (UTC plus 2), 2013/04/30 14:38:07]

base::FilePath(target_exe) -> target_exe

[huangs, 2013/04/30 15:58:38]

Ah, right. But moot: Removed code.

+                           L"")) {

[grt (UTC plus 2), 2013/04/30 14:38:07]

L"" -> string16()

[huangs, 2013/04/30 15:58:38]

Moot; removed code.

+    LOG(ERROR) << "Failed to remove start screen shortcuts.";
   }
 }
 
-void ShellUtil::RemoveStartScreenShortcuts(BrowserDistribution* dist,
-                                           const string16& target_exe) {
+// static
+bool ShellUtil::MigrateShortcut(ShellUtil::ShortcutLocation location,
+                                BrowserDistribution* dist,
+                                ShellChange level,
+                                const base::FilePath& old_target_exe,
+                                const base::FilePath& new_target_exe) {
+  return BatchShortcutAction(base::Bind(&ShortcutOpSetTarget, new_target_exe),
+                             kNullFileOperation, location, dist, level,
+                             old_target_exe, kAllShortcuts);
+}
+
+// static
+void ShellUtil::MigrateStartScreenShortcuts(
+    BrowserDistribution* dist,
+    const base::FilePath& old_target_exe,
+    const base::FilePath& new_target_exe) {
   if (base::win::GetVersion() < base::win::VERSION_WIN8)
     return;
 
-  base::FilePath app_shortcuts_path;
-  if (!PathService::Get(base::DIR_APP_SHORTCUTS, &app_shortcuts_path)) {
-    LOG(ERROR) << "Could not get application shortcuts location to delete"
-               << " start screen shortcuts.";
-    return;
+  ShortcutLocation location = SHORTCUT_LOCATION_APP_SHORTCUTS;
+  ShellChange level =
+      InstallUtil::IsPerUserInstall(old_target_exe.value().c_str()) ?
+      ShellUtil::CURRENT_USER : ShellUtil::SYSTEM_LEVEL;
+  VLOG(1) << "Migrating start screen shortcuts.";
+  if (!BatchShortcutAction(base::Bind(&ShortcutOpSetTarget, new_target_exe),
+                           kNullFileOperation, location, dist, level,
+                           old_target_exe, kAllShortcuts)) {
+    LOG(ERROR) << "Failed to migrate start screen shortcuts.";
   }
 
-  app_shortcuts_path = app_shortcuts_path.Append(
-      GetBrowserModelId(dist,
-                        InstallUtil::IsPerUserInstall(target_exe.c_str())));
-  if (!file_util::DirectoryExists(app_shortcuts_path)) {
-    VLOG(1) << "No start screen shortcuts to delete.";
-    return;
-  }
-
-  VLOG(1) << "Removing start screen shortcuts from "
-          << app_shortcuts_path.value();
-  if (!file_util::Delete(app_shortcuts_path, true)) {
-    LOG(ERROR) << "Failed to remove start screen shortcuts from "
-               << app_shortcuts_path.value();
-  }
 }
 
 bool ShellUtil::GetUserSpecificRegistrySuffix(string16* suffix) {
   // Use a thread-safe cache for the user's suffix.
   static base::LazyInstance<UserSpecificRegistrySuffix>::Leaky suffix_instance =
       LAZY_INSTANCE_INITIALIZER;
   return suffix_instance.Get().GetSuffix(suffix);
 }
 
 bool ShellUtil::GetOldUserSpecificRegistrySuffix(string16* suffix) {
@@ skipping 49 matching lines @@
     // are any left...).
     if (free_bits >= 8 && next_byte_index < size) {
       free_bits -= 8;
       bit_stream += bytes[next_byte_index++] << free_bits;
     }
   }
 
   DCHECK_EQ(ret.length(), encoded_length);
   return ret;
 }