Add a mechanism for collecting Rappor samples on a daily interval.

components/rappor/rappor_service.h

Index: components/rappor/rappor_service.h
diff --git a/components/rappor/rappor_service.h b/components/rappor/rappor_service.h
index f02a85a3fe65e48d13738531627496c854a78b18..03177a3d9284178a0d4fc1d4ee6a7f1b5045742b 100644
--- a/components/rappor/rappor_service.h
+++ b/components/rappor/rappor_service.h
@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/timer/timer.h"
+#include "components/metrics/daily_interval.h"
 
 class PrefRegistrySimple;
 class PrefService;
@@ -38,12 +39,17 @@ enum RapporType {
 // and periodically generates and uploads reports based on the collected data.
 class RapporService {
  public:
-  RapporService();
+  // Constructs a RapporService.
+  // Calling code is responsible for ensuring that the lifetime of
+  // |pref_service| is longer than the lifetime of RapporService.
+  RapporService(PrefService* pref_service);

[Alexei Svitkine (slow), 2014/09/08 15:34:21]

Add explicit.

[Steven Holte, 2014/09/12 21:23:32]

Done.

   virtual ~RapporService();
 
+  // Add an observer for collecting daily metrics.
+  void AddDailyMetric(metrics::DailyObserver* observer);

[Alexei Svitkine (slow), 2014/09/08 15:34:21]

Method name and its comment don't appear to be consistent. Method name implies
that this is adding a metric, but comment talks about an observer.

Either make the comment more clear w.r.t. to the metric name or vice versa.
Also, document ownership of the param and mention why it's ok to add without
having a remove method.

[Steven Holte, 2014/09/12 21:23:32]

Changed the ownership model so that RapporService/DailyInterval just owns all of
it's observers.

+
   // Starts the periodic generation of reports and upload attempts.
-  void Start(PrefService* pref_service,
-             net::URLRequestContextGetter* context,
+  void Start(net::URLRequestContextGetter* context,
              bool metrics_enabled);
 
   // Records a sample of the rappor metric specified by |metric_name|.
@@ -63,6 +69,15 @@ class RapporService {
   static void RegisterPrefs(PrefRegistrySimple* registry);
 
  protected:
+  // Retrieves the cohort number this client was assigned to, generating it if
+  // doesn't already exist.  The cohort should be persistent.

[Alexei Svitkine (slow), 2014/09/08 15:34:21]

Nit: I think we're using a single space between sentences in this code. Same for
the comment below.

[Steven Holte, 2014/09/12 21:23:31]

Done.

+  void LoadCohort();
+
+  // Retrieves the value for secret_ from preferences, generating it if doesn't
+  // already exist.  The secret should be persistent, so that additional bits
+  // from the client do not get exposed over time.
+  void LoadSecret();
+
   // Logs all of the collected metrics to the reports proto message and clears
   // the internal map.  Exposed for tests.  Returns true if any metrics were
   // recorded.
@@ -79,15 +94,6 @@ class RapporService {
   // Check if the service has been started successfully.
   bool IsInitialized() const;
 
-  // Retrieves the cohort number this client was assigned to, generating it if
-  // doesn't already exist.  The cohort should be persistent.
-  void LoadCohort(PrefService* pref_service);
-
-  // Retrieves the value for secret_ from preferences, generating it if doesn't
-  // already exist.  The secret should be persistent, so that additional bits
-  // from the client do not get exposed over time.
-  void LoadSecret(PrefService* pref_service);
-
   // Called whenever the logging interval elapses to generate a new log of
   // reports and pass it to the uploader.
   void OnLogInterval();
@@ -97,6 +103,11 @@ class RapporService {
   RapporMetric* LookUpMetric(const std::string& metric_name,
                              const RapporParameters& parameters);
 
+  // A weak pointer to the PrefService object to read and write preferences
+  // from.  Calling code should ensure this object continues to exist for the
+  // lifetime of the DailyInterval object.

[Alexei Svitkine (slow), 2014/09/08 15:34:20]

Nit: Since this is documenting a private member, I don't think the second
sentence is necessary since you already mention this in the ctor comment. I'd
also tweak the first sentence so it fits in a single line.

[Steven Holte, 2014/09/12 21:23:31]

Done.

+  PrefService* pref_service_;
+
   // Client-side secret used to generate fake bits.
   std::string secret_;
 
@@ -106,6 +117,9 @@ class RapporService {
   // Timer which schedules calls to OnLogInterval().
   base::OneShotTimer<RapporService> log_rotation_timer_;
 
+  // A daily interval tracker.

[Alexei Svitkine (slow), 2014/09/08 15:34:20]

I'd expand this comment to mention the purpose (i.e. mentioning that it's for
metrics that should be recorded once a day.)

[Steven Holte, 2014/09/12 21:23:32]

Done.

+  metrics::DailyInterval daily_interval_;
+
   // A private LogUploader instance for sending reports to the server.
   scoped_ptr<LogUploader> uploader_;