Adding App Launcher shortcuts on install.

chrome/installer/setup/install.cc

Index: chrome/installer/setup/install.cc
diff --git a/chrome/installer/setup/install.cc b/chrome/installer/setup/install.cc
index d9e2c18564e50eb41355fa45f528ba8e4ece30f5..52c5b24fef6234f308b5f65f7c6bdbd586210f39 100644
--- a/chrome/installer/setup/install.cc
+++ b/chrome/installer/setup/install.cc
@@ -23,6 +23,7 @@
 #include "base/win/shortcut.h"
 #include "base/win/windows_version.h"
 #include "chrome/common/chrome_constants.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/installer/setup/install_worker.h"
 #include "chrome/installer/setup/setup_constants.h"
 #include "chrome/installer/util/auto_launch_util.h"
@@ -284,6 +285,23 @@ void CleanupLegacyShortcuts(const InstallerState& installer_state,
   }
 }
 
+// Returns the appropriate shortcut operations for App Launcher,
+// based on state of installation and master preference.

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

s/master preference/master_preferences

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

Done.  Also s/master preference/master_preferences/g

+installer::InstallShortcutOperation GetAppLauncherShortcutOperation(
+    const InstallationState& original_state,
+    const InstallerState& installer_state) {
+  const installer::ProductState* original_app_host_state =
+      original_state.GetProductState(installer_state.system_install(),
+                                     BrowserDistribution::CHROME_APP_HOST);
+  bool app_launcher_exists = original_app_host_state &&
+      CommandLine(original_app_host_state->uninstall_command()).

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

nit: '.' on the next line to make it more obvious it's continued from the line
above.

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

Done.

+          HasSwitch(installer::switches::kChromeAppLauncher);

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

I feel like checking the presence of a flag on the previous uninstall string
might be a bit risky, is there a better way to determine the current state of
the app_launcher install?

Also, I am not super-familiar with original_state; would this still work if
registration of the uninstall string happened on the system before the shortcuts
are installed? (don't have access to the code right now to double-check... i.e.
order of registration/shortcut creation should be independent).

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

I followed grt@'s recommendation regarding using uninstall string.  As for
original_state, this is passed down from wWinMain() before install occurred. 
The intent IS to check system status before the current install.exe run.

[erikwright (departed), 2012/11/01 20:02:20]

This is standard practice. The uninstall string is the one-true source of
installation state.
The original state is populated before the installation begins. It is a snapshot
of the state of the system prior to installation commencing.

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

Ok, sgtm.
Perfect, that's what I expected from the name, but just wanted to double-check.

+  if (!app_launcher_exists)
+    return installer::INSTALL_SHORTCUT_CREATE_ALL;
+
+  return installer::INSTALL_SHORTCUT_REPLACE_EXISTING;
+}
+
 }  // end namespace
 
 namespace installer {
@@ -356,13 +374,13 @@ bool CreateVisualElementsManifest(const FilePath& src_path,
 }
 
 void CreateOrUpdateShortcuts(
-    const FilePath& chrome_exe,
+    const FilePath& target,
     const Product& product,
     const MasterPreferences& prefs,
     InstallShortcutLevel install_level,
     InstallShortcutOperation install_operation) {
   // TODO(tommi): Change this function to use WorkItemList.
-  DCHECK(product.is_chrome());
+  DCHECK(product.is_chrome() || product.is_chrome_app_host());
 
   // Extract shortcut preferences from |prefs|.
   bool do_not_create_desktop_shortcut = false;
@@ -376,9 +394,6 @@ void CreateOrUpdateShortcuts(
                 &alternate_desktop_shortcut);
 
   BrowserDistribution* dist = product.distribution();
-  // Shortcuts are always installed per-user unless specified.
-  ShellUtil::ShellChange shortcut_level = (install_level == ALL_USERS ?
-      ShellUtil::SYSTEM_LEVEL : ShellUtil::CURRENT_USER);
 
   // The default operation on update is to overwrite shortcuts with the
   // currently desired properties, but do so only for shortcuts that still
@@ -397,10 +412,24 @@ void CreateOrUpdateShortcuts(
       break;
   }
 
+  // Shortcuts are always installed per-user unless specified.

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

any reason for this moving from above?

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

shortcut_level exists solely to initialize base_properties.  I find it bizarre
that the switch block lies between.

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

Ah right, it used to be used for other stuff that has been removed, makes sense
to bring by |base_properties| now.

+  ShellUtil::ShellChange shortcut_level = (install_level == ALL_USERS ?
+      ShellUtil::SYSTEM_LEVEL : ShellUtil::CURRENT_USER);
+
   // |base_properties|: The basic properties to set on every shortcut installed
   // (to be refined on a per-shortcut basis).
   ShellUtil::ChromeShortcutProperties base_properties(shortcut_level);
-  base_properties.set_chrome_exe(chrome_exe);
+  base_properties.set_chrome_exe(target);

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

The reason the ShellUtil::ChromeShortcutProperties were made chrome only and not
general (they were at first) is that it simplified giving defaults to non-set
properties (see ShellUtil::GetShortcutPropertiesFromChromeShortcutProperties()
or something like that).

This will not work as the values you are not setting will default to Chrome's
(i.e. app_id, icon, etc.).

What you probably want to do is as we had discussed and you had suggested:
make the mapping method in ShellUtil take a set of default properties and then
use those to set the non-set properties, then you can call that method with
different default properties based on the situation down in ShellUtil.

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

I went through the flow, and found app_id is assigned by:
GetShortcutPropertiesFromChromeShortcutProperties()
  => ShellUtil::GetBrowserModelId()
  => ChromeAppHostDistribution::GetBaseAppId()
to retrieve L"ChromeAppList".
Then it adds some user-specific registry suffix stuff, WHICH I ASSUMED is okay
for both Chrome and App Host.  Is this okay?

[erikwright (departed), 2012/11/01 20:02:20]

We agreed F2F on the following:

We will rename ShellUtil::ChromeShortcutProperties to
ShellUtil::ShortcutProperties.

We will rename ShellUtil::ChromeShortcutOperation to
ShellUtil::ShortcutOperation.

We will prefix all values of the ShellUtil::ShortcutOperation enum with SHELL_.

We will expose a function from shell util:

static ShellUtil::ShortcutProperties
GetDefaultChromeShortcutProperties(BrowserDistribution* dist,
ShellUtil::ShortcutOperation operation).

We will modify GetShortcutPropertiesFromChromeShortcutProperties to no longer
provide default values.

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

sgtm, though there are more methods than this that will need rename in ShellUtil
(e.g. RemoveChromeShortcuts, CreateOrUpdateChromeShortcuts, etc.)

+
+  if (product.is_chrome_app_host()) {

[grt (UTC plus 2), 2012/11/01 01:50:04]

consider if it makes sense to add to Product and ProductOperation so
conditionals like this aren't needed.

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

Eventually App Host and App Launcher will be the same thing.  In
https://codereview.chromium.org/11267023/ , the decision is to minimize
divergence between App Host and App Launcher.  For the case of Product, I
thought is_chrome_app_launcher() would be redundant.  This DCHECK can be removed
once we have unification, but keeping it is also harmless.

[erikwright (departed), 2012/11/01 20:02:20]

Based on the above response to gab@, I suppose we could define:

ShellUtil::ShortcutProperties Product::GetShortcutProperties(FilePath
target_path, ShellUtil::ShortcutOperation)

and a corresponding method on ProductOperations.

The Chrome one would delegate to the static ShellUtil function mentioned
earlier.

SGTY, grt?

[erikwright (departed), 2012/11/01 20:43:36]

What grt meant was all the stuff inside the if () {} block.

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

sgtm

[grt (UTC plus 2), 2012/11/02 19:55:30]

sure

+    DCHECK(product.HasOption(kOptionAppHostIsLauncher));
+    // Should consult AppListController::GetAppListCommandLine().

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

Can you provide some context to this comment?

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

In AppListController::GetAppListCommandLine(), a similar command line is formed,
but more stuff is added.  Updating comment.

+    CommandLine app_host_args(CommandLine::NO_PROGRAM);
+    // This is cleaner than appending strings, but as a result we'll get

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

An extra space in the command line does not merit a comment.

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

The extra space is really annoying.  Can I add an extra static routine in
CommandLine to e.g. render "--xxx" from "xxx", and then use it?

[erikwright (departed), 2012/11/01 20:43:36]

Does it require code changes somewhere? If not, why is it annoying?

If so, just trim whitespace from the result of GetCommandLineString.

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

This is probably just a tiny bug in GetCommandLineString() which can be fixed in
another CL, I also don't think it deserves a comment here, this paradigm is used
all over the place already.

+    // 2 spaces: "app_host.exe   --show-app-list".
+    app_host_args.AppendSwitch(::switches::kShowAppList);

[huangs, 2012/10/31 22:09:03]

Note the insertion of 2 spaces!

+    base_properties.set_arguments(app_host_args.GetCommandLineString());

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

Things like this that are meant as a property for all of your shortcuts will
probably end up in the set of default properties discussed in the comment above
(unless this doesn't make sense in all app_launcher cases...?).

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

The command line to run the App Launcher is

app_host.exe --show-app-list

I should get benwells@ to confirm.

+  }
 
   if (!do_not_create_desktop_shortcut ||
       shortcut_operation == ShellUtil::SHORTCUT_REPLACE_EXISTING) {
@@ -527,8 +556,24 @@ InstallStatus InstallOrUpdateProduct(
     if (result == FIRST_INSTALL_SUCCESS && !prefs_path.empty())
       CopyPreferenceFileForFirstRun(installer_state, prefs_path);
 
-    // Currently this only creates shortcuts for Chrome, but for other products
-    // we might want to create shortcuts.
+    // Creates shortcuts for App Launcher.
+    const Product* app_launcher_product =
+        installer_state.FindProduct(BrowserDistribution::CHROME_APP_HOST);
+    if (app_launcher_product &&
+        app_launcher_product->HasOption(kOptionAppHostIsLauncher)) {
+      installer_state.UpdateStage(installer::CREATING_SHORTCUTS);

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

Should we introduce a new stage constant? I think each constant is only mapped
to 1 section of the code thus far, but I could be wrong..

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

Yeah, it's weird to have UpdateState((installer::CREATING_SHORTCUTS) to appear
twice.  Rearranging the logic to uniquify this.

[erikwright (departed), 2012/11/01 20:02:20]

Let's just set this on line 558. It will encompass setting of shortcuts for both
products, and if we don't actually need any shortcuts we will just transition in
and transition out without doing any work. SGTY?

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

SGTM, although it might be cleaner if we split the shortcut creation logic on
its own.

i.e.

UpdateStage(installer::CREATING_SHORTCUTS)
if (is app host) {
  shortcut app host...
} else if (is_chrome) {
  shortcut for chrome...
}

if (is_chrome) {
  UpdateStage(installer::REGISTERING_CHROME)
  make default... (currently done before switching the stage but it makes more
sense as part of the registration stage imo)
  other chrome registration...
  ...
}
...

+
+      const FilePath app_host_exe(
+          installer_state.target_path().Append(kChromeAppHostExe));
+      InstallShortcutOperation app_launcher_shortcut_operation =
+          GetAppLauncherShortcutOperation(original_state, installer_state);
+
+      // Always install per-user shortcuts for App Launcher.
+      CreateOrUpdateShortcuts(app_host_exe, *app_launcher_product, prefs,
+                              CURRENT_USER, app_launcher_shortcut_operation);
+    }
+
+    // Creates shortcuts for Chrome.
     const Product* chrome_install =
         installer_state.FindProduct(BrowserDistribution::CHROME_BROWSER);
     if (chrome_install) {