Refactor MetricsLogChromeOS to ChromeOSMetricsProvider.

Refactor MetricsLogChromeOS to ChromeOSMetricsProvider.

Turns MetricsLogChromeOS into a metrics::MetricsProvider. Splits the
ChromeOS-specific unittests out from the MetricsLog test into separate
ChromeOSMetricsProvider tests. Also moves LogChromeOSCrash() from
MetricsService to ChromeOSMetricsProvider.

BUG=374221
R=asvitkine@chromium.org
TBR=derat, pam

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272708

[Alexei Svitkine (slow), 2014-05-23 08:41:46]

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chromeos_metrics_provider.cc:106:
IncrementPrefValue(prefs::kStabilityKernelCrashCount);
Can you move the registration of these prefs to a static RegisterPrefs() method
in this class and out of MetricsService?

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chromeos_metrics_provider.cc:241: if (user_count !=
user_count_at_log_initialization_) {
Hmm, what happens if chromeos::UserManager::IsInitialized() is false during
initialization?

The previous code handled that via the
system_profile->has_multi_profile_user_count() check. Perhaps you could add a
bool member to track this to preserve the old behaviour?

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chromeos_metrics_provider.h (right):

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chromeos_metrics_provider.h:49: virtual void
WriteBluetoothProto(
Does this still need to be virtual?

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

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_log_unittest.cc:72: #endif  // OS_CHROMEOS
Sweet!

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

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:229: #include
"chrome/browser/chromeos/settings/cros_settings.h"
Do you mind doing the hardware class fetching and setting in this CL as well? I
don't think it should be a big addition - similar to
GoogleUpdateMetricsProviderWin.

[Alexei Svitkine (slow), 2014-05-23 08:42:31]

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

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:229: #include
"chrome/browser/chromeos/settings/cros_settings.h"
On 2014/05/23 08:41:47, Alexei Svitkine wrote:
> Do you mind doing the hardware class fetching and setting in this CL as well?
I
> don't think it should be a big addition - similar to
> GoogleUpdateMetricsProviderWin.

(Similar to what GoogleUpdateMetricsProviderWin does, that is.)

[blundell, 2014-05-23 12:09:22]

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chromeos_metrics_provider.cc:106:
IncrementPrefValue(prefs::kStabilityKernelCrashCount);
On 2014/05/23 08:41:47, Alexei Svitkine wrote:
> Can you move the registration of these prefs to a static RegisterPrefs()
method
> in this class and out of MetricsService?

Done.

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chromeos_metrics_provider.cc:241: if (user_count !=
user_count_at_log_initialization_) {
On 2014/05/23 08:41:47, Alexei Svitkine wrote:
> Hmm, what happens if chromeos::UserManager::IsInitialized() is false during
> initialization?
> 
> The previous code handled that via the
> system_profile->has_multi_profile_user_count() check. Perhaps you could add a
> bool member to track this to preserve the old behaviour?

Done.

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chromeos_metrics_provider.h (right):

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chromeos_metrics_provider.h:49: virtual void
WriteBluetoothProto(
On 2014/05/23 08:41:47, Alexei Svitkine wrote:
> Does this still need to be virtual?

Done.

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

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:229: #include
"chrome/browser/chromeos/settings/cros_settings.h"
On 2014/05/23 08:41:47, Alexei Svitkine wrote:
> Do you mind doing the hardware class fetching and setting in this CL as well?
I
> don't think it should be a big addition - similar to
> GoogleUpdateMetricsProviderWin.

I'd prefer to leave this as a followup (and create a new bug to track it). Both
MetricsService and MetricsLog have ivars related to the hardware class, so the
switchover is not entirely trivial, and this CL is already large enough as it
is.

[Alexei Svitkine (slow), 2014-05-23 14:16:19]

LGTM

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

https://codereview.chromium.org/292433015/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:229: #include
"chrome/browser/chromeos/settings/cros_settings.h"
On 2014/05/23 12:09:22, blundell wrote:
> On 2014/05/23 08:41:47, Alexei Svitkine wrote:
> > Do you mind doing the hardware class fetching and setting in this CL as
well?
> I
> > don't think it should be a big addition - similar to
> > GoogleUpdateMetricsProviderWin.
> 
> I'd prefer to leave this as a followup (and create a new bug to track it).
Both
> MetricsService and MetricsLog have ivars related to the hardware class, so the
> switchover is not entirely trivial, and this CL is already large enough as it
> is.

OK

[blundell, 2014-05-24 07:08:35]

TBR=pam for //chrome/browser/prefs/browser_prefs.cc, derat for
//chrome/browser/chromeos/external_metrics.cc

[blundell, 2014-05-24 07:08:51]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-24 07:09:33]

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

[blundell, 2014-05-24 10:04:22]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-05-24 10:05:12]

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

[commit-bot: I haz the power, 2014-05-24 11:40:27]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-24 11:44:28]

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

[commit-bot: I haz the power, 2014-05-24 11:44:28]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[blundell, 2014-05-24 12:17:39]

Committed patchset #6 manually as r272708 (presubmit successful).