Factor a scheduler object out of MetricsService.

chrome/browser/metrics/metrics_service.h

Index: chrome/browser/metrics/metrics_service.h
diff --git a/chrome/browser/metrics/metrics_service.h b/chrome/browser/metrics/metrics_service.h
index 922cf3818154be18d04e8e497ae3f98cfc121d67..63c5e98d27ffbb7002e710d04b5aff3ce755aab8 100644
--- a/chrome/browser/metrics/metrics_service.h
+++ b/chrome/browser/metrics/metrics_service.h
@@ -31,6 +31,7 @@ class DictionaryValue;
 class ListValue;
 class HistogramSynchronizer;
 class MetricsLogBase;
+class MetricsReportingScheduler;
 class PrefService;
 class TemplateURLModel;
 
@@ -40,12 +41,6 @@ struct WebPluginInfo;
 }
 }
 
-// Forward declaration of the xmlNode to avoid having tons of gyp files
-// needing to depend on the libxml third party lib.
-struct _xmlNode;
-typedef struct _xmlNode xmlNode;
-typedef xmlNode* xmlNodePtr;
-
 
 class MetricsService : public NotificationObserver,
                        public URLFetcher::Delegate,
@@ -135,6 +130,10 @@ class MetricsService : public NotificationObserver,
   bool reporting_active() const;
 
  private:
+  // The scheduler needs to be able to trigger an upload, but that shouldn't
+  // be part of the API.
+  friend class MetricsReportingScheduler;

[jar (doing other things), 2011/04/16 02:03:13]

Making classes friends of each other, outside of the context of test classes
playing ith non-public access, is usually a bad direction.

Better is to work to surface *only* the API you want called from the would-be
friend. For example, exporting to said class a method it can call.

[stuartmorgan, 2011/04/18 17:03:00]

True; do you have any pointers to how we normally handle that pattern in
Chromium? E.g., do we use RunnableMethod objects for this kind of thing, or are
those only intended for message-loop usage?

+
   // The MetricsService has a lifecycle that is stored as a state.
   // See metrics_service.cc for description of this lifecycle.
   enum State {
@@ -212,8 +211,8 @@ class MetricsService : public NotificationObserver,
   // Start timer for next log transmission.
   void StartLogTransmissionTimer();
 
-  // Internal function to collect process memory information.
-  void LogTransmissionTimerDone();
+  // Starts the process of uploading metrics data.
+  void StartScheduledUpload();
 
   // Do not call OnHistogramSynchronizationDone() directly.
   // Use StartLogTransmissionTimer() to schedule a call.
@@ -224,10 +223,6 @@ class MetricsService : public NotificationObserver,
   // MakePendingLog does nothing and returns.
   void MakePendingLog();
 
-  // Determines from state_ and permissions set out by the server whether the
-  // pending_log_ should be sent or discarded.
-  bool ServerPermitsTransmission() const;
-
   // Check to see if there are any unsent logs from previous sessions.
   bool unsent_logs() const {
     return !unsent_initial_logs_.empty() || !unsent_ongoing_logs_.empty();
@@ -262,9 +257,9 @@ class MetricsService : public NotificationObserver,
                                   const ResponseCookies& cookies,
                                   const std::string& data);
 
-  // Called by OnURLFetchComplete to handle the case when the server returned
-  // a response code not equal to 200.
-  void HandleBadResponseCode();
+  // Logs debugging details, for the case where the server returns a response
+  // code other than 200.
+  void LogBadResponseCode();
 
   // Records a window-related notification.
   void LogWindowChange(NotificationType type,
@@ -343,12 +338,6 @@ class MetricsService : public NotificationObserver,
   bool recording_active_;
   bool reporting_active_;
 
-  // The variable server_permits_upload_ is set true when the response
-  // data forbids uploading.  This should coinside with the "die roll"
-  // with probability in the upload tag of the response data came out
-  // affirmative.
-  bool server_permits_upload_;
-
   // The progession of states made by the browser are recorded in the following
   // state.
   State state_;
@@ -407,14 +396,11 @@ class MetricsService : public NotificationObserver,
   // at creation time from the prefs.
   scoped_ptr<DictionaryValue> profile_dictionary_;
 
-  // The interval between consecutive log transmissions (to avoid hogging the
-  // outbound network link).  This is usually also the duration for which we
-  // build up a log, but if other unsent-logs from previous sessions exist, we
-  // quickly transmit those unsent logs while we continue to build a log.
-  base::TimeDelta interlog_duration_;
+  // The scheduler for determining when uploads should happen.
+  scoped_ptr<MetricsReportingScheduler> scheduler_;
 
-  // Indicate that a timer for sending the next log has already been queued.
-  bool timer_pending_;
+  // Indicates that an asynchronous reporting step is running.

[jar (doing other things), 2011/04/16 02:03:13]

If this is only for debugging, you should probably indicate it in the comment. 
I think you didn't every branch on this value (from what I could spot).

[stuartmorgan, 2011/04/18 17:03:00]

Done. (This used to be a variable that sometimes was used for branching and
sometimes was used for DCHECKs, and only the latter remained in this class after
refactoring.)

+  bool waiting_for_asynchronus_reporting_step_;
 
 #if defined(OS_CHROMEOS)
   // The external metric service is used to log ChromeOS UMA events.