Adding App Launcher shortcuts on install.

chrome/installer/setup/uninstall.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 the methods useful for uninstalling Chrome.
 
 #include "chrome/installer/setup/uninstall.h"
 
 #include <windows.h>
 
@@ skipping 238 matching lines @@
     }
   }
 
   if (kill) {
     VLOG(1) << installer::kChromeFrameHelperExe << " hung.  Killing.";
     base::CleanupProcesses(installer::kChromeFrameHelperExe, base::TimeDelta(),
                            content::RESULT_CODE_HUNG, NULL);
   }
 }
 
+// This method deletes shortcut from Windows Start menu, for the given
+// |install_level|.  Either the standard desktop shortcut or the alternate
+// desktop shortcut are present.  We don't know which one is present, so we
+// delete both.  Finally, we delete the quick lauch shortcut.

[gab, 2012/11/01 04:46:19]

This is overly wordy, details about why we do which shortcut can go in the
implementation if you feel it's necessary (it's actually already documented
below).

I feel something like this is sufficient:
Deletes shortcuts at |install_level| from Start menu, Desktop, and Quick Launch.
Only shortcuts pointing to |target| will be removed.

[huangs, 2012/11/01 19:20:38]

Done.

+void DeleteShortcutsCommon(ShellUtil::ShellChange install_level,
+                           BrowserDistribution* dist,
+                           const string16& target) {
+  VLOG(1) << "Deleting Desktop shortcut.";
+  if (!ShellUtil::RemoveChromeShortcut(
+          ShellUtil::SHORTCUT_DESKTOP, dist, target, install_level, NULL)) {
+    LOG(WARNING) << "Failed to delete Desktop shortcut.";
+  }
+  // Also try to delete the alternate desktop shortcut. It is not sufficient
+  // to do so upon failure of the above call as ERROR_FILE_NOT_FOUND on
+  // delete is considered success.
+  if (!ShellUtil::RemoveChromeShortcut(
+          ShellUtil::SHORTCUT_DESKTOP, dist, target, install_level,
+          &dist->GetAlternateApplicationName())) {
+    LOG(WARNING) << "Failed to delete alternate Desktop shortcut.";
+  }
+
+  VLOG(1) << "Deleting Quick Launch shortcut.";
+  if (!ShellUtil::RemoveChromeShortcut(
+          ShellUtil::SHORTCUT_QUICK_LAUNCH, dist, target, install_level,
+          NULL)) {
+    LOG(WARNING) << "Failed to delete Quick Launch shortcut.";
+  }
+}
+
+
 // This method deletes Chrome shortcut folder from Windows Start menu. It

[gab, 2012/11/01 04:46:19]

Can you also make this cleaner while you're at it :)?

// Deletes Chrome shortcuts as per DeleteShortcutsCommon() and removes 
// secondary tiles on the Start Screen (Win8+).

[huangs, 2012/11/01 19:20:38]

Done.

 // checks system_uninstall to see if the shortcut is in all users start menu
