Move ChromeOS hardware class init out of MetricsService.

This CL moves the initialization of the hardware class on ChromeOS out of
MetricsService and into ChromeOSMetricsProvider via ChromeMetricsServiceClient.
MetricsService now pass |OnInitTaskGotHardwareClass()| as the callback to
ChromeMetricsServiceClient::StartGatheringMetrics().
ChromeMetricsServiceClient::StartGatheringMetrics() posts a task to the FILE
thread calling ChromeMetricsServiceClient::InitTaskGetHardwareClass(). The
latter function calls into ChromeOSMetricsProvider when on ChromeOS and
directly calls the callback passed to it on other platforms.

A followup CL will move the rest of the embedder-specific initial metrics
gathering flow out of MetricsService.

BUG=375776

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

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

chrome/browser/metrics/chrome_metrics_service_client.cc

@@ -18,6 +18,7 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/google/google_util.h"
 #include "chrome/browser/memory_details.h"
+#include "chrome/browser/metrics/extensions_metrics_provider.h"
 #include "chrome/browser/metrics/metrics_service.h"
 #include "chrome/browser/ui/browser_otr_state.h"
 #include "chrome/common/chrome_constants.h"
@@ -25,6 +26,7 @@
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/crash_keys.h"
 #include "chrome/common/render_messages.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/histogram_fetcher.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
@@ -33,6 +35,10 @@
 #include "chrome/browser/service_process/service_process_control.h"
 #endif
 
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/metrics/chromeos_metrics_provider.h"
+#endif
+
 #if defined(OS_WIN)
 #include <windows.h>
 #include "base/win/registry.h"
