Refactor: Inject StatisticsProvider as a dependency of DeviceCloudPolicyInitializer.

Refactor: Inject StatisticsProvider as a dependency of DeviceCloudPolicyInitializer.

Also move GetMachineID() from DeviceCloudPolicyManager() to StatisticsProvider
which seems to be a more natural home for it.  Also exit enrollment early when
serial is missing from VPD.

BUG=none

Committed: https://crrev.com/9a445b35c5fd83eb837ba87f99e67b0dd1611903
Cr-Commit-Position: refs/heads/master@{#422153}

[Thiemo Nagel, 2016-09-27 16:02:56]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 16:03:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2016-09-27 16:06:24]

Patchset #1 (id:1) has been deleted

[Thiemo Nagel, 2016-09-27 16:09:06]

drcrash fyi: Changes to MetricEnrollment require a corresponding change in
histograms.xml.

https://codereview.chromium.org/2371213002/diff/20001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/2371213002/diff/20001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_client.h:389: // Whether to
send |machine_id_| as part of policy fetch.
On the occasion, add a comment to clarify the role of submit_machine_id_.

https://codereview.chromium.org/2371213002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2371213002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:77503: +  <int value="53" label="Cannot
sign enrollment request">
Adding an entry that was forgotten in a previous CL.

[Thiemo Nagel, 2016-09-27 16:10:51]

tnagel@chromium.org changed reviewers:
+ achuith@chromium.org

[Thiemo Nagel, 2016-09-27 16:10:51]

Hi Achuith,

could you please take a look at the UI parts?

Thank you!
Thiemo

[Thiemo Nagel, 2016-09-27 16:12:24]

tnagel@chromium.org changed reviewers:
+ isherman@chromium.org

[Thiemo Nagel, 2016-09-27 16:12:24]

Hi Ilya,

could you please take a look at histograms.xml?

Thank you!
Thiemo

[Ilya Sherman, 2016-09-27 21:17:18]

histograms.xml LGTM

[achuithb, 2016-09-28 21:04:53]

Didn't look at policy parts, but UI parts lgtm

[Thiemo Nagel, 2016-09-29 08:48:00]

tnagel@chromium.org changed reviewers:
+ mnissler@chromium.org

[Thiemo Nagel, 2016-09-29 08:48:01]

Hi Mattias,

since I had discussed this CL with you over chat: Would it be ok for you to take
a look at the enrollment part?  (No ownership required.)

Thank you,
Thiemo

[Mattias Nissler (ping if slow), 2016-09-29 09:12:26]

Looks reasonable to me, feel free to submit. If you're willing to spend one
extra iteration on code health, see my inline comment.

https://codereview.chromium.org/2371213002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2371213002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:133: }
Since device policy is the only case where machine id and machine model are
supposed to be configured, it seems a bit backwards to use CloudPolicyClient as
the vehicle to carry them around now.

Maybe a cleaner approach would be to just remove them from the constructor of
CloudPolicyClient and pass them to CloudPolicyClient::Register() as parameters?
I think that would clear up the data flow. While you're doing this, you probably
also want to get rid of the submit_machine_id_ thing (this was put in to recover
devices in the field from an earlier bug, but is no longer needed AFAIK).

[Thiemo Nagel, 2016-09-29 12:59:43]

On 2016/09/29 09:12:26, Mattias Nissler (ping if slow) wrote:
> Looks reasonable to me, feel free to submit. If you're willing to spend one
> extra iteration on code health, see my inline comment.

Thanks a lot!  I'll submit immediately since Dan has another CL that is waiting
on this one, but I've created crbug.com/651425 to track your suggestion.

[Thiemo Nagel, 2016-09-29 12:59:47]

The CQ bit was checked by tnagel@chromium.org

