Factor ChromeOS specific code out of MetricsLog.

Factor ChromeOS specific code out of MetricsLog.

This patch moves ChromeOS specific logging from MetricsLog
to MetricsLogChromeOS.

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

[tdresser, 2014-01-30 13:51:10]

https://codereview.chromium.org/134773004/diff/280001/chrome/browser/metrics/...

[tdresser, 2014-01-30 13:51:10]

https://codereview.chromium.org/134773004/diff/280001/chrome/browser/metrics/...

[tdresser, 2014-01-30 13:51:56]

asvitkine@chromium.org requested I perform this refactoring here:
https://codereview.chromium.org/134773004/diff/280001/chrome/browser/metrics/...

[Alexei Svitkine (slow), 2014-01-30 16:30:44]

Thanks!

Change generally looks good, though I have some comments.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:746:
metrics_log_chromeos_->LogChromeOSMetrics(uma_proto());
Please move this to where the old code was (above). It should be above the
SerializeToString() block, so that the updated system profile is correctly
serialized.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log_chromeos.cc (right):

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.cc:22: enum
device::BluetoothDevice::DeviceType device_type) {
Nit: I think the "enum" is unnecessary here.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.cc:66: ChromeUserMetricsExtension*
uma_proto) {
I would make this take system_profile.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.cc:79: PrefService* pref,
metrics::SystemProfileProto::Stability* stability) {
If the first param is on a separate line, it should be 1 param per line.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.cc:148: uint32 user_count =
chromeos::UserManager::Get()
Nit: This should probably be a size_t.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.cc:154: user_count !=
system_profile->multi_profile_user_count())
Nit: Add {}'s.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log_chromeos.h (right):

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.h:27: PrefService* pref,
metrics::SystemProfileProto::Stability* stability);
1 param per line.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.h:29: protected:
Can this section be private?

[tdresser, 2014-01-30 18:04:24]

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:746:
metrics_log_chromeos_->LogChromeOSMetrics(uma_proto());
On 2014/01/30 16:30:45, Alexei Svitkine wrote:
> Please move this to where the old code was (above). It should be above the
> SerializeToString() block, so that the updated system profile is correctly
> serialized.

Done.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log_chromeos.cc (right):

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.cc:22: enum
device::BluetoothDevice::DeviceType device_type) {
On 2014/01/30 16:30:45, Alexei Svitkine wrote:
> Nit: I think the "enum" is unnecessary here.

Done.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.cc:66: ChromeUserMetricsExtension*
uma_proto) {
On 2014/01/30 16:30:45, Alexei Svitkine wrote:
> I would make this take system_profile.

Don't I need the uma_proto to do the add_perf_data() call?
In the most recent patch I pass both (the worst of both worlds).
What do you think I should do here?

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.cc:79: PrefService* pref,
metrics::SystemProfileProto::Stability* stability) {
On 2014/01/30 16:30:45, Alexei Svitkine wrote:
> If the first param is on a separate line, it should be 1 param per line.

Done.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.cc:148: uint32 user_count =
chromeos::UserManager::Get()
On 2014/01/30 16:30:45, Alexei Svitkine wrote:
> Nit: This should probably be a size_t.

Done.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.cc:154: user_count !=
system_profile->multi_profile_user_count())
On 2014/01/30 16:30:45, Alexei Svitkine wrote:
> Nit: Add {}'s.

Done.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log_chromeos.h (right):

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.h:27: PrefService* pref,
metrics::SystemProfileProto::Stability* stability);
On 2014/01/30 16:30:45, Alexei Svitkine wrote:
> 1 param per line.

Done.

https://codereview.chromium.org/146913005/diff/60001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.h:29: protected:
On 2014/01/30 16:30:45, Alexei Svitkine wrote:
> Can this section be private?

Done.

[Alexei Svitkine (slow), 2014-01-30 18:17:54]

Another round of comments and I think it should be good to go after that. Thanks
again!

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:358: new
MetricsLogChromeOS(uma_proto()->mutable_system_profile()));
Hmm, I think given that a given MetricsLog will always have the same uma_proto()
instance and that you have to pass part of it to the ctor for
MetricsLogChromeOS, I suggest just passing uma_proto() here and storing a
pointer to it as a member of MetricsLogChromeOS.

