Callback flow to register Chrome and update shortcuts after OS upgrade to Windows 8

chrome/installer/util/installation_validator.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.
 
 // Implementation of the installation validator.
 
 #include "chrome/installer/util/installation_validator.h"
 
 #include <algorithm>
 #include <set>

[gab, 2012/08/30 14:24:32]

#include <string>
since you (correctly) removed the include from the header, but std::string is
still used here.

[huangs, 2012/08/30 17:13:07]

Done.

 
 #include "base/logging.h"
 #include "base/version.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/installer/util/browser_distribution.h"
 #include "chrome/installer/util/helper.h"
 #include "chrome/installer/util/installation_state.h"
 
 namespace installer {
 
@@ skipping 213 matching lines @@
     LOG(ERROR) << "install-application command is not configured to send "
                << "pings.";
   }
 
   if (!command.is_web_accessible()) {
     *is_valid = false;
     LOG(ERROR) << "install-application command is not web accessible.";
   }
 }
 
+// Validates the "on-os-upgrade" Google Update internal command.
+void InstallationValidator::ValidateOnOsUpgradeCommand(
+    const ProductContext& ctx,
+    const AppCommand& command,
+    bool* is_valid) {
+  DCHECK(is_valid);
+
+  CommandLine the_command(CommandLine::FromString(command.command_line()));
+
+  ValidateSetupPath(ctx, the_command.GetProgram(), "on os upgrade", is_valid);
+
+  SwitchExpectations expected;
+

[gab, 2012/08/30 14:24:32]

Remove this line (i.e. declare variable + initialize it belongs together).

[huangs, 2012/08/30 17:13:07]

Done.

+  expected.push_back(std::make_pair(std::string(switches::kOnOsUpgrade), true));
+  expected.push_back(std::make_pair(std::string(switches::kSystemLevel),
+                                    ctx.system_install));
+  expected.push_back(std::make_pair(std::string(switches::kMultiInstall),
+                                    ctx.state.is_multi_install()));
+  const ProductState* product_state =

[gab, 2012/08/30 14:24:32]

Move this before "SwitchExpectations expected;" so that |expected| can be
initialized in one block without interruption.

[huangs, 2012/08/30 17:13:07]

Made unnecessary by new change.

+      ctx.machine_state.GetProductState(ctx.system_install,
+                                        BrowserDistribution::CHROME_BROWSER);
+  expected.push_back(std::make_pair(std::string(switches::kChrome),

[grt (UTC plus 2), 2012/08/30 04:15:25]

--chrome is expected to only be present if --multi-install

[huangs, 2012/08/30 17:13:07]

Done.  And also "if", so "if and only if" => Checking identical conditions.

+                                    product_state != NULL));
+
+  ValidateCommandExpectations(ctx, the_command, expected, "on os upgrade",
+                              is_valid);
+
+  if (!command.is_auto_run_on_os_upgrade()) {
+    *is_valid = false;
+    LOG(ERROR) << "On-os-upgrade command is not marked to run on OS upgrade.";
+  }
+
+  if (command.sends_pings()) {
+    *is_valid = false;
+    LOG(ERROR) << "On-os-upgrade command should not be able to send pings.";
+  }
+
+  if (command.is_web_accessible()) {
+    *is_valid = false;
+    LOG(ERROR) << "On-os-upgrade command should not be web accessible.";
+  }
+}
+
 // Validates the "quick-enable-cf" Google Update product command.
 void InstallationValidator::ValidateQuickEnableCfCommand(
     const ProductContext& ctx,
     const AppCommand& command,
     bool* is_valid) {
   DCHECK(is_valid);
 
   CommandLine the_command(CommandLine::FromString(command.command_line()));
 
   ValidateSetupPath(ctx, the_command.GetProgram(), "quick enable cf", is_valid);
@@ skipping 70 matching lines @@
     const CommandExpectations& expectations,
     bool* is_valid) {
   DCHECK(is_valid);
 
   CommandExpectations the_expectations(expectations);
 
   AppCommands::CommandMapRange cmd_iterators(
       ctx.state.commands().GetIterators());
   CommandExpectations::iterator expectation;
   for (; cmd_iterators.first != cmd_iterators.second; ++cmd_iterators.first) {
-    const std::wstring& cmd_id = cmd_iterators.first->first;
+    const string16& cmd_id = cmd_iterators.first->first;
     // Do we have an expectation for this command?
     expectation = the_expectations.find(cmd_id);
     if (expectation != the_expectations.end()) {
       (expectation->second)(ctx, cmd_iterators.first->second, is_valid);
       // Remove this command from the set of expectations since we found it.
       the_expectations.erase(expectation);
     } else {
       *is_valid = false;
       LOG(ERROR) << ctx.dist->GetAppShortCutName()
                  << " has an unexpected Google Update product command named \""
@@ skipping 136 matching lines @@
 
   // Chrome Frame must be multi-install if Chrome & App Host are not present.
   if (cf_state != NULL && app_host_state == NULL && chrome_state == NULL &&
       !cf_state->is_multi_install()) {
     *is_valid = false;
     LOG(ERROR) << "Chrome Binaries are present without Chrome nor App Host "
                << "yet Chrome Frame is not multi-install.";
   }
 
   ChromeBinariesRules binaries_rules;
-  ProductContext ctx = {
-    machine_state,
-    system_install,
-    BrowserDistribution::GetSpecificDistribution(
-        BrowserDistribution::CHROME_BINARIES),
-    binaries_state,
-    binaries_rules
-  };
+  ProductContext ctx(machine_state, system_install, binaries_state,
+                     binaries_rules);
 
   ValidateBinariesCommands(ctx, is_valid);
 
   ValidateUsageStats(ctx, is_valid);
 }
 
 // Validates the path to |setup_exe| for the product described by |ctx|.
 void InstallationValidator::ValidateSetupPath(const ProductContext& ctx,
                                               const FilePath& setup_exe,
                                               const char* purpose,
@@ skipping 175 matching lines @@
 void InstallationValidator::ValidateAppCommands(
     const ProductContext& ctx,
     bool* is_valid) {
   DCHECK(is_valid);
 
   CommandExpectations expectations;
 
   if (ctx.dist->GetType() == BrowserDistribution::CHROME_APP_HOST) {
     expectations[kCmdInstallApp] = &ValidateInstallAppCommand;
   }
+  if (ctx.dist->GetType() == BrowserDistribution::CHROME_BROWSER) {
+    expectations[kCmdOnOsUpgrade] = &ValidateOnOsUpgradeCommand;
+  }
 
   ValidateAppCommandExpectations(ctx, expectations, is_valid);
 }
 
 // Validates usagestats for the product or binaries in |ctx|.
 void InstallationValidator::ValidateUsageStats(const ProductContext& ctx,
                                                bool* is_valid) {
   DWORD usagestats = 0;
   if (ctx.state.GetUsageStats(&usagestats)) {
     if (!ctx.rules.UsageStatsAllowed(ctx.state)) {
       *is_valid = false;
       LOG(ERROR) << ctx.dist->GetAppShortCutName()
                  << " has a usagestats value (" << usagestats
                  << "), yet should not.";
     } else if (usagestats != 0 && usagestats != 1) {
       *is_valid = false;
       LOG(ERROR) << ctx.dist->GetAppShortCutName()
                  << " has an unsupported usagestats value (" << usagestats
                  << ").";
     }
   }
 }
 
 // Validates the product described in |product_state| according to |rules|.
-void InstallationValidator::ValidateProduct(
-    const InstallationState& machine_state,
-    bool system_install,
-    const ProductState& product_state,
-    const ProductRules& rules,
-    bool* is_valid) {
+void InstallationValidator::ValidateProduct(const ProductContext& ctx,
+                                            bool* is_valid) {
   DCHECK(is_valid);
-  ProductContext ctx = {
-    machine_state,
-    system_install,
-    BrowserDistribution::GetSpecificDistribution(rules.distribution_type()),
-    product_state,
-    rules
-  };
 
-  ValidateUninstallCommand(ctx, product_state.uninstall_command(),
+  ValidateUninstallCommand(ctx, ctx.state.uninstall_command(),
                            "Google Update uninstall command", is_valid);
 
   ValidateOldVersionValues(ctx, is_valid);
 
-  if (product_state.is_multi_install())
+  if (ctx.state.is_multi_install())
     ValidateMultiInstallProduct(ctx, is_valid);
 
   ValidateAppCommands(ctx, is_valid);
 
   ValidateUsageStats(ctx, is_valid);
 }
 
 // static
 bool InstallationValidator::ValidateInstallationTypeForState(
     const InstallationState& machine_state,
     bool system_level,
     InstallationType* type) {
   DCHECK(type);
   bool rock_on = true;
   *type = NO_PRODUCTS;
 
   // Does the system have any multi-installed products?
   const ProductState* multi_state =
       machine_state.GetProductState(system_level,
                                     BrowserDistribution::CHROME_BINARIES);
   if (multi_state != NULL)
     ValidateBinaries(machine_state, system_level, *multi_state, &rock_on);
 
   // Is Chrome installed?
   const ProductState* product_state =
       machine_state.GetProductState(system_level,
                                     BrowserDistribution::CHROME_BROWSER);
   if (product_state != NULL) {
     ChromeRules chrome_rules;
-    ValidateProduct(machine_state, system_level, *product_state,
-                    chrome_rules, &rock_on);
+    ProductContext ctx(machine_state, system_level, *product_state,

[grt (UTC plus 2), 2012/08/30 04:15:25]

this change appears to make ValidateProduct harder to use rather than easier. 
please explain.

[gab, 2012/08/30 14:24:32]

I agree, the goal of making a struct to pass in arguments is usually to make it
clear what each argument is (i.e. as the caller has to explicitly set them), but
now that you made a constructor for that struct it's just as opaque and I don't
see the benefit either.

[huangs, 2012/08/30 17:13:07]

Done.

+                       chrome_rules);
+    ValidateProduct(ctx, &rock_on);
     *type = static_cast<InstallationType>(
         *type | (product_state->is_multi_install() ?
                  ProductBits::CHROME_MULTI :
                  ProductBits::CHROME_SINGLE));
   }
 
   // Is Chrome Frame installed?
   product_state =
       machine_state.GetProductState(system_level,
                                     BrowserDistribution::CHROME_FRAME);
   if (product_state != NULL) {
     ChromeFrameRules chrome_frame_rules;
-    ValidateProduct(machine_state, system_level, *product_state,
-                    chrome_frame_rules, &rock_on);
+    ProductContext ctx(machine_state, system_level, *product_state,
+                       chrome_frame_rules);
+    ValidateProduct(ctx, &rock_on);
     int cf_bit = !product_state->is_multi_install() ?
         ProductBits::CHROME_FRAME_SINGLE :
         (product_state->uninstall_command().HasSwitch(
              switches::kChromeFrameReadyMode) ?
                  ProductBits::CHROME_FRAME_READY_MODE :
                  ProductBits::CHROME_FRAME_MULTI);
     *type = static_cast<InstallationType>(*type | cf_bit);
   }
 
   // Is Chrome App Host installed?
   product_state =
       machine_state.GetProductState(system_level,
                                     BrowserDistribution::CHROME_APP_HOST);
   if (product_state != NULL) {
     ChromeAppHostRules chrome_app_host_rules;
-    ValidateProduct(machine_state, system_level, *product_state,
-                    chrome_app_host_rules, &rock_on);
+    ProductContext ctx(machine_state, system_level, *product_state,
+                       chrome_app_host_rules);
+    ValidateProduct(ctx, &rock_on);
     *type = static_cast<InstallationType>(*type | ProductBits::CHROME_APP_HOST);
     if (system_level) {
       LOG(ERROR) << "Chrome App Host must not be installed at system level.";
       rock_on = false;
     }
     if (!product_state->is_multi_install()) {
       LOG(ERROR) << "Chrome App Host must always be multi-install.";
       rock_on = false;
     }
   }
@@ skipping 12 matching lines @@
                                                      InstallationType* type) {
   DCHECK(type);
   InstallationState machine_state;
 
   machine_state.Initialize();
 
   return ValidateInstallationTypeForState(machine_state, system_level, type);
 }
 
 }  // namespace installer