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

[iOS] Merge all filter lists (including general ones) into one engine #36035

Closed
cuba opened this issue Sep 6, 2022 · 4 comments · Fixed by brave/brave-core#22127
Closed

[iOS] Merge all filter lists (including general ones) into one engine #36035

cuba opened this issue Sep 6, 2022 · 4 comments · Fixed by brave/brave-core#22127
Assignees
Labels
feature/shields/adblock Blocking ads & trackers with Shields OS/iOS Fixes related to iOS browser functionality priority/P4 Planned work. We expect to get to it "soon". privacy QA Pass - iPhone QA/Test-All-Platforms QA/Yes release-notes/include

Comments

@cuba
Copy link

cuba commented Sep 6, 2022

Description:

The filter lists work better when they are merged into a single engine since some filter lists are designed to work with parent filter lists.

EDIT: This is not fully possible. However we can probably still achieve a part of this by doing the following

  1. Keep the multiple engines. (But we can remove any cosmetic and script rules...only keep network rules. We can also remove any network rules we're sure block async javascript requests (i.e. xhr/async fetch)). This will only be used to determine stats for anything blocked by content blockers, so only the rules that get compiled into content blocking rules matter here. (Might not be done initially as it will likely lead to minor stats miscalculations)
  2. Create a separate engine which contains all rules. This will be used for scriplets, cosmetic filters and any async network requests blocked by the request blocker script.

This requires the following Issues to be completed first:

  1. [iOS] Use correct components for Adblock services #32799

Note: The network blocking will still not be combined if we do this. But we should get better results for cosmetic filter, script injection and some async network blocking rules. This will also allow us to add custom lists exceptions to test CF and scripts.

Additional Information

For this we need the raw text versions of the files to be exposed. This is done here: brave/brave-core-crx-packager#468

@antonok-edm
Copy link
Collaborator

Copying the text from brave/brave-ios#6624, for reference:

Shipping precompiled DATs is problematic for several reasons detailed in the corresponding brave-browser issue: #25363

The FilterListCatalogEntry struct in brave-core currently has component id and key fields for two components, one of which is labeled as iOS-specific. The iOS-specific component is the legacy precompiled DAT version; the other one serves plaintext lists. brave-ios should migrate to the plaintext list format for better compatibility going forwards.

See also the related brave-core PR accomplishing this: #25363

@ShivanKaul ShivanKaul added privacy priority/P4 Planned work. We expect to get to it "soon". labels Jan 8, 2024
@cuba cuba transferred this issue from brave/brave-ios Feb 13, 2024
@cuba cuba changed the title Merge all filter lists (including general ones) into one engine [iOS] Merge all filter lists (including general ones) into one engine Feb 13, 2024
@cuba cuba added QA/Yes release-notes/include OS/iOS Fixes related to iOS browser functionality feature/shields/adblock Blocking ads & trackers with Shields labels Apr 4, 2024
@cuba cuba closed this as completed Apr 4, 2024
@kjozwiak kjozwiak added this to the 1.66.x - Beta milestone Apr 16, 2024
@kjozwiak
Copy link
Member

Adding missing milestone labels as brave/brave-core#22127 (comment) was formatted incorrectly so the automation didn't move the issue into the correct milestone.

@Uni-verse Uni-verse added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Apr 23, 2024
@Uni-verse
Copy link
Contributor

Uni-verse commented Apr 24, 2024

Verified on iPhone 12 Pro running iOS 17.3.1 using version 1.66.80

Using Test Plan in PR brave/brave-core#22127 (comment):

  • Ensured that ad-blocking works on https://youtube.com and other sites after upgrading.
  • Ensured that ad-blocking works when loading videos on https://youtube.com after fresh install.
  • Ensured that there are no issues with ad-block after engines are ready when relaunching app.
  • Ensured ads are blocked using standard and aggressive settings.
  • Ensured all test pages are passing mentioned in PR test plan.
Example Example Example Example Example
IMG_5983 IMG_5984 IMG_5985 IMG_5986 IMG_5987

@Uni-verse Uni-verse added QA Pass - iPhone QA/Test-All-Platforms and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Apr 26, 2024
@Uni-verse
Copy link
Contributor

Should be tested on iPad as well. Added QA/Test-All-Platforms label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/shields/adblock Blocking ads & trackers with Shields OS/iOS Fixes related to iOS browser functionality priority/P4 Planned work. We expect to get to it "soon". privacy QA Pass - iPhone QA/Test-All-Platforms QA/Yes release-notes/include
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants