Refactoring of the NetworkPortalDetector.

chrome/browser/chromeos/net/network_portal_detector_impl.h

Index: chrome/browser/chromeos/net/network_portal_detector_impl.h
diff --git a/chrome/browser/chromeos/net/network_portal_detector_impl.h b/chrome/browser/chromeos/net/network_portal_detector_impl.h
index f0a1fea67fe81a2bb45dfa520e05627b85a1b3f3..7b09c8fc63c148871f8b19a3f98c57fcbf93fb69 100644
--- a/chrome/browser/chromeos/net/network_portal_detector_impl.h
+++ b/chrome/browser/chromeos/net/network_portal_detector_impl.h
@@ -19,6 +19,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/captive_portal/captive_portal_detector.h"
 #include "chrome/browser/chromeos/net/network_portal_detector.h"
+#include "chrome/browser/chromeos/net/network_portal_detector_strategy.h"
 #include "chrome/browser/chromeos/net/network_portal_notification_controller.h"
 #include "chromeos/network/network_state_handler_observer.h"
 #include "content/public/browser/notification_observer.h"
@@ -41,7 +42,8 @@ class NetworkPortalDetectorImpl
     : public NetworkPortalDetector,
       public base::NonThreadSafe,
       public chromeos::NetworkStateHandlerObserver,
