Use GetDeterministicMachineSpecificId instead of RLZ for device_id

components/user_prefs/tracked/pref_hash_store_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/user_prefs/tracked/pref_hash_store_impl.h"
 
 #include <stddef.h>
 #include <utility>
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/metrics/histogram.h"
+#include "components/user_prefs/tracked/device_id.h"
 #include "components/user_prefs/tracked/hash_store_contents.h"
 
+namespace {
+
+// Returns a deterministic ID for this machine.
+std::string GenerateDeviceId() {

[proberge, 2017/01/18 16:10:58]

Possible optimization:

This method is called up to 6 times on Windows on startup. Maybe we could cache
the result static-ly to avoid calls; I don't know how fast the
GetDeterministicMachineSpecificId methods are on Windows or MacOSX. wdyt? 

[gab, 2017/01/19 18:52:05]

Ah yes, that's a great idea let's do it. A static variable in this method would
do just fine, thanks

[proberge, 2017/01/19 20:42:59]

Done.

+  std::string device_id;
+  if (GetDeterministicMachineSpecificId(&device_id) == MachineIdStatus::SUCCESS)

[proberge, 2017/01/18 16:10:58]

One thing I worry about is the stability of this method. I assume that the
implementation is correct and that the returned ID is deterministic, but what if
there are intermittent failures? It seems like this method would return the
empty string and we would reset preferences on Windows.

I think I'll add a DCHECK and an histogram. wdyt?

[gab, 2017/01/19 18:52:05]

It better be 100% deterministic and non-flaky. I'm pretty sure the Windows one
is because it's a subset of the logic for the RLZ id we've been using for a
while and have never had complaints about AFAIK.

Let's confirm that the Mac one also is.

A DCHECK and a histogram sounds good to me. In fact we might even want to CHECK
against failure because failure here is very serious and would result in erasing
user settings... but I'm happy to start with a histogram.

[proberge, 2017/01/19 20:42:59]

Done.

+    return device_id;
+
+  return std::string();
+}
+
+}  // namespace
+
 class PrefHashStoreImpl::PrefHashStoreTransactionImpl
     : public PrefHashStoreTransaction {
  public:
   // Constructs a PrefHashStoreTransactionImpl which can use the private
   // members of its |outer| PrefHashStoreImpl.
   PrefHashStoreTransactionImpl(PrefHashStoreImpl* outer,
                                HashStoreContents* storage);
   ~PrefHashStoreTransactionImpl() override;
 
   // PrefHashStoreTransaction implementation.
@@ skipping 17 matching lines @@
   PrefHashStoreImpl* outer_;
   HashStoreContents* contents_;
 
   bool super_mac_valid_;
   bool super_mac_dirty_;
 
   DISALLOW_COPY_AND_ASSIGN(PrefHashStoreTransactionImpl);
 };
 
 PrefHashStoreImpl::PrefHashStoreImpl(const std::string& seed,
-                                     const std::string& device_id,
+                                     const std::string& legacy_device_id,
                                      bool use_super_mac)
-    : pref_hash_calculator_(seed, device_id), use_super_mac_(use_super_mac) {
-}
+    : pref_hash_calculator_(seed, GenerateDeviceId(), legacy_device_id),
+      use_super_mac_(use_super_mac) {}
 
 PrefHashStoreImpl::~PrefHashStoreImpl() {
 }
 
 std::unique_ptr<PrefHashStoreTransaction> PrefHashStoreImpl::BeginTransaction(
     HashStoreContents* storage) {
   return std::unique_ptr<PrefHashStoreTransaction>(
       new PrefHashStoreTransactionImpl(this, std::move(storage)));
 }
 
@@ skipping 216 matching lines @@
 bool PrefHashStoreImpl::PrefHashStoreTransactionImpl::IsSuperMACValid() const {
   return super_mac_valid_;
 }
 
 bool PrefHashStoreImpl::PrefHashStoreTransactionImpl::StampSuperMac() {
   if (!outer_->use_super_mac_ || super_mac_valid_)
     return false;
   super_mac_dirty_ = true;
   return true;
 }