-
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
Implement cosmetic filtering #3303
Conversation
f87078a
to
de99c72
Compare
8421e0d
to
923c2d4
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.
This is great. On my first review pass, I was attempting to understand the code architecture via the method / API naming and calls. I couldn't do that so I had to read each function implementation to get a basic understanding. I recommend changing the naming to become more understandable. The API method descriptions in the JSON are clearer and could be a good starting point. Here's some suggestions:
-
chrome.braveShields.{get,set}CosmeticFilteredElementsAsync
⟶{get,set}CosmeticFilteringControlTypeAsync
-
chrome.braveShields.hostnameCosmeticResources
⟶getCosemeticStylesheetForHostname
Though perhaps...StyleRules...
is less confusing since it returns a string and not a javascript StyleSheet object. Also perhapscreate
/generate
is clearer thanget
? Behind the scenes is it fetching something or doing some heavy generation work? -
chrome.braveShields.classIdStylesheet
⟶getCosmeticStyleSheetForElements
Same considerations as forhostnameCosmeticResources
, perhaps...ForElementSelectors
is clearer as it doesn't accept actual DOM Elements, but that's quite long.
components/brave_extension/extension/brave_extension/content.ts
Outdated
Show resolved
Hide resolved
cssOrigin: 'user', | ||
runAt: 'document_start' | ||
}) | ||
}) |
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.
)
should be on separate line since {
is separate line to (
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.
Hmm, not sure what you mean? The first })
matches to ...insertCSS({
, the second matches to ...Stylesheet(... => {
.
This actually returns 3 distinct things dependent on the hostname: the stylesheet in string format, a list of exceptions to generic cosmetic rules, and a script to be injected in the page. I figured just calling it a stylesheet might be confusing. I'd lean towards By the way, do you know if there is a convenient way to build a Javascript StyleSheet object as part of the API? That sounds like it would make more sense, although it seems like it would be hard to do on the C++ side / without another layer of wrapping functions.
Perhaps |
93877bb
to
25d6c39
Compare
Also just added a toggle in settings corresponding to the new shields panel option. |
Any update here? |
@szaimen Still blocked on code reviews for brave/adblock-rust#50 unfortunately |
@antonok-edm any update here now that brave/adblock-rust#50 is merged? |
@szaimen #3419 and brave/brave-core-crx-packager#74 are required to fully support brave/adblock-rust#50 in the browser. Cosmetic filtering will be unblocked once that's sorted. |
b7d74d3
to
6aa0f4c
Compare
exceptions: genericExceptions | ||
}) | ||
|
||
chrome.tabs.executeScript(tabId, { |
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.
Can we call this only when injectedScript
is empty.
Also checking with @diracdeltas about if we can do this and trust the different lists we use. Are these generic scripts or only in a certain guaranteed format to hide things? Could you give an example of an ABP filter syntax rule that uses 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.
The scripts come from the same set of resources as for network redirects.
There are a few extra parameterized templated scriptlets, which were included in the same security review.
Here's an example of a scriptlet injection rule with 1 argument, directly from uBlock's filters:
computerbild.de##+js(abort-on-property-read.js, Date.prototype.toUTCString)
The argument (Date.prototype.toUTCString
) is interpolated into this function at the location of the first occurrence of {{1}}
, and the resulting function would be used in this executeScript
call.
|
||
function getCurrentURL () { | ||
return window.location.hostname | ||
} | ||
|
||
const getClassesAndIds = function (addedNodes: Element[]) { |
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.
cc @bridiver is there an observer you know of that we can use in c++ to get the list of IDs and class names on all elements instead of getting them here? It seems a little expensive to do on each page load.
How the API works, is you pass in any class names or IDs that might be used, and then the adblock engine determines the stylesheet to use for blocking.
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 is what uBlock Origin does and I haven't found the performance hit to be noticeable in practice, but it would be nice to take advantage of native APIs if possible.
components/brave_extension/extension/brave_extension/background/reducers/shieldsPanelReducer.ts
Outdated
Show resolved
Hide resolved
classIdBuffer.classes.push(...classes) | ||
classIdBuffer.ids.push(...ids) | ||
} else { | ||
chrome.runtime.sendMessage({ |
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.
In practice, do you know how often classIdStylesheet
gets hit for pages? Just making sure we're not inserting like 1000 stylesheets. I think maybe we should be debouncing these calls.
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.
It depends entirely on how often the site makes mutations to the page. I can try on a few different ones if you'd like specific examples.
Note that this is already batched on a per-mutation basis - i.e. if a node tree is added, it's only called once for all the classes and ids in the entire tree.
components/brave_extension/extension/brave_extension/_locales/en_US/messages.json
Outdated
Show resolved
Hide resolved
components/brave_shields/browser/ad_block_service_browsertest.cc
Outdated
Show resolved
Hide resolved
browser/resources/settings/default_brave_shields_page/default_brave_shields_page.html
Outdated
Show resolved
Hide resolved
6aa0f4c
to
6d321f1
Compare
d3a2595
to
cab4209
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 apart from the small redux point I raised. Let's get CI passing or investigate the issue. Great work!
DEPS
Outdated
@@ -1,7 +1,7 @@ | |||
use_relative_paths = True | |||
|
|||
deps = { | |||
"vendor/adblock_rust_ffi": "https://github.com/brave/adblock-rust-ffi.git@89127a30655eaf54cf73794309846084ea8b91b9", | |||
"vendor/adblock_rust_ffi": "https://github.com/brave/adblock-rust-ffi.git@6dad8c1b47c1b866fa1b1c19e36da6eb5797a524", |
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.
Before this gets merged please merge this:
brave/adblock-rust-ffi#7
And then update this changeset ref. As I only want to commit the ref here for things in master on the ffi repo.
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.
Approved, just one blocking note about the changeset 👍
Most of how this works is explained in doc comments from edit: brave/adblock-rust#61 |
e3b6f91
to
165eb3f
Compare
165eb3f
to
5ea7413
Compare
return a single script for cosmetic scriptlet injection
mutation observer applied only if rule exists notes change order mutation observer WIP browser tests isIdempotent isIdempotent mutation observer mutationObserver mutation observer mutationObserver mutation observer commented mutation observer for anton cleanup comments
Occasionally, chrome.tabs.insertCSS fails when triggered from chrome.webNavigation events. It cannot work unless some page content is already loaded. See https://bugs.chromium.org/p/chromium/issues/detail?id=331654#c15
d8fa2fe
to
ec80ce7
Compare
Jenkins is failing because of some glibc error that does not appear to be related to this PR |
I'm approving this as ok to merge because @antonok-edm has verified the build on linux |
This reverts commit fe9f891, reversing changes made to 4e6ba95. Was causing problems specifically on Linux because of the Rust dependency. This should fix brave/brave-browser#7055 This will un-fix brave/brave-browser#5381
Reverted with #4255 Note: when this was merged, milestone should have been set @antonok-edm (ex: set milestone to 1.4, set milestone for issue also). I re-opened the issue and left it without a milestone |
Submitter Checklist:
Closes brave/brave-browser#5381
Depends on brave/adblock-rust-ffi#3
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Majority of tests, covering fine-grained API behavior, are implemented in unit tests within https://github.com/brave/adblock-rust.
FFI behavior is validated by tests in https://github.com/brave/adblock-rust-ffi.
End-to-end browser behavior is verified by preloading custom cosmetic rules into the
adblock-rust
engine, loading a test page, and verifying that DOM elements show as expected usinggetComputedStyle
.Reviewer Checklist:
After-merge Checklist:
changes has landed on.