Remove notification from Safe Browsing.

BUG=268984
TEST=no changes
TBR=sky@chromium.org

Change-Id: I80b45655bf30d714458494220bd36fce745e0d98
Reviewed-on: https://chromium-review.googlesource.com/979192Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545963}

chrome/browser/extensions/blacklist.cc

@@ -8,6 +8,7 @@
 #include <iterator>
 
 #include "base/bind.h"
+#include "base/callback_list.h"
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
@@ -19,11 +20,8 @@
 #include "chrome/browser/extensions/blacklist_state_fetcher.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "components/prefs/pref_service.h"
-#include "components/safe_browsing/db/notification_types.h"
 #include "components/safe_browsing/db/util.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_source.h"
 #include "extensions/browser/extension_prefs.h"
 
 using content::BrowserThread;
@@ -53,10 +51,17 @@ class LazySafeBrowsingDatabaseManager {
 
   void set(scoped_refptr<SafeBrowsingDatabaseManager> instance) {
     instance_ = instance;
+    database_changed_callback_list_.Notify();
+  }
+
+  std::unique_ptr<base::CallbackList<void()>::Subscription>
+  RegisterDatabaseChangedCallback(const base::RepeatingClosure& cb) {
+    return database_changed_callback_list_.Add(cb);
   }
 
  private:
   scoped_refptr<SafeBrowsingDatabaseManager> instance_;
+  base::CallbackList<void()> database_changed_callback_list_;
 };
 
 static base::LazyInstance<LazySafeBrowsingDatabaseManager>::DestructorAtExit
