Introduce the ability to wait for the watcher process to initialize.

chrome/app/chrome_watcher_client_win.cc

Index: chrome/app/chrome_watcher_client_win.cc
diff --git a/chrome/app/chrome_watcher_client_win.cc b/chrome/app/chrome_watcher_client_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ef90f8443b1346359dd6802026aed62448f50283
--- /dev/null
+++ b/chrome/app/chrome_watcher_client_win.cc
@@ -0,0 +1,93 @@
+// Copyright 2015 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/app/chrome_watcher_client_win.h"
+
+#include <limits>
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "components/browser_watcher/watcher_client_win.h"
+
+ChromeWatcherClient::ChromeWatcherClient(
+    const CommandLineGenerator& command_line_generator)
+    : command_line_generator_(command_line_generator) {

[Sigurður Ásgeirsson, 2015/01/29 16:59:56]

wonky indent?

[erikwright (departed), 2015/01/30 20:39:57]

This is what clang format does.

+}
+
+ChromeWatcherClient::~ChromeWatcherClient() {
+}
+
+bool ChromeWatcherClient::LaunchWatcher() {
+  SECURITY_ATTRIBUTES on_initialized_event_attributes = {

[Sigurður Ásgeirsson, 2015/01/29 16:59:56]

nit: worth providing running commentary here. Inheritable handles are
uncommon...

[erikwright (departed), 2015/01/30 20:39:57]

Done.

+      sizeof(SECURITY_ATTRIBUTES),  // nLength
+      nullptr,                      // lpSecurityDescriptor
+      TRUE                          // bInheritHandle
+  };
+  on_initialized_event_.Set(
+      ::CreateEvent(&on_initialized_event_attributes, FALSE, FALSE, nullptr));
+  if (!on_initialized_event_.IsValid()) {
+    PLOG(ERROR) << "Failed to create an event.";
+    return false;
+  }
+  browser_watcher::WatcherClient watcher_client(
+      base::Bind(command_line_generator_, on_initialized_event_.Get()));
+  watcher_client.AddInheritedHandle(on_initialized_event_.Get());
+  watcher_client.LaunchWatcher();
+  process_ = watcher_client.process().Duplicate();
+  return process_.IsValid();
+}
+
+bool ChromeWatcherClient::WaitForInitialization() {

[Sigurður Ásgeirsson, 2015/01/29 16:59:56]

nit: this assumes single-threaded use. Maybe mention as much in the header - or
perhaps this doesn't warrant comment as it's the default for Chrome?

[erikwright (departed), 2015/01/30 20:39:57]

This is the default for Chrome.

+  if (!on_initialized_event_.IsValid() || !process_.IsValid()) {
+    LOG(ERROR)
+        << "WaitForInitialization called twice, after a failed LaunchWatcher "
+           "call, or without calling LaunchWatcher.";
+    return false;
+  }
+
+  HANDLE handles[] = {on_initialized_event_.Get(), process_.Handle()};
+  DWORD result = ::WaitForMultipleObjects(arraysize(handles), handles,
+                                          FALSE, INFINITE);
+
+  // We don't need this anymore.
+  on_initialized_event_.Close();
+
+  switch (result) {
+    case WAIT_OBJECT_0:
+      return true;
+    case WAIT_OBJECT_0 + 1:
+      LOG(ERROR) << "Chrome watcher process failed to launch.";
+      return false;
+    case WAIT_FAILED:
+      PLOG(ERROR)
+          << "Failure while waiting on Chrome watcher process launch.";
+      return false;
+    default:
+      NOTREACHED() << "Unexpected result while waiting on Chrome watcher "
+                      "process launch: " << result;
+      return false;
+  }
+}
+
+int ChromeWatcherClient::TerminateWatcher(int exit_code) {
+  // In case WaitForInitialization was never called.
+  on_initialized_event_.Close();
+
+  if (!process_.IsValid()) {
+    LOG(ERROR) << "TerminateWatcher called twice, after a failed LaunchWatcher "
+                  "call, or without calling LaunchWatcher.";
+    return std::numeric_limits<int>::max();
+  }
+
+  int actual_exit_code = 0;
+  bool result =
+      process_.WaitForExitWithTimeout(base::TimeDelta(), &actual_exit_code);
+  if (result)
+    return actual_exit_code;  // The watcher already exited.
+
+  process_.Terminate(exit_code);

[Sigurður Ásgeirsson, 2015/01/29 16:59:56]

interesting detail - I don't think this'll change the exit code of an already
exited process?

[erikwright (departed), 2015/01/30 20:39:57]

Correct. That's why we return the actual exit code afterwards.

The tests do rely on this to assert that the mock watcher process had (or had
not) already exited.

[Sigurður Ásgeirsson, 2015/02/02 16:59:20]

In that case the wait above is redundant - no?

[erikwright (departed), 2015/02/03 20:05:34]

Correct.

+  result = process_.WaitForExit(&actual_exit_code);
+  DCHECK(result);
+  return actual_exit_code;
+}