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

chrome/installer/util/shell_util.h

Index: chrome/installer/util/shell_util.h
diff --git a/chrome/installer/util/shell_util.h b/chrome/installer/util/shell_util.h
index 538a42e2373cd2de891c3dc74b05f17c2a8073c1..c42312e42f64f2b4da93c464349ba1cb2bedca71 100644
--- a/chrome/installer/util/shell_util.h
+++ b/chrome/installer/util/shell_util.h
@@ -15,11 +15,11 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/file_path.h"
 #include "base/string16.h"
 #include "chrome/installer/util/work_item_list.h"
 
 class BrowserDistribution;
-class FilePath;
 
 namespace base {
 class DictionaryValue;
@@ -35,12 +35,128 @@ class ShellUtil {
     SYSTEM_LEVEL = 0x2   // Make any shell changes only at the system level
   };
 
-  enum VerifyShortcutStatus {
-    VERIFY_SHORTCUT_SUCCESS = 0,
-    VERIFY_SHORTCUT_FAILURE_UNEXPECTED,
-    VERIFY_SHORTCUT_FAILURE_PATH,
-    VERIFY_SHORTCUT_FAILURE_DESCRIPTION,
-    VERIFY_SHORTCUT_FAILURE_ICON_INDEX,
+  // Typical shortcut directories. Resolved in GetShortcutPath().
+  enum ChromeShortcutLocation {
+    SHORTCUT_DESKTOP = 0,

[grt (UTC plus 2), 2012/10/02 13:22:30]

personal preference: remove " = 0".

[gab, 2012/10/03 15:14:59]

in src/chrome/* 230 instances use " = 0". 474 don't... so you win :)

Done.

+    SHORTCUT_QUICK_LAUNCH,
+    SHORTCUT_START_MENU,
+  };
+
+  enum ChromeShortcutOperation {
+    // Create a new shortcut (overwriting if necessary).
+    SHORTCUT_CREATE_ALWAYS = 0,
+    // Create the per-user shortcut only if its system-level equivalent is not
+    // present on this machine.

[grt (UTC plus 2), 2012/10/02 13:22:30]

" on this machine" is superfluous

[gab, 2012/10/03 15:14:59]

Done.

+    SHORTCUT_CREATE_IF_NO_SYSTEM_LEVEL,
+    // Overwrite an existing shortcut (fails if the shortcut doesn't exist).

[grt (UTC plus 2), 2012/10/02 13:22:30]

fails -> fail

[gab, 2012/10/03 15:14:59]

Done.

+    SHORTCUT_REPLACE_EXISTING,
+    // Update specified properties only on an existing shortcut.
+    SHORTCUT_UPDATE_EXISTING,
+  };
+
+  // Properties for Chrome shortcuts. Properties set will be applied to the
+  // shortcut on creation/update. On update, non-set properties are ignored;

[grt (UTC plus 2), 2012/10/02 13:22:30]

"non-set" -> "unset" here and below

[gab, 2012/10/03 15:14:59]

Done.

+  // on create non-set properties might have a default value (see individual
+  // properties below for details).
+  // Callers are encouraged to use the setters provided which take care of
+  // setting |options| as desired.
+  struct ChromeShortcutProperties {
+    enum ChromeIndividualProperties {
+      PROPERTIES_TARGET = 1 << 0,
+      PROPERTIES_ARGUMENTS = 1 << 1,
+      PROPERTIES_DESCRIPTION = 1 << 2,
+      PROPERTIES_ICON = 1 << 3,
+      PROPERTIES_APP_ID = 1 << 4,
+      PROPERTIES_SHORTCUT_NAME = 1 << 5,
+      PROPERTIES_DUAL_MODE = 1 << 6,
+    };
+
+    ChromeShortcutProperties()
+        : options(0U), system_level(false), pin_to_taskbar(false) {}

[grt (UTC plus 2), 2012/10/02 13:22:30]

be sure to list all members of POD type, and always put them in the same order
as they're declared in the class:

: dual_mode(false), system_level(false), pin_to_taskbar(false), options(0U)

the following is equivalent since default initialization leads to
zero-initialization for these types:

: dual_mode(), system_level(), pin_to_taskbar(), options()

[gab, 2012/10/03 15:14:59]

Done.

+
+    void set_target(const FilePath& target_in) {
+      target = target_in;
+      options |= PROPERTIES_TARGET;
+    }
+
+    void set_arguments(const string16& arguments_in) {
+      arguments = arguments_in;
+      options |= PROPERTIES_ARGUMENTS;
+    }
+
+    void set_description(const string16& description_in) {
+      description = description_in;
+      options |= PROPERTIES_DESCRIPTION;
+    }
+
+    void set_icon(const FilePath& icon_in) {
+      icon = icon_in;
+      options |= PROPERTIES_ICON;
+    }
+
+    void set_app_id(const string16& app_id_in) {
+      app_id = app_id_in;
+      options |= PROPERTIES_APP_ID;
+    }
+
+    void set_shortcut_name(const string16& shortcut_name_in) {
+      shortcut_name = shortcut_name_in;
+      options |= PROPERTIES_SHORTCUT_NAME;
+    }
+
+    void set_dual_mode(bool dual_mode_in) {
+      dual_mode = dual_mode_in;
+      options |= PROPERTIES_DUAL_MODE;
+    }
+
+    void set_system_level(bool system_level_in) {
+      system_level = system_level_in;
+    }
+
+    void set_pin_to_taskbar(bool pin_to_taskbar_in) {
+      pin_to_taskbar = pin_to_taskbar_in;
+    }
+
+    // The target to launch from this shortcut. This is mandatory when creating

[grt (UTC plus 2), 2012/10/02 13:22:30]

since callers are expected to use the functions, i think it's more useful to put
these doc strings up on the setters above and put a "// See comments above for
details." here.

[gab, 2012/10/03 15:14:59]

Ya I was also debating doing that, makes sense, done.

+    // a shortcut.
+    FilePath target;
+    // The arguments to be passed to |target| when launching from this shortcut.
+    // The length of this string must be less than MAX_PATH.
+    string16 arguments;
+    // The localized description of the shortcut. If this is not set, the

[grt (UTC plus 2), 2012/10/02 13:22:30]

I think it's nice if the same language is used to describe the default for all
of these.  For example:

// The whozawhatsit. The default value is blah.
or
// The whozawhatsit. If some condition, the default value is blah.

For example:

// The localized description of the shortcut. If |target| is chrome.exe, the
default value is the description for the current browser distribution.

[gab, 2012/10/03 15:14:59]

Agreed, I'll switch to use a specific syntax for each default so that it's easy
to see what are the defaults in a quick glance.

+    // shortcut is being created, and |target| is chrome.exe, this defaults to
+    // the default description for the current distribution.
+    // The length of this string must be less than MAX_PATH.
+    string16 description;
+    // The path to the icon. If this is not set and the shortcut is being
+    // created, this defaults to |target| (and if |target| is chrome.exe, the
+    // icon's index is determined from master preferences if defined there;
+    // otherwise the default icon index for the current distribution is used).
+    FilePath icon;
+    // The app model id for the shortcut (Win7+). If this is not set, the
+    // shortcut is being created, and |target| is chrome.exe, this defaults to
+    // the appid for the current distribution.
+    string16 app_id;
+    // If set, the shortcut's name will be forced to this. By default the
+    // shortcut's name is the current distribution's GetAppShortcutName().
+    // The ".lnk" extension will automatically be added to this name.
+    string16 shortcut_name;
+    // Whether this is a dual mode shortcut (Win8+).
+    bool dual_mode;
+    // Whether the system-level variant of this shortcut should be installed
+    // (the default being to otherwise install the per-user variant of this
+    // shortcut).
+    // Note: This property doesn't have a mask in |options| as it is always
+    // required. It defaults to false.
+    bool system_level;
+    // Whether to pin this shortcut to the taskbar after creating it (ignored if
+    // the shortcut is only being updated).
+    // Note: This property doesn't have a mask in |options|. It is only
+    // acknowledged if set to true when creating the shortcut on Windows 7+.
+    bool pin_to_taskbar;
+    // Bitfield made of IndividualProperties. Properties set in |options| will
+    // be used to create the shortcut, others will be ignored.
+    uint32 options;
   };
 
   // Relative path of the URL Protocol registry entry (prefixed with '\').
@@ -142,43 +258,36 @@ class ShellUtil {
                                             const string16& chrome_exe,
                                             const string16& suffix);
 
-  // Creates Chrome shortcut on the Desktop.
+  // Sets |path| to the path for a shortcut at the |location| desired at
+  // |system_level| or not.
+  // Returns false on failure (in which case |path| shouldn't to be trusted).

[grt (UTC plus 2), 2012/10/02 13:22:30]

does the "trust" statement mean that |path| might be modified even on failure? 
if so, i think "|path| may be modified on failure" is more clear.

[gab, 2012/10/03 15:14:59]

This used to be the case, but no longer is, removing this part of the comment.
Thanks!

+  static bool GetShortcutPath(ChromeShortcutLocation location,
+                              BrowserDistribution* dist,
+                              bool system_level,
+                              FilePath* path);
+
+  // Updates Chrome shortcut in |location| (or creates it if |options| specify
+  // SHORTCUT_CREATE_ALWAYS).
   // |dist| gives the type of browser distribution currently in use.
-  // |chrome_exe| provides the target path information.
+  // |exe_path| provides the target path information.
   // |description| provides the shortcut's "comment" property.
-  // |appended_name| provides a string to be appended to the distribution name,
+  // |appended_name| provides a string to be appended to the distribution name

[grt (UTC plus 2), 2012/10/02 13:22:30]

while you're changing this, how about:
// |appended_name| provides a (possibly empty) string to be appended to the
distribution name.

[gab, 2012/10/03 15:14:59]

Oops, this description was out of date, this method no longer takes these params
in this patch set.

   //     and can be the empty string.
   // |arguments| gives a set of arguments to be passed to the executable.
   // |icon_path| provides the path to the icon file to use.
   // |icon_index| provides the index of the icon within the provided icon file.
-  // If |shell_change| is CURRENT_USER, the shortcut is created in the
-  //     Desktop folder of current user's profile.
-  // If |shell_change| is SYSTEM_LEVEL, the shortcut is created in the
-  //     Desktop folder of the "All Users" profile.
+  //     If |icon_path| is |exe_path| and |exe_path| points to chrome.exe:
+  //     |icon_index is overriden with the master preferences value.
   // |options|: bitfield for which the options come from ChromeShortcutOptions.
+  // If SHORTCUT_CREATE_ALWAYS is not set in |options|, only specified (non-
+  // empty) properties on an existing shortcut will be modified. If the shortcut
+  // does not exist, this method is a no-op and returns false.
   // Returns true iff the method causes a shortcut to be created / updated.
-  static bool 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);
-
-  // Create Chrome shortcut on Quick Launch Bar.
-  // If shell_change is CURRENT_USER, the shortcut is created in the
-  // Quick Launch folder of current user's profile.
-  // If shell_change is SYSTEM_LEVEL, the shortcut is created in the
-  // Quick Launch folder of "Default User" profile. This will make sure
-  // that this shortcut will be seen by all the new users logging into the
-  // system.
-  // |options|: bitfield for which the options come from ChromeShortcutOptions.
-  static bool CreateChromeQuickLaunchShortcut(BrowserDistribution* dist,
-                                              const string16& chrome_exe,
-                                              int shell_change,
-                                              uint32 options);
+  static bool CreateOrUpdateChromeShortcut(
+      ChromeShortcutLocation location,
+      BrowserDistribution* dist,
+      const ChromeShortcutProperties& properties,
+      ChromeShortcutOperation operation);
 
   // This method appends the Chrome icon index inside chrome.exe to the
   // chrome.exe path passed in as input, to generate the full path for
@@ -198,27 +307,6 @@ class ShellUtil {
   // |chrome_exe|: the full path to chrome.exe
   static string16 GetChromeDelegateCommand(const string16& chrome_exe);
 
-  // Returns the localized name of Chrome shortcut in |shortcut|. If
-  // |appended_name| is not empty, it is included in the shortcut name. If
-  // |alternate| is true, a second localized text that is better suited for
-  // certain scenarios is used.
-  static bool GetChromeShortcutName(BrowserDistribution* dist,
-                                    bool alternate,
-                                    const string16& appended_name,
-                                    string16* shortcut);
-
-  // Gets the desktop path for the current user or all users (if system_level
-  // is true) and returns it in 'path' argument. Return true if successful,
-  // otherwise returns false.
-  static bool GetDesktopPath(bool system_level, FilePath* path);
-
-  // Gets the Quick Launch shortcuts path for the current user and
-  // returns it in 'path' argument. Return true if successful, otherwise
-  // returns false. If system_level is true this function returns the path
-  // to Default Users Quick Launch shortcuts path. Adding a shortcut to Default
-  // User's profile only affects any new user profiles (not existing ones).
-  static bool GetQuickLaunchPath(bool system_level, FilePath* path);
-
   // Gets a mapping of all registered browser names (excluding browsers in the
   // |dist| distribution) and their reinstall command (which usually sets
   // browser as default).
@@ -378,64 +466,21 @@ class ShellUtil {
                                         const string16& protocol,
                                         bool elevate_if_not_admin);
 
-  // Remove Chrome shortcut from Desktop.
-  // If |shell_change| is CURRENT_USER, the shortcut is removed from the
-  // Desktop folder of current user's profile.
-  // If |shell_change| is SYSTEM_LEVEL, the shortcut is removed from the
-  // Desktop folder of "All Users" profile.
-  // |options|: bitfield for which the options come from ChromeShortcutOptions.
-  // Only SHORTCUT_ALTERNATE is a valid option for this function.
-  static bool RemoveChromeDesktopShortcut(BrowserDistribution* dist,
-                                          int shell_change,
-                                          uint32 options);
-
-  // Removes a set of existing Chrome desktop shortcuts. |appended_names| is a
-  // list of shortcut file names as obtained from
-  // ShellUtil::GetChromeShortcutName.
-  static bool RemoveChromeDesktopShortcutsWithAppendedNames(
-      const std::vector<string16>& appended_names);
-
-  // Remove Chrome shortcut from Quick Launch Bar.
-  // If shell_change is CURRENT_USER, the shortcut is removed from
-  // the Quick Launch folder of current user's profile.
-  // If shell_change is SYSTEM_LEVEL, the shortcut is removed from
-  // the Quick Launch folder of "Default User" profile.
-  static bool RemoveChromeQuickLaunchShortcut(BrowserDistribution* dist,
-                                              int shell_change);
+  // Removes installed Chrome shortcut at |location|.
+  // |properties|: Only properties.system_level and properties.shortcut_name are

[grt (UTC plus 2), 2012/10/02 13:22:30]

i think system_level and shortcut_name should be passed independently
(|properties| seems like overkill).

[gab, 2012/10/03 15:14:59]

Done, I was using properties to avoid having NULLs all over the place for
|shortcut_name|, but I agree that using properties is equally confusing and
overkill (given most properties are irrelevant).

+  // relevant for this method.
+  // Also attempts to unpin the removed shortcut from the taskbar.
+  // Returns true unless an unexpected failure occurs (i.e. returns true even if
+  // not shortcut is found at that location).
+  static bool RemoveChromeShortcut(
+      ChromeShortcutLocation location,
+      BrowserDistribution* dist,
+      const ChromeShortcutProperties& properties);
 
   // This will remove all secondary tiles from the start screen for |dist|.
   static void RemoveChromeStartScreenShortcuts(BrowserDistribution* dist,
                                                const string16& chrome_exe);
 
-  enum ChromeShortcutOptions {
-    SHORTCUT_NO_OPTIONS = 0,
-    // Set DualMode property for Windows 8 Metro-enabled shortcuts.
-    SHORTCUT_DUAL_MODE = 1 << 0,
-    // Create a new shortcut (overwriting if necessary).
-    SHORTCUT_CREATE_ALWAYS = 1 << 1,
-    // Use an alternate application name for the shortcut (e.g. "The Internet").
-    // This option is only applied to the Desktop shortcut.
-    SHORTCUT_ALTERNATE = 1 << 2,
-  };
-
-  // Updates shortcut (or creates a new shortcut) at destination given by
-  // shortcut to a target given by chrome_exe. The arguments are given by
-  // |arguments| for the target and icon is set based on |icon_path| and
-  // |icon_index|. If create_new is set to true, the function will create a new
-  // shortcut if it doesn't exist.
-  // |options|: bitfield for which the options come from ChromeShortcutOptions.
-  // If SHORTCUT_CREATE_ALWAYS is not set in |options|, only specified (non-
-  // null) properties on an existing shortcut will be modified. If the shortcut
-  // does not exist, this method is a no-op and returns false.
-  static bool 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);
-
   // Sets |suffix| to the base 32 encoding of the md5 hash of this user's sid
   // preceded by a dot.
   // This is guaranteed to be unique on the machine and 27 characters long