Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(142)

Issue 5840485868371968: Issue 616 - Add $generichide + $genericblock filter options and enforce them. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 6 months ago by kzar
Modified:
3 years, 11 months ago
CC:
Wladimir Palant
Visibility:
Public.

Description

Issue 616 - Add $generichide + $genericblock filter options and enforce them. If you want to you can use this page for testing http://holly.cat/genericblockhide-test.html I added these custom filters: ~somewhere.com,static.kzar.co.uk###kzarNonGenericHideWithException kzar_exception_specific_block.jpg$domain=~blah.kzar.co.uk|kzar.co.uk ||static.kzar.co.uk/genericblockhide-test/kzar_specific_block.jpg kzar.co.uk###kzarAd ~somewhere.com###kzarGenericHideOnlyExceptions ?KzarPopupBlock=$popup,domain=static.kzar.co.uk @@holly.cat$genericblock @@holly.cat$generichide Note: Now depends on this review https://codereview.adblockplus.org/5991150368325632/ because we make use of the specificOnly parameter.

Patch Set 1 #

Patch Set 2 : Rebased. #

Patch Set 3 : Tidied up genericblock logic in contentPolicy code. #

Patch Set 4 : Rebased onto typeMask changes #

Total comments: 5

Patch Set 5 : Simplified generic block logic #

Total comments: 4

Patch Set 6 : Use camelCase for genericblockMatch variable name #

Total comments: 7

Patch Set 7 : Fixed interaction with sitekey and generichide logic #

Total comments: 8

Patch Set 8 : Addressed Thomas' feedback #

Total comments: 8

Patch Set 9 : Improved logic based on Wladimir's suggestions #

Patch Set 10 : Removed duplicate comment #

Total comments: 2

Patch Set 11 : Remove extra parenthesis #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -4 lines) Patch
M lib/contentPolicy.js View 1 2 3 4 5 6 7 8 3 chunks +24 lines, -1 line 0 comments Download
M lib/filterClasses.js View 1 2 3 4 5 6 7 8 9 10 2 chunks +19 lines, -3 lines 0 comments Download

Messages

