Refactor net::BackoffEntry to not require subclassing

chromeos/network/portal_detector/network_portal_detector_strategy.h

 // Copyright 2014 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 CHROMEOS_NETWORK_PORTAL_DETECTOR_NETWORK_PORTAL_DETECTOR_STRATEGY_H_
 #define CHROMEOS_NETWORK_PORTAL_DETECTOR_NETWORK_PORTAL_DETECTOR_STRATEGY_H_
 
 #include "base/basictypes.h"
 #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"
 
 namespace chromeos {
 
 class CHROMEOS_EXPORT PortalDetectorStrategy {
  public:
   enum StrategyId {
     STRATEGY_ID_LOGIN_SCREEN,
     STRATEGY_ID_ERROR_SCREEN,
     STRATEGY_ID_SESSION
   };
 
-  class Delegate {
+  class Delegate : public base::TickClock {

[stevenjb, 2015/04/13 15:20:10]

I'm not actually very familiar with this code, but having a Delegate derive from
TickClock seems strange. +pneubeck@, +ygorshenin@ who are more familiar with
this code.

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

Looking more into the usage of this, I think it might be best to have a
  // The returned object lives (at least) until the destruction of the delegate.
  base::TickClock* GetClock() = 0;

This allows one to pass _in_ an existing TickClock without having to know about
the actual TickClock interface.
E.g. NetworkPortalDetectorImpl::NowTicks should in turn take a TickClock for
testing and just pass it on, instead of maintaining a time_ticks_for_testing_
member.


As this is probably affecting more code than what the objective of this CL is,
feel free to go with your current implementation. Which I think is fine in this
corner case where TickClock has only a single method.

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

Acknowledged.

    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;
+    // TickClock implementation.

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

nit: not required to repeat abstract functions.
I'd just remove these two lines.

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

Done.

+    base::TimeTicks NowTicks() override = 0;
   };
 
   virtual ~PortalDetectorStrategy();
 
   static scoped_ptr<PortalDetectorStrategy> CreateById(StrategyId id,

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

as you probably had to read/understand this code, please add a comment about the
expected lifetime of |delegate|.

[johnme, 2015/04/20 15:57:55]

Done.

                                                        Delegate* delegate);
 
   // Returns delay before next detection attempt. This delay is needed
   // to separate detection attempts in time.
   base::TimeDelta GetDelayTillNextAttempt();
 
   // Returns timeout for the next detection attempt.
   base::TimeDelta GetNextAttemptTimeout();
 
   virtual StrategyId Id() const = 0;
 
   // Resets strategy to the initial state.
   void Reset();
 
   const net::BackoffEntry::Policy& policy() const { return policy_; }
 
   // Resets strategy to the initial stater and sets custom policy.
   void SetPolicyAndReset(const net::BackoffEntry::Policy& policy);
 
   // Should be called when portal detection is completed and timeout before next
   // attempt should be adjusted.
   void OnDetectionCompleted();
 
  protected:
-  class BackoffEntryImpl;
-
   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;
   friend class NetworkPortalDetectorImplBrowserTest;
 
   static void set_delay_till_next_attempt_for_testing(
       const base::TimeDelta& timeout) {
     delay_till_next_attempt_for_testing_ = timeout;
     delay_till_next_attempt_for_testing_initialized_ = true;
   }
@@ skipping 20 matching lines @@
 
   // True when |next_attempt_timeout_for_testing_| is initialized.
   static bool next_attempt_timeout_for_testing_initialized_;
 
   DISALLOW_COPY_AND_ASSIGN(PortalDetectorStrategy);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROMEOS_NETWORK_PORTAL_DETECTOR_NETWORK_PORTAL_DETECTOR_STRATEGY_H_