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

Add custom text filter list #23548

Merged
merged 2 commits into from
May 31, 2024
Merged

Add custom text filter list #23548

merged 2 commits into from
May 31, 2024

Conversation

cuba
Copy link
Collaborator

@cuba cuba commented May 9, 2024

Resolves brave/brave-browser#36034
Security review: https://github.com/brave/reviews/issues/1612

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 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:

Valid rules

  1. Add the following rules and click save:
! Hide the header on example.com (CF Test)
example.com,example.net##h1

! Hide the logo on iana.org/help/example-domains (Network test)
||iana.org/_img/2022/iana-logo-header.svg

! Hide the brave logo on brave.com (Network test)
||brave.com/static-assets/images/brave-logo-sans-text.svg
  1. Ensure the parent screen shows a 2 line preview of the rules
  2. Go to https://example.com and ensure the header title is blocked (CF Blocking)
  3. Go to https://www.iana.org/help/example-domains (or click on "more information...") and ensure the logo is blocked (network blocking)

Invalid rules

  1. Add the following rule on a new line and click save:
||video.twimg.com/ext_tw_video/*/*.m3u8$domain=/^i[a-z]*\\.strmrdr[a-z]+\\..*/
  1. Ensure an error message appears

Placeholder

  1. Remove all content
  2. Ensure that the placeholder appears
  3. Save and ensure the placeholder appears on the parent screen

Video

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2024-05-29.at.13.01.18.mp4

Screenshots

Simulator Screenshot - iPhone 15 Pro - 2024-05-13 at 09 55 05

@cuba cuba force-pushed the js/add-text-filter-list branch 7 times, most recently from b0477f9 to a3a539d Compare May 11, 2024 10:24
@cuba cuba marked this pull request as ready for review May 11, 2024 10:28
@cuba cuba requested a review from a team as a code owner May 11, 2024 10:28
@cuba cuba added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 labels May 11, 2024
@cuba cuba force-pushed the js/add-text-filter-list branch 2 times, most recently from 9309c4a to 5c44aca Compare May 12, 2024 08:07
Copy link
Contributor

@stoletheminerals stoletheminerals left a comment

Choose a reason for hiding this comment

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

lgtm. One nit: we have Custom filter lists menu on top and custom filters on the bottom. Can we group it in one place? It's confusing right now

@cuba cuba force-pushed the js/add-text-filter-list branch from e970436 to 3db6aa4 Compare May 29, 2024 11:03
@cuba cuba force-pushed the js/add-text-filter-list branch from 3db6aa4 to 420082c Compare May 30, 2024 12:29
Copy link
Contributor

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

Description

This PR makes a few changes to the filter list and custom filter functionality in the iOS app:

  1. Adds a new CustomFilterListView which allows users to enter custom adblock rules directly in the app. This includes:

    • Saving/loading custom rules from disk
    • Validating the custom rules and showing any errors
    • Limiting the number of lines to 10,000 to prevent performance issues
    • Integrating custom rules into the adblock engine compilation process
  2. Renames some existing types and methods related to filter lists for clarity and consistency.

  3. Moves many of the filter list related strings to the BraveShields strings module.

The motivation seems to be to allow users to easily add their own custom adblock rules within the app itself, in addition to the existing functionality of adding custom filter list URLs.

Changes

Changes

  • CustomFilterListView.swift:
    • Added new view for entering custom filter rules with saving/loading/validation functionality
  • FilterListsView.swift:
    • Updated to include new CustomFilterListView
    • Renamed "Custom Filter Lists" section to "External Filter Lists"
    • Moved "Add Custom Filter List" button into the "External Filter Lists" section
  • AdBlockGroupsManager.swift:
    • Added methods to immediately update filter list files and trigger recompilation
  • ContentBlockerManager.swift:
    • Added testRules method to validate custom filter rules
    • Renamed customFilterList type to filterListURL
    • Added new filterListText type for custom filter rules
  • CustomFilterListStorage.swift:
    • Added methods to load/save/delete custom filter rules
    • Set a maximum limit of 10,000 lines for custom rules
  • BraveShields/ShieldStrings.swift:
    • Moved many filter list related strings here from BraveStrings module
  • Other minor changes and refactoring for clarity/consistency

Overall, this is a significant feature addition that is implemented across several files. The code looks well-organized and includes proper error handling and validation.

@cuba cuba merged commit 258f674 into master May 31, 2024
19 checks passed
@cuba cuba deleted the js/add-text-filter-list branch May 31, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 puLL-Merge
Projects
None yet
4 participants