[Thiemo Nagel, 2016-09-29 12:59:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2371213002/#ps60001
(title: "Remove artefact of manual testing.")

[commit-bot: I haz the power, 2016-09-29 12:59:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 13:49:05]

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

[commit-bot: I haz the power, 2016-09-29 13:49:06]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Thiemo Nagel, 2016-09-30 11:47:26]

Patchset #5 (id:100001) has been deleted

[Thiemo Nagel, 2016-09-30 11:50:37]

Description was changed from

==========
Exit enrollment early when serial is missing from VPD.

BUG=none
==========

to

==========
Refactor: Inject StatisticsProvider as a dependency of
DeviceCloudPolicyInitializer.

Also move GetMachineID() from DeviceCloudPolicyManager() to StatisticsProvider
which seems to be a more natural home for it.  Also exit enrollment early when
serial is missing from VPD.

BUG=none
==========

[Thiemo Nagel, 2016-09-30 11:50:57]

Description was changed from

==========
Refactor: Inject StatisticsProvider as a dependency of
DeviceCloudPolicyInitializer.

Also move GetMachineID() from DeviceCloudPolicyManager() to StatisticsProvider
which seems to be a more natural home for it.  Also exit enrollment early when
serial is missing from VPD.

BUG=none
==========

to

==========
Refactor: Inject StatisticsProvider as a dependency of
DeviceCloudPolicyInitializer.

Also move GetMachineID() from DeviceCloudPolicyManager() to StatisticsProvider
which seems to be a more natural home for it.  Also exit enrollment early when
serial is missing from VPD.

BUG=none
==========

[Thiemo Nagel, 2016-09-30 11:54:04]

Wow, that escalated quickly.  Getting unittests right required quite some
refactoring.  I'll carry forward the l-g-t-m by Achuith and Ilya because these
aspects of the code are untouched, but the refactoring itself requires review.

[Thiemo Nagel, 2016-09-30 11:56:30]

Mattias, would you be interested/willing to continue with the review or -- given
that you're not an OWNER -- would you prefer me asking someone else?

[Mattias Nissler (ping if slow), 2016-09-30 12:01:55]

