Refactor: Inject StatisticsProvider as a dependency of DeviceCloudPolicyInitializer.

chromeos/system/statistics_provider.cc

Index: chromeos/system/statistics_provider.cc
diff --git a/chromeos/system/statistics_provider.cc b/chromeos/system/statistics_provider.cc
index 15db2c36874de0a1acd8689f6e20312174aead28..1bebdb62a4e62b82a6be5fd016a531d2e9ac1569 100644
--- a/chromeos/system/statistics_provider.cc
+++ b/chromeos/system/statistics_provider.cc
@@ -81,6 +81,26 @@ const char kKeyboardsPath[] = "keyboards";
 const char kLocalesPath[] = "locales";
 const char kTimeZonesPath[] = "time_zones";
 
+// These are the machine serial number keys that we check in order until we
+// find a non-empty serial number. The VPD spec says the serial number should be
+// in the "serial_number" key for v2+ VPDs. However, legacy devices used a
+// different key to report their serial number, which we fall back to if
+// "serial_number" is not present.
+//
+// Product_S/N is still special-cased due to inconsistencies with serial
+// numbers on Lumpy devices: On these devices, serial_number is identical to
+// Product_S/N with an appended checksum. Unfortunately, the sticker on the
+// packaging doesn't include that checksum either (the sticker on the device
+// does though!). The former sticker is the source of the serial number used by
+// device management service, so we prefer Product_S/N over serial number to
+// match the server.
+const char* const kMachineInfoSerialNumberKeys[] = {
+    "Product_S/N",     // Lumpy/Alex devices

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

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]

Actually I pulled it out of policy because its being used in VersionInfoUpdater.

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

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]

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]

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:13:11]

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.
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.

+    kSerialNumberKey,  // VPD v2+ devices
+    "Product_SN",      // Mario
+    "sn",              // old ZGB devices (more recent ones use serial_number)
+};
+
 // Gets ListValue from given |dictionary| by given |key| and (unless |result| is
 // nullptr) sets |result| to a string with all list values joined by ','.
 // Returns true on success.
@@ -168,6 +188,7 @@ const char kWriteProtectSwitchBootKey[] = "wpsw_boot";
 const char kWriteProtectSwitchBootValueOff[] = "0";
 const char kWriteProtectSwitchBootValueOn[] = "1";
 const char kRegionKey[] = "region";
+const char kSerialNumberKey[] = "serial_number";
 const char kInitialLocaleKey[] = "initial_locale";
 const char kInitialTimezoneKey[] = "initial_timezone";
 const char kKeyboardLayoutKey[] = "keyboard_layout";
@@ -182,6 +203,17 @@ bool HasOemPrefix(const std::string& name) {
   return name.substr(0, 4) == "oem_";
 }
 
+std::string StatisticsProvider::GetMachineID() {
+  std::string machine_id;
+  for (size_t i = 0; i < arraysize(kMachineInfoSerialNumberKeys); i++) {

[emaxx, 2016/09/30 16:17:59]

nit: Use range-based for loop?

[Thiemo Nagel, 2016/09/30 16:57:58]

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

+    if (GetMachineStatistic(kMachineInfoSerialNumberKeys[i], &machine_id) &&
+        !machine_id.empty()) {
+      break;
+    }
+  }
+  return machine_id;
+}
+
 // The StatisticsProvider implementation used in production.
 class StatisticsProviderImpl : public StatisticsProvider {
  public:
@@ -577,10 +609,6 @@ StatisticsProviderImpl* StatisticsProviderImpl::GetInstance() {
       base::DefaultSingletonTraits<StatisticsProviderImpl>>::get();
 }
 
-bool StatisticsProvider::HasMachineStatistic(const std::string& name) {
-  return GetMachineStatistic(name, nullptr);
-}
-
 static StatisticsProvider* g_test_statistics_provider = NULL;
 
 // static