-
Notifications
You must be signed in to change notification settings - Fork 851
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
Force aggressive cosmetic filtering on youtube #18813
Force aggressive cosmetic filtering on youtube #18813
Conversation
@@ -186,7 +186,7 @@ EngineFlags ShouldBlockRequestOnTaskRunner( | |||
|
|||
bool force_aggressive = SameDomainOrHost( | |||
ctx->initiator_url, | |||
url::Origin::CreateFromNormalizedTuple("https", "youtube.com", 80), | |||
url::Origin::CreateFromNormalizedTuple("https", "youtube.com", 443), |
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.
Doesn't seem to matter, but I changed the port to 443 here for consistency.
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 you also please add a comment that links a ticket(s) in both cases
looks great to me, though please make sure to sync with @cuba when hes back from PTO to make sure iOS matches |
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
content_settings->IsFirstPartyCosmeticFilteringEnabled(url_) || | ||
net::registry_controlled_domains::SameDomainOrHost( | ||
url_, | ||
url::Origin::CreateFromNormalizedTuple("https", "youtube.com", 443), |
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 would rather move this check to a separate statement, to avoid mixing exceptional/hardcoded cases with normal code. I.e. making corner cases very clear and visible never hurts
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.
+1
Also ask iOS team to make a ticket to sync these changes.
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.
BTW we already use per-frame GetBlinkPreferences().force_cosmetic_filtering
.
IMHO if we change force_cosmetic_filtering
to cosmetic_filtering_mode
we will get a single point to in the browser process to setup cosmetic filter mode for a single frame.
The exceptions like youtube & g_vetted_search_engines
could be moved there.
And cosmetic_filtering_mode
will be the one source to check for CosmeticFiltersJsHandler
.
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 agree this check is not great, but I think these changes could probably move to a follow-up if done immediately following this since it's apparently a high priority fix, but my bigger concern here is that there is no test to verify this.
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 relevant iOS changes to correspond with these changes are here: It's introducing network level standard mode and the youtube.com exception to both network blocking and CF.
cc @atuchin-m |
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.
pls make the requested changes in a followup
2a3e0a5
to
3e4d430
Compare
3e4d430
to
a9d5f08
Compare
Resolves brave/brave-browser#30896
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
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Visit
youtube.com
and verify that there's no difference in parts of the page visible when toggling betweenBlock trackers & ads
andAggressively block trackers & ads
in theAdvanced controls
section of the Shields panel.Toggling to
Allow all trackers & ads
or turning Shields off completely should still show preroll and page ads.