-
Notifications
You must be signed in to change notification settings - Fork 884
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 support for ABP filter redirects as mock responses #3419
Conversation
8315b0d
to
1de6056
Compare
6dcdec8
to
fa91445
Compare
f271ca1
to
fa4057a
Compare
04c6fcc
to
291f124
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just mentioned a couple of nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This results in less code i wrote shipping in brave-core, so thats an automatic good thing for product ;)
More seriously, terrific and fantastic :)
291f124
to
8267ce2
Compare
components/brave_shields/browser/adblock_stub_response_unittest.cc
Outdated
Show resolved
Hide resolved
components/brave_shields/browser/adblock_stub_response_unittest.cc
Outdated
Show resolved
Hide resolved
components/brave_shields/browser/adblock_stub_response_unittest.cc
Outdated
Show resolved
Hide resolved
components/brave_shields/browser/ad_block_service_browsertest.cc
Outdated
Show resolved
Hide resolved
components/brave_shields/browser/ad_block_service_browsertest.cc
Outdated
Show resolved
Hide resolved
LGTM in general, mostly readability and style-related comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether this is the right PR for this comment, but as far as I can see from https://github.com/brave/brave-core-crx-packager/pull/74/files#diff-90ea3bb8e019a7c1cb21f2332bf78c75R13, we appear to be pulling the scriptlets directly from the uBlock origin repo.
In the security review, @diracdeltas and I manually reviewed all existing scriptlets and then asked for a Brave clone of that repo so that we could review any new ones as they get added.
@fmarier that directory It looks like the URL of the uBlock git archive did end up sneaking through into that PR, but it's not used anywhere and I just filed brave/brave-core-crx-packager#75 to remove it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarifications @antonok-edm.
7736440
to
7434b24
Compare
7434b24
to
7e2ccbc
Compare
Fix: brave/brave-browser#6894
Unlike extensions like uBlock, we don't do this via
data:
URLs.This means that the response will appear to a website as a 200 with the proper mime type, but with the content we want from the resources JSON file.
See also:
https://github.com/brave/internal/issues/660
brave/adblock-rust#29
brave/adblock-rust#50
brave/adblock-rust-ffi#8
brave/brave-core-crx-packager#74
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.