-
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
Fix performance of cosmetic filters #12950
Conversation
A Storybook has been deployed to preview UI for the latest push |
@atuchin-m nice find. Need perhaps to polish and profile more what @antonok-edm did before we can re-merge it back. |
we have also reports on Android that 1.37.x is slower comparing to 1.36.x |
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.
need to fix a few tests before merge, but otherwise looks good
@@ -2158,7 +2153,6 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CosmeticFilteringIframeScriptlet) { | |||
ASSERT_EQ(true, EvalJs(contents, "show_ad")); | |||
} | |||
|
|||
<<<<<<< HEAD | |||
// Test cosmetic filtering on an element that already has an `!important` | |||
// marker on its `display` style. | |||
IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, |
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.
AdBlockServiceTest.CosmeticFilteringOverridesImportant
will have to be disabled as well, that was the primary issue fixed by the original PR
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.
@antonok-edm also CosmeticFilteringDynamicCustom doesn't work. Is it expected?
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.
Yeah, unfortunately it looks like I inserted two different fixes into the same PR. If you un-revert the changes in CosmeticFilteringDynamicCustom
and in test/data/cosmetic_filtering.html
, I think that one should work fine.
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 fact it's not enough..
[55080:56304:0411/175238.315:INFO:CONSOLE(5)] "still waiting for css selector", source: __const_std::string&_script__ (5)
content/public/test/browser_test_utils.cc(2659): error: Failed
RunLoop::Run() timed out. Timeout set at ProxyRunTestOnMainThreadLoop@content/public/test/browser_test_base.cc:809.
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 just pushed a couple of fixes for that as well.
fff2121
to
b8190f6
Compare
b8190f6
to
ad53d05
Compare
A Storybook has been deployed to preview UI for the latest push |
macOS CI failures are known/unrelated |
Reproduced the original issue on
Verification PASSED on
|
For brave/brave-browser#22030
1.37.5 => good
1.37.6 => bad
Try to revert #12038 that makes cosmetic filters slower.
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#22030 (comment)