Refactor net::BackoffEntry to not require subclassing

net/base/backoff_entry.h

Index: net/base/backoff_entry.h
diff --git a/net/base/backoff_entry.h b/net/base/backoff_entry.h
index d8f03d3476b7add5f83a17f66e7f34f15f7ec198..640fbfa192cf6844aa549874cfa00f59c556a9a7 100644
--- a/net/base/backoff_entry.h
+++ b/net/base/backoff_entry.h
@@ -9,6 +9,10 @@
 #include "base/time/time.h"
 #include "net/base/net_export.h"
 
+namespace base {
+class TickClock;
+}
+
 namespace net {
 
 // Provides the core logic needed for randomized exponential back-off
@@ -58,6 +62,7 @@ class NET_EXPORT BackoffEntry : NON_EXPORTED_BASE(public base::NonThreadSafe) {
   // Lifetime of policy must enclose lifetime of BackoffEntry. The
   // pointer must be valid but is not dereferenced during construction.
   explicit BackoffEntry(const Policy* const policy);

[pneubeck (no reviews), 2015/04/14 10:27:04]

arguments are usually not declared const.
(same for the next c'tor)

[johnme, 2015/04/20 15:52:44]

Done.

+  BackoffEntry(const Policy* const policy, base::TickClock* clock);

[pneubeck (no reviews), 2015/04/14 10:27:04]

this needs a comment about the lifetime of |clock| as well.

as there's a separate constructor with a single argument, you should probably
require |clock| to be not null.

[johnme, 2015/04/20 15:52:44]

Added a comment. Requiring non-null |clock| seems a little awkward though, since
it requires callers such as CaptivePortalService::ResetBackoffEntry to
null-check the TickClocks they are passing through, and call a different
constructor accordingly (it also prevents using C++11 delegated constructors
here). Instead I commented that it's ok for clock to be null.

[pneubeck (no reviews), 2015/04/20 16:35:50]

Doesn't sound like a convincing argument to me (to adapt the interface to the
implementation; it should be the other way round). But I'm not an owner of this
code.

   virtual ~BackoffEntry();
 
   // Inform this item that a request for the network resource it is
@@ -90,14 +95,13 @@ class NET_EXPORT BackoffEntry : NON_EXPORTED_BASE(public base::NonThreadSafe) {
   // Returns the failure count for this entry.
   int failure_count() const { return failure_count_; }
 
- protected:
-  // Equivalent to TimeTicks::Now(), virtual so unit tests can override.
-  virtual base::TimeTicks ImplGetTimeNow() const;
-
  private:
   // Calculates when requests should again be allowed through.
   base::TimeTicks CalculateReleaseTime() const;
 
+  // Equivalent to TimeTicks::Now(), using clock_ if provided.
+  base::TimeTicks GetTimeTicksNow() const;
+
   // Timestamp calculated by the exponential back-off algorithm at which we are
   // allowed to start sending requests again.
   base::TimeTicks exponential_backoff_release_time_;
@@ -107,6 +111,8 @@ class NET_EXPORT BackoffEntry : NON_EXPORTED_BASE(public base::NonThreadSafe) {
 
   const Policy* const policy_;
 
+  base::TickClock* clock_;  // Not owned.

[pneubeck (no reviews), 2015/04/14 10:27:04]

should this ever change during the lifetime of the BackoffEntry?
if not, it should be 
  base::TickClock* const clock_;

[johnme, 2015/04/20 15:52:44]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(BackoffEntry);
 };