Then, the other methods of that class do not need to have any part of the log as
params, since they can just get them from the uma_proto pointer.

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:609:
metrics_log_chromeos_->WriteRealtimeStabilityAttributes(pref, stability);
Can remove stability arg per my comment above.

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:736:
metrics_log_chromeos_->LogChromeOSMetrics(system_profile, uma_proto());
Can remove both these args per my comment above.

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.h:22: #include "metrics_log_chromeos.h"
This should still be a full include path.

Though, perhaps you can get away with a forward decl here?

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log_chromeos.cc (right):

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.cc:150: size_t user_count =
chromeos::UserManager::Get()
Nit: I'd wrap this like so:

  size_t user_count =
      chromeos::UserManager::Get()->GetLoggedInUsers().size();

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log_chromeos.h (right):

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.h:13: #include
"ui/events/x/touch_factory_x11.h"
Do you need all these includes in the header? For that that aren't needed by the
header, move them to the .cc file.

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.h:25: void LogChromeOSMetrics(
Add a short comment about each of these methods.

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.h:51: // Bluetooth Adapter instance
for collecting information about paired devices.
Nit: Add an empty line before this comment.

[tdresser, 2014-01-30 19:32:30]

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:358: new
MetricsLogChromeOS(uma_proto()->mutable_system_profile()));
On 2014/01/30 18:17:54, Alexei Svitkine wrote:
> Hmm, I think given that a given MetricsLog will always have the same
uma_proto()
> instance and that you have to pass part of it to the ctor for
> MetricsLogChromeOS, I suggest just passing uma_proto() here and storing a
> pointer to it as a member of MetricsLogChromeOS.
> 
> Then, the other methods of that class do not need to have any part of the log
as
> params, since they can just get them from the uma_proto pointer.

Done.

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:609:
metrics_log_chromeos_->WriteRealtimeStabilityAttributes(pref, stability);
On 2014/01/30 18:17:54, Alexei Svitkine wrote:
> Can remove stability arg per my comment above.

Done.

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.cc:736:
metrics_log_chromeos_->LogChromeOSMetrics(system_profile, uma_proto());
On 2014/01/30 18:17:54, Alexei Svitkine wrote:
> Can remove both these args per my comment above.

Done.

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log.h:22: #include "metrics_log_chromeos.h"
On 2014/01/30 18:17:54, Alexei Svitkine wrote:
> This should still be a full include path.
> 
> Though, perhaps you can get away with a forward decl here?

Done.

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log_chromeos.cc (right):

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.cc:150: size_t user_count =
chromeos::UserManager::Get()
On 2014/01/30 18:17:54, Alexei Svitkine wrote:
> Nit: I'd wrap this like so:
> 
>   size_t user_count =
>       chromeos::UserManager::Get()->GetLoggedInUsers().size();

Didn't actually need wrapping...

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_log_chromeos.h (right):

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.h:13: #include
"ui/events/x/touch_factory_x11.h"
On 2014/01/30 18:17:54, Alexei Svitkine wrote:
> Do you need all these includes in the header? For that that aren't needed by
the
> header, move them to the .cc file.

