Refactor net::BackoffEntry to not require subclassing

net/base/backoff_entry.h

 // Copyright (c) 2012 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.
 
 #ifndef NET_BASE_BACKOFF_ENTRY_H_
 #define NET_BASE_BACKOFF_ENTRY_H_
 
 #include "base/threading/non_thread_safe.h"
 #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
 // on requests to a given resource, given a back-off policy.
 //
 // This utility class knows nothing about network specifics; it is
 // intended for reuse in various networking scenarios.
 class NET_EXPORT BackoffEntry : NON_EXPORTED_BASE(public base::NonThreadSafe) {
  public:
   // The set of parameters that define a back-off policy.
@@ skipping 28 matching lines @@
     // is the first delay once we start exponential backoff.
     //
     // So if we're ignoring 1 error, we'll see (N, N, Nm, Nm^2, ...) if true,
     // and (0, 0, N, Nm, ...) when false, where N is initial_backoff_ms and
     // m is multiply_factor, assuming we've already seen one success.
     bool always_use_initial_delay;
   };
 
   // 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
   // tracking was made, and whether it failed or succeeded.
   void InformOfRequest(bool succeeded);
 
   // Returns true if a request for the resource this item tracks should
   // be rejected at the present time due to exponential back-off policy.
   bool ShouldRejectRequest() const;
 
@@ skipping 12 matching lines @@
   // just as well start with a fresh BackoffEntry object), and hasn't
   // had for Policy::entry_lifetime_ms.
   bool CanDiscard() const;
 
   // Resets this entry to a fresh (as if just constructed) state.
   void Reset();
 
   // 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_;
 
   // Counts request errors; decremented on success.
   int failure_count_;
 
   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);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_BACKOFF_ENTRY_H_