Refactor NetworkPortalDetector and NetworkPortalNotificationController.

chrome/browser/chromeos/net/network_portal_detector_impl.cc

Index: chrome/browser/chromeos/net/network_portal_detector_impl.cc
diff --git a/chrome/browser/chromeos/net/network_portal_detector_impl.cc b/chrome/browser/chromeos/net/network_portal_detector_impl.cc
index e58698103d432c11710eb725c0029d14839210ee..b30b331c84410cd5a9462e4363a9c211f3125144 100644
--- a/chrome/browser/chromeos/net/network_portal_detector_impl.cc
+++ b/chrome/browser/chromeos/net/network_portal_detector_impl.cc
@@ -13,6 +13,7 @@
 #include "base/metrics/histogram.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/chromeos/net/network_portal_notification_controller.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_profile_client.h"
 #include "chromeos/login/login_state.h"
@@ -139,6 +140,32 @@ void RecordPortalToOnlineTransition(const base::TimeDelta& duration) {
   }
 }
 
+class NetworkPortalDetectorStubImpl : public NetworkPortalDetector {
+ public:
+  NetworkPortalDetectorStubImpl() {}
+  ~NetworkPortalDetectorStubImpl() override {}
+
+ private:
+  // NetworkPortalDetector:
+  void AddObserver(Observer* observer) override {}
+  void AddAndFireObserver(Observer* observer) override {
+    if (observer)
+      observer->OnPortalDetectionCompleted(nullptr, CaptivePortalState());
+  }
+  void RemoveObserver(Observer* observer) override {}
+  CaptivePortalState GetCaptivePortalState(
+      const std::string& service_path) override {
+    return CaptivePortalState();
+  }
+  bool IsEnabled() override { return false; }
+  void Enable(bool start_detection) override {}
+  bool StartDetectionIfIdle() override { return false; }
+  void SetStrategy(PortalDetectorStrategy::StrategyId id) override {}
+  void OnLockScreenRequest() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkPortalDetectorStubImpl);
+};
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -207,36 +234,37 @@ const char NetworkPortalDetectorImpl::kSessionPortalToOnlineHistogram[] =
     "CaptivePortal.Session.PortalToOnlineTransition";
 
 // static
-void NetworkPortalDetectorImpl::Initialize(
+void NetworkPortalDetector::Initialize(

[stevenjb, 2015/09/17 20:45:52]

I don't feel strongly about this, but I think this is a bit confusing. Better I
think to put the stub impl in it's own file, and move this code to
network_portal_detector.cc.

[achuithb, 2015/09/17 22:03:32]

I can move the stub impl to it's own file, but it's currently small and in the
anonymous namespace, which reduces linker burden somewhat.

I can't move this to network_portal_detector.cc because I would need to include
network_portal_detector_impl.h, which isn't possible because of DEPS
restrictions :(

[stevenjb, 2015/09/18 22:04:09]

We can't implement a NetworkPortalDetector method outside of the directory it is
defined in, that breaks all sorts of compontentization rules. Basically, changes
to this method should be reverted, it should continue to be a
NetworkPortalDetectorImpl method.

Also, I really think that the stub impl should be in its own file in
chromeos/network/portal_detector/.

[achuithb, 2015/09/22 17:37:23]

ok, I've created the stub files and reverted these changes.

     net::URLRequestContextGetter* url_context) {
-  if (NetworkPortalDetector::set_for_testing())
+  if (set_for_testing_)
     return;
-  CHECK(!NetworkPortalDetector::network_portal_detector())
+  CHECK(!network_portal_detector_)
       << "NetworkPortalDetector was initialized twice.";
   NET_LOG(EVENT) << "NetworkPortalDetectorImpl::Initialize()";
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(::switches::kTestType))
-    set_network_portal_detector(new NetworkPortalDetectorStubImpl());
+    network_portal_detector_ = new NetworkPortalDetectorStubImpl();
   else
-    set_network_portal_detector(new NetworkPortalDetectorImpl(url_context));
+    network_portal_detector_ = new NetworkPortalDetectorImpl(url_context, true);
 }
 
 NetworkPortalDetectorImpl::NetworkPortalDetectorImpl(
-    const scoped_refptr<net::URLRequestContextGetter>& request_context)
-    : state_(STATE_IDLE),
-      test_url_(CaptivePortalDetector::kDefaultURL),
-      enabled_(false),
+    const scoped_refptr<net::URLRequestContextGetter>& request_context,
+    bool create_notification_controller)
+    : test_url_(CaptivePortalDetector::kDefaultURL),
       strategy_(PortalDetectorStrategy::CreateById(
           PortalDetectorStrategy::STRATEGY_ID_LOGIN_SCREEN,
           this)),
-      last_detection_result_(CAPTIVE_PORTAL_STATUS_UNKNOWN),
-      same_detection_result_count_(0),
-      no_response_result_count_(0),
       weak_factory_(this) {
   NET_LOG(EVENT) << "NetworkPortalDetectorImpl::NetworkPortalDetectorImpl()";
   captive_portal_detector_.reset(new CaptivePortalDetector(request_context));
 
-  notification_controller_.set_retry_detection_callback(base::Bind(
-      &NetworkPortalDetectorImpl::RetryDetection, base::Unretained(this)));
+  if (create_notification_controller) {
+    notification_controller_.reset(
+        new NetworkPortalNotificationController(this));
+    notification_controller_->set_retry_detection_callback(
+        base::Bind(&NetworkPortalDetectorImpl::RetryDetection,
+                   weak_factory_.GetWeakPtr()));
+  }
 
   registrar_.Add(this,
                  chrome::NOTIFICATION_LOGIN_PROXY_CHANGED,
@@ -337,14 +365,14 @@ void NetworkPortalDetectorImpl::SetStrategy(
 }
 
 void NetworkPortalDetectorImpl::OnLockScreenRequest() {
-  notification_controller_.CloseDialog();
+  if (notification_controller_.get())

[stevenjb, 2015/09/17 20:45:52]

nit: if (notification_controller_)

[achuithb, 2015/09/17 22:03:32]

Done.

+    notification_controller_->CloseDialog();
 }
 
 void NetworkPortalDetectorImpl::DefaultNetworkChanged(
     const NetworkState* default_network) {
   DCHECK(CalledOnValidThread());
 
-  notification_controller_.DefaultNetworkChanged(default_network);
   if (!default_network) {
     NET_LOG(EVENT) << "Default network changed: None";
 
@@ -354,7 +382,7 @@ void NetworkPortalDetectorImpl::DefaultNetworkChanged(
 
     CaptivePortalState state;
     state.status = CAPTIVE_PORTAL_STATUS_OFFLINE;
-    OnDetectionCompleted(NULL, state);
+    OnDetectionCompleted(nullptr, state);
     return;
   }
 
@@ -608,7 +636,6 @@ void NetworkPortalDetectorImpl::NotifyDetectionCompleted(
     const CaptivePortalState& state) {
   FOR_EACH_OBSERVER(
       Observer, observers_, OnPortalDetectionCompleted(network, state));
-  notification_controller_.OnPortalDetectionCompleted(network, state);
 }
 
 bool NetworkPortalDetectorImpl::AttemptTimeoutIsCancelledForTesting() const {