Done.

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.h:25: void LogChromeOSMetrics(
On 2014/01/30 18:17:54, Alexei Svitkine wrote:
> Add a short comment about each of these methods.

Done.

https://codereview.chromium.org/146913005/diff/80001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_log_chromeos.h:51: // Bluetooth Adapter instance
for collecting information about paired devices.
On 2014/01/30 18:17:54, Alexei Svitkine wrote:
> Nit: Add an empty line before this comment.

Done.

[Alexei Svitkine (slow), 2014-01-30 19:37:21]

Thanks for the clean up. LGTM with a couple last nits.

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

https://codereview.chromium.org/146913005/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_chromeos.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
Nit: Copyright 2014

(No (c) for new messages.)

Same in the other file.

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

https://codereview.chromium.org/146913005/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_unittest.cc:116: MetricsLog::uma_proto()));
Nit: Indent is off here.

[tdresser, 2014-01-30 20:29:36]

Thanks for the thorough review!

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

https://codereview.chromium.org/146913005/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_chromeos.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2014/01/30 19:37:21, Alexei Svitkine wrote:
> Nit: Copyright 2014
> 
> (No (c) for new messages.)
> 
> Same in the other file.

Done.

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

https://codereview.chromium.org/146913005/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_unittest.cc:116: MetricsLog::uma_proto()));
On 2014/01/30 19:37:21, Alexei Svitkine wrote:
> Nit: Indent is off here.

Done.

[tdresser, 2014-01-31 13:23:19]

On 2014/01/30 20:29:36, tdresser wrote:
> Thanks for the thorough review!
> 
>
https://codereview.chromium.org/146913005/diff/100001/chrome/browser/metrics/...
> File chrome/browser/metrics/metrics_log_chromeos.cc (right):
> 
>
https://codereview.chromium.org/146913005/diff/100001/chrome/browser/metrics/...
> chrome/browser/metrics/metrics_log_chromeos.cc:1: // Copyright (c) 2013 The
> Chromium Authors. All rights reserved.
> On 2014/01/30 19:37:21, Alexei Svitkine wrote:
> > Nit: Copyright 2014
> > 
> > (No (c) for new messages.)
> > 
> > Same in the other file.
> 
> Done.
> 
>
https://codereview.chromium.org/146913005/diff/100001/chrome/browser/metrics/...
> File chrome/browser/metrics/metrics_log_unittest.cc (right):
> 
>
https://codereview.chromium.org/146913005/diff/100001/chrome/browser/metrics/...
> chrome/browser/metrics/metrics_log_unittest.cc:116: MetricsLog::uma_proto()));
> On 2014/01/30 19:37:21, Alexei Svitkine wrote:
> > Nit: Indent is off here.
> 
> Done.

rtenneti@, PTAL.

[ramant (doing other things), 2014-02-03 17:45:26]

lgtm

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

https://codereview.chromium.org/146913005/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_chromeos.cc:77: void
MetricsLogChromeOS::WriteRealtimeStabilityAttributes(
nit: line# 77 and 78 could be combined into one line.

https://codereview.chromium.org/146913005/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_chromeos.cc:127: // address is
xx:xx:xx:xx:xx:xx, extract the first three components and
nit: address -> |address|
nit: line# 128, period at the end of sentence.

https://codereview.chromium.org/146913005/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_log_chromeos.h (right):

https://codereview.chromium.org/146913005/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_chromeos.h:33: void
WriteRealtimeStabilityAttributes(
nit: line#33 and 34 could be combined into one line.

[tdresser, 2014-02-03 17:55:33]

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

https://codereview.chromium.org/146913005/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_chromeos.cc:77: void
MetricsLogChromeOS::WriteRealtimeStabilityAttributes(
On 2014/02/03 17:45:27, ramant wrote:
> nit: line# 77 and 78 could be combined into one line.

Done.

https://codereview.chromium.org/146913005/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_chromeos.cc:127: // address is
xx:xx:xx:xx:xx:xx, extract the first three components and
On 2014/02/03 17:45:27, ramant wrote:
> nit: address -> |address|
> nit: line# 128, period at the end of sentence.

Done.

https://codereview.chromium.org/146913005/diff/120001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_log_chromeos.h (right):

https://codereview.chromium.org/146913005/diff/120001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_chromeos.h:33: void
WriteRealtimeStabilityAttributes(
On 2014/02/03 17:45:27, ramant wrote:
> nit: line#33 and 34 could be combined into one line.

Done.

[tdresser, 2014-02-03 17:55:44]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-02-03 17:56:23]

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

[commit-bot: I haz the power, 2014-02-03 19:26:35]

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

[commit-bot: I haz the power, 2014-02-03 19:26:36]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-02-03 19:26:37]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 19:26:42]

CQ bit was unchecked on CL. Ignoring.

[tdresser, 2014-02-03 19:30:10]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-02-03 19:30:22]

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

[commit-bot: I haz the power, 2014-02-03 22:12:40]

Change committed as 248588

[commit-bot: I haz the power, 2014-02-03 22:12:40]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 22:12:44]

CQ bit was unchecked on CL. Ignoring.