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

Decouple adblock engines from component loading methods #19366

Closed
antonok-edm opened this issue Nov 11, 2021 · 6 comments · Fixed by brave/brave-core#10994
Closed

Decouple adblock engines from component loading methods #19366

antonok-edm opened this issue Nov 11, 2021 · 6 comments · Fixed by brave/brave-core#10994

Comments

@antonok-edm
Copy link
Collaborator

Initially, adblock engines were only loaded with sources from Brave's component server. Over time, additional adblock engines were added that load from the browser's local preferences (custom filters) or from remote 3rd party servers (subscription lists). The code is still structured to support the initial use-case, which is confusing and limits future improvements. Once this is implemented properly, it will unblock future work like being able to combine the engines together for performance reasons, or untangling threading concerns.

This should ultimately be achieved by removing BraveComponent from the AdBlockService inheritance tree, and replacing it with a common SourceProvider interface that is generic enough to be implemented by the necessary components, custom filters loader, or subscription lists.

@LaurenWags
Copy link
Member

@antonok-edm could you help @brave/legacy_qa with specific test cases (particularly in high impact areas) that we could use to check this one? There are a number of previous issues related to adblock, so any assistance you can provide in focusing in on the scope would be appreciated. cc @rebron due to this being a large refactor per brave/brave-core#10994 (comment)

@antonok-edm
Copy link
Collaborator Author

@LaurenWags in terms of high impact areas, it'd definitely be good to check that ads on popular sites are still blocked consistently (in particular YouTube and perhaps some news sites).

It could be good to double-check the test plan from brave/brave-core#11621. Other than that, I don't have a good sense of where there might be any issues (or I would have fixed them already 😉). We do have a decent amount of automated test coverage here, but creativity would be appreciated!

@MadhaviSeelam MadhaviSeelam added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Mar 9, 2022
@MadhaviSeelam
Copy link

Verified 'Passed' using

