Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjusted some adblock components update check interval #20557

Merged
merged 3 commits into from
Oct 18, 2023

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Oct 17, 2023

Resolves brave/brave-browser#32274

Check below three adblock components update every 100mins. and this value can be updated via griffin.

  • Brave Ad Block Resources Library
  • Brave Ad Block Updater
  • Brave Ad Block First Party Filters

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:

Test Plan:

Open brave://components and check adblock default resource component is updated every 100mins.

Pass --variations-server-url=https://variations.bravesoftware.com/seed to use 1min update check interval for QA tesing.

  1. Launch browser with above staging griffin url and check AdBlockComponentUpdateIntervalStudy:Enabled is shown
  2. Check above three adblock components are checked every 1 min

@simonhong simonhong self-assigned this Oct 17, 2023
@simonhong simonhong requested a review from pes10k October 17, 2023 01:39
@simonhong simonhong force-pushed the adjust_adblock_resource_component_update_interval branch from 0de0af1 to ac38e41 Compare October 17, 2023 05:30
@simonhong simonhong marked this pull request as ready for review October 17, 2023 06:06
@ryanbr
Copy link
Collaborator

ryanbr commented Oct 17, 2023

Lets change it too 100mins, the adblock component update is every hour. to keep in sync with atleast 1 adblock component update.

Copy link
Collaborator

@antonok-edm antonok-edm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed with @mschfh, can we make the interval configurable via a Griffin parameter? We don't know the optimal update frequency yet and we should be able to significantly shorten it once we get differential update support.

@simonhong
Copy link
Member Author

@ryanbr @antonok-edm My PR have griffin support already :) Do we want 100mins interval as a default?

@simonhong
Copy link
Member Author

Updated to 100mins for default update interval.

@antonok-edm
Copy link
Collaborator

ah, somehow I missed that 😄 thanks!

@simonhong simonhong requested a review from mschfh October 18, 2023 01:27
@simonhong simonhong force-pushed the adjust_adblock_resource_component_update_interval branch 2 times, most recently from ebb66f9 to 5d3d3ce Compare October 18, 2023 02:02
@simonhong simonhong force-pushed the adjust_adblock_resource_component_update_interval branch from 5d3d3ce to f3e8f53 Compare October 18, 2023 02:16
@simonhong simonhong changed the title Adjusted adblock default resource update check interval Adjusted some adblock components update check interval Oct 18, 2023
@simonhong simonhong merged commit 0049f41 into master Oct 18, 2023
6 checks passed
@simonhong simonhong deleted the adjust_adblock_resource_component_update_interval branch October 18, 2023 04:56
@github-actions github-actions bot added this to the 1.61.x - Nightly milestone Oct 18, 2023
BraveOnDemandUpdater::GetInstance()->OnDemandUpdate(
kAdBlockResourceComponentId);
}),
base::Seconds(base::RandInt(0, 10)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonhong
I wonder why we don't synchronize all the updates.
AFAIK, the component updater is designed to process multiple updates in a bunch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atuchin-m I didn't know we coud check update multiple components at once.
Could you point the example if possible? I'll try to find how to do it also.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonhong see SequentialUpdateChecker in our code base.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonhong see SequentialUpdateChecker in our code base.

Oh, Thanks. I'll check it out!

bsclifton pushed a commit that referenced this pull request Oct 21, 2023
bsclifton pushed a commit that referenced this pull request Oct 21, 2023
@kjozwiak
Copy link
Member

kjozwiak commented Oct 23, 2023

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.61.45 Chromium: 118.0.5993.96 (Official Build) nightly (64-bit)
-- | --
Revision | 31c36588a37ea88c2276d901ebd20dbf2344fde3
OS | Windows 11 Version 22H2 (Build 22621.2428)

Using the STR/Cases outlined via #20557 (comment), ensured the following:

  • launched 1.61.45 Chromium: 118.0.5993.96 using brave.exe --enable-logging=stderr --variations-server-url=https://variations.bravesoftware.com/seed and restarted the browser
  • ensured that AdBlockComponentUpdateIntervalStudy:Enabled via brave://version
  • ensured that Brave Ad Block First Party Filters, Brave Ad Block Updater, Brave Ad Block Resources Library were being updated every ~1min
    • open brave://components and refresh the page
    • all of the components should be appearing as Up-to-date
    • wait ~1min and you'll notice that Brave Ad Block First Party Filters, Brave Ad Block Updater, Brave Ad Block Resources Library will appear as Status - Component already up to date.
Example Example Example Example
AdBlockComponentUpdateIntervalStudyEnabled 1 2 3

Verification PASSED on Pixel 6 running Android 14 using the following build(s):

Brave | 1.61.46 Chromium: 118.0.5993.96 (Official Build) canary (64-bit)
--- | ---
Revision | af0652fa4009f20be16489fe6666fac382550c74
OS | Android 14; Build/U1B2.230922.006; 34; REL
  • launched 1.61.46 Chromium: 118.0.5993.96 and ensured that AdBlockComponentUpdateIntervalStudy:Enabled via brave://version
  • ensured that Brave Ad Block First Party Filters, Brave Ad Block Updater, Brave Ad Block Resources Library were being updated every ~1min
    • open brave://components and refresh the page
    • all of the components should be appearing as Up-to-date
    • wait ~1min and you'll notice that Brave Ad Block First Party Filters, Brave Ad Block Updater, Brave Ad Block Resources Library will appear as Status - Component already up to date.
Example Example
Screenshot_20231023-204517 Screenshot_20231023-204648

kjozwiak pushed a commit that referenced this pull request Oct 24, 2023
…0.x) (#20591)

* Fix #32799: Use correct components for ad-blocking (#20137)

* Uplift of #20557 (squashed) to beta

---------

Co-authored-by: Jacob Sikorski <jacob.sikorski@gmail.com>
kjozwiak pushed a commit that referenced this pull request Oct 24, 2023
…9.x) (#20592)

* Fix #32799: Use correct components for ad-blocking (#20137)

* Uplift of #20557 (squashed) to release

---------

Co-authored-by: Jacob Sikorski <jacob.sikorski@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component updater should check for Brave Ad Block Resources Library updates more often
6 participants