From 51994b2ad08c78172e0e1dc4a8e06c4205f005ca Mon Sep 17 00:00:00 2001 From: "blundell@chromium.org" Date: Fri, 30 May 2014 13:24:21 +0000 Subject: 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 --- .../metrics/chrome_metrics_service_client.cc | 39 ++++++++++++-- .../metrics/chrome_metrics_service_client.h | 11 ++++ .../browser/metrics/chromeos_metrics_provider.cc | 24 ++++++++- chrome/browser/metrics/chromeos_metrics_provider.h | 14 +++++ .../metrics/chromeos_metrics_provider_unittest.cc | 3 +- chrome/browser/metrics/metrics_log.cc | 4 ++ chrome/browser/metrics/metrics_service.cc | 62 +++++----------------- chrome/browser/metrics/metrics_service.h | 17 ++---- components/metrics/metrics_log_base.h | 5 -- components/metrics/metrics_log_base_unittest.cc | 3 -- 10 files changed, 106 insertions(+), 76 deletions(-) diff --git a/chrome/browser/metrics/chrome_metrics_service_client.cc b/chrome/browser/metrics/chrome_metrics_service_client.cc index 832f7a6..1998bab 100644 --- a/chrome/browser/metrics/chrome_metrics_service_client.cc +++ b/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 #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::Create( // receives pointers to fully constructed objects. scoped_ptr 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( + 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(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) diff --git a/chrome/browser/metrics/chrome_metrics_service_client.h b/chrome/browser/metrics/chrome_metrics_service_client.h index a7dcace..aef2120 100644 --- a/chrome/browser/metrics/chrome_metrics_service_client.h +++ b/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 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(). diff --git a/chrome/browser/metrics/chromeos_metrics_provider.cc b/chrome/browser/metrics/chromeos_metrics_provider.cc index 0a4fd15..5ecec29 100644 --- a/chrome/browser/metrics/chromeos_metrics_provider.cc +++ b/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); diff --git a/chrome/browser/metrics/chromeos_metrics_provider.h b/chrome/browser/metrics/chromeos_metrics_provider.h index ff2023d..39da740 100644 --- a/chrome/browser/metrics/chromeos_metrics_provider.h +++ b/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 weak_ptr_factory_; + DISALLOW_COPY_AND_ASSIGN(ChromeOSMetricsProvider); }; diff --git a/chrome/browser/metrics/chromeos_metrics_provider_unittest.cc b/chrome/browser/metrics/chromeos_metrics_provider_unittest.cc index c6a7a8d..1e32848 100644 --- a/chrome/browser/metrics/chromeos_metrics_provider_unittest.cc +++ b/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(); } diff --git a/chrome/browser/metrics/metrics_log.cc b/chrome/browser/metrics/metrics_log.cc index bd90025..f0ba00a 100644 --- a/chrome/browser/metrics/metrics_log.cc +++ b/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) diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc index d787dbd..b12aae4 100644 --- a/chrome/browser/metrics/metrics_service.cc +++ b/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(new ChromeOSMetricsProvider)); -#endif } MetricsService::~MetricsService() { @@ -698,27 +687,8 @@ void MetricsService::InitializeMetricsState() { ScheduleNextStateSave(); } -// static -void MetricsService::InitTaskGetHardwareClass( - base::WeakPtr 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 synthetic_trials; GetCurrentSyntheticFieldTrials(&synthetic_trials); diff --git a/chrome/browser/metrics/metrics_service.h b/chrome/browser/metrics/metrics_service.h index 9d7cde5..705392f 100644 --- a/chrome/browser/metrics/metrics_service.h +++ b/chrome/browser/metrics/metrics_service.h @@ -258,14 +258,11 @@ class MetricsService typedef std::vector SyntheticTrialGroups; - // First part of the init task. Called on the FILE thread to load hardware - // class information. - static void InitTaskGetHardwareClass(base::WeakPtr 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 diff --git a/components/metrics/metrics_log_base.h b/components/metrics/metrics_log_base.h index ff760ad..9d6d2a6 100644 --- a/components/metrics/metrics_log_base.h +++ b/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: diff --git a/components/metrics/metrics_log_base_unittest.cc b/components/metrics/metrics_log_base_unittest.cc index cc7a173..4a635bc 100644 --- a/components/metrics/metrics_log_base_unittest.cc +++ b/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); } -- cgit v1.1