Refactor net::BackoffEntry to not require subclassing

chrome/browser/captive_portal/captive_portal_service.h

Index: chrome/browser/captive_portal/captive_portal_service.h
diff --git a/chrome/browser/captive_portal/captive_portal_service.h b/chrome/browser/captive_portal/captive_portal_service.h
index c48ca5d37c1bf40459f6e60051f1a9d88621dfc8..f239c61e1c04f80bf1e0a15c570807d0405f9a97 100644
--- a/chrome/browser/captive_portal/captive_portal_service.h
+++ b/chrome/browser/captive_portal/captive_portal_service.h
@@ -9,6 +9,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/prefs/pref_member.h"
 #include "base/threading/non_thread_safe.h"
+#include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "components/captive_portal/captive_portal_detector.h"
@@ -25,7 +26,8 @@ class Profile;
 // Captive portal checks are rate-limited.  The CaptivePortalService may only
 // be accessed on the UI thread.
 // Design doc: https://docs.google.com/document/d/1k-gP2sswzYNvryu9NcgN7q5XrsMlUdlUdoW9WRaEmfM/edit
-class CaptivePortalService : public KeyedService, public base::NonThreadSafe {
+class CaptivePortalService : public KeyedService, public base::NonThreadSafe,
+                             private base::TickClock {

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

as you've hidden TickClock::NowTicks below, TickClock should apparently not be
part of the _public_ interface of CaptivePortalService.

Thus it should be an implementation detail and you'd have to delegate to a
member instead of using inheritance.
RecheckTickClock would be exactly that internal implementation, but is currently
unused. Please, move CaptivePortalService::NowTicks to RecheckTickClock,
instantiate one RecheckTickClock member and pass that to the BackoffEntry.

Ideally, the |base::TimeTicks time_ticks_for_testing_;| part would be ripped out
of this class and instead a TickClock would be passed as dependency to this
Service.

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

Note that I was using private inheritance, so AFAIK keeping NowTicks private was
fine (indeed, the tests compiled and passed). But I've refactored this to take a
TickClock dependency, since I agree that that's cleaner.

I didn't need RecheckTickClock in the end, so I've deleted it.

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

Ha! I didn't even see that. Private inheritance is not allowed by the
styleguide:
"All inheritance should be public. If you want to do private inheritance, you
should be including an instance of the base class as a member instead."
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html

  public:
   enum TestingState {
     NOT_TESTING,
@@ -79,9 +81,9 @@ class CaptivePortalService : public KeyedService, public base::NonThreadSafe {
   friend class CaptivePortalServiceTest;
   friend class CaptivePortalBrowserTest;
 
-  // Subclass of BackoffEntry that uses the CaptivePortalService's
-  // GetCurrentTime function, for unit testing.
-  class RecheckBackoffEntry;
+  // Subclass of TickClock that uses the CaptivePortalService's
+  // GetCurrentTimeTicks function, for unit testing.

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

this function doesn't exist anymore.

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

Done (removed comment).

+  class RecheckTickClock;

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

unused

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

Done (removed).

 
   enum State {
     // No check is running or pending.
@@ -138,8 +140,8 @@ class CaptivePortalService : public KeyedService, public base::NonThreadSafe {
   //               Android, since it lacks the Browser class.
   void UpdateEnabledState();
 
-  // Returns the current TimeTicks.
-  base::TimeTicks GetCurrentTimeTicks() const;
+  // base::TickClock implementation.

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

visibility of inherited functions should usually not be changed.

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

Done (it was private inheritance, as explained above; but I've refactored this
to be has-a-TickClock instead of is-a-TickClock).

+  base::TimeTicks NowTicks() override;
 
   bool DetectionInProgress() const;