Refactor ShellUtil shortcut code -- single multi-purpose methods as opposed to many slighlty diffe…

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 37ed8c214ea4ccb25235cdf4606505f92aea8a4e..9407d4638f7a6685f0a4f847e8428530a888cf27 100644
--- a/chrome/installer/util/shell_util.cc
+++ b/chrome/installer/util/shell_util.cc
@@ -971,103 +971,153 @@ bool ShellUtil::QuickIsChromeRegisteredInHKLM(BrowserDistribution* dist,
                                  CONFIRM_SHELL_REGISTRATION_IN_HKLM);
 }
 
-bool ShellUtil::CreateChromeDesktopShortcut(BrowserDistribution* dist,
-                                            const string16& chrome_exe,
-                                            const string16& description,
-                                            const string16& appended_name,
-                                            const string16& arguments,
-                                            const string16& icon_path,
-                                            int icon_index,
-                                            ShellChange shell_change,
-                                            uint32 options) {
-  string16 shortcut_name;
-  bool alternate = (options & ShellUtil::SHORTCUT_ALTERNATE) != 0;
-  if (!ShellUtil::GetChromeShortcutName(dist, alternate, appended_name,
-                                        &shortcut_name))
-    return false;
-
-  bool ret = false;
-  if (shell_change == ShellUtil::CURRENT_USER) {
-    FilePath shortcut_path;
-    // We do not want to create a desktop shortcut to Chrome in the current
-    // user's desktop folder if there is already one in the "All Users"
-    // desktop folder.
-    bool got_system_desktop = ShellUtil::GetDesktopPath(true, &shortcut_path);
-    FilePath shortcut = shortcut_path.Append(shortcut_name);
-    if (!got_system_desktop || !file_util::PathExists(shortcut)) {
-      // Either we couldn't query the "All Users" Desktop folder or there's
-      // nothing in it, so let's continue.
-      if (ShellUtil::GetDesktopPath(false, &shortcut_path)) {
-        shortcut = shortcut_path.Append(shortcut_name);
-        ret = ShellUtil::UpdateChromeShortcut(dist,
-                                              chrome_exe,
-                                              shortcut.value(),
-                                              arguments,
-                                              description,
-                                              icon_path,
-                                              icon_index,
-                                              options);
+bool ShellUtil::GetShortcutPath(ShellUtil::ShortcutLocation location,
+                                bool system_level,
+                                FilePath* path) {
+  switch (location) {
+    case ShellUtil::SHORTCUT_DESKTOP: {
+      wchar_t desktop[MAX_PATH];
+      int dir = system_level ? CSIDL_COMMON_DESKTOPDIRECTORY :
+                               CSIDL_DESKTOPDIRECTORY;
+      if (FAILED(SHGetFolderPath(NULL, dir, NULL, SHGFP_TYPE_CURRENT, desktop)))
+        return false;
+      *path = FilePath(desktop);
+      break;
+    }
+    case ShellUtil::SHORTCUT_QUICK_LAUNCH: {
+      if (system_level) {
+        wchar_t qlaunch[MAX_PATH];
+        // We are accessing GetDefaultUserProfileDirectory this way so that we
+        // do not have to declare dependency to Userenv.lib for chrome.exe
+        typedef BOOL (WINAPI *PROFILE_FUNC)(LPWSTR, LPDWORD);
+        HMODULE module = LoadLibrary(L"Userenv.dll");
+        PROFILE_FUNC p = reinterpret_cast<PROFILE_FUNC>(GetProcAddress(module,
+            "GetDefaultUserProfileDirectoryW"));
+        DWORD size = _countof(qlaunch);
+        if ((p == NULL) || ((p)(qlaunch, &size) != TRUE))
+          return false;
+        *path = FilePath(qlaunch);
+        if (base::win::GetVersion() >= base::win::VERSION_VISTA) {
+          *path = path->AppendASCII("AppData");
+          *path = path->AppendASCII("Roaming");
+        } else {
+          *path = path->AppendASCII("Application Data");

[robertshield, 2012/08/22 21:39:07]

This doesn't look like the right way of getting these paths. I think what you
want is to use the shell functions to find the user's app data dir. See
SHGetKnownFolderPath
(http://msdn.microsoft.com/en-us/library/windows/desktop/bb762188(v=vs.85).aspx)

+        }
+      } else {
+        if (!PathService::Get(base::DIR_APP_DATA, path)) {
+          return false;
+        }
       }
+      *path = path->AppendASCII("Microsoft");
+      *path = path->AppendASCII("Internet Explorer");
+      *path = path->AppendASCII("Quick Launch");

[robertshield, 2012/08/22 21:39:07]

Same comment about not hard-coding paths. Doing this makes Raymond Chen cry:
http://blogs.msdn.com/b/oldnewthing/archive/2003/11/03/55532.aspx

+      break;
     }
-  } else if (shell_change == ShellUtil::SYSTEM_LEVEL) {
-    FilePath shortcut_path;
-    if (ShellUtil::GetDesktopPath(true, &shortcut_path)) {
-      FilePath shortcut = shortcut_path.Append(shortcut_name);
-      ret = ShellUtil::UpdateChromeShortcut(dist,
-                                            chrome_exe,
-                                            shortcut.value(),
-                                            arguments,
-                                            description,
-                                            icon_path,
-                                            icon_index,
-                                            options);
+    case ShellUtil::SHORTCUT_START_MENU:
+    case ShellUtil::SHORTCUT_START_MENU_UNINSTALL: {
+      int dir = system_level ? base::DIR_COMMON_START_MENU :
+                               base::DIR_START_MENU;
+      if (!PathService::Get(dir, path))
+        return false;
+      BrowserDistribution* dist = BrowserDistribution::GetDistribution();
+      *path = path->Append(dist->GetAppShortCutName());
+      break;
+    }
+    default: {
+      NOTREACHED();
+      return false;
     }
+  }
+  return true;
+}
+
+FilePath ShellUtil::DetermineShortcutPathToCreateFromOptions(
+    const FilePath& user_level_shortcut,
+    const FilePath& system_level_shortcut,
+    uint32 options) {
+  if (options & ShellUtil::SHORTCUT_SYSTEM_LEVEL) {
+    // Install the system-level shortcut if requested.
+    DCHECK(!system_level_shortcut.empty());
+    return system_level_shortcut;
+  } else if (system_level_shortcut.empty() ||
+             !file_util::PathExists(system_level_shortcut)){
+    // Otherwise install the user-level shortcut, but do so iff no system-level
+    // shortcut is present.
+    DCHECK(!user_level_shortcut.empty());
+    return user_level_shortcut;
   } else {
-    NOTREACHED();
+    // Do not install anything if the system-level shortcut is present, but we
+    // are told to install the user-level shortcut.
+    return FilePath();
   }
-  return ret;
 }
 
-bool ShellUtil::CreateChromeQuickLaunchShortcut(BrowserDistribution* dist,
-                                                const string16& chrome_exe,
-                                                int shell_change,
-                                                uint32 options) {
+bool ShellUtil::CreateOrUpdateChromeShortcut(
+    ShellUtil::ShortcutLocation location,
+    BrowserDistribution* dist,
+    const string16& exe_path,
+    const string16& description,
+    const string16& appended_name,
+    const string16& arguments,
+    const string16& icon_path,
+    int icon_index,
+    uint32 options) {
   string16 shortcut_name;
-  if (!ShellUtil::GetChromeShortcutName(dist, false, L"", &shortcut_name))
+  const bool alternate = ((options & ShellUtil::SHORTCUT_ALTERNATE) != 0 &&
+                          location == SHORTCUT_DESKTOP);
+  if (!ShellUtil::GetChromeShortcutName(location, dist, alternate,
+                                        appended_name, &shortcut_name)) {
     return false;
+  }
 
-  bool ret = true;
-  // First create shortcut for the current user.
-  if (shell_change & ShellUtil::CURRENT_USER) {
-    FilePath user_ql_path;
-    if (ShellUtil::GetQuickLaunchPath(false, &user_ql_path)) {
-      user_ql_path = user_ql_path.Append(shortcut_name);
-      ret = ShellUtil::UpdateChromeShortcut(dist, chrome_exe,
-                                            user_ql_path.value(),
-                                            L"", L"", chrome_exe,
-                                            dist->GetIconIndex(),
-                                            options);
-    } else {
-      ret = false;
-    }
+  FilePath user_shortcut_path;
+  FilePath system_shortcut_path;
+  if (!GetShortcutPath(location, false, &user_shortcut_path))
+    user_shortcut_path.clear();
+  if (!GetShortcutPath(location, true, &system_shortcut_path))
+    system_shortcut_path.clear();
+
+  FilePath path_to_create(DetermineShortcutPathToCreateFromOptions(
+      user_shortcut_path, system_shortcut_path, options));
+
+  // No shortcut needs to be created/updated.
+  if (path_to_create.empty())
+    return true;
+
+  // Make sure the parent directories exist.
+  if ((options & SHORTCUT_CREATE_ALWAYS) &&
+      !file_util::CreateDirectory(path_to_create.DirName())) {
+    NOTREACHED();
+    return false;
   }
 
-  // Add a shortcut to Default User's profile so that all new user profiles
-  // get it.
-  if (shell_change & ShellUtil::SYSTEM_LEVEL) {
-    FilePath default_ql_path;
-    if (ShellUtil::GetQuickLaunchPath(true, &default_ql_path)) {
-      default_ql_path = default_ql_path.Append(shortcut_name);
-      ret = ShellUtil::UpdateChromeShortcut(dist, chrome_exe,
-                                            default_ql_path.value(),
-                                            L"", L"", chrome_exe,
-                                            dist->GetIconIndex(),
-                                            options) && ret;
-    } else {
-      ret = false;
+  string16 app_id;
+  FilePath exe_dir(FilePath(exe_path).DirName());
+  // Set the |app_id| and |icon_index| correctly if this is a shortcut to
+  // chrome.exe.
+  if (exe_path.find(installer::kChromeExe) != string16::npos) {
+    app_id.assign(GetBrowserModelId(dist, exe_path));
+
+    installer::MasterPreferences prefs(
+        exe_dir.AppendASCII(installer::kDefaultMasterPrefs));
+    if (InstallUtil::ProgramCompare(FilePath(exe_path)).Evaluate(icon_path)) {
+      prefs.GetInt(installer::master_preferences::kChromeShortcutIconIndex,
+                   &icon_index);
     }
   }
 
+  bool ret = file_util::CreateOrUpdateShortcutLink(
+      exe_path.c_str(), path_to_create.value().c_str(),
+      exe_dir.value().c_str(), arguments.c_str(), description.c_str(),
+      icon_path.c_str(), icon_index, app_id.c_str(),
+      ConvertShellUtilShortcutOptionsToFileUtil(options));
+
+  if (ret && (options & SHORTCUT_PIN_TO_TASKBAR) &&
+      (options & SHORTCUT_CREATE_ALWAYS) &&
+      base::win::GetVersion() >= base::win::VERSION_WIN7) {
+    ret = file_util::TaskbarPinShortcutLink(path_to_create.value().c_str());
+  }
+
   return ret;
 }
 
@@ -1087,12 +1137,32 @@ string16 ShellUtil::GetChromeDelegateCommand(const string16& chrome_exe) {
   return L"\"" + chrome_exe + L"\" -- %*";
 }
 
-bool ShellUtil::GetChromeShortcutName(BrowserDistribution* dist,
+bool ShellUtil::GetChromeShortcutName(ShellUtil::ShortcutLocation location,
+                                      BrowserDistribution* dist,
                                       bool alternate,
                                       const string16& appended_name,
                                       string16* shortcut) {
-  shortcut->assign(alternate ? dist->GetAlternateApplicationName() :
-                               dist->GetAppShortCutName());
+  // Only the Desktop shortcut can use the alternate name.
+  DCHECK(!alternate || location == SHORTCUT_DESKTOP);
+
+  switch (location) {
+    case SHORTCUT_DESKTOP:
+      shortcut->assign(alternate ? dist->GetAlternateApplicationName() :
+                                   dist->GetAppShortCutName());
+      break;
+    case SHORTCUT_QUICK_LAUNCH:
+    case SHORTCUT_START_MENU:
+      shortcut->assign(dist->GetAppShortCutName());
+      break;
+    case SHORTCUT_START_MENU_UNINSTALL:
+      shortcut->assign(dist->GetUninstallLinkName());
+      break;
+    default:
+      NOTREACHED();
+      shortcut->clear();
+      return false;
+  }
+
   if (!appended_name.empty()) {
     shortcut->append(L" (");
     shortcut->append(appended_name);
@@ -1102,46 +1172,6 @@ bool ShellUtil::GetChromeShortcutName(BrowserDistribution* dist,
   return true;
 }
 
-bool ShellUtil::GetDesktopPath(bool system_level, FilePath* path) {
-  wchar_t desktop[MAX_PATH];
-  int dir = system_level ? CSIDL_COMMON_DESKTOPDIRECTORY :
-                           CSIDL_DESKTOPDIRECTORY;
-  if (FAILED(SHGetFolderPath(NULL, dir, NULL, SHGFP_TYPE_CURRENT, desktop)))
-    return false;
-  *path = FilePath(desktop);
-  return true;
-}
-
-bool ShellUtil::GetQuickLaunchPath(bool system_level, FilePath* path) {
-  if (system_level) {
-    wchar_t qlaunch[MAX_PATH];
-    // We are accessing GetDefaultUserProfileDirectory this way so that we do
-    // not have to declare dependency to Userenv.lib for chrome.exe
-    typedef BOOL (WINAPI *PROFILE_FUNC)(LPWSTR, LPDWORD);
-    HMODULE module = LoadLibrary(L"Userenv.dll");
-    PROFILE_FUNC p = reinterpret_cast<PROFILE_FUNC>(GetProcAddress(module,
-        "GetDefaultUserProfileDirectoryW"));
-    DWORD size = _countof(qlaunch);
-    if ((p == NULL) || ((p)(qlaunch, &size) != TRUE))
-      return false;
-    *path = FilePath(qlaunch);
-    if (base::win::GetVersion() >= base::win::VERSION_VISTA) {
-      *path = path->AppendASCII("AppData");
-      *path = path->AppendASCII("Roaming");
-    } else {
-      *path = path->AppendASCII("Application Data");
-    }
-  } else {
-    if (!PathService::Get(base::DIR_APP_DATA, path)) {
-      return false;
-    }
-  }
-  *path = path->AppendASCII("Microsoft");
-  *path = path->AppendASCII("Internet Explorer");
-  *path = path->AppendASCII("Quick Launch");
-  return true;
-}
-
 void ShellUtil::GetRegisteredBrowsers(
     BrowserDistribution* dist,
     std::map<string16, string16>* browsers) {
@@ -1542,33 +1572,33 @@ bool ShellUtil::RegisterChromeForProtocol(BrowserDistribution* dist,
   }
 }
 
-bool ShellUtil::RemoveChromeDesktopShortcut(BrowserDistribution* dist,
-                                            int shell_change, uint32 options) {
-  // Only SHORTCUT_ALTERNATE is a valid option for this function.
-  DCHECK(!options || options == ShellUtil::SHORTCUT_ALTERNATE);
-
+bool ShellUtil::RemoveChromeShortcut(ShellUtil::ShortcutLocation location,
+                                     BrowserDistribution* dist,
+                                     uint32 options) {
   string16 shortcut_name;
-  bool alternate = (options & ShellUtil::SHORTCUT_ALTERNATE) != 0;
-  if (!ShellUtil::GetChromeShortcutName(dist, alternate, L"",
+  bool alternate = (options & ShellUtil::SHORTCUT_ALTERNATE) != 0 &&
+                   location == SHORTCUT_DESKTOP;
+  if (!ShellUtil::GetChromeShortcutName(location, dist, alternate, string16(),
                                         &shortcut_name))
     return false;
 
   bool ret = true;
-  if (shell_change & ShellUtil::CURRENT_USER) {
-    FilePath shortcut_path;
-    if (ShellUtil::GetDesktopPath(false, &shortcut_path)) {
-      FilePath shortcut = shortcut_path.Append(shortcut_name);
-      ret = file_util::Delete(shortcut, false);
-    } else {
-      ret = false;
-    }
+
+  // Always delete the user-level shortcut.
+  FilePath user_shortcut_path;
+  if (GetShortcutPath(location, false, &user_shortcut_path)) {
+    user_shortcut_path = user_shortcut_path.Append(shortcut_name);
+    ret = file_util::Delete(user_shortcut_path, false);
+  } else {
+    ret = false;
   }
 
-  if (shell_change & ShellUtil::SYSTEM_LEVEL) {
-    FilePath shortcut_path;
-    if (ShellUtil::GetDesktopPath(true, &shortcut_path)) {
-      FilePath shortcut = shortcut_path.Append(shortcut_name);
-      ret = file_util::Delete(shortcut, false) && ret;
+  // Delete the system-level shortcut if requested.
+  if (options & ShellUtil::SHORTCUT_SYSTEM_LEVEL) {
+    FilePath system_shortcut_path;
+    if (GetShortcutPath(location, true, &system_shortcut_path)) {
+      system_shortcut_path = system_shortcut_path.Append(shortcut_name);
+      ret = ret && file_util::Delete(system_shortcut_path, false);

[robertshield, 2012/08/22 21:39:07]

if ret is already false, the Delete operation is never called

     } else {
       ret = false;
     }
@@ -1580,11 +1610,9 @@ bool ShellUtil::RemoveChromeDesktopShortcutsWithAppendedNames(
     const std::vector<string16>& appended_names) {
   FilePath shortcut_path;
   bool ret = true;
-  if (ShellUtil::GetDesktopPath(false, &shortcut_path)) {
-    for (std::vector<string16>::const_iterator it =
-             appended_names.begin();
-         it != appended_names.end();
-         ++it) {
+  if (GetShortcutPath(ShellUtil::SHORTCUT_DESKTOP, false, &shortcut_path)) {
+    for (std::vector<string16>::const_iterator it = appended_names.begin();
+         it != appended_names.end(); ++it) {
       FilePath delete_shortcut = shortcut_path.Append(*it);
       ret = ret && file_util::Delete(delete_shortcut, false);
     }
@@ -1594,69 +1622,6 @@ bool ShellUtil::RemoveChromeDesktopShortcutsWithAppendedNames(
   return ret;
 }
 
-bool ShellUtil::RemoveChromeQuickLaunchShortcut(BrowserDistribution* dist,
-                                                int shell_change) {
-  string16 shortcut_name;
-  if (!ShellUtil::GetChromeShortcutName(dist, false, L"", &shortcut_name))
-    return false;
-
-  bool ret = true;
-  // First remove shortcut for the current user.
-  if (shell_change & ShellUtil::CURRENT_USER) {
-    FilePath user_ql_path;
-    if (ShellUtil::GetQuickLaunchPath(false, &user_ql_path)) {
-      user_ql_path = user_ql_path.Append(shortcut_name);
-      ret = file_util::Delete(user_ql_path, false);
-    } else {
-      ret = false;
-    }
-  }
-
-  // Delete shortcut in Default User's profile
-  if (shell_change & ShellUtil::SYSTEM_LEVEL) {
-    FilePath default_ql_path;
-    if (ShellUtil::GetQuickLaunchPath(true, &default_ql_path)) {
-      default_ql_path = default_ql_path.Append(shortcut_name);
-      ret = file_util::Delete(default_ql_path, false) && ret;
-    } else {
-      ret = false;
-    }
-  }
-
-  return ret;
-}
-
-bool ShellUtil::UpdateChromeShortcut(BrowserDistribution* dist,
-                                     const string16& chrome_exe,
-                                     const string16& shortcut,
-                                     const string16& arguments,
-                                     const string16& description,
-                                     const string16& icon_path,
-                                     int icon_index,
-                                     uint32 options) {
-  const FilePath chrome_path(FilePath(chrome_exe).DirName());
-
-  installer::MasterPreferences prefs(
-      chrome_path.AppendASCII(installer::kDefaultMasterPrefs));
-  if (FilePath::CompareEqualIgnoreCase(icon_path, chrome_exe)) {
-    prefs.GetInt(installer::master_preferences::kChromeShortcutIconIndex,
-                 &icon_index);
-  }
-
-  const string16 app_id(GetBrowserModelId(dist, chrome_exe));
-
-  return file_util::CreateOrUpdateShortcutLink(
-      chrome_exe.c_str(),
-      shortcut.c_str(),
-      chrome_path.value().c_str(),
-      arguments.c_str(),
-      description.c_str(),
-      icon_path.c_str(),
-      icon_index,
-      app_id.c_str(),
-      ConvertShellUtilShortcutOptionsToFileUtil(options));
-}
-
 ShellUtil::VerifyShortcutStatus ShellUtil::VerifyChromeShortcut(
     const string16& exe_path, const string16& shortcut,
     const string16& description, int icon_index) {