Retrieve client_id from GoogleUpdateSettings when its missing from Local State.

components/metrics/metrics_state_manager.h

Index: components/metrics/metrics_state_manager.h
diff --git a/components/metrics/metrics_state_manager.h b/components/metrics/metrics_state_manager.h
index 6f13ee9573588c9db1bc298dccd677926c0f1ef2..66870d6275acfd0b86b8c0462d8d22593d867ed3 100644
--- a/components/metrics/metrics_state_manager.h
+++ b/components/metrics/metrics_state_manager.h
@@ -25,6 +25,14 @@ class ClonedInstallDetector;
 // 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.
@@ -57,7 +65,9 @@ class MetricsStateManager {
   // 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);
@@ -81,11 +91,15 @@ class MetricsStateManager {
 
   // 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
@@ -112,6 +126,9 @@ class MetricsStateManager {
 
   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_;