-
Notifications
You must be signed in to change notification settings - Fork 885
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
Cosmetic filtering fixes #12038
Cosmetic filtering fixes #12038
Conversation
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.
Just a few comments with some additional context around what changed in this PR
}; | ||
})();)"; | ||
|
||
const char kStyleSelectorsInjectScript[] = |
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.
I've removed kForceHideSelectorsInjectScript
and kStyleSelectorsInjectScript
entirely. They were previously adding rules to window.content_cosmetic.cosmeticStyleSheet
in order to keep track of the indices of rules, but this is no longer necessary as a result of using injectStylesheet
with an id of 0
. At that point, there is no special behavior in JS and we can construct the corresponding stylesheets directly in C++ instead.
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.
cool refactor!
web_frame->GetDocument().InsertStyleSheet( | ||
blink::WebString::FromUTF8(stylesheet), style_sheet_key, | ||
blink::WebDocument::kUserOrigin); |
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.
InsertStyleSheet
with kUserOrigin
provides the desired "injected stylesheet" behavior as if it were added by an extension, rather than "constructed stylesheet" from document.adoptedStyleSheets
.
It also has the added bonus of being able to pass in a custom blink::WebString
as an identifier for each stylesheet, allowing those sheets to be removed later by RemoveInsertedStyleSheet
. There was previously a lot of JS code to manage this around the document.adoptedStyleSheets
API.
InsertStyleSheet
and RemoveInsertedStyleSheet
are exposed directly to the JS cosmetic filtering script through injectStylesheet
and uninjectStylesheet
.
R"(addElementsDynamically(); | ||
async function waitCSSSelector() { |
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.
A Storybook has been deployed to preview UI for the latest push |
json_selectors = "[]"; | ||
base::Value* force_hide_selectors_list = | ||
resources_dict->FindListKey("force_hide_selectors"); | ||
if (force_hide_selectors_list->GetList().size() != 0) { |
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.
perhaps worth to check force_hide_selectors_list != nullptr
in case we don't have that key.
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.
oops, good catch! I thought I had it but I guess not
641229c
to
e109cb7
Compare
if (force_hide_selectors_list && | ||
base::Value* force_hide_selectors_list = | ||
resources_dict->FindListKey("force_hide_selectors"); | ||
if (force_hide_selectors && |
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.
force_hide_selectors_list
?
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.
🤦 yep...
updated
e109cb7
to
6185388
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.
++
A Storybook has been deployed to preview UI for the latest push |
Resolves brave/brave-browser#20177
Resolves brave/brave-browser#20722
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
See brave/brave-browser#20177 (comment)