Refactor net::BackoffEntry to not require subclassing

chrome/browser/captive_portal/captive_portal_service.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 CHROME_BROWSER_CAPTIVE_PORTAL_CAPTIVE_PORTAL_SERVICE_H_
 #define CHROME_BROWSER_CAPTIVE_PORTAL_CAPTIVE_PORTAL_SERVICE_H_
 
 #include "base/basictypes.h"
 #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"
 #include "components/keyed_service/core/keyed_service.h"
 #include "net/base/backoff_entry.h"
 #include "url/gurl.h"
 
 class Profile;
 
 // Service that checks for captive portals when queried, and sends a
 // NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT with the Profile as the source and
 // a CaptivePortalService::Results as the details.
 //
 // 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,
     DISABLED_FOR_TESTING,        // The service is always disabled.
     SKIP_OS_CHECK_FOR_TESTING,   // The service can be enabled even if the OS
                                  // has native captive portal detection.
     IGNORE_REQUESTS_FOR_TESTING  // Disables actual portal checks. This also
                                  // implies SKIP_OS_CHECK_FOR_TESTING.
   };
 
@@ skipping 33 matching lines @@
   // tests.  Should be called before the service is created.
   static void set_state_for_testing(TestingState testing_state) {
     testing_state_ = testing_state;
   }
   static TestingState get_state_for_testing() { return testing_state_; }
 
  private:
   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.
     STATE_IDLE,
     // The timer to check for a captive portal is running.
     STATE_TIMER_RUNNING,
     // There's an outstanding HTTP request to check for a captive portal.
     STATE_CHECKING_FOR_PORTAL,
   };
 
@@ skipping 36 matching lines @@
   void ResetBackoffEntry(captive_portal::CaptivePortalResult result);
 
   // Updates |enabled_| based on command line flags and Profile preferences,
   // and sets |state_| to STATE_NONE if it's false.
   // TODO(mmenke): Figure out on which platforms, if any, should not use
   //               automatic captive portal detection.  Currently it's enabled
   //               on all platforms, though this code is not compiled on
   //               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;
 
   // Returns true if the timer to try and detect a captive portal is running.
   bool TimerRunning() const;
 
   State state() const { return state_; }
 
   RecheckPolicy& recheck_policy() { return recheck_policy_; }
 
   void set_test_url(const GURL& test_url) { test_url_ = test_url; }
 
   // Sets current test time ticks. Used by unit tests.
   void set_time_ticks_for_testing(const base::TimeTicks& time_ticks) {

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

bad style. should be passed as TickClock dependency to the service.

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

Done.

     time_ticks_for_testing_ = time_ticks;
   }
 
   // Advances current test time ticks. Used by unit tests.
   void advance_time_ticks_for_testing(const base::TimeDelta& delta) {
     time_ticks_for_testing_ += delta;
   }
 
   // The profile that owns this CaptivePortalService.
   Profile* profile_;
@@ skipping 40 matching lines @@
 
   static TestingState testing_state_;
 
   // Test time ticks used by unit tests.
   base::TimeTicks time_ticks_for_testing_;
 
   DISALLOW_COPY_AND_ASSIGN(CaptivePortalService);
 };
 
 #endif  // CHROME_BROWSER_CAPTIVE_PORTAL_CAPTIVE_PORTAL_SERVICE_H_