Unified adblock catalog callbacks

android/android_browser_tests.gni

@@ -89,7 +89,6 @@ android_test_exception_sources = [
   "//brave/components/brave_rewards/browser/test/rewards_publisher_browsertest.cc",
   "//brave/components/brave_rewards/browser/test/rewards_state_browsertest.cc",
   "//brave/components/brave_shields/browser/https_everywhere_service_browsertest.cc",
-  "//brave/components/brave_shields/browser/test_filters_provider.cc",
   "//brave/components/content_settings/renderer/brave_content_settings_agent_impl_browsertest.cc",
   "//brave/third_party/blink/renderer/modules/brave/navigator_browsertest.cc",
   "//chrome/browser/extensions/browsertest_util.cc",
@@ -119,6 +118,7 @@ android_test_exception_deps = [
   "//brave/browser/ui:browser_tests",
   "//brave/browser/ui/tabs/test:browser_tests",
   "//brave/browser/ui/views/tabs:browser_tests",
+  "//brave/components/brave_shields/browser:test_support",
   "//brave/components/brave_wallet/resources:ethereum_provider_generated_resources",
   "//brave/components/de_amp/browser/test:browser_tests",
   "//brave/renderer/skus:browser_tests",

browser/brave_shields/ad_block_service_browsertest.cc

@@ -26,6 +26,7 @@
 #include "brave/components/brave_shields/browser/ad_block_subscription_service_manager.h"
 #include "brave/components/brave_shields/browser/ad_block_subscription_service_manager_observer.h"
 #include "brave/components/brave_shields/browser/brave_shields_util.h"
+#include "brave/components/brave_shields/browser/engine_test_observer.h"
 #include "brave/components/brave_shields/browser/filter_list_catalog_entry.h"
 #include "brave/components/brave_shields/browser/test_filters_provider.h"
 #include "brave/components/brave_shields/common/brave_shield_constants.h"
@@ -159,7 +160,10 @@ void AdBlockServiceTest::UpdateAdBlockInstanceWithRules(
 
   source_providers_.push_back(std::move(source_provider));
 
-  WaitForAdBlockServiceThreads();
+  auto* engine =
+      g_brave_browser_process->ad_block_service()->default_engine_.get();
+  EngineTestObserver engine_observer(engine);
+  engine_observer.Wait();
 }
 
 void AdBlockServiceTest::UpdateCustomAdBlockInstanceWithRules(
@@ -175,7 +179,10 @@ void AdBlockServiceTest::UpdateCustomAdBlockInstanceWithRules(
 
   source_providers_.push_back(std::move(source_provider));
 
-  WaitForAdBlockServiceThreads();
+  auto* engine = g_brave_browser_process->ad_block_service()
+                     ->additional_filters_engine_.get();
+  EngineTestObserver engine_observer(engine);
+  engine_observer.Wait();
 }
 
 void AdBlockServiceTest::AssertTagExists(const std::string& tag,
@@ -237,31 +244,6 @@ bool AdBlockServiceTest::InstallDefaultAdBlockComponent(
   return true;
 }
 
-// A test observer that allows blocking waits for an AdBlockEngine to be
-// updated with new rules.
-class EngineTestObserver : public brave_shields::AdBlockEngine::TestObserver {
- public:
-  // Constructs an EngineTestObserver which will observe the given adblock
-  // engine for filter data updates.
-  explicit EngineTestObserver(brave_shields::AdBlockEngine* engine)
-      : engine_(engine) {
-    engine_->AddObserverForTest(this);
-  }
-  ~EngineTestObserver() override { engine_->RemoveObserverForTest(); }
-
-  EngineTestObserver(const EngineTestObserver& other) = delete;
-  EngineTestObserver& operator=(const EngineTestObserver& other) = delete;
-
-  // Blocks until the engine is updated
-  void Wait() { run_loop_.Run(); }
-
- private:
-  void OnEngineUpdated() override { run_loop_.Quit(); }
-
-  base::RunLoop run_loop_;
-  raw_ptr<brave_shields::AdBlockEngine> engine_ = nullptr;
-};
-
 bool AdBlockServiceTest::InstallRegionalAdBlockComponent(
     const std::string& uuid,
     bool enable_list) {
@@ -1591,7 +1573,6 @@ IN_PROC_BROWSER_TEST_F(Default1pBlockingFlagDisabledTest, Custom1pBlocking) {
   DisableAggressiveMode();
   EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL);
   UpdateCustomAdBlockInstanceWithRules("^ad_banner.png");
-  WaitForAdBlockServiceThreads();
 
   GURL url = embedded_test_server()->GetURL(kAdBlockTestPage);
   ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url));
@@ -1661,8 +1642,6 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, RedirectWithoutBlockIsNoop) {
       "js_mock_me.js$redirect-rule=noopjs",
       resources);
 
-  WaitForAdBlockServiceThreads();
-
   EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL);
 
   const GURL url =

browser/ephemeral_storage/BUILD.gn

@@ -27,6 +27,7 @@ source_set("browser_tests") {
       "//brave/browser",
       "//brave/components/brave_component_updater/browser",
       "//brave/components/brave_shields/browser",
+      "//brave/components/brave_shields/browser:test_support",
       "//brave/components/brave_shields/common",
       "//brave/components/ephemeral_storage",
       "//chrome/browser",

browser/ephemeral_storage/ephemeral_storage_1p_domain_block_browsertest.cc

@@ -12,6 +12,7 @@
 #include "brave/components/brave_component_updater/browser/local_data_files_service.h"
 #include "brave/components/brave_shields/browser/ad_block_service.h"
 #include "brave/components/brave_shields/browser/brave_shields_util.h"
+#include "brave/components/brave_shields/browser/engine_test_observer.h"
 #include "brave/components/brave_shields/browser/test_filters_provider.h"
 #include "brave/components/brave_shields/common/features.h"
 #include "chrome/browser/interstitials/security_interstitial_page_test_utils.h"
@@ -48,7 +49,11 @@ class EphemeralStorage1pDomainBlockBrowserTest
         g_brave_browser_process->ad_block_service();
     ad_block_service->UseSourceProvidersForTest(source_provider_.get(),
                                                 source_provider_.get());
-    WaitForAdBlockServiceThreads();
+
+    auto* engine =
+        g_brave_browser_process->ad_block_service()->default_engine_.get();
+    EngineTestObserver engine_observer(engine);
+    engine_observer.Wait();
   }
 
   void WaitForAdBlockServiceThreads() {

browser/net/BUILD.gn

@@ -207,6 +207,7 @@ source_set("unit_tests") {
     "//brave/components/brave_rewards/common",
     "//brave/components/brave_rewards/common:features",
     "//brave/components/brave_shields/browser",
+    "//brave/components/brave_shields/browser:test_support",
     "//brave/components/l10n/common:test_support",
     "//chrome/test:test_support",
     "//content/test:test_support",

browser/net/brave_ad_block_tp_network_delegate_helper_unittest.cc

@@ -128,6 +128,7 @@ class BraveAdBlockTPNetworkDelegateHelperTest : public testing::Test {
     filters_provider_ = std::make_unique<TestFiltersProvider>(rules, resources);
     g_brave_browser_process->ad_block_service()->UseSourceProvidersForTest(
         filters_provider_.get(), filters_provider_.get());
+    task_environment_.RunUntilIdle();
   }
 
   // Returns true if the request handler deferred control back to the calling

browser/permissions/BUILD.gn

@@ -25,6 +25,7 @@ if (!is_android) {
       "//brave/browser/brave_wallet",
       "//brave/components/brave_component_updater/browser:browser",
       "//brave/components/brave_shields/browser:browser",
+      "//brave/components/brave_shields/browser:test_support",
       "//brave/components/brave_shields/common",
       "//brave/components/brave_wallet/browser",
       "//brave/components/brave_wallet/browser:permission_utils",

components/brave_shields/browser/BUILD.gn

@@ -126,3 +126,21 @@ source_set("component_installer") {
     "//crypto",
   ]
 }
+
+if (!is_ios) {
+  source_set("test_support") {
+    testonly = true
+    sources = [
+      "engine_test_observer.cc",
+      "engine_test_observer.h",
+      "test_filters_provider.cc",
+      "test_filters_provider.h",
+    ]
+
+    deps = [
+      "//base",
+      "//brave/components/brave_component_updater/browser",
+      "//brave/components/brave_shields/browser:browser",
+    ]
+  }
+}

components/brave_shields/browser/ad_block_component_filters_provider.cc

@@ -23,13 +23,23 @@ namespace brave_shields {
 
 namespace {
 
+void AddNothingToFilterSet(rust::Box<adblock::FilterSet>*) {}
+
 // static
-void AddDATBufferToFilterSet(base::OnceCallback<void()> cb,
-                             rust::Box<adblock::FilterSet>* filter_set,
-                             uint8_t permission_mask,
-                             DATFileDataBuffer buffer) {
+void AddDATBufferToFilterSet(uint8_t permission_mask,
+                             DATFileDataBuffer buffer,
+                             rust::Box<adblock::FilterSet>* filter_set) {
   (*filter_set)->add_filter_list_with_permissions(buffer, permission_mask);
-  std::move(cb).Run();
+}
+
+// static
+void OnReadDATFileData(
+    base::OnceCallback<
+        void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)> cb,
+    uint8_t permission_mask,
+    DATFileDataBuffer buffer) {
+  std::move(cb).Run(
+      base::BindOnce(&AddDATBufferToFilterSet, permission_mask, buffer));
 }
 
 }  // namespace
@@ -86,12 +96,12 @@ void AdBlockComponentFiltersProvider::OnComponentReady(
 }
 
 void AdBlockComponentFiltersProvider::LoadFilterSet(
-    rust::Box<adblock::FilterSet>* filter_set,
-    base::OnceCallback<void()> cb) {
+    base::OnceCallback<
+        void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)> cb) {
   if (component_path_.empty()) {
-    // If the path is not ready yet, run the callback with no changes. An
+    // If the path is not ready yet, provide a no-op callback immediately. An
     // update will be pushed later to notify about the newly available list.
-    std::move(cb).Run();
+    std::move(cb).Run(base::BindOnce(AddNothingToFilterSet));
     return;
   }
 
@@ -100,8 +110,7 @@ void AdBlockComponentFiltersProvider::LoadFilterSet(
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE, {base::MayBlock()},
       base::BindOnce(&brave_component_updater::ReadDATFileData, list_file_path),
-      base::BindOnce(&AddDATBufferToFilterSet, std::move(cb), filter_set,
-                     permission_mask_));
+      base::BindOnce(&OnReadDATFileData, std::move(cb), permission_mask_));
 }
 
 }  // namespace brave_shields

components/brave_shields/browser/ad_block_component_filters_provider.h

@@ -51,8 +51,9 @@ class AdBlockComponentFiltersProvider : public AdBlockFiltersProvider {
   AdBlockComponentFiltersProvider& operator=(
       const AdBlockComponentFiltersProvider&) = delete;
 
-  void LoadFilterSet(rust::Box<adblock::FilterSet>* filter_set,
-                     base::OnceCallback<void()>) override;
+  void LoadFilterSet(
+      base::OnceCallback<void(
+          base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)>) override;
 
   // Remove the component. This will force it to be redownloaded next time it
   // is registered.

components/brave_shields/browser/ad_block_custom_filters_provider.cc

@@ -17,6 +17,12 @@ namespace brave_shields {
 
 namespace {
 
+void AddDATBufferToFilterSet(uint8_t permission_mask,
+                             DATFileDataBuffer buffer,
+                             rust::Box<adblock::FilterSet>* filter_set) {
+  (*filter_set)->add_filter_list_with_permissions(buffer, permission_mask);
+}
+
 // Custom filters get all permissions granted, i.e. all bits of the mask set,
 // i.e. the maximum possible uint8_t.
 const uint8_t kCustomFiltersPermissionLevel = UINT8_MAX;
@@ -68,19 +74,20 @@ bool AdBlockCustomFiltersProvider::UpdateCustomFilters(
 }
 
 void AdBlockCustomFiltersProvider::LoadFilterSet(
-    rust::Box<adblock::FilterSet>* filter_set,
-    base::OnceCallback<void()> cb) {
+    base::OnceCallback<
+        void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)> cb) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   auto custom_filters = GetCustomFilters();
 
   auto buffer =
       std::vector<unsigned char>(custom_filters.begin(), custom_filters.end());
-  (*filter_set)
-      ->add_filter_list_with_permissions(buffer, kCustomFiltersPermissionLevel);
 
   // PostTask so this has an async return to match other loaders
   base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(cb)));
+      FROM_HERE,
+      base::BindOnce(std::move(cb),
+                     base::BindOnce(&AddDATBufferToFilterSet,
+                                    kCustomFiltersPermissionLevel, buffer)));
 }
 
 // The custom filters provider can provide its filters immediately after being

