If Chrome Binaries version > App Host version, then update App Host.

chrome/browser/extensions/app_host/app_host_main.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 "base/at_exit.h"
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/process_util.h"
+#include "base/version.h"
+#include "chrome/browser/extensions/app_host/app_host_upgrade.h"
 #include "chrome/browser/extensions/app_host/binaries_installer.h"
 #include "chrome/installer/launcher_support/chrome_launcher_support.h"
 
 int main(int /* argc */, char* /* argv[] */) {
+  using namespace app_host;
+  using namespace chrome_launcher_support;
   base::AtExitManager exit_manager;
 
   // Initialize the commandline singleton from the environment.
   CommandLine::Init(0, NULL);
 
-  FilePath chrome_exe(chrome_launcher_support::GetAnyChromePath());
+  FilePath chrome_exe(GetAnyChromePath());
 
   if (chrome_exe.empty()) {
     LOG(INFO) << "No Chrome executable could be found. Let's install it.";
     HRESULT hr = app_host::InstallBinaries();
     if (FAILED(hr)) {
       LOG(ERROR) << "Failed to install the Chrome Binaries. Error: " << hr;
       return 1;
     } else {
-      chrome_exe = chrome_launcher_support::GetAnyChromePath();
+      chrome_exe = GetAnyChromePath();
       if (chrome_exe.empty()) {
         LOG(ERROR) << "Failed to find the Chrome Binaries despite a "
                    << "'successful' installation.";
         return 1;
       }
     }
   }
 
   CommandLine chrome_exe_command_line(chrome_exe);
   chrome_exe_command_line.AppendArguments(
       *CommandLine::ForCurrentProcess(), false);
 
-  if (base::LaunchProcess(chrome_exe_command_line,
-                          base::LaunchOptions(),
-                          NULL)) {
+  // Launch Chrome before checking for updates, for faster user experience.
+  bool launch_result = base::LaunchProcess(chrome_exe_command_line,
+                                           base::LaunchOptions(),
+                                           NULL);
+  if (launch_result) {

[grt (UTC plus 2), 2012/09/13 13:14:42]

no braces for single-line conditionals like this

[huangs, 2012/09/13 18:46:00]

Done.

     LOG(INFO) << "Delegated to Chrome executable at " << chrome_exe.value();
-    return 0;
   } else {
     LOG(INFO) << "Failed to launch Chrome executable at " << chrome_exe.value();
-    return 1;
   }
+
+  // If system-level Chrome Binary is installed, and if its version is

[grt (UTC plus 2), 2012/09/13 13:14:42]

i think this block of code is big enough and isolated enough that it would fit
nicely in its own function.

[huangs, 2012/09/13 18:46:00]

Okay, moving into app_host_upgrade.cc.  Also hiding constants and function that
don't need to be exposed any more (per discussion).

+  // newer than App Host's, then upgrade App Host by calling setup.exe.

[grt (UTC plus 2), 2012/09/13 13:14:42]

please change "upgrade" in all cases to "update" .the latter implies that the
new version is better in some way, which is subjective) also, "update" is
consistent with Google Update and as much of the codebase as i've been able to
influence.

[erikwright (departed), 2012/09/13 13:48:26]

By "the latter" grt@ meant "the former".

[huangs, 2012/09/13 18:46:00]

Makes sense.  Done!

+  Version chrome_binaries_version;
+  if (GetAppVersionForInstallationLevel(kMultiInstallAppGuid,
+                                        true,  // System-level.
+                                        &chrome_binaries_version)) {
+    Version app_host_version;
+    if (GetAppVersionForInstallationLevel(kChromeAppHostAppGuid,

[grt (UTC plus 2), 2012/09/13 13:14:42]

if this is used to get the version of the currently running app host exe, a much
more reliable way is to introspect into the version resource of the running
process.  our use of the registry for this has historically been problematic. 
search for CreateFileVersionInfoForCurrentModule in client_util.cc for an
example of this.

[huangs, 2012/09/13 18:46:00]

Done.

+                                          false,  // User-level.
+                                          &app_host_version)) {
+      if (app_host_version.IsOlderThan(chrome_binaries_version.GetString())) {
+        LOG(INFO) << "App Host version (" << app_host_version.GetString() <<

[grt (UTC plus 2), 2012/09/13 13:14:43]

wrap use of the streaming operator by putting the "<<" down on the next line and
align it with the one on this line.

[erikwright (departed), 2012/09/13 13:48:26]

Correct. Note this is an exception to my previous comment about preferring to
wrap after operators.

[huangs, 2012/09/13 18:46:00]

Done.

+          ") is older than Chrome Binaries version (" <<
+          chrome_binaries_version.GetString() <<
+          "), so running setup.exe to upgrade";

[grt (UTC plus 2), 2012/09/13 13:14:43]

general comment: make the log message as concise as possible since it
contributes to the size of the binary and download.  i suggest something simple
and clear like: "Updating App Host from foo to bar".

[huangs, 2012/09/13 18:46:00]

Done.

+        if (!UpgradeAppHost(true)) {  // Use system-level setup.exe

[grt (UTC plus 2), 2012/09/13 13:14:43]

no braces

[huangs, 2012/09/13 18:46:00]

Done.

+          LOG(ERROR) << "Failed to upgrade App Host";
+        }
+      }
+    } else {
+      LOG(INFO) << "Failed to get App Host version";

[grt (UTC plus 2), 2012/09/13 13:14:43]

why isn't this an ERROR?

[huangs, 2012/09/13 18:46:00]

Done.

+    }
+  }  // Else there is no system-level Chrome, so do nothing.
+
+  return launch_result ? 0 : 1;

[grt (UTC plus 2), 2012/09/13 13:14:43]

optional:
return !launch_result; is equivalent and tighter (integral promotion dictates
that a bool is promoted to 0 or 1).

[huangs, 2012/09/13 18:46:00]

Done.

 }