Move ChromeOS hardware class init out of MetricsService.

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273820

[blundell, 2014-05-27 13:14:19]

https://codereview.chromium.org/301633006/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (left):

https://codereview.chromium.org/301633006/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:893:
log_manager_.current_log()->set_hardware_class(hardware_class_);
Two questions here:

(1) The timing of these calls have changed (it is now made inside the
RecordEnvironment() calls). Is this a problem?

(2) The hardware class is no longer being set to the empty string on platforms
other than ChromeOS. Is that a problem?

[Alexei Svitkine (slow), 2014-05-27 13:56:36]

https://codereview.chromium.org/301633006/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (left):

https://codereview.chromium.org/301633006/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:893:
log_manager_.current_log()->set_hardware_class(hardware_class_);
On 2014/05/27 13:14:19, blundell wrote:
> Two questions here:
> 
> (1) The timing of these calls have changed (it is now made inside the
> RecordEnvironment() calls). Is this a problem?

I don't think so.

> 
> (2) The hardware class is no longer being set to the empty string on platforms
> other than ChromeOS. Is that a problem?

Let's keep the behaviour the same here and set it to the empty string first in
cross-platform code and let the ChromeOS-specific code set it to the real value.

[blundell, 2014-05-27 14:58:10]

https://codereview.chromium.org/301633006/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (left):

https://codereview.chromium.org/301633006/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:893:
log_manager_.current_log()->set_hardware_class(hardware_class_);
On 2014/05/27 13:56:37, Alexei Svitkine wrote:
> On 2014/05/27 13:14:19, blundell wrote:
> > Two questions here:
> > 
> > (1) The timing of these calls have changed (it is now made inside the
> > RecordEnvironment() calls). Is this a problem?
> 
> I don't think so.
> 
> > 
> > (2) The hardware class is no longer being set to the empty string on
platforms
> > other than ChromeOS. Is that a problem?
> 
> Let's keep the behaviour the same here and set it to the empty string first in
> cross-platform code and let the ChromeOS-specific code set it to the real
value.

Done.

[Alexei Svitkine (slow), 2014-05-27 15:31:06]

https://codereview.chromium.org/301633006/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/301633006/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:353:
content::BrowserThread::FILE,
I think this should be an implementation detail of ChromeOSMetricsProvider.

i.e. StartGatheringMetrics() should simply call:

  chromeos_metrics_provider_->InitTaskGetHardwareClass(target_loop,
                                                                                
          done_callback);

The small challenge there is the kInitializationDelaySeconds. One thing that we
can do is keep that in metrics_service.cc and only call StartGatheringMetrics
after kInitializationDelaySeconds has elapsed.

[blundell, 2014-05-28 15:42:37]

https://codereview.chromium.org/301633006/diff/60001/chrome/browser/metrics/c...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/301633006/diff/60001/chrome/browser/metrics/c...
chrome/browser/metrics/chrome_metrics_service_client.cc:353:
content::BrowserThread::FILE,
On 2014/05/27 15:31:07, Alexei Svitkine wrote:
> I think this should be an implementation detail of ChromeOSMetricsProvider.
> 
> i.e. StartGatheringMetrics() should simply call:
> 
>   chromeos_metrics_provider_->InitTaskGetHardwareClass(target_loop,
>                                                                               
 
>           done_callback);
> 
> The small challenge there is the kInitializationDelaySeconds. One thing that
we
> can do is keep that in metrics_service.cc and only call StartGatheringMetrics
> after kInitializationDelaySeconds has elapsed.

Done.

[Alexei Svitkine (slow), 2014-05-28 15:50:10]

https://codereview.chromium.org/301633006/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://codereview.chromium.org/301633006/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/chromeos_metrics_provider.cc:148: callback));
Can you avoid passing the two params by using PostTaskAndReply() here instead of
PostTask()?

https://codereview.chromium.org/301633006/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/301633006/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log.cc:345: set_hardware_class("");
Nit: Inline set_hardware_class() and remove that method and pass in
std::string() instead of "".

[blundell, 2014-05-28 18:39:34]

https://codereview.chromium.org/301633006/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://codereview.chromium.org/301633006/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/chromeos_metrics_provider.cc:148: callback));
On 2014/05/28 15:50:10, Alexei Svitkine wrote:
> Can you avoid passing the two params by using PostTaskAndReply() here instead
of
> PostTask()?

Done.

https://codereview.chromium.org/301633006/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/301633006/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log.cc:345: set_hardware_class("");
On 2014/05/28 15:50:10, Alexei Svitkine wrote:
> Nit: Inline set_hardware_class() and remove that method and pass in
> std::string() instead of "".

Done.

[Alexei Svitkine (slow), 2014-05-28 18:45:57]

Looks great, just a few more nits.

https://codereview.chromium.org/301633006/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/301633006/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:119:
client->metrics_service_.reset(new MetricsService(
Nit: Now that there's a lot of code here, I suggest making a private
CreateMetricsService() method on client, and have the code below live in it,
rather than having this static method reach into client->metrics_service_ etc.

https://codereview.chromium.org/301633006/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/301633006/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log.cc:345:
uma_proto()->mutable_system_profile()->mutable_hardware()->set_hardware_class(
Nit: you have a |hardware| section just below this, just use that.

[blundell, 2014-05-28 18:59:48]

https://codereview.chromium.org/301633006/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/301633006/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:119:
client->metrics_service_.reset(new MetricsService(
On 2014/05/28 18:45:57, Alexei Svitkine wrote:
> Nit: Now that there's a lot of code here, I suggest making a private
> CreateMetricsService() method on client, and have the code below live in it,
> rather than having this static method reach into client->metrics_service_ etc.

Done.

https://codereview.chromium.org/301633006/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/301633006/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log.cc:345:
uma_proto()->mutable_system_profile()->mutable_hardware()->set_hardware_class(
On 2014/05/28 18:45:57, Alexei Svitkine wrote:
> Nit: you have a |hardware| section just below this, just use that.

Done.

[Alexei Svitkine (slow), 2014-05-28 19:04:17]

LGTM

[blundell, 2014-05-29 11:54:23]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-29 11:56:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/301633006/160001

[commit-bot: I haz the power, 2014-05-29 12:22:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10463)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/13541)

[commit-bot: I haz the power, 2014-05-29 12:27:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-29 12:27:54]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[blundell, 2014-05-30 05:46:09]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-30 05:48:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/301633006/160001

[blundell, 2014-05-30 08:48:37]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-30 08:49:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/301633006/180001

[commit-bot: I haz the power, 2014-05-30 13:24:24]

Change committed as 273820