Retrieve client_id from GoogleUpdateSettings when its missing from Local State.

components/metrics/metrics_state_manager.h

 // Copyright 2014 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.
 
 #ifndef COMPONENTS_METRICS_METRICS_STATE_MANAGER_H_
 #define COMPONENTS_METRICS_METRICS_STATE_MANAGER_H_
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/field_trial.h"
 
 class PrefService;
 class PrefRegistrySimple;
 
 namespace metrics {
 
 class ClonedInstallDetector;
 
 // Responsible for managing MetricsService state prefs, specifically the UMA
 // client id and low entropy source. Code outside the metrics directory should
 // not be instantiating or using this class directly.
 class MetricsStateManager {
  public:
+  // A callback that can be invoked to save client info.
+  typedef base::Callback<bool(const std::string& client_id,

[grt (UTC plus 2), 2014/07/08 14:48:01]

why not void? the return value is ignored in the impl.

[gab, 2014/07/09 19:16:59]

Done.

+                              int64 installation_date)> SaveClientInfoCallback;
+  // A callback that can be invoked to retrieve client info saved through the

[grt (UTC plus 2), 2014/07/08 14:48:01]

newline above
how about Load/Store rather than Retrieve/Save?

[gab, 2014/07/09 19:16:59]

I like it, done in precursor rename CL.

+  // SaveClientInfoCallback.
+  typedef base::Callback<bool(std::string* client_id, int64* installation_date)>

[grt (UTC plus 2), 2014/07/08 14:48:01]

consider making this a void function too, and stipulating that implementations
clear client_id on error.

[gab, 2014/07/09 19:16:59]

Actually changed the API altogether here, introducing a struct to contain the 3
bits we now want to backup.

+      RetrieveClientInfoCallback;
+
   virtual ~MetricsStateManager();
 
   // Returns true if the user opted in to sending metric reports.
   bool IsMetricsReportingEnabled();
 
   // Returns the client ID for this client, or the empty string if the user is
   // not opted in to metrics reporting.
   const std::string& client_id() const { return client_id_; }
 
   // Forces the client ID to be generated. This is useful in case it's needed
@@ skipping 12 matching lines @@
   // If metrics reporting is enabled, this method returns an entropy provider
   // that has a high source of entropy, partially based on the client ID.
   // Otherwise, it returns an entropy provider that is based on a low entropy
   // source.
   scoped_ptr<const base::FieldTrial::EntropyProvider> CreateEntropyProvider();
 
   // Creates the MetricsStateManager, enforcing that only a single instance
   // of the class exists at a time. Returns NULL if an instance exists already.
   static scoped_ptr<MetricsStateManager> Create(
       PrefService* local_state,
-      const base::Callback<bool(void)>& is_reporting_enabled_callback);
+      const base::Callback<bool(void)>& is_reporting_enabled_callback,
+      const SaveClientInfoCallback& save_client_info,
+      const RetrieveClientInfoCallback& retrieve_client_info);
 
   // Registers local state prefs used by this class.
   static void RegisterPrefs(PrefRegistrySimple* registry);
 
  private:
   FRIEND_TEST_ALL_PREFIXES(MetricsStateManagerTest, EntropySourceUsed_Low);
   FRIEND_TEST_ALL_PREFIXES(MetricsStateManagerTest, EntropySourceUsed_High);
   FRIEND_TEST_ALL_PREFIXES(MetricsStateManagerTest, LowEntropySource0NotReset);
   FRIEND_TEST_ALL_PREFIXES(MetricsStateManagerTest,
                            PermutedEntropyCacheClearedWhenLowEntropyReset);
   FRIEND_TEST_ALL_PREFIXES(MetricsStateManagerTest, ResetMetricsIDs);
 
   // Designates which entropy source was returned from this class.
   // This is used for testing to validate that we return the correct source
   // depending on the state of the service.
   enum EntropySourceType {
     ENTROPY_SOURCE_NONE,
     ENTROPY_SOURCE_LOW,
     ENTROPY_SOURCE_HIGH,
   };
 
   // Creates the MetricsStateManager with the given |local_state|. Calls
   // |is_reporting_enabled_callback| to query whether metrics reporting is
-  // enabled. Clients should instead use Create(), which enforces a single
-  // instance of this class is alive at any given time.
+  // enabled. Clients should instead use Create(), which enforces that a single
+  // instance of this class be alive at any given time.
+  // |save_client_info| should back up client info to solid state such that it

[grt (UTC plus 2), 2014/07/08 14:48:01]

solid state -> persistent storage (not everyone rocks an SSD)

[gab, 2014/07/09 19:16:59]

Done.

+  // is later retrievable by |retrieve_client_info|.
   MetricsStateManager(
       PrefService* local_state,
-      const base::Callback<bool(void)>& is_reporting_enabled_callback);
+      const base::Callback<bool(void)>& is_reporting_enabled_callback,
+      const SaveClientInfoCallback& save_client_info,
+      const RetrieveClientInfoCallback& retrieve_client_info);
 
   // Returns the low entropy source for this client. This is a random value
   // that is non-identifying amongst browser clients. This method will
   // generate the entropy source value if it has not been called before.
   int GetLowEntropySource();
 
   // Returns the first entropy source that was returned by this service since
   // start up, or NONE if neither was returned yet. This is exposed for testing
   // only.
   EntropySourceType entropy_source_returned() const {
     return entropy_source_returned_;
   }
 
   // Reset the client id and low entropy source if the kMetricsResetMetricIDs
   // pref is true.
   void ResetMetricsIDsIfNecessary();
 
   // Whether an instance of this class exists. Used to enforce that there aren't
   // multiple instances of this class at a given time.
   static bool instance_exists_;
 
   // Weak pointer to the local state prefs store.
   PrefService* const local_state_;
 
   const base::Callback<bool(void)> is_reporting_enabled_callback_;
 
