Add unit tests for sub-components of CaptureScheduler.

remoting/base/rate_counter.h

Index: remoting/base/rate_counter.h
diff --git a/remoting/base/rate_counter.h b/remoting/base/rate_counter.h
index c427b9603f8c6041dcd40ad8bc1efcd2f23f08b8..31fd84559e4a1e477ac30f8d92f39b1412ce23c9 100644
--- a/remoting/base/rate_counter.h
+++ b/remoting/base/rate_counter.h
@@ -22,37 +22,45 @@ namespace remoting {
 
 class RateCounter {
  public:
-  // Construct a counter for a specific time window.
+  // Constructs a rate counter over |time_window|.
   RateCounter(base::TimeDelta time_window);

[alexeypa (please no reviews), 2013/03/18 19:39:39]

nit: explicit

[Wez, 2013/03/19 23:23:14]

Done.

+  ~RateCounter();
 
-  virtual ~RateCounter();
-
-  // Record the data point.
+  // Records a data point.
   void Record(int64 value);
 
-  // Report the rate recorded. At the beginning of recording the numbers before
-  // |time_window| is reached the reported rate will not be accurate.
+  // Returns the rate-per-second of recorded values over the time window.
+  // Rates reported before |time_window| has elapsed are not accurate.
   double Rate();
 
+  // Overrides the current time for testing.
+  void SetCurrentTimeForTest(base::Time current_time);

[alexeypa (please no reviews), 2013/03/18 19:39:39]

It might be better to pass |current_time| explicitly to Record() and Rate()
methods. This way RateCounter will be able to work with virtual time.

Regardless of how |current_time| is passed, it is important to check that it is
not decreasing between the calls, otherwise the logic in this class will break.

[Wez, 2013/03/19 23:23:14]

That would require the caller to pass base::Time::Now() on every call, which
doesn't seem worth it right now.
True, although the only code calling this will be tests. :)

+
  private:
-  // Helper function to evict old data points.
-  void Evict(base::Time current_time);
+  // Removes data points more than |time_window| older than |current_time|.
+  void EvictOldDataPoints(base::Time current_time);
 
-  // A data point consists of a timestamp and a data value.
+  // Returns the current time specified for test, if set, or base::Time::Now().
+  base::Time CurrentTime() const;
+
+  // Type used to store data points with timestamps.
   typedef std::pair<base::Time, int64> DataPoint;

[alexeypa (please no reviews), 2013/03/18 19:39:39]

nit: I think this can be moved down, closer to |data_points_| declaration.

[Wez, 2013/03/19 23:23:14]

According to the style guide, it actually needs to move up, before the methods.

 
-  // Duration of the time window.
+  // Time window over which to calculate the rate.
   base::TimeDelta time_window_;

[alexeypa (please no reviews), 2013/03/18 19:39:39]

nit: |time_window_| can be const.

[Wez, 2013/03/19 23:23:14]

Done.

 
   // Protects |data_points_| and |sum_|.

[alexeypa (please no reviews), 2013/03/18 19:39:39]

nit: It also protects |time_window_|.

[Wez, 2013/03/19 23:23:14]

Nope.  |time_window| is set once, at init time (see your comment above re it
being const).

   base::Lock lock_;

[alexeypa (please no reviews), 2013/03/18 19:39:39]

nit: How about inheriting this class from base::NonThreadSafe instead? Is it
going to be a large change?

Same question about RunningAverage.

[Wez, 2013/03/19 23:23:14]

I wanted to avoid too much refactoring in this CL, but it does look like both
Record and Rate are called on the plugin thread, so yes, that should work.

 
-  // Keep the values of all the data points in a queue.
+  // Queue containing data points in the order in which they were recorded.
   std::queue<DataPoint> data_points_;
 
   // Sum of values in |data_points_|.
   int64 sum_;
 
+  // If set, used to calculate the running average, in place of Now().
+  base::Time current_time_for_test_;
+
   DISALLOW_COPY_AND_ASSIGN(RateCounter);
 };