-// or current user start menu.
-// We try to remove the standard desktop shortcut but if that fails we try
-// to remove the alternate desktop shortcut. Only one of them should be
-// present in a given install but at this point we don't know which one.
-// We remove all start screen secondary tiles by removing the folder Windows
+// or current user start menu.  After deleting individual shortcuts, we then
+// remove all start screen secondary tiles by removing the folder Windows
 // uses to store this installation's tiles.
 void DeleteChromeShortcuts(const InstallerState& installer_state,
                            const Product& product,
                            const string16& chrome_exe) {
   if (!product.is_chrome()) {
     VLOG(1) << __FUNCTION__ " called for non-CHROME distribution";
     return;
   }
 
-  BrowserDistribution* dist = product.distribution();
-
   // The per-user shortcut for this user, if present on a system-level install,
   // has already been deleted in chrome_browser_main_win.cc::DoUninstallTasks().
   ShellUtil::ShellChange install_level = installer_state.system_install() ?
       ShellUtil::SYSTEM_LEVEL : ShellUtil::CURRENT_USER;
 
-  VLOG(1) << "Deleting Desktop shortcut.";
-  if (!ShellUtil::RemoveChromeShortcut(
-          ShellUtil::SHORTCUT_DESKTOP, dist, chrome_exe, install_level, NULL)) {
-    LOG(WARNING) << "Failed to delete Desktop shortcut.";
-  }
-  // Also try to delete the alternate desktop shortcut. It is not sufficient
-  // to do so upon failure of the above call as ERROR_FILE_NOT_FOUND on
-  // delete is considered success.
-  if (!ShellUtil::RemoveChromeShortcut(
-          ShellUtil::SHORTCUT_DESKTOP, dist, chrome_exe, install_level,
-          &dist->GetAlternateApplicationName())) {
-    LOG(WARNING) << "Failed to delete alternate Desktop shortcut.";
-  }
-
-  VLOG(1) << "Deleting Quick Launch shortcut.";
-  if (!ShellUtil::RemoveChromeShortcut(
-          ShellUtil::SHORTCUT_QUICK_LAUNCH, dist, chrome_exe, install_level,
-          NULL)) {
-    LOG(WARNING) << "Failed to delete Quick Launch shortcut.";
-  }
+  BrowserDistribution* dist = product.distribution();

[erikwright (departed), 2012/11/01 01:21:50]

For a cleaner diff, move the declaration/assignment of 'dist' to its original
location.

[huangs, 2012/11/01 19:20:38]

Done.

+  DeleteShortcutsCommon(install_level, dist, chrome_exe);

[erikwright (departed), 2012/11/01 01:21:50]

Leave a log statement in DeleteChromeShortcuts like "Deleting Chrome shortcuts"
so that the generic log statements from DeleteShortcutsCommon can be attributed
to Chrome vs. Launcher.

OR put a log statement in DeleteShortcutsCommon saying "Deleting shortcuts to "
<< target

[huangs, 2012/11/01 19:20:38]

Using method #1, in case there are more product-specific stuff.

 
   VLOG(1) << "Deleting Start Menu shortcuts.";

[gab, 2012/11/01 04:46:19]

This wasn't moved to the Common method above as the comment on it says...

[huangs, 2012/11/01 19:20:38]

Moving all these into common, along with
ShellUtil::RemoveChromeStartScreenShortcuts().
Also changed comments.

[gab, 2012/11/02 04:19:58]

Hmmm  ShellUtil::RemoveChromeStartScreenShortcuts() is for secondary tiles will
the app launcher use any secondary tiles?

I'm also adding ShellUtil::RemoveChromeTaskbarShortcuts() in
http://codereview.chromium.org/11368040/. This should be moved to the Common
section when it lands and you merge.

FYI, all of these methods made common should no longer have "Chrome" in them.

   if (!ShellUtil::RemoveChromeShortcut(
           ShellUtil::SHORTCUT_START_MENU, dist, chrome_exe, install_level,
           NULL)) {
     LOG(WARNING) << "Failed to delete Start Menu shortcuts.";
   }
 
-  ShellUtil::RemoveChromeStartScreenShortcuts(product.distribution(),
-                                              chrome_exe);
+  ShellUtil::RemoveChromeStartScreenShortcuts(dist, chrome_exe);
 }
 
+// This method deletes App Host shortcuts.

[gab, 2012/11/01 04:46:19]

// Deletes App Host shortcuts.

[huangs, 2012/11/01 19:20:38]

Done (same style as DeleteChromeShortcuts()).

+void DeleteAppHostShortcuts(const InstallerState& installer_state,
+                            const Product& product,
+                            const string16& app_host_exe) {
+  if (!product.is_chrome_app_host()) {
+    VLOG(1) << __FUNCTION__ " called for non-APP HOST distribution";
+    return;
+  }
+
+  // The per-user shortcut for this user, if present on a system-level install,
+  // has already been deleted in chrome_browser_main_win.cc::DoUninstallTasks().

[gab, 2012/11/01 04:46:19]

This comment doesn't apply to the app_host (it was copied from the method
above).

[huangs, 2012/11/01 19:20:38]

Oops.  Deleted!

+  ShellUtil::ShellChange install_level = installer_state.system_install() ?

[gab, 2012/11/01 04:46:19]

Aren't your shortcuts always user-level? At least this is what install.cc does.

[huangs, 2012/11/01 19:20:38]