-      public content::NotificationObserver {
+      public content::NotificationObserver,
+      public PortalDetectorStrategy::Delegate {
  public:
   static const char kDetectionResultHistogram[];
   static const char kDetectionDurationHistogram[];
@@ -62,12 +64,15 @@ class NetworkPortalDetectorImpl
   virtual bool IsEnabled() OVERRIDE;
   virtual void Enable(bool start_detection) OVERRIDE;
   virtual bool StartDetectionIfIdle() OVERRIDE;
-  virtual void EnableLazyDetection() OVERRIDE;
-  virtual void DisableLazyDetection() OVERRIDE;
+  virtual void EnableErrorScreenStrategy() OVERRIDE;
+  virtual void DisableErrorScreenStrategy() OVERRIDE;
 
   // NetworkStateHandlerObserver implementation:
   virtual void DefaultNetworkChanged(const NetworkState* network) OVERRIDE;
 
+  // PortalDetectorStrategy::Delegate implementation:
+  virtual base::TimeTicks GetCurrentTimeTicks() OVERRIDE;
+
  private:
   friend class NetworkPortalDetectorImplTest;
 
@@ -83,89 +88,60 @@ class NetworkPortalDetectorImpl
     STATE_CHECKING_FOR_PORTAL,
   };
 
-  // Basic unit used in detection timeout computation.
-  static const int kBaseRequestTimeoutSec = 5;
+  // Starts detection process.
+  void StartDetection();
 
-  // Single detection attempt timeout in lazy mode.
-  static const int kLazyRequestTimeoutSec = 15;
+  // Cancels whole detection process.
+  void CancelDetection();

[Denis Kuznetsov (DE-MUC), 2014/03/05 16:59:09]

StopDetection() ?

[ygorshenin1, 2014/03/06 11:59:33]

Done.

 
   // Internal predicate which describes set of states from which
   // DetectCaptivePortal() can be called.
-  bool CanPerformDetection() const;
-
-  // Initiates Captive Portal detection after |delay|.
-  // CanPerformDetection() *must* be kept before call to this method.
-  void DetectCaptivePortal(const base::TimeDelta& delay);
+  bool CanPerformAttempt() const;
 
-  void DetectCaptivePortalTask();
+  // Initiates Captive Portal detection attempt after |delay|.
+  // CanPerformAttempt() *must* be kept before call to this method.

[Denis Kuznetsov (DE-MUC), 2014/03/05 16:59:09]

Maybe reword it to "You should check CanPerformAttempt() before calling this
method".

[ygorshenin1, 2014/03/06 11:59:33]

Done.

+  void ScheduleAttempt(const base::TimeDelta& delay);
 
-  // Called when portal check is timed out. Cancels portal check and
-  // calls OnPortalDetectionCompleted() with RESULT_NO_RESPONSE as
-  // a result.
-  void PortalDetectionTimeout();
+  // Starts detection attempt.
+  void StartAttempt();
 
-  void CancelPortalDetection();
+  // Called when portal check is timed out. Cancels portal check and calls
+  // OnPortalDetectionCompleted() with RESULT_NO_RESPONSE as a result.
+  void OnAttemptTimeout();
 
-  // Called by CaptivePortalDetector when detection completes.
-  void OnPortalDetectionCompleted(
+  // Called by CaptivePortalDetector when detection attempt completes.
+  void OnAttemptCompleted(
       const captive_portal::CaptivePortalDetector::Results& results);
 
-  // Tries to perform portal detection in "lazy" mode. Does nothing in
-  // the case of already pending/processing detection request.
-  void TryLazyDetection();
-
   // content::NotificationObserver implementation:
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
-  // Returns true if we're waiting for portal check.
-  bool IsPortalCheckPending() const;
-
-  // Returns true if portal check is in progress.
-  bool IsCheckingForPortal() const;
-
-  // Stores captive portal state for a |network|.
-  void SetCaptivePortalState(const NetworkState* network,
-                             const CaptivePortalState& results);
+  // Stores captive portal state for a |network| and notifies observers.
+  void OnDetectionCompleted(const NetworkState* network,
+                            const CaptivePortalState& results);
 
   // Notifies observers that portal detection is completed for a |network|.
-  void NotifyPortalDetectionCompleted(const NetworkState* network,
-                                      const CaptivePortalState& state);
-
-  // Returns the current TimeTicks.
-  base::TimeTicks GetCurrentTimeTicks() const;
+  void NotifyDetectionCompleted(const NetworkState* network,
+                                const CaptivePortalState& state);
 
   State state() const { return state_; }
 
-  bool lazy_detection_enabled() const { return lazy_detection_enabled_; }
-
-  // Returns current number of portal detection attempts.
-  // Used by unit tests.
-  int attempt_count_for_testing() const { return attempt_count_; }
-
-  // Sets current number of detection attempts.
-  // Used by unit tests.
-  void set_attempt_count_for_testing(int attempt_count) {
-    attempt_count_ = attempt_count;
+  bool is_idle() const {
+    return state_ == STATE_IDLE;
   }
-
-  // Sets minimum time between consecutive portal checks for the same
-  // network. Used by unit tests.
-  void set_min_time_between_attempts_for_testing(const base::TimeDelta& delta) {
-    min_time_between_attempts_ = delta;
+  bool is_portal_check_pending() const {
+    return state_ == STATE_PORTAL_CHECK_PENDING;
   }
-
-  // Sets default interval between consecutive portal checks for a
-  // network in portal state. Used by unit tests.
-  void set_lazy_check_interval_for_testing(const base::TimeDelta& delta) {
-    lazy_check_interval_ = delta;
+  bool is_checking_for_portal() const {
+    return state_ == STATE_CHECKING_FOR_PORTAL;
   }
 
-  // Sets portal detection timeout. Used by unit tests.
-  void set_request_timeout_for_testing(const base::TimeDelta& timeout) {
-    request_timeout_for_testing_ = timeout;
-    request_timeout_for_testing_initialized_ = true;
+  // Sets minimum time between consecutive detection attempts for the
+  // same network. Used by unit tests.
+  void set_min_time_between_attempts_for_testing(const base::TimeDelta& delta) {
+    min_time_between_attempts_ = delta;
   }
 
   // Returns delay before next portal check. Used by unit tests.
@@ -173,6 +149,15 @@ class NetworkPortalDetectorImpl
     return next_attempt_delay_;
   }
 
+  // Returns true if attempt timeout callback isn't fired or
+  // cancelled.
+  bool AttemptTimeoutIsCancelledForTesting() const;
+
+  // Record detection stats such as detection duration and detection
+  // result in UMA.
+  void RecordDetectionStats(const NetworkState* network,
+                            CaptivePortalStatus status);
+
   // 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;
@@ -183,22 +168,6 @@ class NetworkPortalDetectorImpl
     time_ticks_for_testing_ += delta;
   }
 
-  // Returns true if detection timeout callback isn't fired or
-  // cancelled.
-  bool DetectionTimeoutIsCancelledForTesting() const;
-
-  // Returns timeout for current (or immediate) detection attempt.
-  // The following rules are used for timeout computation:
-  // * if default (active) network is NULL, kBaseRequestTimeoutSec is used
-  // * if lazy detection mode is enabled, kLazyRequestTimeoutSec is used
-  // * otherwise, timeout equals to |attempt_count_| * kBaseRequestTimeoutSec
-  int GetRequestTimeoutSec() const;
-
-  // Record detection stats such as detection duration and detection
-  // result in UMA.
-  void RecordDetectionStats(const NetworkState* network,
-                            CaptivePortalStatus status);
-
   // Name of the default network.
   std::string default_network_name_;
 
@@ -215,8 +184,8 @@ class NetworkPortalDetectorImpl
   CaptivePortalStateMap portal_state_map_;
   ObserverList<Observer> observers_;
 
-  base::CancelableClosure detection_task_;
-  base::CancelableClosure detection_timeout_;
+  base::CancelableClosure attempt_task_;
+  base::CancelableClosure attempt_timeout_;
 
   // URL that returns a 204 response code when connected to the Internet.
   GURL test_url_;
@@ -227,16 +196,7 @@ class NetworkPortalDetectorImpl
   // True if the NetworkPortalDetector is enabled.
   bool enabled_;
 
-  base::WeakPtrFactory<NetworkPortalDetectorImpl> weak_ptr_factory_;
-
-  // Number of portal detection attemps for a default network.
-  int attempt_count_;
-
-  bool lazy_detection_enabled_;
-
-  // Time between consecutive portal checks for a network in lazy
-  // mode.
-  base::TimeDelta lazy_check_interval_;
+  base::WeakPtrFactory<NetworkPortalDetectorImpl> weak_factory_;
 
   // Minimum time between consecutive portal checks for the same
   // default network.
@@ -245,26 +205,22 @@ class NetworkPortalDetectorImpl
   // Start time of portal detection.
   base::TimeTicks detection_start_time_;
 
-  // Start time of portal detection attempt.
-  base::TimeTicks attempt_start_time_;
-
   // Delay before next portal detection.
   base::TimeDelta next_attempt_delay_;
 
-  // Test time ticks used by unit tests.
-  base::TimeTicks time_ticks_for_testing_;
-
-  // Test timeout for a portal detection used by unit tests.
-  base::TimeDelta request_timeout_for_testing_;
-
-  // True if |request_timeout_for_testing_| is initialized.
-  bool request_timeout_for_testing_initialized_;
+  // Available portal detection strategies.
+  LoginScreenStrategy login_screen_strategy_;
+  ErrorScreenStrategy error_screen_strategy_;
+  PortalDetectorStrategy* strategy_;

[Denis Kuznetsov (DE-MUC), 2014/03/05 16:59:09]

I'd prefer if there was one scoped_ptr with current strategy.

[ygorshenin1, 2014/03/06 11:59:33]

Done.

 
   // UI notification controller about captive portal state.
   NetworkPortalNotificationController notification_controller_;
 
   content::NotificationRegistrar registrar_;
 
+  // Test time ticks used by unit tests.
+  base::TimeTicks time_ticks_for_testing_;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkPortalDetectorImpl);
 };