components/brave_shields/browser/ad_block_custom_filters_provider.h

@@ -35,8 +35,9 @@ class AdBlockCustomFiltersProvider : public AdBlockFiltersProvider {
   std::string GetCustomFilters();
   bool UpdateCustomFilters(const std::string& custom_filters);
 
-  void LoadFilterSet(rust::Box<adblock::FilterSet>* filter_set,
-                     base::OnceCallback<void()>) override;
+  void LoadFilterSet(
+      base::OnceCallback<void(
+          base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)>) override;
 
   // AdBlockFiltersProvider
   void AddObserver(AdBlockFiltersProvider::Observer* observer);

components/brave_shields/browser/ad_block_filters_provider.h

@@ -38,8 +38,9 @@ class AdBlockFiltersProvider {
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
-  virtual void LoadFilterSet(rust::Box<adblock::FilterSet>* filter_set,
-                             base::OnceCallback<void()>) = 0;
+  virtual void LoadFilterSet(
+      base::OnceCallback<
+          void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)>) = 0;
 
   base::WeakPtr<AdBlockFiltersProvider> AsWeakPtr();
 

components/brave_shields/browser/ad_block_filters_provider_manager.cc

@@ -9,7 +9,7 @@
 #include <string>
 #include <utility>
 
-#include "base/barrier_closure.h"
+#include "base/barrier_callback.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/no_destructor.h"
@@ -60,27 +60,47 @@ void AdBlockFiltersProviderManager::OnChanged() {
 
 // Use LoadDATBufferForEngine instead, for Filter Provider Manager.
 void AdBlockFiltersProviderManager::LoadFilterSet(
-    rust::Box<adblock::FilterSet>* filter_set,
-    base::OnceCallback<void()>) {
+    base::OnceCallback<
+        void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)>) {
   NOTREACHED();
 }
 
 void AdBlockFiltersProviderManager::LoadFilterSetForEngine(
     bool is_for_default_engine,
-    rust::Box<adblock::FilterSet>* filter_set,
-    base::OnceCallback<void()> cb) {
+    base::OnceCallback<
+        void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)> cb) {
   auto& filters_providers = is_for_default_engine
                                 ? default_engine_filters_providers_
                                 : additional_engine_filters_providers_;
-  const auto collect_and_merge =
-      base::BarrierClosure(filters_providers.size(), std::move(cb));
+  const auto collect_and_merge = base::BarrierCallback<
+      base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>>(
+      filters_providers.size(),
+      base::BindOnce(&AdBlockFiltersProviderManager::FinishCombinating,
+                     weak_factory_.GetWeakPtr(), std::move(cb)));
   for (auto* const provider : filters_providers) {
     task_tracker_.PostTask(
         base::SequencedTaskRunner::GetCurrentDefault().get(), FROM_HERE,
         base::BindOnce(&AdBlockFiltersProvider::LoadFilterSet,
-                       provider->AsWeakPtr(), filter_set,
-                       std::move(collect_and_merge)));
+                       provider->AsWeakPtr(), std::move(collect_and_merge)));
   }
 }
 
+// static
+void RunAllResults(
+    std::vector<base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>>
+        results,
+    rust::Box<adblock::FilterSet>* filter_set) {
+  for (auto& cb : results) {
+    std::move(cb).Run(filter_set);
+  }
+}
+
+void AdBlockFiltersProviderManager::FinishCombinating(
+    base::OnceCallback<
+        void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)> cb,
+    std::vector<base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>>
+        results) {
+  std::move(cb).Run(base::BindOnce(&RunAllResults, std::move(results)));
+}
+
 }  // namespace brave_shields