For now it is, but we want to future-proof.  If setup.exe is run with
--system-level with app-host or app-launcher, then failure should occur earlier
on.

+      ShellUtil::SYSTEM_LEVEL : ShellUtil::CURRENT_USER;
+
+  DeleteShortcutsCommon(install_level, product.distribution(), app_host_exe);
+}
+
+
 bool ScheduleParentAndGrandparentForDeletion(const FilePath& path) {
   FilePath parent_dir = path.DirName();
   bool ret = ScheduleFileSystemEntityForDeletion(parent_dir.value().c_str());
   if (!ret) {
     LOG(ERROR) << "Failed to schedule parent dir for deletion: "
                << parent_dir.value();
   } else {
     FilePath grandparent_dir(parent_dir.DirName());
     ret = ScheduleFileSystemEntityForDeletion(grandparent_dir.value().c_str());
     if (!ret) {
@@ skipping 678 matching lines @@
                                const CommandLine& cmd_line) {
   InstallStatus status = installer::UNINSTALL_CONFIRMED;
   BrowserDistribution* browser_dist = product.distribution();
   const string16 chrome_exe(
       installer_state.target_path().Append(installer::kChromeExe).value());
 
   const string16 suffix(ShellUtil::GetCurrentInstallationSuffix(browser_dist,
                                                                 chrome_exe));
 
   bool is_chrome = product.is_chrome();
+  bool is_app_host = product.is_chrome_app_host();
 
   VLOG(1) << "UninstallProduct: " << browser_dist->GetAppShortCutName();
 
   if (force_uninstall) {
     // Since --force-uninstall command line option is used, we are going to
     // do silent uninstall. Try to close all running Chrome instances.
     // NOTE: We don't do this for Chrome Frame.
     if (is_chrome)
       CloseAllChromeProcesses();
   } else if (is_chrome) {
@@ skipping 24 matching lines @@
       new_cmd.AppendSwitch(installer::switches::kRemoveChromeRegistration);
       if (!suffix.empty()) {
         new_cmd.AppendSwitchNative(
             installer::switches::kRegisterChromeBrowserSuffix, suffix);
       }
       DWORD exit_code = installer::UNKNOWN_STATUS;
       InstallUtil::ExecuteExeAsAdmin(new_cmd, &exit_code);
     }
   }
 
+  if (is_app_host) {

[gab, 2012/11/01 04:46:19]

|is_app_host| can be inlined here, no need for a variable at the top imo.

[huangs, 2012/11/01 19:20:38]

Done.  The motivation was code symmetry, since is_chrome is also defined.  But I
guess this doesn't impact readability.

+    const string16 app_host_exe(installer_state.target_path().
+                                Append(installer::kChromeAppHostExe).value());

[gab, 2012/11/01 04:46:19]

Bring '.' down to this line and indent 4 more spaces (probably doesn't fit)

Or bring the whole thing down to this line, indented 4 spaces (I prefer that).

[huangs, 2012/11/01 19:20:38]

The whole thing won't fit.  Trying with keeping installer_state upstairs.

+    // First delete shortcuts from Start->Programs, Desktop & Quick Launch.

[gab, 2012/11/01 04:46:19]

-First (this is the only thing you do...)

s/Start->Programs/Start menu (this is how it's called everywhere else).
s/&/, and

[huangs, 2012/11/01 19:20:38]

Done, also changed the comment that I copied this from.

+    DeleteAppHostShortcuts(installer_state, product, app_host_exe);
+  }
+
   // Chrome is not in use so lets uninstall Chrome by deleting various files
   // and registry entries. Here we will just make best effort and keep going
   // in case of errors.
   if (is_chrome) {
     ClearRlzProductState();
     // Delete the key that delegate_execute might make.
     if (base::win::GetVersion() >= base::win::VERSION_WIN8) {
       InstallUtil::DeleteRegistryKey(HKEY_CURRENT_USER,
                                      chrome::kMetroRegistryPath);
     }
@@ skipping 199 matching lines @@
 
   // Try and delete the preserved local state once the post-install
   // operations are complete.
   if (!backup_state_file.empty())
     file_util::Delete(backup_state_file, false);
 
   return ret;
 }
 
 }  // namespace installer