[Safe Browsing] Abstract sync dep from RealTimeURLLookupServiceBase

This CL completes the abstraction of signin & sync deps from
//components/safe_browsing/core/realtime by abstracting the usage of
SyncService to determine whether history sync is enabled. As with the
other abstracted dependencies, this information is now obtained via a
callback that is passed in from the embedder. With this CL, WebLayer
can now use this component without needing to reference signin or sync
at all.

There is no behavioral change in this CL.

Bug: 1080748
Change-Id: I320e7f225fb65258862768e411dd85c51dfac9be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2624211
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843476}

chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service.cc

@@ -18,7 +18,6 @@
 #include "components/safe_browsing/core/realtime/policy_engine.h"
 #include "components/safe_browsing/core/realtime/url_lookup_service_base.h"
 #include "components/safe_browsing/core/verdict_cache_manager.h"
-#include "components/sync/driver/sync_service.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "url/gurl.h"
@@ -30,7 +29,7 @@ ChromeEnterpriseRealTimeUrlLookupService::
         scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
         VerdictCacheManager* cache_manager,
         Profile* profile,
-        syncer::SyncService* sync_service,
+        const IsHistorySyncEnabledCallback& is_history_sync_enabled_callback,
         enterprise_connectors::ConnectorsService* connectors_service,
         PrefService* pref_service,
         const ChromeUserPopulation::ProfileManagementStatus&
@@ -39,7 +38,7 @@ ChromeEnterpriseRealTimeUrlLookupService::
         bool is_off_the_record)
     : RealTimeUrlLookupServiceBase(url_loader_factory,
                                    cache_manager,
-                                   sync_service,
+                                   is_history_sync_enabled_callback,
                                    pref_service,
                                    profile_management_status,
                                    is_under_advanced_protection,

chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service.h

@@ -21,10 +21,6 @@ namespace network {
 class SharedURLLoaderFactory;
 }  // namespace network
 
-namespace syncer {
-class SyncService;
-}
-
 class PrefService;
 
 class Profile;
@@ -41,7 +37,7 @@ class ChromeEnterpriseRealTimeUrlLookupService
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       VerdictCacheManager* cache_manager,
       Profile* profile,
-      syncer::SyncService* sync_service,
+      const IsHistorySyncEnabledCallback& is_history_sync_enabled_callback,
       enterprise_connectors::ConnectorsService* connectors_service,
       PrefService* pref_service,
       const ChromeUserPopulation::ProfileManagementStatus&

chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service_factory.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/safe_browsing/verdict_cache_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/safe_browsing/core/browser/sync/sync_utils.h"
 #include "components/safe_browsing/core/common/utils.h"
 #include "components/safe_browsing/core/features.h"
 #include "components/safe_browsing/core/verdict_cache_manager.h"
@@ -70,7 +71,8 @@ ChromeEnterpriseRealTimeUrlLookupServiceFactory::BuildServiceInstanceFor(
   return new ChromeEnterpriseRealTimeUrlLookupService(
       network::SharedURLLoaderFactory::Create(std::move(url_loader_factory)),
       VerdictCacheManagerFactory::GetForProfile(profile), profile,
-      ProfileSyncServiceFactory::GetForProfile(profile),
+      base::BindRepeating(&safe_browsing::SyncUtils::IsHistorySyncEnabled,
+                          ProfileSyncServiceFactory::GetForProfile(profile)),
       enterprise_connectors::ConnectorsServiceFactory::GetForBrowserContext(
           profile),
       profile->GetPrefs(), GetProfileManagementStatus(browser_policy_connector),

chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/safe_browsing/chrome_enterprise_url_lookup_service.h"
 
+#include "base/bind.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/mock_callback.h"
 #include "chrome/browser/enterprise/connectors/connectors_service.h"
@@ -12,6 +13,7 @@
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/policy/core/common/cloud/dm_token.h"
 #include "components/policy/core/common/policy_types.h"
+#include "components/safe_browsing/core/browser/sync/sync_utils.h"
 #include "components/safe_browsing/core/common/safe_browsing_prefs.h"
 #include "components/safe_browsing/core/proto/csd.pb.h"
 #include "components/safe_browsing/core/verdict_cache_manager.h"
@@ -56,7 +58,8 @@ class ChromeEnterpriseRealTimeUrlLookupServiceTest : public PlatformTest {
     enterprise_rt_service_ = std::make_unique<
         ChromeEnterpriseRealTimeUrlLookupService>(
         test_shared_loader_factory_, cache_manager_.get(), test_profile_.get(),
-        &test_sync_service_,
+        base::BindRepeating(&safe_browsing::SyncUtils::IsHistorySyncEnabled,
+                            &test_sync_service_),
         enterprise_connectors::ConnectorsServiceFactory::GetForBrowserContext(
             test_profile_.get()),
         &test_pref_service_, ChromeUserPopulation::NOT_MANAGED,

chrome/browser/safe_browsing/url_lookup_service_factory.cc

@@ -72,7 +72,9 @@ KeyedService* RealTimeUrlLookupServiceFactory::BuildServiceInstanceFor(
   return new RealTimeUrlLookupService(
       network::SharedURLLoaderFactory::Create(std::move(url_loader_factory)),
       VerdictCacheManagerFactory::GetForProfile(profile),
-      ProfileSyncServiceFactory::GetForProfile(profile), profile->GetPrefs(),
+      base::BindRepeating(&safe_browsing::SyncUtils::IsHistorySyncEnabled,
+                          ProfileSyncServiceFactory::GetForProfile(profile)),
+      profile->GetPrefs(),
       std::make_unique<SafeBrowsingPrimaryAccountTokenFetcher>(
           IdentityManagerFactory::GetForProfile(profile)),
       base::BindRepeating(&safe_browsing::SyncUtils::

components/safe_browsing/DEPS

 include_rules = [
+  # NOTE: Dependencies on signin and sync are restricted to specific
+  # subdirectories to facilitate reuse of this component on WebLayer, which
+  # doesn't use signin or sync.
   "+components/content_settings/core/browser",
   "+components/grit/components_resources.h",
   "+components/history/core/browser",
@@ -7,8 +10,6 @@ include_rules = [
   "+components/prefs",
   "+components/security_interstitials/content",
   "+components/security_interstitials/core",
-  "+components/signin/public",
-  "+components/sync",
   "+components/sync_preferences/testing_pref_service_syncable.h",
   "+components/unified_consent",
   "+components/user_prefs/user_prefs.h",

components/safe_browsing/content/web_ui/DEPS

@@ -7,4 +7,5 @@ include_rules = [
   "+components/strings/grit/components_strings.h",
   "+components/grit/components_scaled_resources.h",
   "+components/safe_browsing_db",
+  "+components/sync",
 ]

components/safe_browsing/core/browser/safe_browsing_url_checker_impl_unittest.cc

@@ -171,7 +171,9 @@ class MockRealTimeUrlLookupService : public RealTimeUrlLookupService {
       : RealTimeUrlLookupService(
             /*url_loader_factory=*/nullptr,
             /*cache_manager=*/nullptr,
-            /*sync_service=*/nullptr,
+            /*is_history_sync_enabled_callback=*/base::BindRepeating([]() {
+              return false;
+            }),
             /*pref_service=*/nullptr,
             /*token_fetcher=*/nullptr,
             /*client_token_config_callback=*/base::BindRepeating([](bool) {

components/safe_browsing/core/browser/sync/DEPS

+include_rules = [
+  "+components/signin/public",
+  "+components/sync",
+]

components/safe_browsing/core/browser/sync/sync_utils.cc

@@ -47,4 +47,13 @@ bool SyncUtils::AreSigninAndSyncSetUpForSafeBrowsingTokenFetches(
          !sync_service->GetUserSettings()->IsUsingSecondaryPassphrase();
 }
 
+// TODO(bdea): Migrate other SB classes that define this method to call the one
+// here instead.
+bool SyncUtils::IsHistorySyncEnabled(syncer::SyncService* sync_service) {
+  return sync_service && sync_service->IsSyncFeatureActive() &&
+         !sync_service->IsLocalSyncEnabled() &&
+         sync_service->GetActiveDataTypes().Has(
+             syncer::HISTORY_DELETE_DIRECTIVES);
+}
+
 }  // namespace safe_browsing

components/safe_browsing/core/browser/sync/sync_utils.h

@@ -28,6 +28,10 @@ class SyncUtils {
       signin::IdentityManager* identity_manager,
       bool user_has_enabled_enhanced_protection);
 
+  // Returns true iff history sync is enabled in |sync_service|. |sync_service|
+  // may be null, in which case this method returns false.
+  static bool IsHistorySyncEnabled(syncer::SyncService* sync_service);
+
  private:
   // Whether the primary account is signed in. Sync is not required.
   static bool IsPrimaryAccountSignedIn(

components/safe_browsing/core/browser/sync/sync_utils_unittest.cc

@@ -110,4 +110,43 @@ TEST_F(SyncUtilsTest,
       /* user_has_enabled_enhanced_protection=*/false));
 }
 
+TEST_F(SyncUtilsTest, IsHistorySyncEnabled) {
+  syncer::TestSyncService sync_service;
+
+  // By default |sync_service| syncs everything.
+  EXPECT_TRUE(SyncUtils::IsHistorySyncEnabled(&sync_service));
+
+  // Just history being synced should also be sufficient for the method to
+  // return true.
+  sync_service.SetActiveDataTypes(
+      {syncer::ModelType::HISTORY_DELETE_DIRECTIVES});
+  EXPECT_TRUE(SyncUtils::IsHistorySyncEnabled(&sync_service));
+
+  sync_service.SetActiveDataTypes(syncer::ModelTypeSet::All());
+
+  // The method should return false if:
+
+  // The |sync_service| is null.
+  EXPECT_FALSE(SyncUtils::IsHistorySyncEnabled(nullptr));
+
+  // History is not being synced.
+  sync_service.SetActiveDataTypes({syncer::ModelType::AUTOFILL});
+  EXPECT_FALSE(SyncUtils::IsHistorySyncEnabled(&sync_service));
+
+  sync_service.SetActiveDataTypes(syncer::ModelTypeSet::All());
+
+  // Local sync is enabled.
+  sync_service.SetLocalSyncEnabled(true);
+  EXPECT_FALSE(SyncUtils::IsHistorySyncEnabled(&sync_service));
+
+  sync_service.SetLocalSyncEnabled(false);
+
+  // The sync feature is disabled.
+  sync_service.SetDisableReasons(
+      {syncer::SyncService::DISABLE_REASON_USER_CHOICE});
+  EXPECT_FALSE(SyncUtils::IsHistorySyncEnabled(&sync_service));
+
+  sync_service.SetDisableReasons({});
+}
+
 }  // namespace safe_browsing

components/safe_browsing/core/realtime/BUILD.gn

@@ -40,7 +40,6 @@ static_library("url_lookup_service") {
     "//components/safe_browsing/core/common:safe_browsing_prefs",
     "//components/safe_browsing/core/common:thread_utils",
     "//components/safe_browsing/core/db:v4_protocol_manager_util",
-    "//components/sync",
     "//components/variations/service:service",
     "//services/network/public/cpp:cpp",
     "//url:url",
@@ -67,7 +66,6 @@ static_library("url_lookup_service_base") {
     "//components/safe_browsing/core/common:safe_browsing_prefs",
     "//components/safe_browsing/core/common:thread_utils",
     "//components/safe_browsing/core/db:v4_protocol_manager_util",
-    "//components/sync",
     "//services/network/public/cpp:cpp",
     "//url:url",
   ]
@@ -91,7 +89,6 @@ source_set("unit_tests") {
     "//components/safe_browsing/core/common",
     "//components/safe_browsing/core/common:safe_browsing_prefs",
     "//components/safe_browsing/core/common:test_support",
-    "//components/sync:test_support",
     "//components/sync_preferences:test_support",
     "//components/unified_consent",
     "//components/user_prefs",

components/safe_browsing/core/realtime/url_lookup_service.cc

@@ -18,7 +18,6 @@
 #include "components/safe_browsing/core/common/thread_utils.h"
 #include "components/safe_browsing/core/db/v4_protocol_manager_util.h"
 #include "components/safe_browsing/core/realtime/policy_engine.h"
-#include "components/sync/driver/sync_service.h"
 #include "net/base/ip_address.h"
 #include "net/base/load_flags.h"
 #include "net/base/url_util.h"
@@ -32,7 +31,7 @@ namespace safe_browsing {
 RealTimeUrlLookupService::RealTimeUrlLookupService(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     VerdictCacheManager* cache_manager,
-    syncer::SyncService* sync_service,
+    const IsHistorySyncEnabledCallback& is_history_sync_enabled_callback,
     PrefService* pref_service,
     std::unique_ptr<SafeBrowsingTokenFetcher> token_fetcher,
     const ClientConfiguredForTokenFetchesCallback& client_token_config_callback,
@@ -43,12 +42,11 @@ RealTimeUrlLookupService::RealTimeUrlLookupService(
     variations::VariationsService* variations_service)
     : RealTimeUrlLookupServiceBase(url_loader_factory,
                                    cache_manager,
-                                   sync_service,
+                                   is_history_sync_enabled_callback,
                                    pref_service,
                                    profile_management_status,
                                    is_under_advanced_protection,
                                    is_off_the_record),
-      sync_service_(sync_service),
       pref_service_(pref_service),
       token_fetcher_(std::move(token_fetcher)),
       client_token_config_callback_(client_token_config_callback),

components/safe_browsing/core/realtime/url_lookup_service.h

@@ -28,10 +28,6 @@ namespace network {
 class SharedURLLoaderFactory;
 }  // namespace network
 
-namespace syncer {
-class SyncService;
-}
-
 namespace variations {
 class VariationsService;
 }
@@ -59,7 +55,7 @@ class RealTimeUrlLookupService : public RealTimeUrlLookupServiceBase {
   RealTimeUrlLookupService(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       VerdictCacheManager* cache_manager,
-      syncer::SyncService* sync_service,
+      const IsHistorySyncEnabledCallback& is_history_sync_enabled_callback,
       PrefService* pref_service,
       std::unique_ptr<SafeBrowsingTokenFetcher> token_fetcher,
       const ClientConfiguredForTokenFetchesCallback&
@@ -95,9 +91,6 @@ class RealTimeUrlLookupService : public RealTimeUrlLookupServiceBase {
                         base::TimeTicks get_token_start_time,
                         const std::string& access_token);
 
-  // Unowned object used for checking sync status of the profile.
-  syncer::SyncService* sync_service_;
-
   // Unowned object used for getting preference settings.
   PrefService* pref_service_;
 

components/safe_browsing/core/realtime/url_lookup_service_base.cc

@@ -18,7 +18,6 @@
 #include "components/safe_browsing/core/common/thread_utils.h"
 #include "components/safe_browsing/core/common/utils.h"
 #include "components/safe_browsing/core/verdict_cache_manager.h"
-#include "components/sync/driver/sync_service.h"
 #include "net/base/ip_address.h"
 #include "net/base/load_flags.h"
 #include "net/base/url_util.h"
@@ -124,7 +123,7 @@ RTLookupRequest::OSType GetRTLookupRequestOSType() {
 RealTimeUrlLookupServiceBase::RealTimeUrlLookupServiceBase(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     VerdictCacheManager* cache_manager,
-    syncer::SyncService* sync_service,
+    const IsHistorySyncEnabledCallback& is_history_sync_enabled_callback,
     PrefService* pref_service,
     const ChromeUserPopulation::ProfileManagementStatus&
         profile_management_status,
@@ -132,7 +131,7 @@ RealTimeUrlLookupServiceBase::RealTimeUrlLookupServiceBase(
     bool is_off_the_record)
     : url_loader_factory_(url_loader_factory),
       cache_manager_(cache_manager),
-      sync_service_(sync_service),
+      is_history_sync_enabled_callback_(is_history_sync_enabled_callback),
       pref_service_(pref_service),
       profile_management_status_(profile_management_status),
       is_under_advanced_protection_(is_under_advanced_protection),
@@ -448,22 +447,14 @@ std::unique_ptr<RTLookupRequest> RealTimeUrlLookupServiceBase::FillRequestProto(
                 : ChromeUserPopulation::SAFE_BROWSING);
 
   user_population->set_profile_management_status(profile_management_status_);
-  user_population->set_is_history_sync_enabled(IsHistorySyncEnabled());
+  user_population->set_is_history_sync_enabled(
+      is_history_sync_enabled_callback_.Run());
   user_population->set_is_under_advanced_protection(
       is_under_advanced_protection_);
   user_population->set_is_incognito(is_off_the_record_);
   return request;
 }
 
-// TODO(bdea): Refactor this method into a util class as multiple SB classes
-// have this method.
-bool RealTimeUrlLookupServiceBase::IsHistorySyncEnabled() {
-  return sync_service_ && sync_service_->IsSyncFeatureActive() &&
-         !sync_service_->IsLocalSyncEnabled() &&
-         sync_service_->GetActiveDataTypes().Has(
-             syncer::HISTORY_DELETE_DIRECTIVES);
-}
-
 void RealTimeUrlLookupServiceBase::Shutdown() {
   for (auto& pending : pending_requests_) {
     // Treat all pending requests as safe, and not from cache so that a

components/safe_browsing/core/realtime/url_lookup_service_base.h

@@ -30,14 +30,12 @@ class SimpleURLLoader;
 class SharedURLLoaderFactory;
 }  // namespace network
 
-namespace syncer {
-class SyncService;
-}
-
 class PrefService;
 
 namespace safe_browsing {
 
+using IsHistorySyncEnabledCallback = base::RepeatingCallback<bool()>;
+
 using RTLookupRequestCallback =
     base::OnceCallback<void(std::unique_ptr<RTLookupRequest>, std::string)>;
 
@@ -53,7 +51,7 @@ class RealTimeUrlLookupServiceBase : public KeyedService {
   explicit RealTimeUrlLookupServiceBase(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       VerdictCacheManager* cache_manager,
-      syncer::SyncService* sync_service,
+      const IsHistorySyncEnabledCallback& is_history_sync_enabled_callback,
       PrefService* pref_service,
       const ChromeUserPopulation::ProfileManagementStatus&
           profile_management_status,
@@ -193,8 +191,6 @@ class RealTimeUrlLookupServiceBase : public KeyedService {
   // Fills in fields in |RTLookupRequest|.
   std::unique_ptr<RTLookupRequest> FillRequestProto(const GURL& url);
 
-  bool IsHistorySyncEnabled();
-
   // Count of consecutive failures to complete URL lookup requests. When it
   // reaches |kMaxFailuresToEnforceBackoff|, we enter the backoff mode. It gets
   // reset when we complete a lookup successfully or when the backoff reset
@@ -220,8 +216,9 @@ class RealTimeUrlLookupServiceBase : public KeyedService {
   // Unowned object used for getting and storing real time url check cache.
   VerdictCacheManager* cache_manager_;
 
-  // Unowned object used for checking sync status of the profile.
-  syncer::SyncService* sync_service_;
+  // Used for determining whether history sync is enabled in the safe browsing
+  // embedder.
+  IsHistorySyncEnabledCallback is_history_sync_enabled_callback_;
 
   // Unowned object used for getting preference settings.
   PrefService* pref_service_;

components/safe_browsing/core/realtime/url_lookup_service_unittest.cc

@@ -16,7 +16,6 @@
 #include "components/safe_browsing/core/common/test_task_environment.h"
 #include "components/safe_browsing/core/features.h"
 #include "components/safe_browsing/core/verdict_cache_manager.h"
-#include "components/sync/driver/test_sync_service.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "components/unified_consent/pref_names.h"
 #include "components/unified_consent/unified_consent_service.h"
@@ -74,8 +73,9 @@ class RealTimeUrlLookupServiceTest : public PlatformTest {
     auto token_fetcher = std::make_unique<TestSafeBrowsingTokenFetcher>();
     raw_token_fetcher_ = token_fetcher.get();
     rt_service_ = std::make_unique<RealTimeUrlLookupService>(
-        test_shared_loader_factory_, cache_manager_.get(), &test_sync_service_,
-        &test_pref_service_, std::move(token_fetcher),
+        test_shared_loader_factory_, cache_manager_.get(),
+        base::BindRepeating([]() { return true; }), &test_pref_service_,
+        std::move(token_fetcher),
         base::BindRepeating(
             &RealTimeUrlLookupServiceTest::AreTokenFetchesConfiguredInClient,
             base::Unretained(this)),
@@ -183,7 +183,6 @@ class RealTimeUrlLookupServiceTest : public PlatformTest {
   TestSafeBrowsingTokenFetcher* raw_token_fetcher_ = nullptr;
   std::unique_ptr<base::test::TaskEnvironment> task_environment_;
   sync_preferences::TestingPrefServiceSyncable test_pref_service_;
-  syncer::TestSyncService test_sync_service_;
   base::test::ScopedFeatureList feature_list_;
 };
 
@@ -203,6 +202,9 @@ TEST_F(RealTimeUrlLookupServiceTest, TestFillRequestProto) {
     EXPECT_EQ(RTLookupRequest::NAVIGATION, result->lookup_type());
     EXPECT_EQ(ChromeUserPopulation::SAFE_BROWSING,
               result->population().user_population());
+
+    // The value of is_history_sync_enabled() should reflect that of the
+    // callback passed in by the client, which in this case is true.
     EXPECT_TRUE(result->population().is_history_sync_enabled());
     EXPECT_EQ(ChromeUserPopulation::NOT_MANAGED,
               result->population().profile_management_status());

ios/chrome/browser/safe_browsing/real_time_url_lookup_service_factory.mm

@@ -60,7 +60,9 @@ RealTimeUrlLookupServiceFactory::BuildServiceInstanceFor(
   return std::make_unique<safe_browsing::RealTimeUrlLookupService>(
       safe_browsing_service->GetURLLoaderFactory(),
       VerdictCacheManagerFactory::GetForBrowserState(chrome_browser_state),
-      ProfileSyncServiceFactory::GetForBrowserState(chrome_browser_state),
+      base::BindRepeating(
+          &safe_browsing::SyncUtils::IsHistorySyncEnabled,
+          ProfileSyncServiceFactory::GetForBrowserState(chrome_browser_state)),
       chrome_browser_state->GetPrefs(),
       std::make_unique<safe_browsing::SafeBrowsingPrimaryAccountTokenFetcher>(
           IdentityManagerFactory::GetForBrowserState(chrome_browser_state)),

weblayer/browser/safe_browsing/real_time_url_lookup_service_factory.cc

@@ -50,7 +50,8 @@ KeyedService* RealTimeUrlLookupServiceFactory::BuildServiceInstanceFor(
   return new safe_browsing::RealTimeUrlLookupService(
       network::SharedURLLoaderFactory::Create(std::move(url_loader_factory)),
       VerdictCacheManagerFactory::GetForBrowserContext(context),
-      nullptr /* profile sync service */,
+      // History sync is never enabled in WebLayer.
+      base::BindRepeating([]() { return false; }),
       static_cast<BrowserContextImpl*>(context)->pref_service(),
       // TODO(crbug.com/1080748): Bring up token fetching for URL lookups and
       // configure the following two parameters accordingly.