Just a high-level comment, I haven't looked closely. I'm happy to continue
skimming this, but please find another reviewer to handle the bulk of the
**/policy/** review work.

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
File chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
chromeos/system/statistics_provider.cc:98: "Product_S/N",     // Lumpy/Alex
devices
Note that the Product_S/N preference is a kludge that's purely specific to
device enrollment - I don't think exposing a GetMachineID() in
StatisticsProvider that follows this behavior is a good idea, since this is
likely wrong for all other consumers.

[Thiemo Nagel, 2016-09-30 13:03:00]

On 2016/09/30 12:01:55, Mattias Nissler (ping if slow) wrote:
> Just a high-level comment, I haven't looked closely. I'm happy to continue
> skimming this, but please find another reviewer to handle the bulk of the
> **/policy/** review work.

Sure, thanks.  I wasn't sure about your level of interest.  ;)

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
File chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
chromeos/system/statistics_provider.cc:98: "Product_S/N",     // Lumpy/Alex
devices
Actually I pulled it out of policy because its being used in VersionInfoUpdater.

[Thiemo Nagel, 2016-09-30 13:04:44]

tnagel@chromium.org changed reviewers:
+ emaxx@chromium.org

[Thiemo Nagel, 2016-09-30 13:04:45]

Maksim, may I kindly ask you to take a look?

Thanks,
Thiemo

[Mattias Nissler (ping if slow), 2016-09-30 13:16:44]

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
File chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
chromeos/system/statistics_provider.cc:98: "Product_S/N",     // Lumpy/Alex
devices
On 2016/09/30 13:03:00, Thiemo Nagel (slow) wrote:
> Actually I pulled it out of policy because its being used in
VersionInfoUpdater.

VersionInfoUpdater is specifically for enterprise use. If you really like to
keep this here, I suggest you rename the function to GetEnterpriseMachineID()
and add a comment that this is kludgy and may *not* return the actual serial
number.

[Thiemo Nagel, 2016-09-30 13:22:10]

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
File chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
chromeos/system/statistics_provider.cc:98: "Product_S/N",     // Lumpy/Alex
devices
It looks like I'm missing something here.  My reading of the comment is that
GetMachineID() actually does return the real serial number (whereas the
Product_S/N entry has a checksum appended).  Could you maybe elaborate?

[Mattias Nissler (ping if slow), 2016-09-30 13:30:25]

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
File chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
chromeos/system/statistics_provider.cc:98: "Product_S/N",     // Lumpy/Alex
devices
On 2016/09/30 13:22:10, Thiemo Nagel (slow) wrote:
> It looks like I'm missing something here.  My reading of the comment is that
> GetMachineID() actually does return the real serial number (whereas the
> Product_S/N entry has a checksum appended).  Could you maybe elaborate?

The "correct" implementation of this would be to keep kSerialNumberKey at the
top, so all devices following the standard will return what is actually
considered the serial number. However, "Product_S/N" is at highest precedence
for enterprise code due to a previous buggy version of this that we started
with, causing server-side device records use the "wrong" serial number. We can't
fix the code here since that would cause the server-side records to become
mismatched with the device.

If any other code were to start using this as is, they'd get the incorrect (i.e.
checksum-appended) version of the serial number for Lumpy/Alex, which is likely
to cause grief for them.

[Thiemo Nagel, 2016-09-30 16:05:26]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 16:05:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2016-09-30 16:13:11]

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
File chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
chromeos/system/statistics_provider.cc:98: "Product_S/N",     // Lumpy/Alex
devices
Thanks a lot.  I had to read your review comment and the source code comment
many times and even take a look at physical lumpy and alex until I finally got
it right in my head.

> If any other code were to start using this as is, they'd get the incorrect
(i.e.
> checksum-appended) version of the serial number for Lumpy/Alex, which is
likely
> to cause grief for them.

Minor correction: The check-digit-appended version is actually the correct one
(printed on the device).  The method (now renamed to GetEnterpriseMachineID())
returns the version without the check digit.

[emaxx, 2016-09-30 16:18:00]

lgtm with nits

https://codereview.chromium.org/2371213002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc
(right):

https://codereview.chromium.org/2371213002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:62:
chromeos::system::StatisticsProvider* provider =
nit: "chromeos::" is unnecessary.

https://codereview.chromium.org/2371213002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/version_info_updater.cc (right):

https://codereview.chromium.org/2371213002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/version_info_updater.cc:140:
chromeos::system::StatisticsProvider::GetInstance()->GetMachineID();
nit: "chromeos::" is unnecessary.

https://codereview.chromium.org/2371213002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(left):

https://codereview.chromium.org/2371213002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:237:
LOG(WARNING) << "Failed to get machine id. This is only an error if the "
Was this warning message dropped from the new version intentionally?

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
File chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
chromeos/system/statistics_provider.cc:208: for (size_t i = 0; i <
arraysize(kMachineInfoSerialNumberKeys); i++) {
nit: Use range-based for loop?

https://codereview.chromium.org/2371213002/diff/160001/chromeos/system/statis...
File chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/2371213002/diff/160001/chromeos/system/statis...
chromeos/system/statistics_provider.cc:206: std::string
StatisticsProvider::GetEnterpriseMachineID() {
nit: Move the method definition down just before other StatisticsProvider method
definitions?

[Thiemo Nagel, 2016-09-30 16:57:59]

Addressed nits and carried over one comment from
https://codereview.chromium.org/2343163002/ to make that CL fully obsolete.

https://codereview.chromium.org/2371213002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc
(right):

https://codereview.chromium.org/2371213002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.cc:62:
chromeos::system::StatisticsProvider* provider =
On 2016/09/30 16:17:59, emaxx wrote:
> nit: "chromeos::" is unnecessary.

Done.

https://codereview.chromium.org/2371213002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/version_info_updater.cc (right):

https://codereview.chromium.org/2371213002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/version_info_updater.cc:140:
chromeos::system::StatisticsProvider::GetInstance()->GetMachineID();
On 2016/09/30 16:17:59, emaxx wrote:
> nit: "chromeos::" is unnecessary.

Done.

https://codereview.chromium.org/2371213002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc
(left):

https://codereview.chromium.org/2371213002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos.cc:237:
LOG(WARNING) << "Failed to get machine id. This is only an error if the "
On 2016/09/30 16:17:59, emaxx wrote:
> Was this warning message dropped from the new version intentionally?

Yes.  Making that possible was the origin of the whole CL, see also
https://codereview.chromium.org/2343163002/.

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
File chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/2371213002/diff/120001/chromeos/system/statis...
chromeos/system/statistics_provider.cc:208: for (size_t i = 0; i <
arraysize(kMachineInfoSerialNumberKeys); i++) {
On 2016/09/30 16:17:59, emaxx wrote:
> nit: Use range-based for loop?

Great idea!  I never knew that was even possible...

https://codereview.chromium.org/2371213002/diff/160001/chromeos/system/statis...
File chromeos/system/statistics_provider.cc (right):

https://codereview.chromium.org/2371213002/diff/160001/chromeos/system/statis...
chromeos/system/statistics_provider.cc:206: std::string
StatisticsProvider::GetEnterpriseMachineID() {
On 2016/09/30 16:17:59, emaxx wrote:
> nit: Move the method definition down just before other StatisticsProvider
method
> definitions?

Done.

[Thiemo Nagel, 2016-09-30 16:58:47]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 16:59:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 17:54:43]

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

[commit-bot: I haz the power, 2016-09-30 17:54:45]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-09-30 17:55:46]

The CQ bit was checked by tnagel@chromium.org

[Thiemo Nagel, 2016-09-30 17:55:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, isherman@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2371213002/#ps180001
(title: "Address Maksim's comments.")

[commit-bot: I haz the power, 2016-09-30 17:56:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 18:05:12]

Description was changed from

==========
Refactor: Inject StatisticsProvider as a dependency of
DeviceCloudPolicyInitializer.

Also move GetMachineID() from DeviceCloudPolicyManager() to StatisticsProvider
which seems to be a more natural home for it.  Also exit enrollment early when
serial is missing from VPD.

BUG=none
==========

to

==========
Refactor: Inject StatisticsProvider as a dependency of
DeviceCloudPolicyInitializer.

Also move GetMachineID() from DeviceCloudPolicyManager() to StatisticsProvider
which seems to be a more natural home for it.  Also exit enrollment early when
serial is missing from VPD.

BUG=none
==========

[commit-bot: I haz the power, 2016-09-30 18:05:14]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2016-09-30 18:08:30]

Description was changed from

==========
Refactor: Inject StatisticsProvider as a dependency of
DeviceCloudPolicyInitializer.

Also move GetMachineID() from DeviceCloudPolicyManager() to StatisticsProvider
which seems to be a more natural home for it.  Also exit enrollment early when
serial is missing from VPD.

BUG=none
==========

to

==========
Refactor: Inject StatisticsProvider as a dependency of
DeviceCloudPolicyInitializer.

Also move GetMachineID() from DeviceCloudPolicyManager() to StatisticsProvider
which seems to be a more natural home for it.  Also exit enrollment early when
serial is missing from VPD.

BUG=none

Committed: https://crrev.com/9a445b35c5fd83eb837ba87f99e67b0dd1611903
Cr-Commit-Position: refs/heads/master@{#422153}
==========

[commit-bot: I haz the power, 2016-09-30 18:08:32]

Patchset 8 (id:??) landed as
https://crrev.com/9a445b35c5fd83eb837ba87f99e67b0dd1611903
Cr-Commit-Position: refs/heads/master@{#422153}