Brave 1.37.89 Chromium: 99.0.4844.51 (Official Build) beta (64-bit)
Revision d537ec02474b5afe23684e7963d538896c63ac77-refs/branch-heads/4844@{#875}
OS Windows 11 Version 21H2 (Build 22000.556)

Testcase 1: Verify when Brave flag is enabled, cookie notice is dismissed and when disabled, cookie notice is displayed again

I. Enable #brave-adblock-cookie-list-default in brave://flags

  • Install 1.37.x
  • Launch Brave
  • Visited https://digikey.com
  • Cookie notice displayed on the top
  • Visit brave://flags and enabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://digikey.com and verified cookie notice no longer is displayed
  • Visited brave://adblock and verified an entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is available under
    Additional filters and is checked

II. Disable #brave-adblock-cookie-list-default in brave://flags

  • Visited brave://flag and disabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://digikey.com and verified cookie notice displayed again
  • Verified Easylist-Cookie List - Filter Obtrusive Cookie Notices is unchecked
1 2 3 4 5 6 7
1-cookienotice 2-flagenabled 3 easylistcookiechecked 4 cookienoticedismissed 5 flagdisabled 6 Cookienoticeddisplayedagain 7-easylistcookielistunchecked

Testcase #2 - Verify cookie banner is dismissed with aggressive mode in Shields

I. Enable #brave-adblock-cookie-list-default in brave://flags

  • Clean profile
  • Visited https://gulfnews.com
  • Cookie notice displayed at the bottom
  • Visited brave://flags and enabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://gulfnews.com but the cookie notice still displayed
  • Visited brave://adblock and verified an entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is available and is checked
  • Click on the Shields to select to Aggressive mode in `Trackers & ads blocked``
  • Verified cookie notice no longer displayed

II. Disable #brave-adblock-cookie-list-default in brave://flags

  • Visited brave://flag and disabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://gulfnews.com and verified cookie notice displayed at the bottom
  • Verified Easylist-Cookie List - Filter Obtrusive Cookie Notices in brave://adblock is unchecked
1 2 3 4 5 6 7
1-cookienotice 2 flagenabled 1-cookienotice 3 easlylist-cookie 4 cookienotice 5 flagdisabled 6 Cookienoticeddisplayedagain

Testcase #3 - Manually toggle `Easylist-Cookie List - Filter Obtrusive Cookie Notices'

  • Clean profile
  • Visited brave://adblock and checked Easylist-Cookie List - Filter Obtrusive Cookie Notices
  • Visited brave://flag and enabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://digikey.com and verified cookie notice is not displayed
  • Visited brave://adblock and verified Easylist-Cookie List - Filter Obtrusive Cookie Notices still checked
  • Returned to brave://flags disabled #brave-adblock-cookie-list-default flag and restarted the browser.
  • Visited brave://adblock and verified an entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is still checked
  • Visited brave://adblock and unchecked Easylist-Cookie List - Filter Obtrusive Cookie Notices
  • Visited brave://flags and enabled #brave-adblock-cookie-list-default flag and restarted the browser.
  • Visited https://digikey.com and confirmed cookie notice is displayed
1 2 3 4 5 6 7
1-Manual toggle - easylistcookie 2-flag enabled 3-cookienoticenolongerdisplayed 4 unchecked 5-flagdisabled 6 enabled 1-cookienotice

Note: In addition to above testing, verified Shields functionality works same as expected by comparing 1.37.x with1.36.x .

@MadhaviSeelam MadhaviSeelam added QA Pass-Win64 and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Mar 12, 2022
@stephendonner
Copy link

stephendonner commented Mar 16, 2022

Verified PASSED using

Brave 1.37.92 Chromium: 99.0.4844.51 (Official Build) beta (x86_64)
Revision d537ec02474b5afe23684e7963d538896c63ac77-refs/branch-heads/4844@{#875}
OS macOS Version 11.6.3 (Build 20G415)

Testcase 1: Verify when Brave flag is enabled, cookie notice is dismissed and when disabled, cookie notice is displayed again

I. Enable #brave-adblock-cookie-list-default in brave://flags

  • Install 1.37.x
  • Launch Brave
  • Visited https://digikey.com
  • Cookie notice displayed on the top
  • Visit brave://flags and enabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://digikey.com and verified cookie notice no longer is displayed
  • Visited brave://adblock and verified an entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is available under
    Additional filters and is checked

II. Disable #brave-adblock-cookie-list-default in brave://flags

  • Visited brave://flag and disabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://digikey.com and verified cookie notice displayed again
  • Verified Easylist-Cookie List - Filter Obtrusive Cookie Notices is unchecked
1 2 3 4 5 6 7
Screen Shot 2022-03-16 at 1 27 29 PM Screen Shot 2022-03-16 at 1 27 20 PM Screen Shot 2022-03-16 at 1 26 12 PM Screen Shot 2022-03-16 at 1 25 25 PM Screen Shot 2022-03-16 at 1 22 51 PM Screen Shot 2022-03-16 at 1 20 05 PM Screen Shot 2022-03-16 at 1 20 24 PM

Testcase 2 - Verify cookie banner is dismissed with aggressive mode in Shields

I. Enable #brave-adblock-cookie-list-default in brave://flags

  • Clean profile
  • Visited https://gulfnews.com
  • Cookie notice displayed at the bottom
  • Visited brave://flags and enabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://gulfnews.com but the cookie notice still displayed
  • Visited brave://adblock and verified an entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is available and is checked
  • Click on the Shields to select to Aggressive mode in `Trackers & ads blocked``
  • Verified cookie notice no longer displayed

II. Disable #brave-adblock-cookie-list-default in brave://flags

  • Visited brave://flag and disabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://gulfnews.com and verified cookie notice displayed at the bottom
  • Verified Easylist-Cookie List - Filter Obtrusive Cookie Notices in brave://adblock is unchecked
1 2 3 4 5 6 7
Screen Shot 2022-03-16 at 1 55 07 PM Screen Shot 2022-03-16 at 1 55 35 PM Screen Shot 2022-03-16 at 1 56 04 PM Screen Shot 2022-03-16 at 1 57 09 PM Screen Shot 2022-03-16 at 1 52 46 PM Screen Shot 2022-03-16 at 1 43 42 PM Screen Shot 2022-03-16 at 2 02 11 PM

Testcase 3 - Manually toggle `Easylist-Cookie List - Filter Obtrusive Cookie Notices'

  • Clean profile
  • Visited brave://adblock and checked Easylist-Cookie List - Filter Obtrusive Cookie Notices
  • Visited brave://flag and enabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser

NOTE: after enabling Easylist-Cookie List via brave://adblock, my experience diverges from @MadhaviSeelam in that I no longer see the cookie dialog here.

  • Visited brave://adblock and verified Easylist-Cookie List - Filter Obtrusive Cookie Notices still checked
  • Returned to brave://flags disabled #brave-adblock-cookie-list-default flag and restarted the browser.
  • Visited brave://adblock and verified an entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is still checked
  • Visited brave://adblock and unchecked Easylist-Cookie List - Filter Obtrusive Cookie Notices
  • Visited brave://flags and enabled #brave-adblock-cookie-list-default flag and restarted the browser.
  • Visited https://digikey.com and confirmed cookie notice is displayed
1 2 3 4 5 6 7
Screen Shot 2022-03-16 at 2 05 33 PM Screen Shot 2022-03-16 at 2 05 56 PM Screen Shot 2022-03-16 at 2 06 22 PM Screen Shot 2022-03-16 at 2 14 29 PM Screen Shot 2022-03-16 at 2 07 49 PM Screen Shot 2022-03-16 at 2 07 49 PM Screen Shot 2022-03-16 at 2 07 58 PM

Note: In addition to above testing, verified Shields functionality works same as expected by comparing 1.37.x with1.36.x on several of the top Alexa 20 sites, and other favorites of mine.

@Uni-verse
Copy link
Contributor

Uni-verse commented Mar 18, 2022

Verification completed using Beta 1.37.98, Chromium 99.0.4844.74 on Samsung GS 21 running Android 12

Testcase 1: Disable/Enable #brave-adblock-cookie-list-default in brave://flags

  • Verified when Brave flag is enabled, cookie notice is dismissed and when disabled, cookie notice is displayed again
  • Verified Easylist-Cookie List - Filter Obtrusive Cookie Notices is checked in brave://adblock if enabled

Test Case 2: Verify cookie banner is dismissed with aggressive mode in Shields

  • Verified when navigating to https://gulfnews.com on a clean profile will show the cookie banner
  • Verified cookie banner still shows when visiting https://gulfnews.com with #brave-adblock-cookie-list-default enabled
  • Verified switching to aggressive mode for adblock in shields will dismiss cookie banner

Issue encountered: when enabling adblock cooklie-list flag, and also with aggressive ad shields mode, cookie banner shows up but quickly blocked on page load on gulfnews.

Test Case 3: Manually toggle Easylist-Cookie List - Filter Obtrusive Cookie Notices

  • Clean profile
  • Visited brave://adblock and checked Easylist-Cookie List - Filter Obtrusive Cookie Notices
  • Visited brave://flag and enabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://digikey.com/ and verified cookie notice is not displayed
  • Visited brave://adblock and verified Easylist-Cookie List - Filter Obtrusive Cookie Notices still checked
  • Returned to brave://flags disabled #brave-adblock-cookie-list-default flag and restarted the browser.
  • Visited brave://adblock and verified an entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is still checked
  • Visited brave://adblock and unchecked Easylist-Cookie List - Filter Obtrusive Cookie Notices
  • Visited brave://flags and enabled #brave-adblock-cookie-list-default flag and restarted the browser.
  • Visited https://digikey.com/ and confirmed cookie notice is displayed

Screen Shot 2022-03-18 at 3 10 20 PM

@btlechowski
Copy link

Verification passed on

Brave 1.37.101 Chromium: 99.0.4844.83 (Official Build) beta (64-bit)
Revision b11086e62d7c1a44b0942ac5568d22a425c7ae35-refs/branch-heads/4844_74@{#5}
OS Ubuntu 20 LTS

Testcase 1: Verify when Brave flag is enabled, cookie notice is dismissed and when disabled, cookie notice is displayed again

I. Enable #brave-adblock-cookie-list-default in brave://flags

  • Install 1.37.x
  • Launch Brave
  • Visited https://digikey.com
  • Cookie notice displayed on the top
  • Visit brave://flags and enabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://digikey.com and verified cookie notice no longer is displayed
  • Visited brave://adblock and verified an entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is available under
    Additional filters and is checked

II. Disable #brave-adblock-cookie-list-default in brave://flags

  • Visited brave://flag and disabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://digikey.com and verified cookie notice displayed again
  • Verified Easylist-Cookie List - Filter Obtrusive Cookie Notices is unchecked

image
image

image
image

Testcase 2 - Verify cookie banner is dismissed with aggressive mode in Shields

I. Enable #brave-adblock-cookie-list-default in brave://flags

  • Clean profile
  • Visited https://gulfnews.com
  • Cookie notice displayed at the bottom
  • Visited brave://flags and enabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://gulfnews.com but the cookie notice still displayed
  • Visited brave://adblock and verified an entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is available and is checked
  • Click on the Shields to select to Aggressive mode in `Trackers & ads blocked``
  • Verified cookie notice no longer displayed

II. Disable #brave-adblock-cookie-list-default in brave://flags

  • Visited brave://flag and disabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser
  • Visited https://gulfnews.com and verified cookie notice displayed at the bottom
  • Verified Easylist-Cookie List - Filter Obtrusive Cookie Notices in brave://adblock is unchecked

image
image
image
image
image

Testcase 3 - Manually toggle `Easylist-Cookie List - Filter Obtrusive Cookie Notices'

  • Clean profile
  • Visited brave://adblock and checked Easylist-Cookie List - Filter Obtrusive Cookie Notices
  • Visited brave://flag and enabled #brave-adblock-cookie-list-default
  • Clicked Relaunch to relaunch the browser

NOTE: after enabling Easylist-Cookie List via brave://adblock, my experience diverges from @MadhaviSeelam in that I no longer see the cookie dialog here.

  • Visited brave://adblock and verified Easylist-Cookie List - Filter Obtrusive Cookie Notices still checked
  • Returned to brave://flags disabled #brave-adblock-cookie-list-default flag and restarted the browser.
  • Visited brave://adblock and verified an entry for Easylist-Cookie List - Filter Obtrusive Cookie Notices is still checked
  • Visited brave://adblock and unchecked Easylist-Cookie List - Filter Obtrusive Cookie Notices
  • Visited brave://flags and enabled #brave-adblock-cookie-list-default flag and restarted the browser.
  • Visited https://digikey.com and confirmed cookie notice is not displayed

image
image

Note: In addition to above testing, verified Shields functionality works same as expected by comparing 1.37.x with1.36.x on several of the top Alexa 20 sites, and other favorites of mine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment