componentize SafeBrowsingService

chrome/browser/safe_browsing/safe_browsing_service.h

Index: chrome/browser/safe_browsing/safe_browsing_service.h
diff --git a/chrome/browser/safe_browsing/safe_browsing_service.h b/chrome/browser/safe_browsing/safe_browsing_service.h
index 666d7aacaeb6124b377c49efd443836bcd46832e..249b9cde37bd07d7e7273aab168c68d1c28b1450 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.h
+++ b/chrome/browser/safe_browsing/safe_browsing_service.h
@@ -13,16 +13,11 @@
 #include <string>
 
 #include "base/callback.h"
-#include "base/callback_list.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
-#include "base/observer_list.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "chrome/browser/safe_browsing/services_delegate.h"
-#include "components/safe_browsing_db/util.h"
-#include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_observer.h"
+#include "components/safe_browsing/base_safe_browsing_service.h"
 #include "content/public/browser/notification_registrar.h"
 
 #if defined(FULL_SAFE_BROWSING)
@@ -63,10 +58,7 @@ struct V4ProtocolConfig;
 // the heavylifting of safebrowsing service. Both of these managers stay
 // alive until SafeBrowsingService is destroyed, however, they are disabled
 // permanently when Shutdown method is called.
-class SafeBrowsingService : public base::RefCountedThreadSafe<
-                                SafeBrowsingService,
-                                content::BrowserThread::DeleteOnUIThread>,
-                            public content::NotificationObserver {
+class SafeBrowsingService : public BaseSafeBrowsingService {
  public:
   // Makes the passed |factory| the factory used to instanciate
   // a SafeBrowsingService. Useful for tests.
@@ -82,10 +74,10 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
   static SafeBrowsingService* CreateSafeBrowsingService();
 
   // Called on the UI thread to initialize the service.
-  void Initialize();
+  void Initialize() override;
 
   // Called on the main thread to let us know that the io_thread is going away.
-  void ShutDown();
+  void ShutDown() override;
 
   // Called on UI thread to decide if the download file's sha256 hash
   // should be calculated for safebrowsing.
@@ -97,21 +89,6 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
   // Create a v4 protocol config struct.
   virtual V4ProtocolConfig GetV4ProtocolConfig() const;
 
-  // Returns the client_name field for both V3 and V4 protocol manager configs.
-  std::string GetProtocolConfigClientName() const;
-
-  // Get current enabled status. Must be called on IO thread.
-  bool enabled() const {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
-    return enabled_;
-  }
-
-  // Whether the service is enabled by the current set of profiles.
-  bool enabled_by_prefs() const {
-    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-    return enabled_by_prefs_;
-  }
-
   ClientSideDetectionService* safe_browsing_detection_service() const {
     return services_delegate_->GetCsdService();
   }
@@ -122,13 +99,14 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
     return services_delegate_->GetDownloadService();
   }
 
-  scoped_refptr<net::URLRequestContextGetter> url_request_context();
+  scoped_refptr<net::URLRequestContextGetter> url_request_context() override;
 
   const scoped_refptr<SafeBrowsingUIManager>& ui_manager() const;
 
   // This returns either the v3 or the v4 database manager, depending on
   // the experiment settings.
-  const scoped_refptr<SafeBrowsingDatabaseManager>& database_manager() const;
+  const scoped_refptr<SafeBrowsingDatabaseManager>& database_manager()
+      const override;
 
   scoped_refptr<SafeBrowsingNavigationObserverManager>
   navigation_observer_manager();
@@ -186,7 +164,7 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
 
   ~SafeBrowsingService() override;
 
-  virtual SafeBrowsingDatabaseManager* CreateDatabaseManager();
+  SafeBrowsingDatabaseManager* CreateDatabaseManager() override;
 
   virtual SafeBrowsingUIManager* CreateUIManager();
 
@@ -199,37 +177,26 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
 
   std::unique_ptr<ServicesDelegate> services_delegate_;
 
- private:
-  friend class SafeBrowsingServiceFactoryImpl;
-  friend struct content::BrowserThread::DeleteOnThread<
-      content::BrowserThread::UI>;
-  friend class base::DeleteHelper<SafeBrowsingService>;
-  friend class SafeBrowsingServerTest;
-  friend class SafeBrowsingServiceTest;
-  friend class SafeBrowsingURLRequestContextGetter;
-  friend class TestSafeBrowsingService;
-  friend class TestSafeBrowsingServiceFactory;
-
   // Called to initialize objects that are used on the io_thread.  This may be
   // called multiple times during the life of the SafeBrowsingService.
   void StartOnIOThread(
-      net::URLRequestContextGetter* url_request_context_getter);
+      net::URLRequestContextGetter* url_request_context_getter) override;

