Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Allow adblock checking within the same origin #11005

Merged
merged 1 commit into from
Sep 19, 2017
Merged

Allow adblock checking within the same origin #11005

merged 1 commit into from
Sep 19, 2017

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented Sep 19, 2017

Fix #11004

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).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@bbondy bbondy added this to the 0.20.x (Developer Channel) milestone Sep 19, 2017
@bbondy bbondy self-assigned this Sep 19, 2017
@bbondy
Copy link
Member Author

bbondy commented Sep 19, 2017

pls hold on reviewing, just adding some tests in another commit first.

@bbondy
Copy link
Member Author

bbondy commented Sep 19, 2017

OK good to go.

@codecov-io
Copy link

codecov-io commented Sep 19, 2017

Codecov Report

Merging #11005 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #11005      +/-   ##
==========================================
- Coverage   54.69%   54.68%   -0.01%     
==========================================
  Files         249      249              
  Lines       21895    21891       -4     
  Branches     3415     3415              
==========================================
- Hits        11976    11972       -4     
  Misses       9919     9919
Flag Coverage Δ
#unittest 54.68% <100%> (-0.01%) ⬇️
Impacted Files Coverage Δ
js/data/siteHacks.js 43.93% <ø> (ø) ⬆️
app/browser/ads/adBlockUtil.js 100% <100%> (ø) ⬆️

Copy link
Member

@diracdeltas diracdeltas left a comment

Choose a reason for hiding this comment

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

this seems to break adops.com (regression of #1353)

@bbondy
Copy link
Member Author

bbondy commented Sep 19, 2017

Mentioned on Slack but this isn't meant to review all filtering list rules. Some won't work like the example you gave but it works the same as uBlock and other adblockers. We should fix cases like this, if we care about them, in our maintained lists on github.com/brave/adblock-lists

@diracdeltas
Copy link
Member

@bbondy noted. in #1353 i asked whether this was an issue that should be fixed in the adblock module and you mentioned (#1353 (comment)) that we should add a check for first-partiness in app/adblock.js. so this appeared to be a regression

@bbondy
Copy link
Member Author

bbondy commented Sep 19, 2017

Yep but things change in a year and a half.

We don't have the same adblock library we had back then, we now have lists we maintain which we didn't then, and also we were consciously trying to not block first party things then. I'm going to re-open that one and fix with an adblock definition update.

@bbondy bbondy merged commit 560f997 into master Sep 19, 2017
@bbondy bbondy deleted the 11004 branch October 23, 2017 14:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants