Refactor net::BackoffEntry to not require subclassing

Before this patch, net::BackoffEntry had a virtual ImplGetTimeNow method
that tests etc would override to change what time is considered "now".

As suggested by rsleevi in https://codereview.chromium.org/1023473003,
this patch removes that method, and instead makes net::BackoffEntry
accept a base::TickClock in the constructor, to allow overriding the
time without subclassing.

(this will allow future changes to net::BackoffEntry without the
fragile base class problem)

Accordingly, I've removed all subclasses of BackoffEntry, and made them
pass TickClocks instead; in most cases this has been a nice
simplification.

BUG=465399
TBR=stevenjb@chromium.org

Review URL: https://codereview.chromium.org/1076853003

Cr-Commit-Position: refs/heads/master@{#325865}

chrome/browser/captive_portal/captive_portal_service.cc

@@ -10,6 +10,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
+#include "base/time/tick_clock.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
@@ -138,24 +139,6 @@ bool ShouldDeferToNativeCaptivePortalDetection() {
 CaptivePortalService::TestingState CaptivePortalService::testing_state_ =
     NOT_TESTING;
 
-class CaptivePortalService::RecheckBackoffEntry : public net::BackoffEntry {
- public:
-  explicit RecheckBackoffEntry(CaptivePortalService* captive_portal_service)
-      : net::BackoffEntry(
-            &captive_portal_service->recheck_policy().backoff_policy),
-        captive_portal_service_(captive_portal_service) {
-  }
-
- private:
-  base::TimeTicks ImplGetTimeNow() const override {
-    return captive_portal_service_->GetCurrentTimeTicks();
-  }
-
-  CaptivePortalService* captive_portal_service_;
-
-  DISALLOW_COPY_AND_ASSIGN(RecheckBackoffEntry);
-};
-
 CaptivePortalService::RecheckPolicy::RecheckPolicy()
     : initial_backoff_no_portal_ms(600 * 1000),
       initial_backoff_portal_ms(20 * 1000) {
@@ -184,13 +167,19 @@ CaptivePortalService::RecheckPolicy::RecheckPolicy()
 }
 
 CaptivePortalService::CaptivePortalService(Profile* profile)
+    : CaptivePortalService(profile, nullptr) {
+}
+
+CaptivePortalService::CaptivePortalService(Profile* profile,
+                                           base::TickClock* clock_for_testing)
     : profile_(profile),
       state_(STATE_IDLE),
       captive_portal_detector_(profile->GetRequestContext()),
       enabled_(false),
       last_detection_result_(captive_portal::RESULT_INTERNET_CONNECTED),
       num_checks_with_same_result_(0),
-      test_url_(captive_portal::CaptivePortalDetector::kDefaultURL) {
+      test_url_(captive_portal::CaptivePortalDetector::kDefaultURL),
+      tick_clock_for_testing_(clock_for_testing) {
   // The order matters here:
   // |resolve_errors_with_web_service_| must be initialized and |backoff_entry_|
   // created before the call to UpdateEnabledState.
@@ -348,7 +337,8 @@ void CaptivePortalService::ResetBackoffEntry(CaptivePortalResult result) {
         recheck_policy_.initial_backoff_no_portal_ms;
   }
 
-  backoff_entry_.reset(new RecheckBackoffEntry(this));
+  backoff_entry_.reset(new net::BackoffEntry(&recheck_policy().backoff_policy,
+                                             tick_clock_for_testing_));
 }
 
 void CaptivePortalService::UpdateEnabledState() {
@@ -387,10 +377,10 @@ void CaptivePortalService::UpdateEnabledState() {
 }
 
 base::TimeTicks CaptivePortalService::GetCurrentTimeTicks() const {
-  if (time_ticks_for_testing_.is_null())
-    return base::TimeTicks::Now();
+  if (tick_clock_for_testing_)
+    return tick_clock_for_testing_->NowTicks();
   else
-    return time_ticks_for_testing_;
+    return base::TimeTicks::Now();
 }
 
 bool CaptivePortalService::DetectionInProgress() const {

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"
@@ -48,6 +49,7 @@ class CaptivePortalService : public KeyedService, public base::NonThreadSafe {
   };
 
   explicit CaptivePortalService(Profile* profile);
+  CaptivePortalService(Profile* profile, base::TickClock* clock_for_testing);
   ~CaptivePortalService() override;
 
   // Triggers a check for a captive portal.  If there's already a check in
@@ -79,10 +81,6 @@ 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;
-
   enum State {
     // No check is running or pending.
     STATE_IDLE,
@@ -138,7 +136,6 @@ class CaptivePortalService : public KeyedService, public base::NonThreadSafe {
   //               Android, since it lacks the Browser class.
   void UpdateEnabledState();
 
-  // Returns the current TimeTicks.
   base::TimeTicks GetCurrentTimeTicks() const;
 
   bool DetectionInProgress() const;
@@ -152,16 +149,6 @@ class CaptivePortalService : public KeyedService, public base::NonThreadSafe {
 
   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) {
-    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_;
 
@@ -207,8 +194,8 @@ class CaptivePortalService : public KeyedService, public base::NonThreadSafe {
 
   static TestingState testing_state_;
 
-  // Test time ticks used by unit tests.
-  base::TimeTicks time_ticks_for_testing_;
+  // Test tick clock used by unit tests.
+  base::TickClock* tick_clock_for_testing_;  // Not owned.
 
   DISALLOW_COPY_AND_ASSIGN(CaptivePortalService);
 };

chrome/browser/captive_portal/captive_portal_service_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/command_line.h"
 #include "base/prefs/pref_service.h"
 #include "base/run_loop.h"
+#include "base/test/simple_test_tick_clock.h"
 #include "base/test/test_timeouts.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
@@ -101,8 +102,9 @@ class CaptivePortalServiceTest : public testing::Test,
     CaptivePortalService::set_state_for_testing(testing_state);
 
     profile_.reset(new TestingProfile());
-    service_.reset(new CaptivePortalService(profile_.get()));
-    service_->set_time_ticks_for_testing(base::TimeTicks::Now());
+    tick_clock_.reset(new base::SimpleTestTickClock());
+    tick_clock_->Advance(base::TimeTicks::Now() - tick_clock_->NowTicks());
+    service_.reset(new CaptivePortalService(profile_.get(), tick_clock_.get()));
 
     // Use no delays for most tests.
     set_initial_backoff_no_portal(base::TimeDelta());
@@ -226,7 +228,7 @@ class CaptivePortalServiceTest : public testing::Test,
   // Changes test time for the service and service's captive portal
   // detector.
   void AdvanceTime(const base::TimeDelta& delta) {
-    service()->advance_time_ticks_for_testing(delta);
+    tick_clock_->Advance(delta);
     CaptivePortalDetectorTestBase::AdvanceTime(delta);
   }
 
@@ -281,6 +283,7 @@ class CaptivePortalServiceTest : public testing::Test,
 
   // Note that the construction order of these matters.
   scoped_ptr<TestingProfile> profile_;
+  scoped_ptr<base::SimpleTestTickClock> tick_clock_;
   scoped_ptr<CaptivePortalService> service_;
 };
 

chrome/browser/chromeos/net/network_portal_detector_impl.cc

@@ -398,7 +398,7 @@ base::TimeTicks NetworkPortalDetectorImpl::AttemptStartTime() {
   return attempt_start_time_;
 }
 
-base::TimeTicks NetworkPortalDetectorImpl::GetCurrentTimeTicks() {
+base::TimeTicks NetworkPortalDetectorImpl::NowTicks() {
   if (time_ticks_for_testing_.is_null())
     return base::TimeTicks::Now();
   return time_ticks_for_testing_;
@@ -412,7 +412,7 @@ void NetworkPortalDetectorImpl::StartDetection() {
   DCHECK(is_idle());
 
   ResetStrategyAndCounters();
-  detection_start_time_ = GetCurrentTimeTicks();
+  detection_start_time_ = NowTicks();
   ScheduleAttempt(base::TimeDelta());
 }
 
@@ -450,7 +450,7 @@ void NetworkPortalDetectorImpl::StartAttempt() {
   DCHECK(is_portal_check_pending());
 
   state_ = STATE_CHECKING_FOR_PORTAL;
-  attempt_start_time_ = GetCurrentTimeTicks();
+  attempt_start_time_ = NowTicks();
 
   captive_portal_detector_->DetectCaptivePortal(
       test_url_,
@@ -511,7 +511,7 @@ void NetworkPortalDetectorImpl::OnAttemptCompleted(
 
   CaptivePortalState state;
   state.response_code = response_code;
-  state.time = GetCurrentTimeTicks();
+  state.time = NowTicks();
   switch (result) {
     case captive_portal::RESULT_NO_RESPONSE:
       if (state.response_code == net::HTTP_PROXY_AUTHENTICATION_REQUIRED) {
@@ -623,7 +623,7 @@ void NetworkPortalDetectorImpl::RecordDetectionStats(
     return;
 
   if (!detection_start_time_.is_null())
-    RecordDetectionDuration(GetCurrentTimeTicks() - detection_start_time_);
+    RecordDetectionDuration(NowTicks() - detection_start_time_);
   RecordDetectionResult(status);
 
   switch (status) {

chrome/browser/chromeos/net/network_portal_detector_impl.h

@@ -86,7 +86,7 @@ class NetworkPortalDetectorImpl
   // PortalDetectorStrategy::Delegate implementation:
   int NoResponseResultCount() override;
   base::TimeTicks AttemptStartTime() override;
-  base::TimeTicks GetCurrentTimeTicks() override;
+  base::TimeTicks NowTicks() override;
 
  private:
   friend class ::NetworkingConfigTest;

chromeos/network/portal_detector/network_portal_detector_strategy.cc

@@ -92,25 +92,9 @@ class SessionStrategy : public PortalDetectorStrategy {
 
 }  // namespace
 
-// PortalDetectorStrategy::BackoffEntryImpl ------------------------------------
+// PortalDetectorStrategy::Delegate --------------------------------------------
 
-class PortalDetectorStrategy::BackoffEntryImpl : public net::BackoffEntry {
- public:
-  BackoffEntryImpl(const net::BackoffEntry::Policy* const policy,
-                   PortalDetectorStrategy::Delegate* delegate)
-      : net::BackoffEntry(policy), delegate_(delegate) {}
-  ~BackoffEntryImpl() override {}
-
-  // net::BackoffEntry overrides:
-  base::TimeTicks ImplGetTimeNow() const override {
-    return delegate_->GetCurrentTimeTicks();
-  }
-
- private:
-  PortalDetectorStrategy::Delegate* delegate_;
-
-  DISALLOW_COPY_AND_ASSIGN(BackoffEntryImpl);
-};
+PortalDetectorStrategy::Delegate::~Delegate() {}
 
 // PortalDetectorStrategy -----------------------------------------------------
 
@@ -142,7 +126,7 @@ PortalDetectorStrategy::PortalDetectorStrategy(Delegate* delegate)
   policy_.maximum_backoff_ms = 2 * 60 * 1000;
   policy_.entry_lifetime_ms = -1;
   policy_.always_use_initial_delay = true;
-  backoff_entry_.reset(new BackoffEntryImpl(&policy_, delegate_));
+  backoff_entry_.reset(new net::BackoffEntry(&policy_, delegate_));
 }
 
 PortalDetectorStrategy::~PortalDetectorStrategy() {
@@ -187,7 +171,7 @@ void PortalDetectorStrategy::Reset() {
 void PortalDetectorStrategy::SetPolicyAndReset(
     const net::BackoffEntry::Policy& policy) {
   policy_ = policy;
-  backoff_entry_.reset(new BackoffEntryImpl(&policy_, delegate_));
+  backoff_entry_.reset(new net::BackoffEntry(&policy_, delegate_));
 }
 
 void PortalDetectorStrategy::OnDetectionCompleted() {

chromeos/network/portal_detector/network_portal_detector_strategy.h

@@ -9,6 +9,7 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "chromeos/chromeos_export.h"
 #include "net/base/backoff_entry.h"
@@ -23,23 +24,21 @@ class CHROMEOS_EXPORT PortalDetectorStrategy {
     STRATEGY_ID_SESSION
   };
 
-  class Delegate {
+  class Delegate : public base::TickClock {
    public:
-    virtual ~Delegate() {}
+    ~Delegate() override;
 
     // Returns number of attempts in a row with NO RESPONSE result.
     // If last detection attempt has different result, returns 0.
     virtual int NoResponseResultCount() = 0;
 
     // Returns time when current attempt was started.
     virtual base::TimeTicks AttemptStartTime() = 0;
-
-    // Returns current TimeTicks.
-    virtual base::TimeTicks GetCurrentTimeTicks() = 0;
   };
 
   virtual ~PortalDetectorStrategy();
 
+  // Lifetime of delegate must enclose lifetime of PortalDetectorStrategy.
   static scoped_ptr<PortalDetectorStrategy> CreateById(StrategyId id,
                                                        Delegate* delegate);
 
@@ -65,16 +64,15 @@ class CHROMEOS_EXPORT PortalDetectorStrategy {
   void OnDetectionCompleted();
 
  protected:
-  class BackoffEntryImpl;
-
+  // Lifetime of delegate must enclose lifetime of PortalDetectorStrategy.
   explicit PortalDetectorStrategy(Delegate* delegate);
 
   // Interface for subclasses:
   virtual base::TimeDelta GetNextAttemptTimeoutImpl();
 
   Delegate* delegate_;
   net::BackoffEntry::Policy policy_;
-  scoped_ptr<BackoffEntryImpl> backoff_entry_;
+  scoped_ptr<net::BackoffEntry> backoff_entry_;
 
  private:
   friend class NetworkPortalDetectorImplTest;

components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc

@@ -156,7 +156,7 @@ void DataReductionProxyConfigServiceClient::SetConfigRefreshTimer(
       &DataReductionProxyConfigServiceClient::RetrieveConfig);
 }
 
-base::Time DataReductionProxyConfigServiceClient::Now() const {
+base::Time DataReductionProxyConfigServiceClient::Now() {
   return base::Time::Now();
 }
 

components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h

@@ -61,7 +61,7 @@ class DataReductionProxyConfigServiceClient {
 
   // Returns the current time.
   // Virtual for testing.
-  virtual base::Time Now() const;
+  virtual base::Time Now();
 
   // Constructs a synthetic response based on |params_|.
   // Virtual for testing.

components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc

@@ -113,7 +113,7 @@ base::TimeDelta TestDataReductionProxyConfigServiceClient::GetDelay() const {
   return config_refresh_timer_.GetCurrentDelay();
 }
 
-base::Time TestDataReductionProxyConfigServiceClient::Now() const {
+base::Time TestDataReductionProxyConfigServiceClient::Now() {
   return tick_clock_.Now();
 }
 
@@ -128,12 +128,12 @@ TestDataReductionProxyConfigServiceClient::TestTickClock::TestTickClock(
 }
 
 base::TimeTicks
-TestDataReductionProxyConfigServiceClient::TestTickClock::NowTicks() const {
+TestDataReductionProxyConfigServiceClient::TestTickClock::NowTicks() {
   return base::TimeTicks::UnixEpoch() + (time_ - base::Time::UnixEpoch());
 }
 
 base::Time
-TestDataReductionProxyConfigServiceClient::TestTickClock::Now() const {
+TestDataReductionProxyConfigServiceClient::TestTickClock::Now() {
   return time_;
 }
 
@@ -142,19 +142,6 @@ void TestDataReductionProxyConfigServiceClient::TestTickClock::SetTime(
   time_ = time;
 }
 
-TestDataReductionProxyConfigServiceClient::TestBackoffEntry::TestBackoffEntry(
-    const net::BackoffEntry::Policy* const policy,
-    const TestTickClock* tick_clock)
-    : net::BackoffEntry(policy),
-      tick_clock_(tick_clock) {
-}
-
-base::TimeTicks
-TestDataReductionProxyConfigServiceClient::TestBackoffEntry::ImplGetTimeNow()
-    const {
-  return tick_clock_->NowTicks();
-}
-
 MockDataReductionProxyService::MockDataReductionProxyService(
     scoped_ptr<DataReductionProxyCompressionStats> compression_stats,
     DataReductionProxySettings* settings,