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.

Sign up for GitHub

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

Jump to bottom

Remove exception for Tracking Protection #1770

Merged
merged 5 commits into from
Feb 25, 2019
Merged

Remove exception for Tracking Protection #1770

merged 5 commits into from
Feb 25, 2019

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented Feb 24, 2019 •
edited by bsclifton
Loading

Fix: brave/brave-browser#3475
Related PR: brave/adblock-lists#45

Related issue: brave/brave-browser#1108

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Verified that all lint errors/warnings are resolved (npm run lint)
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

See brave/brave-browser#3475

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

@bbondy bbondy self-assigned this Feb 24, 2019
@diracdeltas
Copy link
Member

For transparency, I would like to keep our documentation up-to-date on what Brave unblocks for webcompatibility that is not blocked in upstream adblock/tp. https://github.com/brave/brave-browser/wiki/Web-compatibility-issues-with-tracking-protection should be updated when this is merged to say that as of <date this was merged>, exceptions to tracking protection are handled by being added in adblock exceptions (to https://github.com/brave/adblock-lists/blob/master/brave-unbreak.txt ?)

@diracdeltas
Copy link
Member

+1 on this approach though

@diracdeltas diracdeltas mentioned this pull request Feb 24, 2019
Closed
@bbondy
Copy link
Member Author

bbondy commented Feb 24, 2019

Just adding some tests, so I don't want approvals until that's done, but the code changes and plan are ready. See brave/brave-browser#3475 for a detailed description of everything.

@bbondy bbondy force-pushed the exception-filters branch 2 times, most recently from 24422b6 to 49beb4c Compare February 25, 2019 02:18
SergeyZhukovsky
SergeyZhukovsky approved these changes Feb 25, 2019
View reviewed changes
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

I have concerns about speed, but we will check that out on brave-core first and apply to other platforms after. There are optimizations fixes coming to ad block lib as well. Should be good.

@bbondy bbondy changed the title WIP: Remove exception for Tracking Protection Remove exception for Tracking Protection Feb 25, 2019
@bbondy
Copy link
Member Author

bbondy commented Feb 25, 2019

This is ready to go, tests were added.

emerick
emerick approved these changes Feb 25, 2019
View reviewed changes
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM

@bbondy bbondy mentioned this pull request Feb 25, 2019
Merged
@bbondy bbondy added this to the 0.63.x - Nightly milestone Feb 25, 2019
@bbondy bbondy merged commit 4fa50dc into master Feb 25, 2019
@bsclifton bsclifton deleted the exception-filters branch February 25, 2019 21:12
@diracdeltas diracdeltas mentioned this pull request Feb 25, 2019
Closed
18 tasks
This was referenced Mar 4, 2019
Merged
Closed
@bbondy bbondy mentioned this pull request Mar 4, 2019
Merged
bbondy added a commit that referenced this pull request Mar 4, 2019
@bbondy
Merge pull request #1770 from brave/exception-filters
37d55f9
bbondy added a commit that referenced this pull request Mar 4, 2019
@bbondy
Merge pull request #1770 from brave/exception-filters
1b11f17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@emerick emerick emerick approved these changes

@diracdeltas diracdeltas Awaiting requested review from diracdeltas

@fmarier fmarier Awaiting requested review from fmarier

Assignees

@bbondy bbondy

Labels
None yet
Projects
None yet
Milestone
0.63.x - Release
Development

Successfully merging this pull request may close these issues.

Move hardcoded exceptions rules to more specific rules and unify exception handling
5 participants
@bbondy @diracdeltas @bridiver @emerick @SergeyZhukovsky