Skip to content

Commit effbc32

Browse files
antonok-edmantonok-edm
authored
Merge pull request #20284 from brave/unified-adblock-catalog-callbacks
Unified adblock catalog callbacks
2 parents 79c189e + 0d84607 commit effbc32

26 files changed

+286
-111
lines changed

‎android/android_browser_tests.gni

+1-1
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,6 @@ android_test_exception_sources = [
8989
"//brave/components/brave_rewards/browser/test/rewards_publisher_browsertest.cc",
9090
"//brave/components/brave_rewards/browser/test/rewards_state_browsertest.cc",
9191
"//brave/components/brave_shields/browser/https_everywhere_service_browsertest.cc",
92-
"//brave/components/brave_shields/browser/test_filters_provider.cc",
9392
"//brave/components/content_settings/renderer/brave_content_settings_agent_impl_browsertest.cc",
9493
"//brave/third_party/blink/renderer/modules/brave/navigator_browsertest.cc",
9594
"//chrome/browser/extensions/browsertest_util.cc",
@@ -119,6 +118,7 @@ android_test_exception_deps = [
119118
"//brave/browser/ui:browser_tests",
120119
"//brave/browser/ui/tabs/test:browser_tests",
121120
"//brave/browser/ui/views/tabs:browser_tests",
121+
"//brave/components/brave_shields/browser:test_support",
122122
"//brave/components/brave_wallet/resources:ethereum_provider_generated_resources",
123123
"//brave/components/de_amp/browser/test:browser_tests",
124124
"//brave/renderer/skus:browser_tests",

‎browser/brave_shields/ad_block_service_browsertest.cc

+9-30
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include "brave/components/brave_shields/browser/ad_block_subscription_service_manager.h"
2727
#include "brave/components/brave_shields/browser/ad_block_subscription_service_manager_observer.h"
2828
#include "brave/components/brave_shields/browser/brave_shields_util.h"
29+
#include "brave/components/brave_shields/browser/engine_test_observer.h"
2930
#include "brave/components/brave_shields/browser/filter_list_catalog_entry.h"
3031
#include "brave/components/brave_shields/browser/test_filters_provider.h"
3132
#include "brave/components/brave_shields/common/brave_shield_constants.h"
@@ -159,7 +160,10 @@ void AdBlockServiceTest::UpdateAdBlockInstanceWithRules(
159160

160161
source_providers_.push_back(std::move(source_provider));
161162

162-
WaitForAdBlockServiceThreads();
163+
auto* engine =
164+
g_brave_browser_process->ad_block_service()->default_engine_.get();
165+
EngineTestObserver engine_observer(engine);
166+
engine_observer.Wait();
163167
}
164168

165169
void AdBlockServiceTest::UpdateCustomAdBlockInstanceWithRules(
@@ -175,7 +179,10 @@ void AdBlockServiceTest::UpdateCustomAdBlockInstanceWithRules(
175179

176180
source_providers_.push_back(std::move(source_provider));
177181

178-
WaitForAdBlockServiceThreads();
182+
auto* engine = g_brave_browser_process->ad_block_service()
183+
->additional_filters_engine_.get();
184+
EngineTestObserver engine_observer(engine);
185+
engine_observer.Wait();
179186
}
180187

181188
void AdBlockServiceTest::AssertTagExists(const std::string& tag,
@@ -237,31 +244,6 @@ bool AdBlockServiceTest::InstallDefaultAdBlockComponent(
237244
return true;
238245
}
239246

240-
// A test observer that allows blocking waits for an AdBlockEngine to be
241-
// updated with new rules.
242-
class EngineTestObserver : public brave_shields::AdBlockEngine::TestObserver {
243-
public:
244-
// Constructs an EngineTestObserver which will observe the given adblock
245-
// engine for filter data updates.
246-
explicit EngineTestObserver(brave_shields::AdBlockEngine* engine)
247-
: engine_(engine) {
248-
engine_->AddObserverForTest(this);
249-
}
250-
~EngineTestObserver() override { engine_->RemoveObserverForTest(); }
251-
252-
EngineTestObserver(const EngineTestObserver& other) = delete;
253-
EngineTestObserver& operator=(const EngineTestObserver& other) = delete;
254-
255-
// Blocks until the engine is updated
256-
void Wait() { run_loop_.Run(); }
257-
258-
private:
259-
void OnEngineUpdated() override { run_loop_.Quit(); }
260-
261-
base::RunLoop run_loop_;
262-
raw_ptr<brave_shields::AdBlockEngine> engine_ = nullptr;
263-
};
264-
265247
bool AdBlockServiceTest::InstallRegionalAdBlockComponent(
266248
const std::string& uuid,
267249
bool enable_list) {
@@ -1591,7 +1573,6 @@ IN_PROC_BROWSER_TEST_F(Default1pBlockingFlagDisabledTest, Custom1pBlocking) {
15911573
DisableAggressiveMode();
15921574
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL);
15931575
UpdateCustomAdBlockInstanceWithRules("^ad_banner.png");
1594-
WaitForAdBlockServiceThreads();
15951576

15961577
GURL url = embedded_test_server()->GetURL(kAdBlockTestPage);
15971578
ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url));
@@ -1661,8 +1642,6 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, RedirectWithoutBlockIsNoop) {
16611642
"js_mock_me.js$redirect-rule=noopjs",
16621643
resources);
16631644

1664-
WaitForAdBlockServiceThreads();
1665-
16661645
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL);
16671646

16681647
const GURL url =

‎browser/ephemeral_storage/BUILD.gn

+1
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ source_set("browser_tests") {
2727
"//brave/browser",
2828
"//brave/components/brave_component_updater/browser",
2929
"//brave/components/brave_shields/browser",
30+
"//brave/components/brave_shields/browser:test_support",
3031
"//brave/components/brave_shields/common",
3132
"//brave/components/ephemeral_storage",
3233
"//chrome/browser",

‎browser/ephemeral_storage/ephemeral_storage_1p_domain_block_browsertest.cc

+6-1
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include "brave/components/brave_component_updater/browser/local_data_files_service.h"
1313
#include "brave/components/brave_shields/browser/ad_block_service.h"
1414
#include "brave/components/brave_shields/browser/brave_shields_util.h"
15+
#include "brave/components/brave_shields/browser/engine_test_observer.h"
1516
#include "brave/components/brave_shields/browser/test_filters_provider.h"
1617
#include "brave/components/brave_shields/common/features.h"
1718
#include "chrome/browser/interstitials/security_interstitial_page_test_utils.h"
@@ -48,7 +49,11 @@ class EphemeralStorage1pDomainBlockBrowserTest
4849
g_brave_browser_process->ad_block_service();
4950
ad_block_service->UseSourceProvidersForTest(source_provider_.get(),
5051
source_provider_.get());
51-
WaitForAdBlockServiceThreads();
52+
53+
auto* engine =
54+
g_brave_browser_process->ad_block_service()->default_engine_.get();
55+
EngineTestObserver engine_observer(engine);
56+
engine_observer.Wait();
5257
}
5358

5459
void WaitForAdBlockServiceThreads() {

‎browser/net/BUILD.gn

+1
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,7 @@ source_set("unit_tests") {
207207
"//brave/components/brave_rewards/common",
208208
"//brave/components/brave_rewards/common:features",
209209
"//brave/components/brave_shields/browser",
210+
"//brave/components/brave_shields/browser:test_support",
210211
"//brave/components/l10n/common:test_support",
211212
"//chrome/test:test_support",
212213
"//content/test:test_support",

‎browser/net/brave_ad_block_tp_network_delegate_helper_unittest.cc

+1
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ class BraveAdBlockTPNetworkDelegateHelperTest : public testing::Test {
128128
filters_provider_ = std::make_unique<TestFiltersProvider>(rules, resources);
129129
g_brave_browser_process->ad_block_service()->UseSourceProvidersForTest(
130130
filters_provider_.get(), filters_provider_.get());
131+
task_environment_.RunUntilIdle();
131132
}
132133

133134
// Returns true if the request handler deferred control back to the calling

‎browser/permissions/BUILD.gn

+1
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ if (!is_android) {
2525
"//brave/browser/brave_wallet",
2626
"//brave/components/brave_component_updater/browser:browser",
2727
"//brave/components/brave_shields/browser:browser",
28+
"//brave/components/brave_shields/browser:test_support",
2829
"//brave/components/brave_shields/common",
2930
"//brave/components/brave_wallet/browser",
3031
"//brave/components/brave_wallet/browser:permission_utils",

‎components/brave_shields/browser/BUILD.gn

+18
Original file line numberDiff line numberDiff line change
@@ -126,3 +126,21 @@ source_set("component_installer") {
126126
"//crypto",
127127
]
128128
}
129+
130+
if (!is_ios) {
131+
source_set("test_support") {
132+
testonly = true
133+
sources = [
134+
"engine_test_observer.cc",
135+
"engine_test_observer.h",
136+
"test_filters_provider.cc",
137+
"test_filters_provider.h",
138+
]
139+
140+
deps = [
141+
"//base",
142+
"//brave/components/brave_component_updater/browser",
143+
"//brave/components/brave_shields/browser:browser",
144+
]
145+
}
146+
}

‎components/brave_shields/browser/ad_block_component_filters_provider.cc

+20-11
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,23 @@ namespace brave_shields {
2323

2424
namespace {
2525

26+
void AddNothingToFilterSet(rust::Box<adblock::FilterSet>*) {}
27+
2628
// static
27-
void AddDATBufferToFilterSet(base::OnceCallback<void()> cb,
28-
rust::Box<adblock::FilterSet>* filter_set,
29-
uint8_t permission_mask,
30-
DATFileDataBuffer buffer) {
29+
void AddDATBufferToFilterSet(uint8_t permission_mask,
30+
DATFileDataBuffer buffer,
31+
rust::Box<adblock::FilterSet>* filter_set) {
3132
(*filter_set)->add_filter_list_with_permissions(buffer, permission_mask);
32-
std::move(cb).Run();
33+
}
34+
35+
// static
36+
void OnReadDATFileData(
37+
base::OnceCallback<
38+
void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)> cb,
39+
uint8_t permission_mask,
40+
DATFileDataBuffer buffer) {
41+
std::move(cb).Run(
42+
base::BindOnce(&AddDATBufferToFilterSet, permission_mask, buffer));
3343
}
3444

3545
} // namespace
@@ -86,12 +96,12 @@ void AdBlockComponentFiltersProvider::OnComponentReady(
8696
}
8797

8898
void AdBlockComponentFiltersProvider::LoadFilterSet(
89-
rust::Box<adblock::FilterSet>* filter_set,
90-
base::OnceCallback<void()> cb) {
99+
base::OnceCallback<
100+
void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)> cb) {
91101
if (component_path_.empty()) {
92-
// If the path is not ready yet, run the callback with no changes. An
102+
// If the path is not ready yet, provide a no-op callback immediately. An
93103
// update will be pushed later to notify about the newly available list.
94-
std::move(cb).Run();
104+
std::move(cb).Run(base::BindOnce(AddNothingToFilterSet));
95105
return;
96106
}
97107

@@ -100,8 +110,7 @@ void AdBlockComponentFiltersProvider::LoadFilterSet(
100110
base::ThreadPool::PostTaskAndReplyWithResult(
101111
FROM_HERE, {base::MayBlock()},
102112
base::BindOnce(&brave_component_updater::ReadDATFileData, list_file_path),
103-
base::BindOnce(&AddDATBufferToFilterSet, std::move(cb), filter_set,
104-
permission_mask_));
113+
base::BindOnce(&OnReadDATFileData, std::move(cb), permission_mask_));
105114
}
106115

107116
} // namespace brave_shields

‎components/brave_shields/browser/ad_block_component_filters_provider.h

+3-2
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,9 @@ class AdBlockComponentFiltersProvider : public AdBlockFiltersProvider {
5151
AdBlockComponentFiltersProvider& operator=(
5252
const AdBlockComponentFiltersProvider&) = delete;
5353

54-
void LoadFilterSet(rust::Box<adblock::FilterSet>* filter_set,
55-
base::OnceCallback<void()>) override;
54+
void LoadFilterSet(
55+
base::OnceCallback<void(
56+
base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)>) override;
5657

5758
// Remove the component. This will force it to be redownloaded next time it
5859
// is registered.

‎components/brave_shields/browser/ad_block_custom_filters_provider.cc

+12-5
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,12 @@ namespace brave_shields {
1717

1818
namespace {
1919

20+
void AddDATBufferToFilterSet(uint8_t permission_mask,
21+
DATFileDataBuffer buffer,
22+
rust::Box<adblock::FilterSet>* filter_set) {
23+
(*filter_set)->add_filter_list_with_permissions(buffer, permission_mask);
24+
}
25+
2026
// Custom filters get all permissions granted, i.e. all bits of the mask set,
2127
// i.e. the maximum possible uint8_t.
2228
const uint8_t kCustomFiltersPermissionLevel = UINT8_MAX;
@@ -68,19 +74,20 @@ bool AdBlockCustomFiltersProvider::UpdateCustomFilters(
6874
}
6975

7076
void AdBlockCustomFiltersProvider::LoadFilterSet(
71-
rust::Box<adblock::FilterSet>* filter_set,
72-
base::OnceCallback<void()> cb) {
77+
base::OnceCallback<
78+
void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)> cb) {
7379
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
7480
auto custom_filters = GetCustomFilters();
7581

7682
auto buffer =
7783
std::vector<unsigned char>(custom_filters.begin(), custom_filters.end());
78-
(*filter_set)
79-
->add_filter_list_with_permissions(buffer, kCustomFiltersPermissionLevel);
8084

8185
// PostTask so this has an async return to match other loaders
8286
base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
83-
FROM_HERE, base::BindOnce(std::move(cb)));
87+
FROM_HERE,
88+
base::BindOnce(std::move(cb),
89+
base::BindOnce(&AddDATBufferToFilterSet,
90+
kCustomFiltersPermissionLevel, buffer)));
8491
}
8592

8693
// The custom filters provider can provide its filters immediately after being

‎components/brave_shields/browser/ad_block_custom_filters_provider.h

+3-2
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,9 @@ class AdBlockCustomFiltersProvider : public AdBlockFiltersProvider {
3535
std::string GetCustomFilters();
3636
bool UpdateCustomFilters(const std::string& custom_filters);
3737

38-
void LoadFilterSet(rust::Box<adblock::FilterSet>* filter_set,
39-
base::OnceCallback<void()>) override;
38+
void LoadFilterSet(
39+
base::OnceCallback<void(
40+
base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)>) override;
4041

4142
// AdBlockFiltersProvider
4243
void AddObserver(AdBlockFiltersProvider::Observer* observer);

‎components/brave_shields/browser/ad_block_filters_provider.h

+3-2
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,9 @@ class AdBlockFiltersProvider {
3838
void AddObserver(Observer* observer);
3939
void RemoveObserver(Observer* observer);
4040

41-
virtual void LoadFilterSet(rust::Box<adblock::FilterSet>* filter_set,
42-
base::OnceCallback<void()>) = 0;
41+
virtual void LoadFilterSet(
42+
base::OnceCallback<
43+
void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)>) = 0;
4344

4445
base::WeakPtr<AdBlockFiltersProvider> AsWeakPtr();
4546

‎components/brave_shields/browser/ad_block_filters_provider_manager.cc

+29-9
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
#include <string>
1010
#include <utility>
1111

12-
#include "base/barrier_closure.h"
12+
#include "base/barrier_callback.h"
1313
#include "base/location.h"
1414
#include "base/logging.h"
1515
#include "base/no_destructor.h"
@@ -60,27 +60,47 @@ void AdBlockFiltersProviderManager::OnChanged() {
6060

6161
// Use LoadDATBufferForEngine instead, for Filter Provider Manager.
6262
void AdBlockFiltersProviderManager::LoadFilterSet(
63-
rust::Box<adblock::FilterSet>* filter_set,
64-
base::OnceCallback<void()>) {
63+
base::OnceCallback<
64+
void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)>) {
6565
NOTREACHED();
6666
}
6767

6868
void AdBlockFiltersProviderManager::LoadFilterSetForEngine(
6969
bool is_for_default_engine,
70-
rust::Box<adblock::FilterSet>* filter_set,
71-
base::OnceCallback<void()> cb) {
70+
base::OnceCallback<
71+
void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)> cb) {
7272
auto& filters_providers = is_for_default_engine
7373
? default_engine_filters_providers_
7474
: additional_engine_filters_providers_;
75-
const auto collect_and_merge =
76-
base::BarrierClosure(filters_providers.size(), std::move(cb));
75+
const auto collect_and_merge = base::BarrierCallback<
76+
base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>>(
77+
filters_providers.size(),
78+
base::BindOnce(&AdBlockFiltersProviderManager::FinishCombinating,
79+
weak_factory_.GetWeakPtr(), std::move(cb)));
7780
for (auto* const provider : filters_providers) {
7881
task_tracker_.PostTask(
7982
base::SequencedTaskRunner::GetCurrentDefault().get(), FROM_HERE,
8083
base::BindOnce(&AdBlockFiltersProvider::LoadFilterSet,
81-
provider->AsWeakPtr(), filter_set,
82-
std::move(collect_and_merge)));
84+
provider->AsWeakPtr(), std::move(collect_and_merge)));
8385
}
8486
}
8587

88+
// static
89+
void RunAllResults(
90+
std::vector<base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>>
91+
results,
92+
rust::Box<adblock::FilterSet>* filter_set) {
93+
for (auto& cb : results) {
94+
std::move(cb).Run(filter_set);
95+
}
96+
}
97+
98+
void AdBlockFiltersProviderManager::FinishCombinating(
99+
base::OnceCallback<
100+
void(base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>)> cb,
101+
std::vector<base::OnceCallback<void(rust::Box<adblock::FilterSet>*)>>
102+
results) {
103+
std::move(cb).Run(base::BindOnce(&RunAllResults, std::move(results)));
104+
}
105+
86106
} // namespace brave_shields

0 commit comments

Comments
 (0)