Add crash keys for the installer covering simple InstallerState fields.

chrome/installer/setup/installer_crash_keys.cc

+// 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/installer/setup/installer_crash_keys.h"
+
+#include "base/debug/leak_annotations.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/installer/setup/installer_crash_reporter_client.h"
+#include "chrome/installer/util/google_update_settings.h"
+#include "chrome/installer/util/installer_state.h"
+#include "components/crash/content/app/breakpad_win.h"
+#include "components/crash/content/app/crash_keys_win.h"
+#include "components/crash/core/common/crash_keys.h"
+
+namespace installer {
+
+namespace {
+
+// Crash Keys
+
+const char kDistributionType[] = "dist-type";
+const char kIsMultiInstall[] = "multi-install";
+const char kIsSystemLevel[] = "system-level";
+const char kOperation[] = "operation";
+const char kStateKey[] = "state-key";
+
+#if defined(COMPONENT_BUILD)
+// Installed via base::debug::SetCrashKeyReportingFunctions.
+void SetCrashKeyValue(const base::StringPiece& key,
+                      const base::StringPiece& value) {
+  DCHECK(breakpad::CrashKeysWin::keeper());
+  breakpad::CrashKeysWin::keeper()->SetCrashKeyValue(base::UTF8ToUTF16(key),
+                                                     base::UTF8ToUTF16(value));
+}
+
+// Installed via base::debug::SetCrashKeyReportingFunctions.
+void ClearCrashKey(const base::StringPiece& key) {
+  DCHECK(breakpad::CrashKeysWin::keeper());
+  breakpad::CrashKeysWin::keeper()->ClearCrashKeyValue(base::UTF8ToUTF16(key));
+}
+#endif  // COMPONENT_BUILD
+
+const char *DistributionTypeToString(BrowserDistribution::Type type) {
+  switch (type) {
+    case BrowserDistribution::CHROME_BROWSER:
+      return "chrome browser";
+    case BrowserDistribution::CHROME_FRAME:
+      return "chrome frame";
+    case BrowserDistribution::CHROME_BINARIES:
+      return "chrome binaries";
+    case BrowserDistribution::NUM_TYPES:
+      // Fall out of switch.
+      break;
+  }
+  NOTREACHED();
+  return "";
+}
+
+const char *OperationToString(InstallerState::Operation operation) {
+  switch (operation) {
+    case InstallerState::SINGLE_INSTALL_OR_UPDATE:
+      return "single-install-or-update";
+    case InstallerState::MULTI_INSTALL:
+      return "multi-install";
+    case InstallerState::MULTI_UPDATE:
+      return "multi-update";
+    case InstallerState::UNINSTALL:
+      return "uninstall";
+    case InstallerState::UNINITIALIZED:
+      // Fall out of switch.
+      break;
+  }
+  NOTREACHED();
+  return "";
+}
+
+const char *PackageTypeToString(InstallerState::PackageType type) {
+  // Returns a boolean string: is this a multi install?
+  switch (type) {
+    case InstallerState::SINGLE_PACKAGE:
+      return "false";
+    case InstallerState::MULTI_PACKAGE:
+      return "true";
+    case InstallerState::UNKNOWN_PACKAGE_TYPE:
+      // Fall out of switch.
+      break;
+  }
+  NOTREACHED();
+  return "";
+}
+
+const char *SystemLevelToString(InstallerState::Level type) {
+  // Returns a boolean string: is this a system level install?
+  switch (type) {
+    case InstallerState::USER_LEVEL:
+      return "false";
+    case InstallerState::SYSTEM_LEVEL:
+      return "true";
+    case InstallerState::UNKNOWN_LEVEL:
+      // Fall out of switch.
+      break;
+  }
+  NOTREACHED();
+  return "";
+}
+
+}  // namespace
+
+void ConfigureCrashReporting(const InstallerState& installer_state) {

[grt (UTC plus 2), 2015/11/26 19:24:45]

this does a lot more than just operate on/for crash keys, so this file isn't
obviously the natural place for it. wdyt of pulling lines 125-132 out of this
function and into InitCrashKeys() in this file? the rest of this function can
stay where it was in setup_main.cc with a call to installer::InitCrashKeys() at
the right place.

could this new InitCrashKeys also do all of the work in SetStartingCrashKeys?

[Joe Mason, 2015/11/26 19:47:02]

What about just renaming installer_crash_keys.* to installer_crash_reporting.*?
I think everything in this function is closely related enough to be worth
keeping together.
I prefer to keep SetStartingCrashKeys separate to give the freedom to change the
parameter list or move the invocation around as more crash keys are added.

[grt (UTC plus 2), 2015/11/26 19:50:35]

Ack.
Ack.

+  // This is inspired by work done in various parts of Chrome startup to connect
+  // to the crash service. Since the installer does not split its work between
+  // a stub .exe and a main .dll, crash reporting can be configured in one place
+  // right here.
+
+  // Create the crash client and install it (a la MainDllLoader::Launch).
+  InstallerCrashReporterClient *crash_client =
+      new InstallerCrashReporterClient(!installer_state.system_install());
+  ANNOTATE_LEAKING_OBJECT_PTR(crash_client);
+  crash_reporter::SetCrashReporterClient(crash_client);
+
+  breakpad::InitCrashReporter("Chrome Installer");
+
+  // Set up crash keys and the client id (a la child_process_logging::Init()).
+#if defined(COMPONENT_BUILD)
+  // breakpad::InitCrashReporter takes care of this for static builds but not
+  // component builds due to intricacies of chrome.exe and chrome.dll sharing a
+  // copy of base.dll in that case (for details, see the comment in
+  // components/crash/content/app/breakpad_win.cc).
+  crash_client->RegisterCrashKeys();
+  base::debug::SetCrashKeyReportingFunctions(&SetCrashKeyValue, &ClearCrashKey);
+#endif // COMPONENT_BUILD
+
+  scoped_ptr<metrics::ClientInfo> client_info =
+      GoogleUpdateSettings::LoadMetricsClientInfo();
+  if (client_info)
+    crash_client->SetCrashReporterClientIdFromGUID(client_info->client_id);
+  // TODO(grt): A lack of a client_id at this point generally means that Chrome
+  // has yet to have been launched and picked one. Consider creating it and
+  // setting it here for Chrome to use.
+}
+
+void RegisterCrashKeys(std::vector<base::debug::CrashKey>& keys) {

[grt (UTC plus 2), 2015/11/26 19:24:45]

out params must be passed by non-const pointer rather than non-const ref
(https://google.github.io/styleguide/cppguide.html#Reference_Arguments).

[Joe Mason, 2015/11/26 20:49:05]

Done.

+  const base::debug::CrashKey kFixedKeys[] = {
+    { crash_keys::kClientId, crash_keys::kSmallSize },
+    { kDistributionType, crash_keys::kSmallSize },
+    { kIsMultiInstall, crash_keys::kSmallSize },
+    { kIsSystemLevel, crash_keys::kSmallSize },
+    { kOperation, crash_keys::kSmallSize },
+
+    // This is a Windows registry key, which maxes out at 255 chars.
+    // (kMediumSize actually maxes out at 252 chars on Windows, but potentially
+    // truncating such a small amount is a fair tradeoff compared to using
+    // kLargeSize, which is wasteful.)
+    { kStateKey, crash_keys::kMediumSize },
+  };
+  keys.insert(keys.end(), std::begin(kFixedKeys), std::end(kFixedKeys));

[grt (UTC plus 2), 2015/11/26 19:24:45]

go c++11!

+}
+
+void SetStartingCrashKeys(const InstallerState& installer_state) {
+  base::debug::SetCrashKeyValue(kDistributionType, DistributionTypeToString(

[grt (UTC plus 2), 2015/11/26 19:24:45]

consider "using base::debug::SetCrashKeyValue;" at the top of this function if
that will reduce wrapping and make the function more readable below.

[Joe Mason, 2015/11/26 20:49:05]

Done.

+      installer_state.state_type()));
+  base::debug::SetCrashKeyValue(kOperation, OperationToString(
+      installer_state.operation()));
+  base::debug::SetCrashKeyValue(kIsMultiInstall, PackageTypeToString(

[grt (UTC plus 2), 2015/11/26 19:24:45]

nit: the following is easier to read:
installer_state.is_multi_install() ? "true" : "false"

[Joe Mason, 2015/11/26 19:47:02]

I agree, but I'm following the comment in InstallerState saying to deprecate
is_multi_install in favour of package_type.

[grt (UTC plus 2), 2015/11/26 19:50:35]

As the author of that comment, I authorize you to completely disregard it (and
even remove it). :-) 

[Joe Mason, 2015/11/26 20:49:05]

Done.

+      installer_state.package_type()));
+  base::debug::SetCrashKeyValue(kIsSystemLevel, SystemLevelToString(

[grt (UTC plus 2), 2015/11/26 19:24:45]

installer_state.system_install() ? "true" : "false"

[Joe Mason, 2015/11/26 20:49:05]

Done.

+      installer_state.level()));
+
+  const std::wstring state_key = installer_state.state_key();
+  if (!state_key.empty()) {

[grt (UTC plus 2), 2015/11/26 19:24:45]

formatting nit:
  if (!state_key.empty())
    base::debug::SetCrashKeyValue(kStateKey, base::UTF16ToUTF8(state_key));

[Joe Mason, 2015/11/26 20:49:05]

Done.

(You're talking about the extra line break, right? I'd prefer to keep the
braces.

The style guide says, "In general, curly braces are not required for single-line
statements, but they are allowed if you like them," and I'm reveling in no
longer having to follow the WebKit style guide that bans them absolutely.)

[grt (UTC plus 2), 2015/11/26 21:37:17]

By and large, Chromium does not use them for single-line conditionals like this,
and the installer in particular definitely does not (though I'm sure you'll find
exceptions if you look). The relevant style rule is "Be consistent with existing
code" (https://google.github.io/styleguide/cppguide.html#Goals), so please
remove them here. You can try using them elsewhere in Chromium, but it will be
at the whim of the reviewer. :-)

+    base::debug::SetCrashKeyValue(kStateKey,
+                                  base::UTF16ToUTF8(state_key));
+  }
+}
+
+}  // namespace installer