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

Unrevert unified adblock catalog #21556

Merged
merged 2 commits into from
Jan 30, 2024
Merged

Conversation

antonok-edm
Copy link
Collaborator

@antonok-edm antonok-edm commented Jan 12, 2024

Revert of #20442

(This reverts commit 4ad53ad, reversing changes made to ec922be.)

Unblocks brave/brave-browser#34275

Resolves brave/brave-browser#33621

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:

Use the test plan from #19946 (comment)

@antonok-edm antonok-edm self-assigned this Jan 12, 2024
@github-actions github-actions bot added the CI/run-network-audit Run network-audit label Jan 12, 2024
@antonok-edm antonok-edm force-pushed the unrevert-unified-adblock-catalog branch 3 times, most recently from 723d39a to 38c6794 Compare January 17, 2024 04:19
@antonok-edm antonok-edm marked this pull request as ready for review January 19, 2024 20:59
@antonok-edm antonok-edm requested review from a team and iefremov as code owners January 19, 2024 20:59
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

android tests ++

Copy link
Contributor

@rillian rillian left a comment

Choose a reason for hiding this comment

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

rust++

We don't usually bump the copyright date for existing license headers, and one looks like it might be the result of a broken merge?

@@ -1,9 +1,9 @@
/* Copyright (c) 2019 The Brave Authors. All rights reserved.
/* Copyright (c) 2023 The Brave Authors. All rights reserved.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is technically a "new" file, although according to GitHub it's similar enough to the previous one that it's renamed instead. I'll keep the older copyright here too.

…atalog"

This reverts commit 4ad53ad, reversing
changes made to ec922be.
Copy link
Contributor

@cuba cuba left a comment

Choose a reason for hiding this comment

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

Tested in iOS and everything is fine there. Code-wise too everything looks good to me.

Copy link
Contributor

[puLL-Merge] - brave/brave-core@21556

Description

This PR removes the ad_block_regional_service_manager, renaming it to ad_block_component_service_manager to better reflect the fact that it is now managing all adblock lists served via CRX components, not just regional ones. It also refactors several related classes and tests, incorporating updates to improve handling of the adblock filter lists.

The motivation for this change seems to be to provide a more accurate and encompassing name according to the code now that the manager isn't solely responsible for regional adblock filters.

Changes

Changes

android/android_browser_tests.gni

  • Removed an entry related to test_filters_provider.cc from android_test_exception_sources, and added a dependency on test_support.

browser/brave_browser_process_impl.cc

  • Removed the inclusion of ad_block_regional_service_manager.h.

browser/brave_shields

  • Various changes across several test files replacing InstallDefaultAdBlockExtension with the new InstallDefaultAdBlockComponent.
  • Corrected a path and service name in domain_block_page_browsertest.cc.
  • Adapted tests in debounce_browsertest.cc to the new service manager.
  • Changes in ephemeral_storage_1p_domain_block_browsertest.cc to move towards engine_test_observer.
  • Updated includes in adblock_filter_list_catalog_entry.h for iOS, reflecting new properties (hidden, defaultEnabled, firstPartyProtections, permissionMask) and updated entries in adblock_filter_list_catalog_entry.mm.

Rust source files under components/brave_shields/adblock/rs

  • Introduced new file filter_set.rs and modifications within engine.rs for new adblock features.
  • Adjustments to error handling and conversions in lib.rs and result.rs.

components/brave_shields/browser directory

  • Renamed ad_block_regional_service_manager to ad_block_component_service_manager.
  • Created engine_test_observer for testing adblock engine updates.
  • Refactored test_filters_provider to include permissions and default flags.
  • Changes in ad_block_service, including commenting and removal of related adblock component IDs and public keys which seems to be relocated elsewhere.

Other Files

  • Removal of test data file redirect-rule.dat.

Security Hotspots

No specific security vulnerabilities were identified in the provided diff. It appears to be a refactor and renaming intended to improve code readability and maintainability.

@antonok-edm antonok-edm force-pushed the unrevert-unified-adblock-catalog branch 3 times, most recently from 37232aa to 2dd357f Compare January 26, 2024 08:15
@antonok-edm antonok-edm force-pushed the unrevert-unified-adblock-catalog branch from 2dd357f to 61424d0 Compare January 26, 2024 08:21
Copy link
Contributor

@iefremov iefremov left a comment

Choose a reason for hiding this comment

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

browser/net/* lgtm

@ShivanKaul ShivanKaul merged commit 14111b4 into master Jan 30, 2024
19 checks passed
@ShivanKaul ShivanKaul deleted the unrevert-unified-adblock-catalog branch January 30, 2024 23:36
@github-actions github-actions bot added this to the 1.64.x - Nightly milestone Jan 30, 2024
@antonok-edm antonok-edm mentioned this pull request Feb 14, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unrevert unified adblock catalog w/ perf fixes
6 participants