Browser watcher first installment.

components/browser_watcher/watcher_win.cc

+// Copyright (c) 2014 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/command_line.h"
+#include "base/process/kill.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
+#include "base/win/registry.h"
+#include "components/browser_watcher/watcher_win.h"

[erikwright (departed), 2014/11/17 18:12:30]

must be the first file included.

[Sigurður Ásgeirsson, 2014/11/17 21:27:12]

Done.

+
+namespace browser_watcher {
+
+namespace {
+
+base::string16 GetValueName(const base::Time creation_time,
+                            base::ProcessId pid) {
+  // Convert the PID and creation time to a string value unique to this
+  // process instance.
+  return base::StringPrintf(L"%d-%lld", pid, creation_time.ToInternalValue());
+}
+
+}  // namespace
+
+ExitCodeWatcher::ExitCodeWatcher(const base::char16* registry_path) :
+    registry_path_(registry_path),
+    process_(base::kNullProcessHandle),

[erikwright (departed), 2014/11/17 18:12:30]

I would assume this is unnecessary for this non-POD member.

[Sigurður Ásgeirsson, 2014/11/17 21:27:12]

Oops, leftover...

+    process_pid_(0) {
+}
+
+ExitCodeWatcher::~ExitCodeWatcher() {
+}
+
+bool ExitCodeWatcher::ParseArguments(const base::CommandLine& cmd_line) {
+  std::string process_handle_str =
+      cmd_line.GetSwitchValueASCII("parent-handle");

[erikwright (departed), 2014/11/17 18:12:30]

this string represents part of the API of this class and should be exposed as a
public static constant.

[Sigurður Ásgeirsson, 2014/11/17 21:27:12]

Done.

+  unsigned process_handle_uint = 0;
+  if (process_handle_str.empty() ||
+      !base::StringToUint(process_handle_str, &process_handle_uint)) {
+    LOG(ERROR) << "Missing or invalid parent-handle argument.";
+    return false;
+  }
+
+  HANDLE process_handle = reinterpret_cast<HANDLE>(process_handle_uint);
+  // Initial test of the handle, a zero PID indicates invalid handle, or not
+  // a process handle. In this case, bail immediately and avoid closing the
+  // handle.
+  DWORD process_pid = ::GetProcessId(process_handle);
+  if (process_pid == 0) {
+    LOG(ERROR) << "Invalid parent-handle, can't get parent PID.";
+    return false;
+  }
+
+  FILETIME creation_time = {};
+  FILETIME dummy = {};
+  if (!::GetProcessTimes(process_handle, &creation_time,
+                         &dummy, &dummy, &dummy)) {
+    LOG(ERROR) << "Invalid parent handle, can't get parent process times.";
+    return false;
+  }
+
+  // Success, take ownership of the process handle.
+  process_.Set(process_handle);
+  process_pid_ = process_pid;
+  process_creation_time_ = base::Time::FromFileTime(creation_time);
+
+  // Start by writing the value STILL_ACTIVE to registry, to allow detection
+  // of the case where the watcher itself is somehow terminated before it can
+  // write the process' actual exit code.
+  base::win::RegKey key(HKEY_CURRENT_USER,

[erikwright (departed), 2014/11/17 18:12:30]

I'm in the habit of factoring out the writing of state into little helper
classes or free functions. This gives a few advantages:

(1) I can quickly test the reading and writing code against itself without
having to trigger all of the logic that causes a given state to be written.
(2) It makes the business logic more readable. Instead of a bunch of
domain-specific code interrupted by 9 lines of registry access (and 7 more in
WaitForExit()), I can read a single "state_->WriteStillActive()" or similar.
(3) Your unit tests, which verify the functionality of this test by reading the
state it has written, currently also reproduce the registry access logic. They
could be simplified by using your data access helpers.

In summary, your current design will have four classes with registry access code
(client, server, and a unit-test for each). By factoring it out into symmetrical
helpers you can reduce this to two focused classes and leave your business logic
more readable.

[Sigurður Ásgeirsson, 2014/11/17 21:27:12]

IMHO there isn't enough code here to warrant a whole lot of abstraction, that
being said, I've refactored the two writes here into a common function.

[erikwright (departed), 2014/11/17 22:07:04]

It's not abstraction (no indirection is introduced, no virtual methods, the
helper is not injected) but factoring out.

I don't mind not doing it for now, but I'd like to make sure that there is not
duplication (with risk of divergence) between the write code here and any code
that may also write the same data in the same place and same format from the
unit test of the corresponding client (and likewise the read code in the unit
test of this class and the implementation of the client).

Since the client and its unit test don't exist yet it's a moot point.

+                        registry_path_.c_str(),
+                        KEY_WRITE);
+  base::string16 value_name(GetValueName(process_creation_time_, process_pid_));
+
+  ULONG result = key.WriteValue(value_name.c_str(), STILL_ACTIVE);
+  if (result != ERROR_SUCCESS) {
+    LOG(ERROR) << "Unable to write to registry, error " << result;

[erikwright (departed), 2014/11/17 18:12:30]

#include "base/logging.h"

[Sigurður Ásgeirsson, 2014/11/17 21:27:12]

Done.

+    return false;
+  }
+
+  return true;
+}
+
+void ExitCodeWatcher::WaitForExit() {
+  int exit_code = 0;
+  if (!base::WaitForExitCode(process_.Get(), &exit_code)) {
+    LOG(ERROR) << "Failed to wait for process.";
+    return;
+  }
+  // WaitForExitCode closes the handle on success.
+  process_.Take();
+
+  base::win::RegKey key(HKEY_CURRENT_USER,
+                        registry_path_.c_str(),
+                        KEY_WRITE);
+  base::string16 value_name(GetValueName(process_creation_time_, process_pid_));
+
+  ULONG result = key.WriteValue(value_name.c_str(), exit_code);
+  if (result != ERROR_SUCCESS)
+    LOG(ERROR) << "Unable to write exit code, error: " << result;
+}
+
+}  // namespace browser_watcher