+  const SaveClientInfoCallback save_client_info_;

[grt (UTC plus 2), 2014/07/08 14:48:01]

add doc comments

[gab, 2014/07/09 19:16:59]

I find that in the case of typedef'ed callbacks, a comment here would either be
very similar to the comment on the ifdef type; or it would be useless (e.g., "An
instance of a SaveClientInfoCallback"...).

I put no comments here as I don't think it helps readability (it in fact hinders
it with unnecessary text IMO).

[grt (UTC plus 2), 2014/07/10 14:03:32]

sorry i left out the justification for the request. i mentioned it for two
reasons (which i think still apply): this surrounding code documents most
members of the class, even ones that one could argue are obvious. chromium style
says "Try to follow any per-file conventions you notice when modifying existing
code." so that's the first reason. the second is that i've recently noticed that
the main style guide has this to say about member variables: "Each class data
member (also called an instance variable or member variable) should have a
comment describing what it is used for." i've recently been putting
implementation details in the doc comments for member variables. for these two,
i might say something like:

  // A callback run during Client ID creation so the embedder can store the new
ID.

or something along those lines aimed at readers trying to understand how
MetricsStateManager makes use of this member. the doc comment for the type
itself is aimed at the consumer of MetricsStateManager, so there isn't
necessarily duplication between the two.

consider introducing a Delegate interface now that there are three callbacks
rather than just one. there have been discussions on chromium-dev suggesting to
use a callback in place of a single-method delegate interface, which leads me to
believe that the groupthink suggests that a delegate interface is preferred over
multiple callbacks. this would avoid adding two new members that i'm asking you
to document, leaving only one! :-)

+  const RetrieveClientInfoCallback retrieve_client_info_;
+
   // The identifier that's sent to the server with the log reports.
   std::string client_id_;
 
   // The non-identifying low entropy source value.
   int low_entropy_source_;
 
   // The last entropy source returned by this service, used for testing.
   EntropySourceType entropy_source_returned_;
 
   scoped_ptr<ClonedInstallDetector> cloned_install_detector_;
 
   DISALLOW_COPY_AND_ASSIGN(MetricsStateManager);
 };
 
 }  // namespace metrics
 
 #endif  // COMPONENTS_METRICS_METRICS_STATE_MANAGER_H_