Factor a scheduler object out of MetricsService.

chrome/browser/metrics/metrics_service.h

 // Copyright (c) 2011 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.
 
 // This file defines a service that collects information about the user
 // experience in order to help improve future versions of the app.
 
 #ifndef CHROME_BROWSER_METRICS_METRICS_SERVICE_H_
 #define CHROME_BROWSER_METRICS_METRICS_SERVICE_H_
 #pragma once
@@ skipping 13 matching lines @@
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/external_metrics.h"
 #endif
 
 class BookmarkModel;
 class BookmarkNode;
 class DictionaryValue;
 class ListValue;
 class HistogramSynchronizer;
 class MetricsLogBase;
+class MetricsReportingScheduler;
 class PrefService;
 class TemplateURLModel;
 
 namespace webkit {
 namespace npapi {
 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,
                        public MetricsServiceBase {
  public:
   // Used to produce a historgram that keeps track of the status of recalling
   // persisted per logs.
   enum LogRecallStatus {
     RECALL_SUCCESS,         // We were able to correctly recall a persisted log.
     LIST_EMPTY,             // Attempting to recall from an empty list.
@@ skipping 69 matching lines @@
   void StartExternalMetrics();
 
   // Records a Chrome OS crash.
   void LogChromeOSCrash(const std::string &crash_type);
 #endif
 
   bool recording_active() const;
   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 {
     INITIALIZED,            // Constructor was called.
     INIT_TASK_SCHEDULED,    // Waiting for deferred init tasks to complete.
     INIT_TASK_DONE,         // Waiting for timer to send initial log.
     INITIAL_LOG_READY,      // Initial log generated, and waiting for reply.
     SEND_OLD_INITIAL_LOGS,  // Sending unsent logs from previous session.
     SENDING_OLD_LOGS,       // Sending unsent logs from previous session.
     SENDING_CURRENT_LOGS,   // Sending standard current logs as they acrue.
@@ skipping 57 matching lines @@
   // appropriate unsent_log vectors.  Called when Chrome shuts down.
   void PushPendingLogsToUnsentLists();
 
   // Save the pending_log_text_ persistently in a pref for transmission when we
   // next run.  Note that IF this text is "too large," we just dicard it.
   void PushPendingLogTextToUnsentOngoingLogs();
 
   // 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.
   void OnHistogramSynchronizationDone();
 
   // Takes whatever log should be uploaded next (according to the state_)
   // and makes it the pending log.  If pending_log_ is not NULL,
   // 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();
   }
   // Record stats, client ID, Session ID, etc. in a special "first" log.
   void PrepareInitialLog();
   // Pull copies of unsent logs from prefs into instance variables.
   void RecallUnsentLogs();
   // Decode and verify written pref log data.
   static MetricsService::LogRecallStatus RecallUnsentLogsHelper(
@@ skipping 14 matching lines @@
 
   // Implementation of URLFetcher::Delegate. Called after transmission
   // completes (either successfully or with failure).
   virtual void OnURLFetchComplete(const URLFetcher* source,
                                   const GURL& url,
                                   const net::URLRequestStatus& status,
                                   int response_code,
                                   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,
                        const NotificationSource& source,
                        const NotificationDetails& details);
 
   // Reads, increments and then sets the specified integer preference.
   void IncrementPrefValue(const char* path);
 
   // Reads, increments and then sets the specified long preference that is
@@ skipping 58 matching lines @@
   void RecordBooleanPrefValue(const char* path, bool value);
 
   NotificationRegistrar registrar_;
 
   // Indicate whether recording and reporting are currently happening.
   // These should not be set directly, but by calling SetRecording and
   // SetReporting.
   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_;
 
   // Chrome OS hardware class (e.g., hardware qualification ID). This
   // class identifies the configured system components such as CPU,
   // WiFi adapter, etc.  For non Chrome OS hosts, this will be an
   // empty string.
   std::string hardware_class_;
 
@@ skipping 38 matching lines @@
   struct ChildProcessStats;
   std::map<std::wstring, ChildProcessStats> child_process_stats_buffer_;
 
   ScopedRunnableMethodFactory<MetricsService> log_sender_factory_;
   ScopedRunnableMethodFactory<MetricsService> state_saver_factory_;
 
   // Dictionary containing all the profile specific metrics. This is set
   // 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.
   scoped_refptr<chromeos::ExternalMetrics> external_metrics_;
 #endif
 
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, EmptyLogList);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, SingleElementLogList);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, OverLimitLogList);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, SmallRecoveredListSize);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, RemoveSizeFromLogList);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, CorruptSizeOfLogList);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, CorruptChecksumOfLogList);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, ClientIdGeneratesAllZeroes);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, ClientIdGeneratesCorrectly);
   FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, ClientIdCorrectlyFormatted);
 
   DISALLOW_COPY_AND_ASSIGN(MetricsService);
 };
 
 #endif  // CHROME_BROWSER_METRICS_METRICS_SERVICE_H_