Support exception rules in the custom filters box of brave://adblock

[antonok-edm]

Resolves brave/brave-browser#5440

Submitter Checklist:

• There is a ticket for my issue.
• Used Github auto-closing keywords in the commit message.
• Wrote a good PR/commit description
• 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, npm run lint, npm run gn_check, npm run tslint
• Ran git rebase master (if needed).
• Requested a security/privacy review as needed.

Reviewer Checklist:

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

• The associated issue milestone is set to the smallest version that the
  changes has landed on.
• All relevant documentation has been updated, for instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
  • https://github.com/brave/brave-browser/wiki/QA-Guide
  • https://github.com/brave/brave-browser/wiki/P3A

Test Plan:

Verify that no ads appear on https://knowyourmeme.com using default Shields settings.

Then, add the following line to the Custom Filters box of brave://adblock:

@@*$domain=knowyourmeme.com

Return to https://knowyourmeme.com/ and force a refresh of the page. There should be new (possibly blank) banner ad spots on the page within one or two page loads.

[iefremov]

input parameters should precede output ones

https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs

[antonok-edm]

Updated here and in brave/adblock-rust-ffi#31, will fix DEPS here once that's merged done

[iefremov]

in fact I don't qute follow why do we need these arguments.

1. The signature already is quite crowded
2. Adding "previous" state to the function logic doesn't look like a good design
3. The core user of this API (ShouldBlockAdOnTaskRunner) just passes stub values - this means that these parameters just shouldn't exist in this API

Can we do "previous" check outside the function calls?

[antonok-edm]

@iefremov the "previous" values are used to avoid redundant filter matching logic within adblock-rust, e.g. it's unnecessary to check for a matching basic filter if we've already found an exception filter from another engine as it will never have any effect. I pulled the important logic outside the function calls since that can short circuit the entirety of the rest of the matching process.

(As a side note, I agree that it would be much better to have these all kept internal to adblock-rust, but the current multi-engine approach means we're limited to this for now. I do have an issue open for combining the engines, but that will require some major refactoring.)

I'm not sure what you mean by 3), could you give an example of how that would be avoided?

[iefremov]

is it possible to add tests for this?

[antonok-edm]

is it possible to add tests for this?

@iefremov I did add some tests in the adblock-rust-ffi repo, but I can add some here as well.

edit: added https://github.com/brave/brave-core/pull/7666/files#diff-a22832051483ad7ab6e9d1689654bbef9abf591a7b4a5c423812f098626b5d35R318-R336

[iefremov]

To reiterate: we should keep the facade API (aka AdBlockService) simple for its users, so I suggest we remove override from AdBlockService::ShouldStartRequest and make it separate method that does not expose any new arguments that are useless for callers.

[antonok-edm]

@iefremov I gave it one more try with inout parameters - the style guide says to prefer other solutions but I think this is one of the cases where it makes sense as it simplifies the multi-engine logic significantly.