Refactor NetworkPortalDetector and NetworkPortalNotificationController.

chrome/browser/chromeos/net/network_portal_detector_impl.cc

 // Copyright (c) 2013 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.
 
 #include "chrome/browser/chromeos/net/network_portal_detector_impl.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #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"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "components/device_event_log/device_event_log.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/common/content_switches.h"
 #include "net/http/http_status_code.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
@@ skipping 106 matching lines @@
     UMA_HISTOGRAM_LONG_TIMES(
         NetworkPortalDetectorImpl::kSessionPortalToOnlineHistogram,
         duration);
   } else {
     UMA_HISTOGRAM_LONG_TIMES(
         NetworkPortalDetectorImpl::kOobePortalToOnlineHistogram,
         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
 
 ////////////////////////////////////////////////////////////////////////////////
 // NetworkPortalDetectorImpl::DetectionAttemptCompletedLogState
 
 NetworkPortalDetectorImpl::DetectionAttemptCompletedReport::
     DetectionAttemptCompletedReport()
     : result(captive_portal::RESULT_COUNT), response_code(-1) {
 }
 
@@ skipping 48 matching lines @@
 const char NetworkPortalDetectorImpl::kSessionShillOnlineHistogram[] =
     "CaptivePortal.Session.DiscrepancyWithShill_Online";
 const char NetworkPortalDetectorImpl::kSessionShillPortalHistogram[] =
     "CaptivePortal.Session.DiscrepancyWithShill_RestrictedPool";
 const char NetworkPortalDetectorImpl::kSessionShillOfflineHistogram[] =
     "CaptivePortal.Session.DiscrepancyWithShill_Offline";
 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,
                  content::NotificationService::AllSources());
   registrar_.Add(this,
                  chrome::NOTIFICATION_AUTH_SUPPLIED,
                  content::NotificationService::AllSources());
   registrar_.Add(this,
                  chrome::NOTIFICATION_AUTH_CANCELLED,
                  content::NotificationService::AllSources());
@@ skipping 80 matching lines @@
 void NetworkPortalDetectorImpl::SetStrategy(
     PortalDetectorStrategy::StrategyId id) {
   if (id == strategy_->Id())
     return;
   strategy_ = PortalDetectorStrategy::CreateById(id, this).Pass();
   StopDetection();
   StartDetectionIfIdle();
 }
 
 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";
 
     default_network_name_.clear();
 
     StopDetection();
 
     CaptivePortalState state;
     state.status = CAPTIVE_PORTAL_STATUS_OFFLINE;
-    OnDetectionCompleted(NULL, state);
+    OnDetectionCompleted(nullptr, state);
     return;
   }
 
   default_network_name_ = default_network->name();
 
   bool network_changed = (default_network_id_ != default_network->guid());
   default_network_id_ = default_network->guid();
 
   bool connection_state_changed =
       (default_connection_state_ != default_network->connection_state());
@@ skipping 233 matching lines @@
     portal_state_map_[network->guid()] = state;
   }
   NotifyDetectionCompleted(network, state);
 }
 
 void NetworkPortalDetectorImpl::NotifyDetectionCompleted(
     const NetworkState* network,
     const CaptivePortalState& state) {
   FOR_EACH_OBSERVER(
       Observer, observers_, OnPortalDetectionCompleted(network, state));
-  notification_controller_.OnPortalDetectionCompleted(network, state);
 }
 
 bool NetworkPortalDetectorImpl::AttemptTimeoutIsCancelledForTesting() const {
   return attempt_timeout_.IsCancelled();
 }
 
 void NetworkPortalDetectorImpl::RecordDetectionStats(
     const NetworkState* network,
     CaptivePortalStatus status) {
   // Don't record stats for offline state.
@@ skipping 31 matching lines @@
 }
 
 void NetworkPortalDetectorImpl::ResetStrategyAndCounters() {
   last_detection_result_ = CAPTIVE_PORTAL_STATUS_UNKNOWN;
   same_detection_result_count_ = 0;
   no_response_result_count_ = 0;
   strategy_->Reset();
 }
 
 }  // namespace chromeos