Use `AdBlockDefaultResourceUpdateInterval` for all adblock-related components #21868

antonok-edm · 2024-02-02T00:26:51Z

Resolves brave/brave-browser#35164

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

components/brave_shields/browser/ad_block_component_installer.cc

simonhong · 2024-02-02T02:31:03Z

components/brave_shields/browser/ad_block_component_service_manager.cc

+void AdBlockComponentServiceManager::CheckAdBlockComponentsUpdate() {
+  auto runner = base::SequencedTaskRunner::GetCurrentDefault();
+
+  runner->PostDelayedTask(FROM_HERE, base::BindOnce([]() {


As @atuchin-m noted at https://github.com/brave/brave-core/pull/20557/files#r1365810455,
It would be great if we could update in a bunch.

agreed... I don't think we have any API exposed for this though.

The best thing I've found so far is this UpdateClient::Update method that takes multiple component IDs, but we can't call it directly (would have to plumb multiple levels of patching). There's also CrxUpdateService::CheckForUpdates, although we don't have the ability to customize the list of components checked. Maybe that's ok?

@atuchin-m do you have any recommendations?

My point is we'd better optimize the number of requests to the backend to check the update. If we want to update the set of components, ideally we should done the one request.

UpdateClient::Update looks like an internal thing.

CheckForUpdates looks better, maybe it's okay to check the update of all the brave components.

Anyway, scheduling a number of task with random delays doesn't looks reasonable to me. 10 sec usually is not enough to process an update. Do you sure that the next PostTasks don't cancel the previous ones?

Also BraveOnDemandUpdater uses priority FOREGROUND, that means there is no chance to combine the requests together. CheckForUpdates uses the same priority internally.

I believe the first thing to do is to understand the right priority to use. if it's BACKGROUND then it's look okay to use CheckForUpdates. Otherwise I believe we need to add CrxUpdateService::OnDemandUpdateMultipleIds via chromium_src here

@iefremov do we have someone who is familiar with component_updater internals?

I'm somewhat familiar with component_updater internals.

zenparsing · 2024-02-02T02:53:58Z

components/brave_shields/browser/ad_block_component_service_manager.cc

@@ -251,6 +262,35 @@ void AdBlockComponentServiceManager::EnableFilterList(const std::string& uuid,
  UpdateFilterListPrefs(uuid, enabled);
 }

+void AdBlockComponentServiceManager::CheckAdBlockComponentsUpdate() {


For brave/brave-browser#35216, the UI will want to know when this process completes (so that it can show a spinner and updated text, and also an indication if the process fails).

Could this method accept a callback that gets executed when all component updates are done? Or is there some other approach we want to take?

Right now OnDemandUpdate has a hardcoded no-op callback. I suppose this could be changed, but let's see if @atuchin-m has any thoughts on the API we should use.

I believe we could add on demand callback in BraveOnDemandUpdate. Also no problem to use chromium OnDemandUpdater directly (a chromium example)

Also installing or updating a components has a several stages.

Updating the component (including downloading .crx)

Unpacking .crx and registering the new version

Processing the content by a client code (=the adblock code here).
The callback is about 2 (maybe 1), but definitely not about 3.
Please be sure that you're waiting for the right things.

atuchin-m · 2024-02-02T09:01:38Z

components/brave_shields/browser/ad_block_component_service_manager.cc

+  update_check_timer_.Start(
+      FROM_HERE, base::Minutes(kComponentUpdateCheckIntervalMins.Get()),
+      base::BindRepeating(
+          [](base::WeakPtr<AdBlockComponentServiceManager> component_manager) {


nit: what about base::BindRepeating(AdBlockComponentServiceManager::CheckAdBlockComponentsUpdate, weak_factory_.GetWeakPtr())?

atuchin-m · 2024-02-02T09:03:17Z

components/brave_shields/browser/ad_block_component_service_manager.cc

+                          }),
+                          base::Seconds(base::RandInt(0, 10)));
+
+  runner->PostDelayedTask(FROM_HERE, base::BindOnce([]() {


Could you please optimize identical blocks here?
There is 3 about the same runner->PostDelayedTask in the method. I hope we could merge it in the one.

atuchin-m · 2024-02-02T09:09:34Z

components/brave_shields/browser/ad_block_component_service_manager.cc

+void AdBlockComponentServiceManager::CheckAdBlockComponentsUpdate() {
+  auto runner = base::SequencedTaskRunner::GetCurrentDefault();
+
+  runner->PostDelayedTask(FROM_HERE, base::BindOnce([]() {


My point is we'd better optimize the number of requests to the backend to check the update. If we want to update the set of components, ideally we should done the one request.

UpdateClient::Update looks like an internal thing.

CheckForUpdates looks better, maybe it's okay to check the update of all the brave components.

Anyway, scheduling a number of task with random delays doesn't looks reasonable to me. 10 sec usually is not enough to process an update. Do you sure that the next PostTasks don't cancel the previous ones?

atuchin-m · 2024-02-02T09:12:32Z

components/brave_shields/browser/ad_block_component_service_manager.cc

+void AdBlockComponentServiceManager::CheckAdBlockComponentsUpdate() {
+  auto runner = base::SequencedTaskRunner::GetCurrentDefault();
+
+  runner->PostDelayedTask(FROM_HERE, base::BindOnce([]() {


Also BraveOnDemandUpdater uses priority FOREGROUND, that means there is no chance to combine the requests together. CheckForUpdates uses the same priority internally.

I believe the first thing to do is to understand the right priority to use. if it's BACKGROUND then it's look okay to use CheckForUpdates. Otherwise I believe we need to add CrxUpdateService::OnDemandUpdateMultipleIds via chromium_src here

@iefremov do we have someone who is familiar with component_updater internals?

atuchin-m · 2024-02-02T09:25:44Z

components/brave_shields/browser/ad_block_component_service_manager.cc

@@ -251,6 +262,35 @@ void AdBlockComponentServiceManager::EnableFilterList(const std::string& uuid,
  UpdateFilterListPrefs(uuid, enabled);
 }

+void AdBlockComponentServiceManager::CheckAdBlockComponentsUpdate() {


I believe we could add on demand callback in BraveOnDemandUpdate. Also no problem to use chromium OnDemandUpdater directly (a chromium example)

Also installing or updating a components has a several stages.

Updating the component (including downloading .crx)

Unpacking .crx and registering the new version

Processing the content by a client code (=the adblock code here).
The callback is about 2 (maybe 1), but definitely not about 3.
Please be sure that you're waiting for the right things.

antonok-edm · 2024-05-07T03:13:32Z

Thanks to @goodov's work on #23169, this PR's implementation is significantly simpler. Should be ready for re-review.

components/brave_shields/core/browser/ad_block_component_service_manager.cc

components/brave_shields/core/common/brave_shield_constants.h

components/brave_shields/core/browser/ad_block_component_service_manager.cc

components/brave_shields/core/common/brave_shield_constants.h

goodov

lgtm with few more nits.

components/brave_shields/core/common/features.h

goodov · 2024-05-10T08:08:07Z

components/brave_shields/core/common/brave_shield_constants.h

+    "UwNavFnj8gQDGVvCf+dse8HRMJn00QH0MOypsZSWFZRmF08ybOu/jTiUo/TuIaHL"
+    "1H8y9SR970LqsUMozu3ioSHtFh/IVgq7Nqy4TljaKsTE+3AdtjiOyHpW9ZaOkA7j"
+    "2QIDAQAB";
+
 const char kCookieListEnabledHistogram[] = "Brave.Shields.CookieListEnabled";
 const char kCookieListPromptHistogram[] = "Brave.Shields.CookieListPrompt";


I expected you to make all strings here to be inline constexpr. What you have right now is wrong.

antonok-edm requested review from cuba and simonhong February 2, 2024 00:26

antonok-edm self-assigned this Feb 2, 2024

simonhong reviewed Feb 2, 2024

View reviewed changes

components/brave_shields/browser/ad_block_component_installer.cc Outdated Show resolved Hide resolved

simonhong reviewed Feb 2, 2024

View reviewed changes

zenparsing reviewed Feb 2, 2024

View reviewed changes

antonok-edm requested a review from atuchin-m February 2, 2024 03:18

antonok-edm force-pushed the update-all-adblock-components-together branch from b0b67b8 to 4861d72 Compare February 2, 2024 03:25

simonhong approved these changes Feb 2, 2024

View reviewed changes

atuchin-m reviewed Feb 2, 2024

View reviewed changes

antonok-edm force-pushed the update-all-adblock-components-together branch from 4861d72 to 43dcdf1 Compare April 5, 2024 19:52

antonok-edm mentioned this pull request Apr 5, 2024

Expose APIs for updating a list of CRX components brave/brave-browser#37370

Closed

goodov mentioned this pull request Apr 27, 2024

Support OnDemandUpdate with multiple components at once. #23169

Merged

24 tasks

antonok-edm force-pushed the update-all-adblock-components-together branch from 43dcdf1 to 706d9f7 Compare May 7, 2024 03:10

antonok-edm requested review from mherrmann, zenparsing, simonhong, atuchin-m and goodov May 7, 2024 03:13

goodov reviewed May 7, 2024

View reviewed changes

components/brave_shields/core/common/brave_shield_constants.h Show resolved Hide resolved

goodov mentioned this pull request May 7, 2024

Use the brave-core AdBlockComponentFiltersProvider and AdBlockComponentServiceManager #22937

Merged

24 tasks

antonok-edm force-pushed the update-all-adblock-components-together branch 2 times, most recently from 3262d17 to a7996b7 Compare May 9, 2024 22:49

antonok-edm requested a review from goodov May 9, 2024 22:50

goodov approved these changes May 10, 2024

View reviewed changes

antonok-edm force-pushed the update-all-adblock-components-together branch from 45654a4 to 1eba796 Compare May 14, 2024 04:51

antonok-edm enabled auto-merge May 14, 2024 05:01

update all adblock components together

e86ccc2

antonok-edm added 2 commits May 14, 2024 09:43

add resource and catalog components to the set of checked components

f02a516

update all brave_shields constants to inline constexpr char

835551c

antonok-edm force-pushed the update-all-adblock-components-together branch from 1eba796 to 835551c Compare May 14, 2024 16:43

antonok-edm merged commit 77af98d into master May 14, 2024
18 checks passed

antonok-edm deleted the update-all-adblock-components-together branch May 14, 2024 18:22

github-actions bot added this to the 1.68.x - Nightly milestone May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `AdBlockDefaultResourceUpdateInterval` for all adblock-related components #21868

Use `AdBlockDefaultResourceUpdateInterval` for all adblock-related components #21868

antonok-edm commented Feb 2, 2024

simonhong Feb 2, 2024

antonok-edm Feb 2, 2024

atuchin-m Feb 2, 2024

atuchin-m Feb 2, 2024

mherrmann Apr 29, 2024

zenparsing Feb 2, 2024

antonok-edm Feb 2, 2024

atuchin-m Feb 2, 2024

atuchin-m Feb 2, 2024

atuchin-m Feb 2, 2024

atuchin-m Feb 2, 2024

atuchin-m Feb 2, 2024

atuchin-m Feb 2, 2024

antonok-edm commented May 7, 2024

goodov left a comment

goodov May 10, 2024

Use AdBlockDefaultResourceUpdateInterval for all adblock-related components #21868

Use AdBlockDefaultResourceUpdateInterval for all adblock-related components #21868

Conversation

antonok-edm commented Feb 2, 2024

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonok-edm commented May 7, 2024

goodov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `AdBlockDefaultResourceUpdateInterval` for all adblock-related components #21868

Use `AdBlockDefaultResourceUpdateInterval` for all adblock-related components #21868