Adding App Launcher shortcuts on install.

chrome/installer/setup/uninstall.cc

Index: chrome/installer/setup/uninstall.cc
diff --git a/chrome/installer/setup/uninstall.cc b/chrome/installer/setup/uninstall.cc
index 2fb193b801d739f67c339aa1ae25dc8ecc1d9c93..1e4b59d6424d9ebf97da18428e9303e12a974b18 100644
--- a/chrome/installer/setup/uninstall.cc
+++ b/chrome/installer/setup/uninstall.cc
@@ -256,49 +256,56 @@ void CloseChromeFrameHelperProcess() {
   }
 }
 
-// This method deletes Chrome shortcut folder from Windows Start menu. It
-// 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
-// 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;
-
+// 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, chrome_exe, install_level, NULL)) {
+          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, chrome_exe, install_level,
+          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, chrome_exe, install_level,
+          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.  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;
+  }
+
+  // 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;
+
+  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(
@@ -307,10 +314,27 @@ void DeleteChromeShortcuts(const InstallerState& installer_state,
     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());
@@ -1009,6 +1033,7 @@ InstallStatus UninstallProduct(const InstallationState& original_state,
                                                                 chrome_exe));
 
   bool is_chrome = product.is_chrome();
+  bool is_app_host = product.is_chrome_app_host();
 
   VLOG(1) << "UninstallProduct: " << browser_dist->GetAppShortCutName();
 
@@ -1053,6 +1078,13 @@ InstallStatus UninstallProduct(const InstallationState& original_state,
     }
   }
 
+  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.