CaptivePortalService: Introduce ability to register for callbacks

We are in the process of componentizing CaptivePortalService. As part
of this we need to eliminate its sending of a notification: as there
is no enum of notification types in //components and we don't want to
introduce one when notifications have been deprecated for 7+ years.

This CL starts the process of that elimination by introducing the
ability to register for callbacks when CaptivePortalService completes a
query, notifying the callbacks at the same time as the notification,
and porting a couple of initial consumers to validate the approach.
There remain a handful of consumers to be converted.

Bug: 1030692
Change-Id: I2bc682a092fac0c3ab2ebf4b152d6ee62e80d324
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2010775
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734039}

chrome/browser/captive_portal/captive_portal_browsertest.cc

@@ -370,7 +370,7 @@ void FailLoadsAfterLoginObserver::Observe(
 
 // An observer for watching the CaptivePortalService.  It tracks the last
 // received result and the total number of received results.
-class CaptivePortalObserver : public content::NotificationObserver {
+class CaptivePortalObserver {
  public:
   explicit CaptivePortalObserver(Profile* profile);
 
@@ -387,9 +387,7 @@ class CaptivePortalObserver : public content::NotificationObserver {
 
  private:
   // Records results and exits the message loop, if needed.
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
+  void Observe(const CaptivePortalService::Results& results);
 
   // Number of times OnPortalResult has been called since construction.
   int num_results_received_;
@@ -401,15 +399,13 @@ class CaptivePortalObserver : public content::NotificationObserver {
   bool waiting_for_result_;
   std::unique_ptr<base::RunLoop> run_loop_;
 
-  Profile* profile_;
-
   CaptivePortalService* captive_portal_service_;
 
+  std::unique_ptr<CaptivePortalService::Subscription> subscription_;
+
   // Last result received.
   CaptivePortalResult captive_portal_result_;
 
-  content::NotificationRegistrar registrar_;
-
   DISALLOW_COPY_AND_ASSIGN(CaptivePortalObserver);
 };
 
@@ -417,13 +413,12 @@ CaptivePortalObserver::CaptivePortalObserver(Profile* profile)
     : num_results_received_(0),
       num_results_to_wait_for_(0),
       waiting_for_result_(false),
-      profile_(profile),
       captive_portal_service_(
           CaptivePortalServiceFactory::GetForProfile(profile)),
       captive_portal_result_(
           captive_portal_service_->last_detection_result()) {
-  registrar_.Add(this, chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT,
-                 content::Source<content::BrowserContext>(profile_));
+  subscription_ = captive_portal_service_->RegisterCallback(
+      base::Bind(&CaptivePortalObserver::Observe, base::Unretained(this)));
 }
 
 void CaptivePortalObserver::WaitForResults(int num_results_to_wait_for) {
@@ -440,20 +435,11 @@ void CaptivePortalObserver::WaitForResults(int num_results_to_wait_for) {
 }
 
 void CaptivePortalObserver::Observe(
-    int type,
-    const content::NotificationSource& source,
-    const content::NotificationDetails& details) {
-  ASSERT_EQ(type, chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT);
-  ASSERT_EQ(profile_, content::Source<content::BrowserContext>(source).ptr());
-
-  CaptivePortalService::Results* results =
-      content::Details<CaptivePortalService::Results>(details).ptr();
-
-  EXPECT_EQ(captive_portal_result_, results->previous_result);
-  EXPECT_EQ(captive_portal_service_->last_detection_result(),
-            results->result);
+    const CaptivePortalService::Results& results) {
+  EXPECT_EQ(captive_portal_result_, results.previous_result);
+  EXPECT_EQ(captive_portal_service_->last_detection_result(), results.result);
 
-  captive_portal_result_ = results->result;
+  captive_portal_result_ = results.result;
   ++num_results_received_;
 
   if (waiting_for_result_ &&

chrome/browser/captive_portal/captive_portal_service.cc

@@ -367,6 +367,8 @@ void CaptivePortalService::OnResult(CaptivePortalResult result,
       chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT,
       content::Source<content::BrowserContext>(browser_context_),
       content::Details<Results>(&results));
+
+  callback_list_.Notify(results);
 }
 
 void CaptivePortalService::ResetBackoffEntry(CaptivePortalResult result) {

chrome/browser/captive_portal/captive_portal_service.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "base/callback_list.h"
 #include "base/macros.h"
 #include "base/time/tick_clock.h"
 #include "base/time/time.h"
@@ -28,9 +29,8 @@ class URLLoaderFactory;
 }
 }  // namespace network
 
-// Service that checks for captive portals when queried, and sends a
-// NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT with the BrowserContext as the
-// source and a CaptivePortalService::Results as the details.
+// Service that checks for captive portals when queried and sends updates to
+// its registered consumers when results are obtained.
 //
 // Captive portal checks are rate-limited.  The CaptivePortalService may only
 // be accessed on the UI thread.
@@ -47,7 +47,7 @@ class CaptivePortalService : public KeyedService {
                                  // implies SKIP_OS_CHECK_FOR_TESTING.
   };
 
-  // The details sent via a NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT.
+  // The details sent to consumers on a query completing.
   struct Results {
     // The result of the second most recent captive portal check.
     captive_portal::CaptivePortalResult previous_result;
@@ -58,6 +58,8 @@ class CaptivePortalService : public KeyedService {
     GURL landing_url;
   };
 
+  using Subscription = base::CallbackList<void(const Results&)>::Subscription;
+
   CaptivePortalService(
       content::BrowserContext* browser_context,
       PrefService* pref_service,
@@ -70,6 +72,11 @@ class CaptivePortalService : public KeyedService {
   // Always sends the result notification asynchronously.
   void DetectCaptivePortal();
 
+  std::unique_ptr<Subscription> RegisterCallback(
+      const base::RepeatingCallback<void(const Results&)>& cb) {
+    return callback_list_.Add(cb);
+  }
+
   // Returns the URL used for captive portal testing.  When a captive portal is
   // detected, this URL will take us to the captive portal landing page.
   const GURL& test_url() const { return test_url_; }
@@ -180,6 +187,8 @@ class CaptivePortalService : public KeyedService {
   // The result of the most recent captive portal check.
   captive_portal::CaptivePortalResult last_detection_result_;
 
+  base::CallbackList<void(const Results&)> callback_list_;
+
   // Number of sequential checks with the same captive portal result.
   int num_checks_with_same_result_;
 

chrome/browser/captive_portal/captive_portal_service_unittest.cc

@@ -10,17 +10,13 @@
 #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/browser/captive_portal/captive_portal_service.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/captive_portal/captive_portal_testing_utils.h"
 #include "components/embedder_support/pref_names.h"
 #include "components/prefs/pref_service.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
-#include "content/public/browser/notification_source.h"
 #include "content/public/test/browser_task_environment.h"
 #include "net/base/net_errors.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -32,18 +28,15 @@ namespace {
 
 // An observer watches the CaptivePortalDetector.  It tracks the last
 // received result and the total number of received results.
-class CaptivePortalObserver : public content::NotificationObserver {
+class CaptivePortalObserver {
  public:
-  CaptivePortalObserver(Profile* profile,
-                        CaptivePortalService* captive_portal_service)
-      : captive_portal_result_(
-            captive_portal_service->last_detection_result()),
+  explicit CaptivePortalObserver(CaptivePortalService* captive_portal_service)
+      : captive_portal_result_(captive_portal_service->last_detection_result()),
         num_results_received_(0),
-        profile_(profile),
-        captive_portal_service_(captive_portal_service) {
-    registrar_.Add(this, chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT,
-                   content::Source<content::BrowserContext>(profile_));
-  }
+        captive_portal_service_(captive_portal_service),
+        subscription_(captive_portal_service->RegisterCallback(
+            base::Bind(&CaptivePortalObserver::Observe,
+                       base::Unretained(this)))) {}
 
   CaptivePortalResult captive_portal_result() const {
     return captive_portal_result_;
@@ -52,30 +45,20 @@ class CaptivePortalObserver : public content::NotificationObserver {
   int num_results_received() const { return num_results_received_; }
 
  private:
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override {
-    ASSERT_EQ(type, chrome::NOTIFICATION_CAPTIVE_PORTAL_CHECK_RESULT);
-    ASSERT_EQ(profile_, content::Source<content::BrowserContext>(source).ptr());
-
-    CaptivePortalService::Results *results =
-        content::Details<CaptivePortalService::Results>(details).ptr();
-
-    EXPECT_EQ(captive_portal_result_, results->previous_result);
-    EXPECT_EQ(captive_portal_service_->last_detection_result(),
-              results->result);
+  void Observe(const CaptivePortalService::Results& results) {
+    EXPECT_EQ(captive_portal_result_, results.previous_result);
+    EXPECT_EQ(captive_portal_service_->last_detection_result(), results.result);
 
-    captive_portal_result_ = results->result;
+    captive_portal_result_ = results.result;
     ++num_results_received_;
   }
 
   CaptivePortalResult captive_portal_result_;
   int num_results_received_;
 
-  Profile* profile_;
   CaptivePortalService* captive_portal_service_;
 
-  content::NotificationRegistrar registrar_;
+  std::unique_ptr<CaptivePortalService::Subscription> subscription_;
 
   DISALLOW_COPY_AND_ASSIGN(CaptivePortalObserver);
 };
@@ -164,7 +147,7 @@ class CaptivePortalServiceTest : public testing::Test,
     AdvanceTime(expected_delay);
     ASSERT_EQ(base::TimeDelta(), GetTimeUntilNextRequest());
 
-    CaptivePortalObserver observer(profile(), service());
+    CaptivePortalObserver observer(service());
     service()->DetectCaptivePortal();
 
     EXPECT_EQ(CaptivePortalService::STATE_TIMER_RUNNING, service()->state());
@@ -196,7 +179,7 @@ class CaptivePortalServiceTest : public testing::Test,
     AdvanceTime(expected_delay);
     ASSERT_EQ(base::TimeDelta(), GetTimeUntilNextRequest());
 
-    CaptivePortalObserver observer(profile(), service());
+    CaptivePortalObserver observer(service());
     service()->DetectCaptivePortal();
 
     EXPECT_EQ(CaptivePortalService::STATE_TIMER_RUNNING, service()->state());
@@ -295,7 +278,7 @@ TEST_F(CaptivePortalServiceTest, CaptivePortalTwoProfiles) {
   TestingProfile profile2;
   std::unique_ptr<CaptivePortalService> service2(
       new CaptivePortalService(&profile2, profile2.GetPrefs()));
-  CaptivePortalObserver observer2(&profile2, service2.get());
+  CaptivePortalObserver observer2(service2.get());
 
   RunTest(captive_portal::RESULT_INTERNET_CONNECTED, net::OK, 204, 0, nullptr);
   EXPECT_EQ(0, observer2.num_results_received());
@@ -376,7 +359,7 @@ TEST_F(CaptivePortalServiceTest, CaptivePortalPrefDisabled) {
 // works.
 TEST_F(CaptivePortalServiceTest, CaptivePortalPrefDisabledWhileRunning) {
   Initialize(CaptivePortalService::SKIP_OS_CHECK_FOR_TESTING);
-  CaptivePortalObserver observer(profile(), service());
+  CaptivePortalObserver observer(service());
 
   // Needed to create the URLFetcher, even if it never returns any results.
   service()->DetectCaptivePortal();
@@ -406,7 +389,7 @@ TEST_F(CaptivePortalServiceTest, CaptivePortalPrefDisabledWhilePending) {
   Initialize(CaptivePortalService::SKIP_OS_CHECK_FOR_TESTING);
   set_initial_backoff_no_portal(base::TimeDelta::FromDays(1));
 
-  CaptivePortalObserver observer(profile(), service());
+  CaptivePortalObserver observer(service());
   service()->DetectCaptivePortal();
   EXPECT_FALSE(FetchingURL());
   EXPECT_TRUE(TimerRunning());
@@ -434,7 +417,7 @@ TEST_F(CaptivePortalServiceTest, CaptivePortalPrefEnabledWhilePending) {
   EnableCaptivePortalDetectionPreference(false);
   RunDisabledTest(0);
 
-  CaptivePortalObserver observer(profile(), service());
+  CaptivePortalObserver observer(service());
   service()->DetectCaptivePortal();
   EXPECT_FALSE(FetchingURL());
   EXPECT_TRUE(TimerRunning());

chrome/browser/chrome_notification_types.h

@@ -197,6 +197,10 @@ enum NotificationType {
   NOTIFICATION_TAB_DRAG_LOOP_DONE,
 #endif
 
+  // DEPRECATED: Instead of listening this notification, use
+  // CaptivePortalService::RegisterCallback().
+  // TODO(blundell): Delete this notification as part of
+  // https://crbug.com/1030692.
   // Sent when the CaptivePortalService checks if we're behind a captive portal.
   // The Source is the Profile the CaptivePortalService belongs to, and the
   // Details are a Details<CaptivePortalService::CheckResults>.