Upon execution of the App Host, ask Omaha to install the Chrome Binaries if they are not present on…

chrome/browser/extensions/app_host/binaries_installer.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 "chrome/browser/extensions/app_host/binaries_installer.h"
+
+#include "base/logging.h"
+#include "base/threading/platform_thread.h"
+#include "base/win/scoped_bstr.h"
+#include "base/win/scoped_comptr.h"
+#include "base/win/scoped_com_initializer.h"
+#include "chrome/browser/extensions/app_host/binaries_installer_internal.h"
+#include "google_update/google_update_idl.h"

[miket_OOO, 2012/08/23 23:51:01]

Please alphabetize.

[erikwright (departed), 2012/08/31 21:04:48]

Done.

+
+using base::win::ScopedBstr;
+using base::win::ScopedComPtr;
+
+namespace app_host {
+
+namespace {
+const int kInstallationPollingIntervalMs = 50;
+}  // namespace

[miket_OOO, 2012/08/23 23:51:01]

You can leave out the closing comment in short cases like this where it's
unlikely the reader will get lost.

+
+// Attempts to install the Chrome Binaries using the IGoogleUpdate3 interface.
+// Blocks until the installation process completes, without message pumping.
+HRESULT InstallBinaries() {
+  base::win::ScopedCOMInitializer initialize_com;
+
+  HRESULT hr = S_OK;
+  if (!initialize_com.succeeded()) {
+    LOG(ERROR) << "COM initialization failed";
+    hr = E_FAIL;
+  }

[miket_OOO, 2012/08/23 23:51:01]

Since you're not early-returning on failure, I expect to see unconditional work
done somewhere in this method, but there isn't. Why not just return hr in case
of failure? This is a bit of a style question, and if it feels better to you as
it is, it's not a big deal. It does read a little weird to me... waiting for the
other shoe to drop.

[erikwright (departed), 2012/08/31 21:04:48]

Other reviewer's (robertshield) preference to minimize return points for
readability. Since I tend towards the same, I'll let him win :)

+
+  ScopedComPtr<IGoogleUpdate3> update3;
+  if (SUCCEEDED(hr)) {
+    hr = internal::CreateGoogleUpdate3(update3.Receive());
+  }
+
+  ScopedBstr ap_value;
+  if (SUCCEEDED(hr)) {
+    hr = internal::SelectBinariesApValue(update3, ap_value.Receive());
+  }
+
+  ScopedComPtr<IAppBundle> app_bundle;
+  if (SUCCEEDED(hr)) {
+    hr = internal::CreateAppBundle(update3, app_bundle.Receive());
+  }
+
+  ScopedComPtr<IApp> app;
+  if (SUCCEEDED(hr)) {
+    hr = internal::CreateBinariesIApp(app_bundle, ap_value, app.Receive());
+  }
+
+  if (SUCCEEDED(hr)) {
+    hr = app_bundle->checkForUpdate();
+    if (FAILED(hr)) {
+      LOG(ERROR) << "Failed to initiate update check: " << hr;
+    }
+  }
+
+  if (SUCCEEDED(hr)) {
+    while (!internal::CheckIfDone(app_bundle, app, &hr)) {

[miket_OOO, 2012/08/23 23:51:01]

Is this guaranteed to return true eventually? If not, we should probably pick a
reasonable timeout instead of looping potentially forever.

[erikwright (departed), 2012/08/31 21:04:48]

Yes, the assumption is that Omaha has appropriate timeouts for the different
phases (check for update, download, install) and will transition to a failed
state when those are exceeded.

+      base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(
+          kInstallationPollingIntervalMs));
+    }
+  }
+
+  return hr;
+}
+
+}  // namespace app_host