@@ -86,7 +92,9 @@ class MetricsMemoryDetails : public MemoryDetails {
 
 ChromeMetricsServiceClient::ChromeMetricsServiceClient(
     metrics::MetricsStateManager* state_manager)
-    : waiting_for_collect_final_metrics_step_(false),
+    : metrics_state_manager_(state_manager),
+      chromeos_metrics_provider_(NULL),
+      waiting_for_collect_final_metrics_step_(false),
       num_async_histogram_fetches_in_progress_(0),
       weak_ptr_factory_(this) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -110,11 +118,29 @@ scoped_ptr<ChromeMetricsServiceClient> ChromeMetricsServiceClient::Create(
   // receives pointers to fully constructed objects.
   scoped_ptr<ChromeMetricsServiceClient> client(
       new ChromeMetricsServiceClient(state_manager));
-  client->metrics_service_.reset(
-      new MetricsService(state_manager, client.get(), local_state));
+  client->Initialize();
+
   return client.Pass();
 }
 
+void ChromeMetricsServiceClient::Initialize() {
+  metrics_service_.reset(new MetricsService(
+      metrics_state_manager_, this, g_browser_process->local_state()));
+
+  // Register metrics providers.
+  metrics_service_->RegisterMetricsProvider(
+      scoped_ptr<metrics::MetricsProvider>(
+          new ExtensionsMetricsProvider(metrics_state_manager_)));
+
+#if defined(OS_CHROMEOS)
+  ChromeOSMetricsProvider* chromeos_metrics_provider =
+      new ChromeOSMetricsProvider;
+  chromeos_metrics_provider_ = chromeos_metrics_provider;
+  metrics_service_->RegisterMetricsProvider(
+      scoped_ptr<metrics::MetricsProvider>(chromeos_metrics_provider));
+#endif
+}
+
 void ChromeMetricsServiceClient::SetClientID(const std::string& client_id) {
   crash_keys::SetClientID(client_id);
 }
@@ -311,8 +337,13 @@ void ChromeMetricsServiceClient::Observe(
 
 void ChromeMetricsServiceClient::StartGatheringMetrics(
     const base::Closure& done_callback) {
-  // TODO(blundell): Move metrics gathering tasks from MetricsService to here.
+// TODO(blundell): Move all metrics gathering tasks from MetricsService to
+// here.
+#if defined(OS_CHROMEOS)
+  chromeos_metrics_provider_->InitTaskGetHardwareClass(done_callback);
+#else
   done_callback.Run();
+#endif
 }
 
 #if defined(OS_WIN)

chrome/browser/metrics/chrome_metrics_service_client.h

@@ -17,6 +17,7 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
+class ChromeOSMetricsProvider;
 class MetricsService;
 
 namespace metrics {
@@ -54,6 +55,9 @@ class ChromeMetricsServiceClient : public metrics::MetricsServiceClient,
   explicit ChromeMetricsServiceClient(
       metrics::MetricsStateManager* state_manager);
 
+  // Completes the two-phase initialization of ChromeMetricsServiceClient.
+  void Initialize();
+
   // Callbacks for various stages of final log info collection. Do not call
   // these directly.
   void OnMemoryDetailCollectionDone();
@@ -81,11 +85,18 @@ class ChromeMetricsServiceClient : public metrics::MetricsServiceClient,
 
   base::ThreadChecker thread_checker_;
 
+  // Weak pointer to the MetricsStateManager.
+  metrics::MetricsStateManager* metrics_state_manager_;
+
   // The MetricsService that |this| is a client of.
   scoped_ptr<MetricsService> metrics_service_;
 
   content::NotificationRegistrar registrar_;
 
+  // On ChromeOS, holds a weak pointer to the ChromeOSMetricsProvider instance
+  // that has been registered with MetricsService. On other platforms, is NULL.
+  ChromeOSMetricsProvider* chromeos_metrics_provider_;
+
   NetworkStatsUploader network_stats_uploader_;
 
   // Saved callback received from CollectFinalMetrics().

chrome/browser/metrics/chromeos_metrics_provider.cc

@@ -13,7 +13,9 @@
 #include "chrome/browser/chromeos/login/users/user_manager.h"
 #include "chrome/browser/metrics/metrics_service.h"
 #include "chrome/common/pref_names.h"
+#include "chromeos/system/statistics_provider.h"
 #include "components/metrics/proto/chrome_user_metrics_extension.pb.h"
+#include "content/public/browser/browser_thread.h"
 #include "device/bluetooth/bluetooth_adapter.h"
 #include "device/bluetooth/bluetooth_adapter_factory.h"
 #include "device/bluetooth/bluetooth_device.h"
@@ -94,7 +96,8 @@ void IncrementPrefValue(const char* path) {
 
 ChromeOSMetricsProvider::ChromeOSMetricsProvider()
     : registered_user_count_at_log_initialization_(false),
-      user_count_at_log_initialization_(0) {
+      user_count_at_log_initialization_(0),
+      weak_ptr_factory_(this) {
 }
 
 ChromeOSMetricsProvider::~ChromeOSMetricsProvider() {
@@ -132,6 +135,24 @@ void ChromeOSMetricsProvider::OnDidCreateMetricsLog() {
   }
 }
 
+void ChromeOSMetricsProvider::InitTaskGetHardwareClass(
+    const base::Closure& callback) {
+  // Run the (potentially expensive) task on the FILE thread to avoid blocking
+  // the UI thread.
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&ChromeOSMetricsProvider::InitTaskGetHardwareClassOnFileThread,
+                 weak_ptr_factory_.GetWeakPtr()),
+      callback);
+}
+
+void ChromeOSMetricsProvider::InitTaskGetHardwareClassOnFileThread() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+  chromeos::system::StatisticsProvider::GetInstance()->GetMachineStatistic(
+      "hardware_class", &hardware_class_);
+}
+
 void ChromeOSMetricsProvider::ProvideSystemProfileMetrics(
     metrics::SystemProfileProto* system_profile_proto) {
   WriteBluetoothProto(system_profile_proto);
@@ -139,6 +160,7 @@ void ChromeOSMetricsProvider::ProvideSystemProfileMetrics(
 
   metrics::SystemProfileProto::Hardware* hardware =
       system_profile_proto->mutable_hardware();
+  hardware->set_hardware_class(hardware_class_);
   gfx::Display::TouchSupport has_touch = ui::GetInternalDisplayTouchSupport();
   if (has_touch == gfx::Display::TOUCH_SUPPORT_AVAILABLE)
     hardware->set_internal_display_supports_touch(true);

chrome/browser/metrics/chromeos_metrics_provider.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_METRICS_CHROMEOS_METRICS_PROVIDER_H_
 #define CHROME_BROWSER_METRICS_CHROMEOS_METRICS_PROVIDER_H_
 
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/metrics/perf_provider_chromeos.h"
 #include "components/metrics/metrics_provider.h"
 
@@ -30,6 +31,10 @@ class ChromeOSMetricsProvider : public metrics::MetricsProvider {
   // Records a crash.
   static void LogCrash(const std::string& crash_type);
 
+  // Loads hardware class information. When this task is complete, |callback|
+  // is run.
+  void InitTaskGetHardwareClass(const base::Closure& callback);
+
   // metrics::MetricsProvider:
   virtual void OnDidCreateMetricsLog() OVERRIDE;
   virtual void ProvideSystemProfileMetrics(
@@ -40,6 +45,9 @@ class ChromeOSMetricsProvider : public metrics::MetricsProvider {
       metrics::ChromeUserMetricsExtension* uma_proto) OVERRIDE;
 
  private:
+  // Called on the FILE thread to load hardware class information.
+  void InitTaskGetHardwareClassOnFileThread();
+
   // Update the number of users logged into a multi-profile session.
   // If the number of users change while the log is open, the call invalidates
   // the user count value.
@@ -66,6 +74,12 @@ class ChromeOSMetricsProvider : public metrics::MetricsProvider {
   // true.
   uint64 user_count_at_log_initialization_;
 
+  // Hardware class (e.g., hardware qualification ID). This class identifies
+  // the configured system components such as CPU, WiFi adapter, etc.
+  std::string hardware_class_;
+
+  base::WeakPtrFactory<ChromeOSMetricsProvider> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ChromeOSMetricsProvider);
 };
 

chrome/browser/metrics/chromeos_metrics_provider_unittest.cc

@@ -77,7 +77,8 @@ class ChromeOSMetricsProviderTest : public testing::Test {
         DBusThreadManager::Get()->GetBluetoothDeviceClient());
 
     // Initialize the login state trackers.
-    chromeos::LoginState::Initialize();
+    if (!chromeos::LoginState::IsInitialized())
+      chromeos::LoginState::Initialize();
   }
 
   virtual void TearDown() OVERRIDE { DBusThreadManager::Shutdown(); }

chrome/browser/metrics/metrics_log.cc

@@ -336,6 +336,10 @@ void MetricsLog::RecordEnvironment(
   system_profile->set_application_locale(client_->GetApplicationLocale());
 
   SystemProfileProto::Hardware* hardware = system_profile->mutable_hardware();
+
+  // By default, the hardware class is empty (i.e., unknown).
+  hardware->set_hardware_class(std::string());
+
   hardware->set_cpu_architecture(base::SysInfo::OperatingSystemArchitecture());
   hardware->set_system_ram_mb(base::SysInfo::AmountOfPhysicalMemoryMB());
 #if defined(OS_WIN)

chrome/browser/metrics/metrics_service.cc

@@ -208,12 +208,6 @@
 #include "chrome/browser/metrics/plugin_metrics_provider.h"
 #endif
 
-#if defined(OS_CHROMEOS)
-#include "chrome/browser/chromeos/settings/cros_settings.h"
-#include "chrome/browser/metrics/chromeos_metrics_provider.h"
-#include "chromeos/system/statistics_provider.h"
-#endif
-
 #if defined(OS_WIN)
 #include "chrome/browser/metrics/google_update_metrics_provider_win.h"
 #endif
@@ -224,7 +218,6 @@
 #endif
 
 using base::Time;
-using content::BrowserThread;
 using metrics::MetricsLogManager;
 
 namespace {
@@ -417,10 +410,6 @@ MetricsService::MetricsService(metrics::MetricsStateManager* state_manager,
       plugin_metrics_provider_));
 #endif
 
-#if defined(OS_CHROMEOS)
-  RegisterMetricsProvider(
-      scoped_ptr<metrics::MetricsProvider>(new ChromeOSMetricsProvider));
-#endif
 }
 
 MetricsService::~MetricsService() {
@@ -698,27 +687,8 @@ void MetricsService::InitializeMetricsState() {
   ScheduleNextStateSave();
 }
 
-// static
-void MetricsService::InitTaskGetHardwareClass(
-    base::WeakPtr<MetricsService> self,
-    base::MessageLoopProxy* target_loop) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
-  std::string hardware_class;
-#if defined(OS_CHROMEOS)
-  chromeos::system::StatisticsProvider::GetInstance()->GetMachineStatistic(
-      "hardware_class", &hardware_class);
-#endif  // OS_CHROMEOS
-
-  target_loop->PostTask(FROM_HERE,
-      base::Bind(&MetricsService::OnInitTaskGotHardwareClass,
-          self, hardware_class));
-}
-
-void MetricsService::OnInitTaskGotHardwareClass(
-    const std::string& hardware_class) {
+void MetricsService::OnInitTaskGotHardwareClass() {
   DCHECK_EQ(INIT_TASK_SCHEDULED, state_);
-  hardware_class_ = hardware_class;
 
   const base::Closure got_plugin_info_callback =
       base::Bind(&MetricsService::OnInitTaskGotPluginInfo,
@@ -856,25 +826,23 @@ void MetricsService::OpenNewLog() {
     // We only need to schedule that run once.
     state_ = INIT_TASK_SCHEDULED;
 
-    // TODO(blundell): Change the callback to be
-    // FinishedReceivingProfilerData() when the initial metrics gathering is
-    // moved to ChromeMetricsServiceClient.
-    client_->StartGatheringMetrics(base::Bind(&base::DoNothing));
-
-    // Schedules a task on the file thread for execution of slower
-    // initialization steps (such as plugin list generation) necessary
-    // for sending the initial log.  This avoids blocking the main UI
-    // thread.
-    BrowserThread::PostDelayedTask(
-        BrowserThread::FILE,
+    content::BrowserThread::PostDelayedTask(
+        content::BrowserThread::UI,
         FROM_HERE,
-        base::Bind(&MetricsService::InitTaskGetHardwareClass,
-            self_ptr_factory_.GetWeakPtr(),
-            base::MessageLoop::current()->message_loop_proxy()),
+        base::Bind(&MetricsService::StartGatheringMetrics,
+                   self_ptr_factory_.GetWeakPtr()),
         base::TimeDelta::FromSeconds(kInitializationDelaySeconds));
   }
 }
 
+void MetricsService::StartGatheringMetrics() {
+  // TODO(blundell): Move all initial metrics gathering to
+  // ChromeMetricsServiceClient.
+  client_->StartGatheringMetrics(
+      base::Bind(&MetricsService::OnInitTaskGotHardwareClass,
+                 self_ptr_factory_.GetWeakPtr()));
+}
+
 void MetricsService::CloseCurrentLog() {
   if (!log_manager_.current_log())
     return;
@@ -888,9 +856,6 @@ void MetricsService::CloseCurrentLog() {
     OpenNewLog();  // Start trivial log to hold our histograms.
   }
 
-  // Adds to ongoing logs.
-  log_manager_.current_log()->set_hardware_class(hardware_class_);
-
   // Put incremental data (histogram deltas, and realtime stats deltas) at the
   // end of all log transmissions (initial log handles this separately).
   // RecordIncrementalStabilityElements only exists on the derived
@@ -1112,7 +1077,6 @@ void MetricsService::PrepareInitialStabilityLog() {
 
 void MetricsService::PrepareInitialMetricsLog() {
   DCHECK(state_ == INIT_TASK_DONE || state_ == SENDING_INITIAL_STABILITY_LOG);
-  initial_metrics_log_->set_hardware_class(hardware_class_);
 
   std::vector<variations::ActiveGroupId> synthetic_trials;
   GetCurrentSyntheticFieldTrials(&synthetic_trials);

chrome/browser/metrics/metrics_service.h

@@ -258,14 +258,11 @@ class MetricsService
 
   typedef std::vector<SyntheticTrialGroup> SyntheticTrialGroups;
 
-  // First part of the init task. Called on the FILE thread to load hardware
-  // class information.
-  static void InitTaskGetHardwareClass(base::WeakPtr<MetricsService> self,
-                                       base::MessageLoopProxy* target_loop);
+  // Calls into the client to start metrics gathering.
+  void StartGatheringMetrics();
 
-  // Callback from InitTaskGetHardwareClass() that continues the init task by
-  // loading plugin information.
-  void OnInitTaskGotHardwareClass(const std::string& hardware_class);
+  // Callback that continues the init task by loading plugin information.
+  void OnInitTaskGotHardwareClass();
 
   // Called after the Plugin init task has been completed that continues the
   // init task by launching a task to gather Google Update statistics.
@@ -443,12 +440,6 @@ class MetricsService
   // Whether the initial stability log has been recorded during startup.
   bool has_initial_stability_log_;
 
-  // Chrome OS hardware class (e.g., hardware qualification ID). This
-  // class identifies the configured system components such as CPU,
-  // WiFi adapter, etc.  For non Chrome OS hosts, this will be an
-  // empty string.
-  std::string hardware_class_;
-
 #if defined(ENABLE_PLUGINS)
   PluginMetricsProvider* plugin_metrics_provider_;
 #endif

components/metrics/metrics_log_base.h

@@ -75,11 +75,6 @@ class MetricsLogBase {
            uma_proto_.user_action_event_size();
   }
 
-  void set_hardware_class(const std::string& hardware_class) {
-    uma_proto_.mutable_system_profile()->mutable_hardware()->set_hardware_class(
-        hardware_class);
-  }
-
   LogType log_type() const { return log_type_; }
 
  protected:

components/metrics/metrics_log_base_unittest.cc

@@ -42,7 +42,6 @@ TEST(MetricsLogBaseTest, LogType) {
 TEST(MetricsLogBaseTest, EmptyRecord) {
   MetricsLogBase log("totally bogus client ID", 137,
                      MetricsLogBase::ONGOING_LOG, "bogus version");
-  log.set_hardware_class("sample-class");
   log.CloseLog();
 
   std::string encoded;
@@ -59,8 +58,6 @@ TEST(MetricsLogBaseTest, EmptyRecord) {
   expected.mutable_system_profile()->set_build_timestamp(
       parsed.system_profile().build_timestamp());
   expected.mutable_system_profile()->set_app_version("bogus version");
-  expected.mutable_system_profile()->mutable_hardware()->set_hardware_class(
-      "sample-class");
 
   EXPECT_EQ(expected.SerializeAsString(), encoded);
 }