/C++ readability/ - Make Chromoting Host report crashes to Breakpad (Windows only). The user must e…

remoting/host/breakpad_win.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 "remoting/host/breakpad.h"
+
+#include <windows.h>
+
+#include "base/string16.h"
+#include "base/stringprintf.h"
+#include "base/win/registry.h"
+#include "remoting/host/constants.h"
+
+namespace {
+
+// The following strings are used to construct the registry key names where
+// the user's consent to collect crash dumps is recorded.

[Peter Kasting, 2012/06/14 20:16:27]

Nit: "...where we record whether the user has consented to crash dump
collection."?

[alexeypa (please no reviews), 2012/06/15 18:45:49]

Done.

+const wchar_t kOmahaClientStateKeyFormat[] = L"Google\\Update\\%ls\\%ls";
+const wchar_t kOmahaClientState[] = L"ClientState";
+const wchar_t kOmahaClientStateMedium[] = L"ClientStateMedium";
+const wchar_t kOmahaUsagestatsValue[] = L"usagestats";
+
+}  // namespace
+
+namespace remoting {
+
+bool IsCrashReportingEnabled() {
+  // The user's consent to collect crash dumps is recored as the "usagestats"
+  // value in the ClientState or ClientStateMedium key. Probe
+  // the ClientStateMedium key first.
+  string16 client_state = StringPrintf(kOmahaClientStateKeyFormat,

[Peter Kasting, 2012/06/14 20:16:27]

|client_state| should be a std::wstring given the types used by StringPrintf()
and RegKey::Open() (and you can replace the string16.h include with <string>).

Nit: Consider factoring this block out into a helper that takes a const
wchar_t[] (the key name), so you can simply call it twice from this function.

[alexeypa (please no reviews), 2012/06/15 18:45:49]

Done.

+                                       kOmahaClientStateMedium,
+                                       remoting::kHostOmahaAppid);
+  base::win::RegKey key;
+  LONG result = key.Open(HKEY_LOCAL_MACHINE, client_state.c_str(), KEY_READ);
+  if (result == ERROR_SUCCESS) {
+    DWORD value = 0;
+    result = key.ReadValueDW(kOmahaUsagestatsValue, &value);
+    if (result == ERROR_SUCCESS) {

[Peter Kasting, 2012/06/14 20:16:27]

Nit: If you like, you may omit {} on one-line conditional bodies (though either
way is fine).

[alexeypa (please no reviews), 2012/06/15 18:45:49]

I tend to keep {} even for one-liners because they outline the boundaries of the
condition statement better.

[Peter Kasting, 2012/06/15 19:02:42]

That's fine.  Just make sure you're consistent with code around you.  The
majority of Chrome tends to omit these, so others might be surprised.  Trying to
be consistent at least across major divisions (e.g. all of remoting/) is
generally a good rule.

+      return value != 0;
+    }
+  }
+
+  client_state = StringPrintf(kOmahaClientStateKeyFormat,
+                              kOmahaClientState,
+                              remoting::kHostOmahaAppid);
+  result = key.Open(HKEY_LOCAL_MACHINE, client_state.c_str(), KEY_READ);
+  if (result == ERROR_SUCCESS) {
+    DWORD value = 0;
+    result = key.ReadValueDW(kOmahaUsagestatsValue, &value);
+    if (result == ERROR_SUCCESS) {
+      return value != 0;
+    }
+  }
+
+  // Do not collect anything unless the user has explicitly allowed it.
+  return false;
+}
+
+}  // namespace remoting