@@ -160,13 +165,14 @@ Blacklist::ScopedDatabaseManagerForTest::~ScopedDatabaseManagerForTest() {
 }
 
 Blacklist::Blacklist(ExtensionPrefs* prefs) {
-  scoped_refptr<SafeBrowsingDatabaseManager> database_manager =
-      GetDatabaseManager();
-  if (database_manager.get()) {
-    registrar_.Add(
-        this, safe_browsing::NOTIFICATION_SAFE_BROWSING_UPDATE_COMPLETE,
-        content::Source<SafeBrowsingDatabaseManager>(database_manager.get()));
-  }
+  auto& lazy_database_manager = g_database_manager.Get();
+  // Using base::Unretained is safe because when this object goes away, the
+  // subscription will automatically be destroyed.
+  database_changed_subscription_ =
+      lazy_database_manager.RegisterDatabaseChangedCallback(base::BindRepeating(
+          &Blacklist::ObserveNewDatabase, base::Unretained(this)));
+
+  ObserveNewDatabase();
 }
 
 Blacklist::~Blacklist() {
@@ -310,6 +316,10 @@ BlacklistStateFetcher* Blacklist::ResetBlacklistStateFetcherForTest() {
   return state_fetcher_.release();
 }
 
+void Blacklist::ResetDatabaseUpdatedListenerForTest() {
+  database_updated_subscription_.reset();
+}
+
 void Blacklist::AddObserver(Observer* observer) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   observers_.AddObserver(observer);
@@ -331,10 +341,21 @@ scoped_refptr<SafeBrowsingDatabaseManager> Blacklist::GetDatabaseManager() {
   return g_database_manager.Get().get();
 }
 
-void Blacklist::Observe(int type,
-                        const content::NotificationSource& unused_source,
-                        const content::NotificationDetails& unused_details) {
-  DCHECK_EQ(safe_browsing::NOTIFICATION_SAFE_BROWSING_UPDATE_COMPLETE, type);
+void Blacklist::ObserveNewDatabase() {
+  auto database_manager = GetDatabaseManager();
+  if (database_manager.get()) {
+    // Using base::Unretained is safe because when this object goes away, the
+    // subscription to the callback list will automatically be destroyed.
+    database_updated_subscription_ =
+        database_manager.get()->RegisterDatabaseUpdatedCallback(
+            base::BindRepeating(&Blacklist::NotifyObservers,
+                                base::Unretained(this)));
+  } else {
+    database_updated_subscription_.reset();
+  }
+}
+
+void Blacklist::NotifyObservers() {
   for (auto& observer : observers_)
     observer.OnBlacklistUpdated();
 }

chrome/browser/extensions/blacklist.h

@@ -13,13 +13,12 @@
 #include <vector>
 
 #include "base/callback.h"
+#include "base/callback_list.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/safe_browsing/db/database_manager.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "extensions/browser/blacklist_state.h"
 
 namespace content {
@@ -33,7 +32,6 @@ class ExtensionPrefs;
 
 // The blacklist of extensions backed by safe browsing.
 class Blacklist : public KeyedService,
-                  public content::NotificationObserver,
                   public base::SupportsWeakPtr<Blacklist> {
  public:
   class Observer {
@@ -110,6 +108,9 @@ class Blacklist : public KeyedService,
   // BlacklistStateFetcher.
   BlacklistStateFetcher* ResetBlacklistStateFetcherForTest();
 
+  // Reset the listening for an updated database.
+  void ResetDatabaseUpdatedListenerForTest();
+
   // Adds/removes an observer to the blacklist.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
@@ -122,10 +123,9 @@ class Blacklist : public KeyedService,
   static scoped_refptr<safe_browsing::SafeBrowsingDatabaseManager>
       GetDatabaseManager();
 
-  // content::NotificationObserver
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
+  void ObserveNewDatabase();
+
+  void NotifyObservers();
 
   void GetBlacklistStateForIDs(const GetBlacklistedIDsCallback& callback,
                                const std::set<std::string>& blacklisted_ids);
@@ -140,7 +140,10 @@ class Blacklist : public KeyedService,
 
   base::ObserverList<Observer> observers_;
 
-  content::NotificationRegistrar registrar_;
+  std::unique_ptr<base::CallbackList<void()>::Subscription>
+      database_updated_subscription_;
+  std::unique_ptr<base::CallbackList<void()>::Subscription>
+      database_changed_subscription_;
 
   // The cached BlacklistState's, received from BlacklistStateFetcher.
   BlacklistStateMap blacklist_state_cache_;

chrome/browser/extensions/test_blacklist.cc

@@ -77,6 +77,7 @@ void TestBlacklist::Attach(Blacklist* blacklist) {
 
 void TestBlacklist::Detach() {
   blacklist_->ResetBlacklistStateFetcherForTest();
+  blacklist_->ResetDatabaseUpdatedListenerForTest();
 }
 
 void TestBlacklist::SetBlacklistState(const std::string& extension_id,

chrome/browser/safe_browsing/local_database_manager.cc

@@ -35,10 +35,8 @@
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "components/safe_browsing/common/safebrowsing_switches.h"
-#include "components/safe_browsing/db/notification_types.h"
 #include "components/safe_browsing/db/util.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_service.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "url/url_constants.h"
 
@@ -693,10 +691,7 @@ void LocalSafeBrowsingDatabaseManager::StopOnIOThread(bool shutdown) {
 void LocalSafeBrowsingDatabaseManager::NotifyDatabaseUpdateFinished(
     bool update_succeeded) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  content::NotificationService::current()->Notify(
-      NOTIFICATION_SAFE_BROWSING_UPDATE_COMPLETE,
-      content::Source<SafeBrowsingDatabaseManager>(this),
-      content::Details<bool>(&update_succeeded));
+  update_complete_callback_list_.Notify();
 }
 
 LocalSafeBrowsingDatabaseManager::QueuedCheck::QueuedCheck(

chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc

@@ -68,7 +68,6 @@
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/db/database_manager.h"
 #include "components/safe_browsing/db/metadata.pb.h"
-#include "components/safe_browsing/db/notification_types.h"
 #include "components/safe_browsing/db/test_database_manager.h"
 #include "components/safe_browsing/db/util.h"
 #include "components/safe_browsing/db/v4_database.h"
@@ -2114,15 +2113,18 @@ class SafeBrowsingDatabaseManagerCookieTest : public InProcessBrowserTest {
 // and can save cookies.
 IN_PROC_BROWSER_TEST_F(SafeBrowsingDatabaseManagerCookieTest,
                        TestSBUpdateCookies) {
-  content::WindowedNotificationObserver observer(
-      NOTIFICATION_SAFE_BROWSING_UPDATE_COMPLETE,
-      content::Source<SafeBrowsingDatabaseManager>(
-          sb_factory_->test_safe_browsing_service()->database_manager().get()));
+  base::RunLoop run_loop;
+  auto callback_subscription =
+      sb_factory_->test_safe_browsing_service()
+          ->database_manager()
+          .get()
+          ->RegisterDatabaseUpdatedCallback(run_loop.QuitClosure());
+
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::BindOnce(&SafeBrowsingDatabaseManagerCookieTest::ForceUpdate,
                      base::Unretained(this)));
-  observer.Wait();
+  run_loop.Run();
 }
 
 // Tests the safe browsing blocking page in a browser.

chrome/browser/ui/global_error/global_error_browsertest.cc

@@ -128,7 +128,7 @@ void GlobalErrorBubbleTest::ShowUi(const std::string& name) {
     // delay, but some tasks run on the IO thread, so post a task there to
     // ensure it was flushed.
     // The test also needs to invoke OnBlacklistUpdated() directly. Usually this
-    // happens via NOTIFICATION_SAFE_BROWSING_UPDATE_COMPLETE but TestBlacklist
+    // happens via a callback from the SafeBrowsing DB, but TestBlacklist
     // replaced the SafeBrowsing DB with a fake one, so the notification source
     // is different.
     static_cast<extensions::Blacklist::Observer*>(extension_service)

components/safe_browsing/db/database_manager.cc

@@ -140,6 +140,12 @@ void SafeBrowsingDatabaseManager::StopOnIOThread(bool shutdown) {
   v4_get_hash_protocol_manager_.reset();
 }
 
+std::unique_ptr<base::CallbackList<void()>::Subscription>
+SafeBrowsingDatabaseManager::RegisterDatabaseUpdatedCallback(
+    const OnDatabaseUpdated& cb) {
+  return update_complete_callback_list_.Add(cb);
+}
+
 SafeBrowsingDatabaseManager::SafeBrowsingApiCheck::SafeBrowsingApiCheck(
     const GURL& url,
     Client* client)

components/safe_browsing/db/database_manager.h

@@ -14,6 +14,7 @@
 #include <unordered_set>
 #include <vector>
 
+#include "base/callback_list.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted_delete_on_sequence.h"
@@ -221,6 +222,17 @@ class SafeBrowsingDatabaseManager
       net::URLRequestContextGetter* request_context_getter,
       const V4ProtocolConfig& config);
 
+  //
+  // Method to manage getting database updates of the DatabaseManager.
+  //
+
+  // Subscribe to receive callbacks when the database is updated, both initially
+  // when it's loaded from disk at startup, and then periodically. These
+  // callbacks will be on the UI thread.
+  using OnDatabaseUpdated = base::RepeatingClosure;
+  std::unique_ptr<base::CallbackList<void()>::Subscription>
+  RegisterDatabaseUpdatedCallback(const OnDatabaseUpdated& cb);
+
   // Called to stop or shutdown operations on the io_thread. All subclasses
   // should override this method, set enabled_ to false and call the base class
   // method at the bottom of it.
@@ -289,6 +301,9 @@ class SafeBrowsingDatabaseManager
   // Created and destroyed via StartOnIOThread/StopOnIOThread.
   std::unique_ptr<V4GetHashProtocolManager> v4_get_hash_protocol_manager_;
 
+  // A list of parties to be notified about database updates.
+  base::CallbackList<void()> update_complete_callback_list_;
+
  private:
   // Returns an iterator to the pending API check with the given |client|.
   ApiCheckSet::iterator FindClientApiCheck(Client* client);

components/safe_browsing/db/notification_types.h

-// Copyright (c) 2017 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 COMPONENTS_SAFE_BROWSING_DB_NOTIFICATION_TYPES_H_
-#define COMPONENTS_SAFE_BROWSING_DB_NOTIFICATION_TYPES_H_
-
-// **
-// ** NOTICE
-// **
-// ** The notification system is deprecated, obsolete, and is slowly being
-// ** removed. See https://crbug.com/268984.
-// **
-// ** Please don't add any new notification types, and please help migrate
-// ** existing uses of the notification types below to use the Observer and
-// ** Callback patterns.
-// **
-
-// Lists of Safe Browsing related notifications, used with NotificationService.
-namespace safe_browsing {
-
-enum NotificationType {
-  NOTIFICATION_SAFE_BROWSING_START = 0,
-
-  // General -----------------------------------------------------------------
-
-  // Special signal value to represent an interest in all notifications.
-  // Not valid when posting a notification.
-  NOTIFICATION_ALL = NOTIFICATION_SAFE_BROWSING_START,
-
-  // A safe browsing database update completed.  Source is the
-  // SafeBrowsingDatabaseManager and the details are none. It is posted on the
-  // UI thread.
-  NOTIFICATION_SAFE_BROWSING_UPDATE_COMPLETE,
-
-  NOTIFICATION_SAFE_BROWSING_END
-};
-
-}  // namespace safe_browsing
-
-// **
-// ** NOTICE
-// **
-// ** The notification system is deprecated, obsolete, and is slowly being
-// ** removed. See https://crbug.com/268984.
-// **
-// ** Please don't add any new notification types, and please help migrate
-// ** existing uses of the notification types below to use the Observer and
-// ** Callback patterns.
-// **
-
-#endif  // COMPONENTS_SAFE_BROWSING_DB_NOTIFICATION_TYPES_H_

components/safe_browsing/db/v4_local_database_manager.cc

@@ -19,11 +19,9 @@
 #include "base/strings/string_util.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/timer/elapsed_timer.h"
-#include "components/safe_browsing/db/notification_types.h"
 #include "components/safe_browsing/db/v4_feature_list.h"
 #include "components/safe_browsing/db/v4_protocol_manager_util.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_service.h"
 #include "crypto/sha2.h"
 
 using content::BrowserThread;
@@ -561,21 +559,14 @@ void V4LocalDatabaseManager::DatabaseUpdated() {
 
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
-        base::Bind(&V4LocalDatabaseManager::PostUpdateNotificationOnUIThread,
-                   content::Source<SafeBrowsingDatabaseManager>(this)));
+        base::BindOnce(
+            &V4LocalDatabaseManager::PostUpdateNotificationOnUIThread, this));
   }
 }
 
-// static
-void V4LocalDatabaseManager::PostUpdateNotificationOnUIThread(
-    const content::NotificationSource& source) {
+void V4LocalDatabaseManager::PostUpdateNotificationOnUIThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  // The notification needs to be posted on the UI thread because the extension
-  // checker is observing UI thread's notification service.
-  content::NotificationService::current()->Notify(
-      NOTIFICATION_SAFE_BROWSING_UPDATE_COMPLETE, source,
-      content::NotificationService::NoDetails());
+  update_complete_callback_list_.Notify();
 }
 
 void V4LocalDatabaseManager::DeletePVer3StoreFiles() {

components/safe_browsing/db/v4_local_database_manager.h

@@ -19,7 +19,6 @@
 #include "components/safe_browsing/db/v4_protocol_manager_util.h"
 #include "components/safe_browsing/db/v4_update_protocol_manager.h"
 #include "components/safe_browsing/proto/webui.pb.h"
-#include "content/public/browser/notification_service.h"
 #include "url/gurl.h"
 
 namespace safe_browsing {
@@ -278,11 +277,10 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
                                     const FullHashToStoreAndHashPrefixesMap&
                                         full_hash_to_store_and_hash_prefixes);
 
-  // Post a notification about the completion of database update process.
-  // This is currently used by the extension blacklist checker to disable any
-  // installed extensions that have been blacklisted since.
-  static void PostUpdateNotificationOnUIThread(
-      const content::NotificationSource& source);
+  // Make callbacks about the completion of database update process. This is
+  // currently used by the extension blacklist checker to disable any installed
+  // extensions that have been blacklisted since.
+  void PostUpdateNotificationOnUIThread();
 
   // When the database is ready to use, process the checks that were queued
   // while the database was loading from disk.

components/safe_browsing/db/v4_local_database_manager_unittest.cc

@@ -13,7 +13,6 @@
 #include "base/run_loop.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
-#include "components/safe_browsing/db/notification_types.h"
 #include "components/safe_browsing/db/v4_database.h"
 #include "components/safe_browsing/db/v4_protocol_manager_util.h"
 #include "components/safe_browsing/db/v4_test_util.h"
@@ -1114,10 +1113,10 @@ TEST_F(V4LocalDatabaseManagerTest, DeleteUnusedStoreFileRandomFileNotDeleted) {
 }
 
 TEST_F(V4LocalDatabaseManagerTest, NotificationOnUpdate) {
-  content::WindowedNotificationObserver observer(
-      NOTIFICATION_SAFE_BROWSING_UPDATE_COMPLETE,
-      content::Source<SafeBrowsingDatabaseManager>(
-          v4_local_database_manager_.get()));
+  base::RunLoop run_loop;
+  auto callback_subscription =
+      v4_local_database_manager_->RegisterDatabaseUpdatedCallback(
+          run_loop.QuitClosure());
 
   // Creates and associates a V4Database instance.
   StoreAndHashPrefixes store_and_hash_prefixes;
@@ -1125,8 +1124,7 @@ TEST_F(V4LocalDatabaseManagerTest, NotificationOnUpdate) {
 
   v4_local_database_manager_->DatabaseUpdated();
 
-  // This observer waits until it receives the notification.
-  observer.Wait();
+  run_loop.Run();
 }
 
 }  // namespace safe_browsing