[Jialiu Lin, 2016/12/29 17:41:37]

Any particular reason for making StartOnIOThread(...), StopOnIOThread(...)
Start(), Stop(), Observe(...), AddPrefService(), RemovePrefService, and
RefreshState() public?

[Nate Fischer, 2017/01/03 22:39:52]

I originally moved them to protected so that the subclass can inherit them.
Since the base class has only dummy implementations, I just moved them back to
where they were before. If the base class really does need these methods, I'll
add them back to the base as either private or protected.

 
   // Called to stop or shutdown operations on the io_thread. This may be called
   // multiple times to stop during the life of the SafeBrowsingService. If
   // shutdown is true, then the operations on the io thread are shutdown
   // permanently and cannot be restarted.
-  void StopOnIOThread(bool shutdown);
+  void StopOnIOThread(bool shutdown) override;
 
   // Start up SafeBrowsing objects. This can be called at browser start, or when
   // the user checks the "Enable SafeBrowsing" option in the Advanced options
   // UI.
-  void Start();
+  void Start() override;
 
   // Stops the SafeBrowsingService. This can be called when the safe browsing
   // preference is disabled. When shutdown is true, operation is permanently
   // shutdown and cannot be restarted.
-  void Stop(bool shutdown);
+  void Stop(bool shutdown) override;
 
   // content::NotificationObserver override
   void Observe(int type,
@@ -244,7 +211,16 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
 
   // Checks if any profile is currently using the safe browsing service, and
   // starts or stops the service accordingly.
-  void RefreshState();
+  void RefreshState() override;
+
+ private:
+  friend class SafeBrowsingServiceFactoryImpl;
+  friend class base::DeleteHelper<SafeBrowsingService>;
+  friend class SafeBrowsingServerTest;
+  friend class SafeBrowsingServiceTest;
+  friend class SafeBrowsingURLRequestContextGetter;
+  friend class TestSafeBrowsingService;
+  friend class TestSafeBrowsingServiceFactory;
 
   void OnSendSerializedDownloadReport(const std::string& report);
 
@@ -269,18 +245,6 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
   // Provides phishing and malware statistics. Accessed on IO thread.
   std::unique_ptr<SafeBrowsingPingManager> ping_manager_;
 
-  // Whether the service is running. 'enabled_' is used by SafeBrowsingService
-  // on the IO thread during normal operations.
-  bool enabled_;
-
-  // Whether SafeBrowsing is enabled by the current set of profiles.
-  // Accessed on UI thread.
-  bool enabled_by_prefs_;
-
-  // Whether SafeBrowsing needs to be enabled in V4Only mode. In this mode, all
-  // SafeBrowsing decisions are made using the PVer4 implementation.
-  bool enabled_v4_only_;

[Jialiu Lin, 2016/12/29 17:41:37]

It probably makes more sense to keep enabled_v4_only_ in the subclass. Since
WebView doesn't need to know whether it is V3 or V4. WDYT?

[Nate Fischer, 2017/01/03 22:39:52]

Sounds good to me. I also moved enabled_by_prefs_, since it was also only used
in the subclass. Let me know if I should move it back to components layer.

-
   // Tracks existing PrefServices, and the safe browsing preference on each.
   // This is used to determine if any profile is currently using the safe
   // browsing service, and to start it up or shut it down accordingly.