Skip to content
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 first-party domain blocking #7952

Merged
merged 1 commit into from
Mar 5, 2021

Conversation

pilgrim-brave
Copy link
Contributor

@pilgrim-brave pilgrim-brave commented Feb 12, 2021

Resolves brave/brave-browser#14134

Submitter Checklist:

  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)
  • Requested a security/privacy review as needed

Reviewer Checklist:

  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_domain_blocking branch from 5d49955 to 20535ff Compare February 12, 2021 17:38
@pilgrim-brave pilgrim-brave self-assigned this Feb 12, 2021
@pilgrim-brave pilgrim-brave requested a review from a team February 12, 2021 17:55
@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_domain_blocking branch from 20535ff to 5d2086f Compare February 12, 2021 18:00
@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_domain_blocking branch from 5d2086f to 098b5de Compare February 12, 2021 18:49
@diracdeltas
Copy link
Member

does this do anything for pages loaded in iframes, or do those just get blocked/allowed without an interstitial?

@antonok-edm
Copy link
Collaborator

does this do anything for pages loaded in iframes, or do those just get blocked/allowed without an interstitial?

@diracdeltas those should already be handled from before this PR with the latter behavior

@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_domain_blocking branch 2 times, most recently from d4750a8 to 416c027 Compare February 17, 2021 19:45
@antonok-edm antonok-edm self-requested a review February 19, 2021 21:30
Copy link
Collaborator

@antonok-edm antonok-edm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested out this PR with DEPS updated to include brave/adblock-rust@732994b, and the resource type from domain_blocked_navigation_throttle.cc updated to kMainFrame.

After manually loading a custom DAT built using the corresponding adblock-rust version, entering any of the domains from Peter Lowe's list brings up an interstitial as expected. Other than the changes above, this looks good to me 😄

2021-02-19-133304_869x558_scrot

@pilgrim-brave the DAT in question is here, if you'd like to try it out on your end as well.

@pilgrim-brave
Copy link
Contributor Author

Just tested out this PR with DEPS updated to include brave/adblock-rust@732994b, and the resource type from domain_blocked_navigation_throttle.cc updated to kMainFrame.

That's great news, thank you. Should I include the DEPS update in this PR, or does adblock-rust have its own integration cycle?

@antonok-edm
Copy link
Collaborator

@pilgrim-brave you can go ahead and include it in this PR as soon as brave/adblock-rust-ffi#34 is merged into master, and then it should get automatically updated by a npm run sync.

@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_domain_blocking branch 4 times, most recently from 3c6e309 to 0ee841e Compare February 20, 2021 00:04
@pilgrim-brave
Copy link
Contributor Author

Ready for review.

Copy link
Collaborator

@antonok-edm antonok-edm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me 👍

@iefremov
Copy link
Contributor

I'll take another look soon

@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_domain_blocking branch from 0ee841e to 4383f57 Compare February 24, 2021 21:15
@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_domain_blocking branch 2 times, most recently from 4699822 to 1d30188 Compare March 3, 2021 20:15
@pilgrim-brave pilgrim-brave requested a review from iefremov March 3, 2021 20:16
@iefremov
Copy link
Contributor

iefremov commented Mar 4, 2021

@pilgrim-brave would be really helpful to see new changes as new commits when they are not trivial

@pilgrim-brave
Copy link
Contributor Author

@pilgrim-brave would be really helpful to see new changes as new commits when they are not trivial

OK. The latest version includes a feature flag as requested and checks it in MaybeCreateThrottle before creating anything; includes WillStartRedirect as requested; includes a SCOPED_UMA_HISTOGRAM_TIMER in ShouldBlockDomainOnTaskRunner as requested.

@pilgrim-brave
Copy link
Contributor Author

Also a new test to check that disabling the feature flag disables the interstitial properly.

@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_domain_blocking branch 3 times, most recently from 9d23fea to 0758881 Compare March 4, 2021 18:23
@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_domain_blocking branch from 0758881 to ed57581 Compare March 4, 2021 22:45
@pilgrim-brave
Copy link
Contributor Author

CI failures are unrelated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

First-party domain blocking
5 participants