Refactor SetClientID such that metrics rather than crash backs up the client id

in Google Update settings.

Consequentially, the backed up client_id now keeps its dashes and crash_keys
strips them at runtime rather than when backing it up
(https://codereview.chromium.org/372473004/ will add support for stripped
client_id backups for some time).

Also rename a lot of methods involved in setting the client id; having all of
them named "SetClientID" makes this series of calls very hard to follow!

BUG=391338
TBR=thestig

Review URL: https://codereview.chromium.org/365133005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@282093 0039d316-1c4b-4281-b951-d872f2087c98

20 files changed, 76 insertions, 62 deletions

diff --git a/chrome/app/chrome_breakpad_client.cc b/chrome/app/chrome_breakpad_client.cc
index 09cf2fa..38abe83 100644
--- a/chrome/app/chrome_breakpad_client.cc
+++ b/chrome/app/chrome_breakpad_client.cc
@@ -80,8 +80,9 @@ ChromeBreakpadClient::ChromeBreakpadClient() {}
 
 ChromeBreakpadClient::~ChromeBreakpadClient() {}
 
-void ChromeBreakpadClient::SetClientID(const std::string& client_id) {
-  crash_keys::SetClientID(client_id);
+void ChromeBreakpadClient::SetBreakpadClientIdFromGUID(
+    const std::string& client_guid) {
+  crash_keys::SetCrashClientIdFromGUID(client_guid);
 }
 
 #if defined(OS_WIN)
diff --git a/chrome/app/chrome_breakpad_client.h b/chrome/app/chrome_breakpad_client.h
index 489e8fe..7b750bd 100644
--- a/chrome/app/chrome_breakpad_client.h
+++ b/chrome/app/chrome_breakpad_client.h
@@ -17,7 +17,8 @@ class ChromeBreakpadClient : public breakpad::BreakpadClient {
   virtual ~ChromeBreakpadClient();
 
   // breakpad::BreakpadClient implementation.
-  virtual void SetClientID(const std::string& client_id) OVERRIDE;
+  virtual void SetBreakpadClientIdFromGUID(
+      const std::string& client_guid) OVERRIDE;
 #if defined(OS_WIN)
   virtual bool GetAlternativeCrashDumpLocation(base::FilePath* crash_dir)
       OVERRIDE;
diff --git a/chrome/app/client_util.cc b/chrome/app/client_util.cc
index e8c0900..ac6dbfc 100644
--- a/chrome/app/client_util.cc
+++ b/chrome/app/client_util.cc
@@ -76,7 +76,7 @@ void GetPreReadPopulationAndGroup(double* population, double* group) {
   // By default we use the metrics id for the user as stable pseudo-random
   // input to a hash.
   std::string metrics_id;
-  GoogleUpdateSettings::GetMetricsId(&metrics_id);
+  GoogleUpdateSettings::LoadMetricsClientId(&metrics_id);
 
   // If this user has not metrics id, we fall back to a purely random value
   // per browser session.
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
index 14511cc..068ddde 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -1470,7 +1470,7 @@ void ChromeContentBrowserClient::AppendExtraCommandLineSwitches(
 #if defined(OS_POSIX)
   if (breakpad::IsCrashReporterEnabled()) {
     std::string enable_crash_reporter;
-    GoogleUpdateSettings::GetMetricsId(&enable_crash_reporter);
+    GoogleUpdateSettings::LoadMetricsClientId(&enable_crash_reporter);
     command_line->AppendSwitchASCII(switches::kEnableCrashReporter,
                                     enable_crash_reporter);
   }
diff --git a/chrome/browser/google/google_update_settings_posix.cc b/chrome/browser/google/google_update_settings_posix.cc
index 67e63cc..99dc0cc 100644
--- a/chrome/browser/google/google_update_settings_posix.cc
+++ b/chrome/browser/google/google_update_settings_posix.cc
@@ -14,8 +14,9 @@
 
 namespace {
 
-base::LazyInstance<std::string>::Leaky g_posix_guid = LAZY_INSTANCE_INITIALIZER;
-base::LazyInstance<base::Lock>::Leaky g_posix_guid_lock =
+base::LazyInstance<std::string>::Leaky g_posix_client_id =
+    LAZY_INSTANCE_INITIALIZER;
+base::LazyInstance<base::Lock>::Leaky g_posix_client_id_lock =
     LAZY_INSTANCE_INITIALIZER;
 
 // File name used in the user data dir to indicate consent.
@@ -28,11 +29,15 @@ bool GoogleUpdateSettings::GetCollectStatsConsent() {
   base::FilePath consent_file;
   PathService::Get(chrome::DIR_USER_DATA, &consent_file);
   consent_file = consent_file.Append(kConsentToSendStats);
+
+  if (!base::DirectoryExists(consent_file.DirName()))
+    return false;
+
   std::string tmp_guid;
   bool consented = base::ReadFileToString(consent_file, &tmp_guid);
   if (consented) {
-    base::AutoLock lock(g_posix_guid_lock.Get());
-    g_posix_guid.Get().assign(tmp_guid);
+    base::AutoLock lock(g_posix_client_id_lock.Get());
+    g_posix_client_id.Get().assign(tmp_guid);
   }
   return consented;
 }
@@ -44,44 +49,40 @@ bool GoogleUpdateSettings::SetCollectStatsConsent(bool consented) {
   if (!base::DirectoryExists(consent_dir))
     return false;
 
-  base::AutoLock lock(g_posix_guid_lock.Get());
+  base::AutoLock lock(g_posix_client_id_lock.Get());
 
   base::FilePath consent_file = consent_dir.AppendASCII(kConsentToSendStats);
   if (consented) {
     if ((!base::PathExists(consent_file)) ||
-        (base::PathExists(consent_file) && !g_posix_guid.Get().empty())) {
-      const char* c_str = g_posix_guid.Get().c_str();
-      int size = g_posix_guid.Get().size();
+        (base::PathExists(consent_file) && !g_posix_client_id.Get().empty())) {
+      const char* c_str = g_posix_client_id.Get().c_str();
+      int size = g_posix_client_id.Get().size();
       return base::WriteFile(consent_file, c_str, size) == size;
     }
   } else {
-    g_posix_guid.Get().clear();
+    g_posix_client_id.Get().clear();
     return base::DeleteFile(consent_file, false);
   }
   return true;
 }
 
 // static
-bool GoogleUpdateSettings::GetMetricsId(std::string* metrics_id) {
-  base::AutoLock lock(g_posix_guid_lock.Get());
-  *metrics_id = g_posix_guid.Get();
+bool GoogleUpdateSettings::LoadMetricsClientId(std::string* metrics_id) {
+  base::AutoLock lock(g_posix_client_id_lock.Get());
+  *metrics_id = g_posix_client_id.Get();
   return true;
 }
 
 // static
-bool GoogleUpdateSettings::SetMetricsId(const std::string& client_id) {
+bool GoogleUpdateSettings::StoreMetricsClientId(const std::string& client_id) {
   // Make sure that user has consented to send crashes.
-  base::FilePath consent_dir;
-  PathService::Get(chrome::DIR_USER_DATA, &consent_dir);
-  if (!base::DirectoryExists(consent_dir) ||
-      !GoogleUpdateSettings::GetCollectStatsConsent()) {
+  if (!GoogleUpdateSettings::GetCollectStatsConsent())
     return false;
-  }
 
   {
     // Since user has consented, write the metrics id to the file.
-    base::AutoLock lock(g_posix_guid_lock.Get());
-    g_posix_guid.Get() = client_id;
+    base::AutoLock lock(g_posix_client_id_lock.Get());
+    g_posix_client_id.Get() = client_id;
   }
   return GoogleUpdateSettings::SetCollectStatsConsent(true);
 }
diff --git a/chrome/browser/metrics/chrome_metrics_service_client.cc b/chrome/browser/metrics/chrome_metrics_service_client.cc
index 6a2c51b..f10cc15 100644
--- a/chrome/browser/metrics/chrome_metrics_service_client.cc
+++ b/chrome/browser/metrics/chrome_metrics_service_client.cc
@@ -35,6 +35,7 @@
 #include "chrome/common/crash_keys.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/render_messages.h"
+#include "chrome/installer/util/google_update_settings.h"
 #include "components/metrics/metrics_service.h"
 #include "components/metrics/net/net_metrics_log_uploader.h"
 #include "content/public/browser/browser_thread.h"
@@ -161,8 +162,12 @@ void ChromeMetricsServiceClient::RegisterPrefs(PrefRegistrySimple* registry) {
 #endif  // defined(ENABLE_PLUGINS)
 }
 
-void ChromeMetricsServiceClient::SetClientID(const std::string& client_id) {
-  crash_keys::SetClientID(client_id);
+void ChromeMetricsServiceClient::SetMetricsClientId(
+    const std::string& client_id) {
+  crash_keys::SetCrashClientIdFromGUID(client_id);
+
+  // Store a backup of the client id in Google Update settings.
+  GoogleUpdateSettings::StoreMetricsClientId(client_id);
 }
 
 bool ChromeMetricsServiceClient::IsOffTheRecordSessionActive() {
diff --git a/chrome/browser/metrics/chrome_metrics_service_client.h b/chrome/browser/metrics/chrome_metrics_service_client.h
index 03f9c83..12015f7 100644
--- a/chrome/browser/metrics/chrome_metrics_service_client.h
+++ b/chrome/browser/metrics/chrome_metrics_service_client.h
@@ -52,7 +52,7 @@ class ChromeMetricsServiceClient
   static void RegisterPrefs(PrefRegistrySimple* registry);
 
   // metrics::MetricsServiceClient:
-  virtual void SetClientID(const std::string& client_id) OVERRIDE;
+  virtual void SetMetricsClientId(const std::string& client_id) OVERRIDE;
   virtual bool IsOffTheRecordSessionActive() OVERRIDE;
   virtual std::string GetApplicationLocale() OVERRIDE;
   virtual bool GetBrand(std::string* brand_code) OVERRIDE;
diff --git a/chrome/common/child_process_logging_win.cc b/chrome/common/child_process_logging_win.cc
index 1befb33..b849734 100644
--- a/chrome/common/child_process_logging_win.cc
+++ b/chrome/common/child_process_logging_win.cc
@@ -65,12 +65,13 @@ void Init() {
   base::debug::SetCrashKeyReportingFunctions(
       &SetCrashKeyValueTrampoline, &ClearCrashKeyValueTrampoline);
 
-  // This would be handled by BreakpadClient::SetClientID(), but because of the
-  // aforementioned issue, crash keys aren't ready yet at the time of Breakpad
-  // initialization.
-  std::string client_id;
-  if (GoogleUpdateSettings::GetMetricsId(&client_id))
-    base::debug::SetCrashKeyValue(crash_keys::kClientID, client_id);
+  // This would be handled by BreakpadClient::SetCrashClientIdFromGUID(), but
+  // because of the aforementioned issue, crash keys aren't ready yet at the
+  // time of Breakpad initialization, load the client id backed up in Google
+  // Update settings instead.
+  std::string client_guid;
+  if (GoogleUpdateSettings::LoadMetricsClientId(&client_guid))
+    crash_keys::SetCrashClientIdFromGUID(client_guid);
 }
 
 }  // namespace child_process_logging
diff --git a/chrome/common/crash_keys.cc b/chrome/common/crash_keys.cc
index edb6cac..84a83ef 100644
--- a/chrome/common/crash_keys.cc
+++ b/chrome/common/crash_keys.cc
@@ -11,7 +11,6 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
-#include "chrome/installer/util/google_update_settings.h"
 
 #if defined(OS_MACOSX)
 #include "breakpad/src/common/simple_string_dictionary.h"
@@ -56,7 +55,7 @@ COMPILE_ASSERT(kMediumSize <= kSingleChunkLength,
                mac_has_medium_size_crash_key_chunks);
 #endif
 
-const char kClientID[] = "guid";
+const char kClientId[] = "guid";
 
 const char kChannel[] = "channel";
 
@@ -119,7 +118,7 @@ size_t RegisterChromeCrashKeys() {
   // The following keys may be chunked by the underlying crash logging system,
   // but ultimately constitute a single key-value pair.
   base::debug::CrashKey fixed_keys[] = {
-    { kClientID, kSmallSize },
+    { kClientId, kSmallSize },
     { kChannel, kSmallSize },
     { kActiveURL, kLargeSize },
     { kNumSwitches, kSmallSize },
@@ -225,15 +224,14 @@ size_t RegisterChromeCrashKeys() {
                                     kSingleChunkLength);
 }
 
-void SetClientID(const std::string& client_id) {
-  std::string guid(client_id);
+void SetCrashClientIdFromGUID(const std::string& client_guid) {
+  std::string stripped_guid(client_guid);
   // Remove all instance of '-' char from the GUID. So BCD-WXY becomes BCDWXY.
-  ReplaceSubstringsAfterOffset(&guid, 0, "-", "");
-  if (guid.empty())
+  ReplaceSubstringsAfterOffset(&stripped_guid, 0, "-", "");
+  if (stripped_guid.empty())
     return;
 
-  base::debug::SetCrashKeyValue(kClientID, guid);
-  GoogleUpdateSettings::SetMetricsId(guid);
+  base::debug::SetCrashKeyValue(kClientId, stripped_guid);
 }
 
 static bool IsBoringSwitch(const std::string& flag) {
diff --git a/chrome/common/crash_keys.h b/chrome/common/crash_keys.h
index dd378fb..e38880b 100644
--- a/chrome/common/crash_keys.h
+++ b/chrome/common/crash_keys.h
@@ -21,8 +21,11 @@ namespace crash_keys {
 // reporting server. Returns the size of the union of all keys.
 size_t RegisterChromeCrashKeys();
 
-// Sets the GUID by which this crash reporting client can be identified.
-void SetClientID(const std::string& client_id);
+// Sets the ID (based on |client_guid| which may either be a full GUID or a
+// GUID that was already stripped from its dashes -- in either cases this method
+// will strip remaining dashes before setting the crash key) by which this crash
+// reporting client can be identified.
+void SetCrashClientIdFromGUID(const std::string& client_guid);
 
 // Sets the kSwitch and kNumSwitches keys based on the given |command_line|.
 void SetSwitchesFromCommandLine(const base::CommandLine* command_line);
diff --git a/chrome/installer/util/google_update_settings.cc b/chrome/installer/util/google_update_settings.cc
index 81e33ed..4d53eca 100644
--- a/chrome/installer/util/google_update_settings.cc
+++ b/chrome/installer/util/google_update_settings.cc
@@ -296,14 +296,14 @@ bool GoogleUpdateSettings::SetCollectStatsConsentAtLevel(bool system_install,
   return (result == ERROR_SUCCESS);
 }
 
-bool GoogleUpdateSettings::GetMetricsId(std::string* metrics_id) {
+bool GoogleUpdateSettings::LoadMetricsClientId(std::string* metrics_id) {
   base::string16 metrics_id16;
   bool rv = ReadGoogleUpdateStrKey(google_update::kRegMetricsId, &metrics_id16);
   *metrics_id = base::UTF16ToUTF8(metrics_id16);
   return rv;
 }
 
-bool GoogleUpdateSettings::SetMetricsId(const std::string& metrics_id) {
+bool GoogleUpdateSettings::StoreMetricsClientId(const std::string& metrics_id) {
   base::string16 metrics_id16 = base::UTF8ToUTF16(metrics_id);
   return WriteGoogleUpdateStrKey(google_update::kRegMetricsId, metrics_id16);
 }
diff --git a/chrome/installer/util/google_update_settings.h b/chrome/installer/util/google_update_settings.h
index b7bf612..a8e43c4 100644
--- a/chrome/installer/util/google_update_settings.h
+++ b/chrome/installer/util/google_update_settings.h
@@ -86,12 +86,12 @@ class GoogleUpdateSettings {
                                             bool consented);
 #endif
 
-  // Returns the metrics id set in the registry (that can be used in crash
-  // reports). If none found, returns empty string.
-  static bool GetMetricsId(std::string* metrics_id);
+  // Returns the metrics client id backed up in the registry. If none found,
+  // returns empty string.
+  static bool LoadMetricsClientId(std::string* metrics_id);
 
-  // Sets the metrics id to be used in crash reports.
-  static bool SetMetricsId(const std::string& metrics_id);
+  // Stores a backup of the metrics client id in the registry.
+  static bool StoreMetricsClientId(const std::string& metrics_id);
 
   // Sets the machine-wide EULA consented flag required on OEM installs.
   // Returns false if the setting could not be recorded.
diff --git a/components/breakpad/app/breakpad_client.cc b/components/breakpad/app/breakpad_client.cc
index 84931ea..5c763ca 100644
--- a/components/breakpad/app/breakpad_client.cc
+++ b/components/breakpad/app/breakpad_client.cc
@@ -27,7 +27,8 @@ BreakpadClient* GetBreakpadClient() {
 BreakpadClient::BreakpadClient() {}
 BreakpadClient::~BreakpadClient() {}
 
-void BreakpadClient::SetClientID(const std::string& client_id) {
+void BreakpadClient::SetBreakpadClientIdFromGUID(
+    const std::string& client_guid) {
 }
 
 #if defined(OS_WIN)
diff --git a/components/breakpad/app/breakpad_client.h b/components/breakpad/app/breakpad_client.h
index fb3b205..7ade184c 100644
--- a/components/breakpad/app/breakpad_client.h
+++ b/components/breakpad/app/breakpad_client.h
@@ -46,7 +46,9 @@ class BreakpadClient {
 
   // Sets the Breakpad client ID, which is a unique identifier for the client
   // that is sending crash reports. After it is set, it should not be changed.
-  virtual void SetClientID(const std::string& client_id);
+  // |client_guid| may either be a full GUID or a GUID that was already stripped
+  // from its dashes.
+  virtual void SetBreakpadClientIdFromGUID(const std::string& client_guid);
 
 #if defined(OS_WIN)
   // Returns true if an alternative location to store the minidump files was
diff --git a/components/breakpad/app/breakpad_linux.cc b/components/breakpad/app/breakpad_linux.cc
index fe9388c..df9d588 100644
--- a/components/breakpad/app/breakpad_linux.cc
+++ b/components/breakpad/app/breakpad_linux.cc
@@ -206,7 +206,7 @@ void SetClientIdFromCommandLine(const CommandLine& command_line) {
   // Get the guid from the command line switch.
   std::string switch_value =
       command_line.GetSwitchValueASCII(switches::kEnableCrashReporter);
-  GetBreakpadClient()->SetClientID(switch_value);
+  GetBreakpadClient()->SetBreakpadClientIdFromGUID(switch_value);
 }
 
 // MIME substrings.
diff --git a/components/breakpad/app/breakpad_mac.mm b/components/breakpad/app/breakpad_mac.mm
index d46b0c4..6cb5504 100644
--- a/components/breakpad/app/breakpad_mac.mm
+++ b/components/breakpad/app/breakpad_mac.mm
@@ -238,9 +238,9 @@ void InitCrashReporter(const std::string& process_type) {
 
   if (!is_browser) {
     // Get the guid from the command line switch.
-    std::string guid =
+    std::string client_guid =
         command_line->GetSwitchValueASCII(switches::kEnableCrashReporter);
-    GetBreakpadClient()->SetClientID(guid);
+    GetBreakpadClient()->SetBreakpadClientIdFromGUID(client_guid);
   }
 
   logging::SetLogMessageHandler(&FatalMessageHandler);
diff --git a/components/metrics/metrics_service.cc b/components/metrics/metrics_service.cc
index b9d8dbb..797ec70 100644
--- a/components/metrics/metrics_service.cc
+++ b/components/metrics/metrics_service.cc
@@ -404,7 +404,7 @@ void MetricsService::EnableRecording() {
   recording_active_ = true;
 
   state_manager_->ForceClientIdCreation();
-  client_->SetClientID(state_manager_->client_id());
+  client_->SetMetricsClientId(state_manager_->client_id());
   if (!log_manager_.current_log())
     OpenNewLog();
 
diff --git a/components/metrics/metrics_service_client.h b/components/metrics/metrics_service_client.h
index 1fcfb6e..d8e2c15 100644
--- a/components/metrics/metrics_service_client.h
+++ b/components/metrics/metrics_service_client.h
@@ -22,9 +22,9 @@ class MetricsServiceClient {
  public:
   virtual ~MetricsServiceClient() {}
 
-  // Register the client id with other services (e.g. crash reporting), called
+  // Registers the client id with other services (e.g. crash reporting), called
   // when metrics recording gets enabled.
-  virtual void SetClientID(const std::string& client_id) = 0;
+  virtual void SetMetricsClientId(const std::string& client_id) = 0;
 
   // Whether there's an "off the record" (aka "Incognito") session active.
   virtual bool IsOffTheRecordSessionActive() = 0;
diff --git a/components/metrics/test_metrics_service_client.cc b/components/metrics/test_metrics_service_client.cc
index 4e1a3c3..e5e9778 100644
--- a/components/metrics/test_metrics_service_client.cc
+++ b/components/metrics/test_metrics_service_client.cc
@@ -19,7 +19,8 @@ TestMetricsServiceClient::TestMetricsServiceClient()
 TestMetricsServiceClient::~TestMetricsServiceClient() {
 }
 
-void TestMetricsServiceClient::SetClientID(const std::string& client_id) {
+void TestMetricsServiceClient::SetMetricsClientId(
+    const std::string& client_id) {
   client_id_ = client_id;
 }
 
diff --git a/components/metrics/test_metrics_service_client.h b/components/metrics/test_metrics_service_client.h
index 0b7bdef..edbee2f 100644
--- a/components/metrics/test_metrics_service_client.h
+++ b/components/metrics/test_metrics_service_client.h
@@ -21,7 +21,7 @@ class TestMetricsServiceClient : public MetricsServiceClient {
   virtual ~TestMetricsServiceClient();
 
   // MetricsServiceClient:
-  virtual void SetClientID(const std::string& client_id) OVERRIDE;
+  virtual void SetMetricsClientId(const std::string& client_id) OVERRIDE;
   virtual bool IsOffTheRecordSessionActive() OVERRIDE;
   virtual std::string GetApplicationLocale() OVERRIDE;
   virtual bool GetBrand(std::string* brand_code) OVERRIDE;