Suffix Chrome's appid on user-level installs

chrome/installer/setup/install.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.
 
 #include "chrome/installer/setup/install.h"
 
 #include <shlobj.h>
 #include <time.h>
 
 #include "base/command_line.h"
@@ skipping 69 matching lines @@
 // uninstalling chrome.
 // If first_install is false, the function only updates the shortcut for
 // uninstalling chrome. According to
 // http://blogs.msdn.com/oldnewthing/archive/2005/11/24/496690.aspx,
 // updating uninstall shortcut should not trigger Windows "new application
 // installed" notification.
 //
 // If the shortcuts do not exist, the function does not recreate them during
 // update.
 // |options|: bitfield for which the options come from ChromeShortcutOptions.
-void CreateOrUpdateChromeShortcuts(const InstallerState& installer_state,
-                                   const FilePath& setup_path,
-                                   const Version& new_version,
-                                   installer::InstallStatus install_status,
-                                   const Product& product,
-                                   uint32 options) {
+void CreateOrUpdateChromeShortcuts(
+    const InstallerState& installer_state,
+    const FilePath& setup_path,
+    const Version& new_version,
+    const installer::InstallStatus& install_status,

[grt (UTC plus 2), 2012/06/15 03:03:03]

why pass a value of an enum by const ref?

[gab, 2012/06/15 19:01:04]

Good question, I somehow thought it was an Object and remember having a reason
wanting this call not to modify its state... my bad!

+    const Product& product,
+    uint32 options) {
   // TODO(tommi): Change this function to use WorkItemList.
   DCHECK(product.is_chrome());
 
   // Information used for all shortcut types
   BrowserDistribution* browser_dist = product.distribution();
   const std::wstring product_name(browser_dist->GetAppShortCutName());
   const std::wstring product_desc(browser_dist->GetAppDescription());
   // Chrome link target
   FilePath chrome_exe(
       installer_state.target_path().Append(installer::kChromeExe));
@@ skipping 355 matching lines @@
 
     // Update the modifiers on the channel values for the product(s) being
     // installed and for the binaries in case of multi-install.
     installer_state.UpdateChannels();
 
     installer_state.UpdateStage(installer::COPYING_PREFERENCES_FILE);
 
     if (result == FIRST_INSTALL_SUCCESS && !prefs_path.empty())
       CopyPreferenceFileForFirstRun(installer_state, prefs_path);
 
-    bool do_not_create_shortcuts = false;
-    prefs.GetBool(master_preferences::kDoNotCreateShortcuts,

[grt (UTC plus 2), 2012/06/15 03:03:03]

robert added this in http://crrev.com/25870.  it explicitly prevented
registration.  i'm not familiar with how it's used in practice, but this CL is a
significant departure from existing behavior.  robert: can you shed some light
on why this preference was added and under what circumstances it's used?

[robertshield, 2012/06/15 03:27:59]

This was added long ago for Chrome Frame, so that installing CF would not create
shortcuts to desktop Chrome. At one point, this argument was implied by the
presence of --chrome-frame, rendering the explicit command line flag obsolete,
but it looks like it never got scrubbed from our Omaha configs and still today,
this argument is passed to CF installers that users run, both stub and MSIs.

Chrome MSI and stub installers never get this, but I don't know whether any OEM
deals pass this explicitly for Chrome.

Several refactorings later, this appears to be the sole remaining usage and I
believe that a standalone CF install would be unaffected by this change due to
the "if (chrome_install)" check. 

Barring any OEM deals passing this explicitly for Chrome installations, making
this change is fine, but please try running a CF install, as well as a multi
CF+Chrome install to verify. 

[grt (UTC plus 2), 2012/06/15 03:33:51]

Do you know who we should ask about that?
How about just removing all traces of kDoNotCreateShortcuts from the code?  It
wouldn't break any vestigial uses of the option unless there are deals that use
it for Chrome.

[gab, 2012/06/15 19:01:04]

This sounds like its wrong/unused, we should probably remove it in another
patch, but I don't need to do this here anymore and thus won't be changing this
in this CL.

-                  &do_not_create_shortcuts);
-
     // Currently this only creates shortcuts for Chrome, but for other products
     // we might want to create shortcuts.
     const Product* chrome_install =
         installer_state.FindProduct(BrowserDistribution::CHROME_BROWSER);
-    if (chrome_install && !do_not_create_shortcuts) {
-      installer_state.UpdateStage(installer::CREATING_SHORTCUTS);
-
-      bool create_all_shortcut = false;
-      prefs.GetBool(master_preferences::kCreateAllShortcuts,
-                    &create_all_shortcut);
-      bool alt_shortcut = false;
-      prefs.GetBool(master_preferences::kAltShortcutText, &alt_shortcut);
-      uint32 shortcut_options = ShellUtil::SHORTCUT_NO_OPTIONS;
-      if (create_all_shortcut)
-        shortcut_options |= ShellUtil::SHORTCUT_CREATE_ALWAYS;
-      if (alt_shortcut)
-        shortcut_options |= ShellUtil::SHORTCUT_ALTERNATE;
-      CreateOrUpdateChromeShortcuts(installer_state, setup_path,
-                                    new_version, result, *chrome_install,
-                                    shortcut_options);
-
+    if (chrome_install) {
       bool make_chrome_default = false;
       prefs.GetBool(master_preferences::kMakeChromeDefault,
                     &make_chrome_default);
 
       // If this is not the user's first Chrome install, but they have chosen
       // Chrome to become their default browser on the download page, we must
       // force it here because the master_preferences file will not get copied
       // into the build.
       bool force_chrome_default_for_user = false;
       if (result == NEW_VERSION_UPDATED ||
           result == INSTALL_REPAIRED) {
         prefs.GetBool(master_preferences::kMakeChromeDefaultForUser,
                       &force_chrome_default_for_user);
       }
 
       installer_state.UpdateStage(installer::REGISTERING_CHROME);
 
+      // Chrome must be registered before creating shortcuts as the registration
+      // determines the appid that will later be embedded in the shortcut.
       RegisterChromeOnMachine(installer_state, *chrome_install,
           make_chrome_default || force_chrome_default_for_user);
 
+      bool do_not_create_shortcuts = false;
+      prefs.GetBool(master_preferences::kDoNotCreateShortcuts,
+                    &do_not_create_shortcuts);
+
+      if (!do_not_create_shortcuts) {
+        installer_state.UpdateStage(installer::CREATING_SHORTCUTS);
+
+        bool create_all_shortcut = false;
+        prefs.GetBool(master_preferences::kCreateAllShortcuts,
+                      &create_all_shortcut);
+        bool alt_shortcut = false;
+        prefs.GetBool(master_preferences::kAltShortcutText, &alt_shortcut);
+        uint32 shortcut_options = ShellUtil::SHORTCUT_NO_OPTIONS;
+        if (create_all_shortcut)
+          shortcut_options |= ShellUtil::SHORTCUT_CREATE_ALWAYS;
+        if (alt_shortcut)
+          shortcut_options |= ShellUtil::SHORTCUT_ALTERNATE;
+        CreateOrUpdateChromeShortcuts(installer_state, setup_path,
+                                      new_version, result, *chrome_install,
+                                      shortcut_options);
+      }
+
       if (result == FIRST_INSTALL_SUCCESS) {
         installer_state.UpdateStage(installer::CONFIGURE_AUTO_LAUNCH);
 
         // Add auto-launch key if specified in master preferences.
         bool auto_launch_chrome = false;
         prefs.GetBool(
             installer::master_preferences::kAutoLaunchChrome,
             &auto_launch_chrome);
         if (auto_launch_chrome) {
           auto_launch_util::EnableForegroundStartAtLogin(
               ASCIIToUTF16(chrome::kInitialProfile),
               installer_state.target_path());
         }
       }
     }
 
     installer_state.UpdateStage(installer::REMOVING_OLD_VERSIONS);
 
     installer_state.RemoveOldVersionDirectories(
         new_version,
         existing_version.get(),
         install_temp_path);
   }
 
   return result;
 }
 
 }  // namespace installer