Total messages: 28
kzar
4 years, 6 months ago (2015-03-06 21:47:13 UTC) #1
kzar
Patch Set 3 : Tidied up genericblock logic in contentPolicy code.
4 years, 6 months ago (2015-03-13 19:25:33 UTC) #2
Sebastian Noack
I don't see anything obvious here. But I'm not too familiar with this code.
4 years, 6 months ago (2015-03-20 10:02:48 UTC) #3
kzar
Patch Set 4 : Rebased onto typeMask changes
4 years, 2 months ago (2015-07-14 16:44:04 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/5840485868371968/diff/29322311/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322311/lib/contentPolicy.js#newcode165 lib/contentPolicy.js:165: let genericblock = { match: null }; Nit: We ...
4 years, 2 months ago (2015-07-14 17:23:34 UTC) #5
kzar
Patch Set 5 : Simplified generic block logic https://codereview.adblockplus.org/5840485868371968/diff/29322311/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322311/lib/contentPolicy.js#newcode165 lib/contentPolicy.js:165: let ...
4 years, 2 months ago (2015-07-15 11:56:21 UTC) #6
kzar
Patch Set 5 : Simplified generic block logic (This patchset is good, sorry I had ...
4 years, 2 months ago (2015-07-15 12:00:02 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/5840485868371968/diff/29322392/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322392/lib/contentPolicy.js#newcode187 lib/contentPolicy.js:187: let genericblock_match = defaultMatcher.matchesAny(testWndLocation, RegExpFilter.typeMap.GENERICBLOCK, parentDocDomain, false); Nit: camelcase? ...
4 years, 2 months ago (2015-07-15 14:57:20 UTC) #8
kzar
Patch Set 6 : Use camelCase for genericblockMatch variable name https://codereview.adblockplus.org/5840485868371968/diff/29322392/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322392/lib/contentPolicy.js#newcode187 ...
4 years, 2 months ago (2015-07-15 15:10:42 UTC) #9
Sebastian Noack
LGTM from my side. Wladimir's go now.
4 years, 2 months ago (2015-07-15 15:36:11 UTC) #10
Felix Dahlke
Talked to Wladimir: He won't find the time to review this, so we agreed to ...
4 years, 1 month ago (2015-08-20 10:13:49 UTC) #11
Thomas Greiner
https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/contentPolicy.js#newcode187 lib/contentPolicy.js:187: let genericblockMatch = defaultMatcher.matchesAny(testWndLocation, RegExpFilter.typeMap.GENERICBLOCK, parentDocDomain, false); You forgot ...
4 years, 1 month ago (2015-08-20 11:36:07 UTC) #12
kzar
Patch Set 7 : Fixed interaction with sitekey and generichide logic https://codereview.adblockplus.org/5840485868371968/diff/29322428/lib/contentPolicy.js File lib/contentPolicy.js (right): ...
4 years ago (2015-08-25 17:42:08 UTC) #13
Thomas Greiner
https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/contentPolicy.js#newcode186 lib/contentPolicy.js:186: else if (contentType != Policy.type.ELEMHIDE) Detail: The "else" keyword ...
4 years ago (2015-08-28 10:28:24 UTC) #14
kzar
Patch Set 8 : Addressed Thomas' feedback https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/contentPolicy.js#newcode186 lib/contentPolicy.js:186: else if ...
4 years ago (2015-09-03 12:37:03 UTC) #15
Wladimir Palant
https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/contentPolicy.js#newcode166 lib/contentPolicy.js:166: let generichide = null; It's either a blocking rule ...
4 years ago (2015-09-03 13:09:43 UTC) #16
kzar
https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/contentPolicy.js#newcode298 lib/contentPolicy.js:298: if (genericblock && match.isGeneric()) On 2015/09/03 13:09:43, Wladimir Palant ...
4 years ago (2015-09-04 15:49:11 UTC) #17
Wladimir Palant
https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/contentPolicy.js#newcode298 lib/contentPolicy.js:298: if (genericblock && match.isGeneric()) On 2015/09/04 15:49:10, kzar wrote: ...
4 years ago (2015-09-04 16:16:23 UTC) #18
kzar
Patch Set 9 : Improved logic based on Wladimir's suggestions https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/contentPolicy.js File lib/contentPolicy.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29325803/lib/contentPolicy.js#newcode166 ...
4 years ago (2015-09-05 14:39:58 UTC) #19
Sebastian Noack
LGTM
3 years, 12 months ago (2015-09-21 14:27:20 UTC) #20
Thomas Greiner
https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/filterClasses.js#newcode484 lib/filterClasses.js:484: isGeneric: function() /**Boolean*/ On 2015/09/03 12:37:02, kzar wrote: > ...
3 years, 12 months ago (2015-09-21 16:53:37 UTC) #21
kzar
Patch Set 10 : Removed duplicate comment https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29324610/lib/filterClasses.js#newcode484 lib/filterClasses.js:484: isGeneric: function() ...
3 years, 12 months ago (2015-09-22 13:16:47 UTC) #22
Thomas Greiner
LGTM
3 years, 12 months ago (2015-09-22 14:12:35 UTC) #23
Sebastian Noack
LGTM
3 years, 12 months ago (2015-09-22 14:15:09 UTC) #24
Felix Dahlke
Looks good to me, just a small nit. https://codereview.adblockplus.org/5840485868371968/diff/29328484/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29328484/lib/filterClasses.js#newcode485 lib/filterClasses.js:485: return ...
3 years, 11 months ago (2015-09-28 08:28:45 UTC) #25
kzar
Patch Set 11 : Remove extra parenthesis https://codereview.adblockplus.org/5840485868371968/diff/29328484/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/5840485868371968/diff/29328484/lib/filterClasses.js#newcode485 lib/filterClasses.js:485: return (!(this.sitekeys ...
3 years, 11 months ago (2015-09-28 10:16:12 UTC) #26
Felix Dahlke
LGTM
3 years, 11 months ago (2015-09-28 12:00:02 UTC) #27
Thomas Greiner
3 years, 11 months ago (2015-09-28 12:32:09 UTC) #28
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5