Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29849575: Issue 6797 - Require active domains in snippet filters (Closed)

Created:
Aug. 7, 2018, 2:39 p.m. by Manish Jethani
Modified:
Aug. 8, 2018, 3:27 p.m.
Reviewers:
kzar
CC:
Thomas Greiner
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 6797 - Require active domains in snippet filters

Patch Set 1 #

Patch Set 2 : Add tests #

Total comments: 2

Patch Set 3 : Use different snippets names #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -14 lines) Patch
M lib/filterClasses.js View 1 chunk +12 lines, -8 lines 0 comments Download
M test/snippets.js View 1 2 1 chunk +19 lines, -6 lines 0 comments Download

Messages

Total messages: 7
Manish Jethani
Aug. 7, 2018, 2:39 p.m. (2018-08-07 14:39:06 UTC) #1
kzar
LGTM, but I guess this requires a UI change to add that new "filter_snippet_nodomain" string ...
Aug. 7, 2018, 2:45 p.m. (2018-08-07 14:45:07 UTC) #2
Manish Jethani
Patch Set 1 Patch Set 2: Add tests On 2018/08/07 14:45:07, kzar wrote: > LGTM, ...
Aug. 7, 2018, 2:52 p.m. (2018-08-07 14:52:44 UTC) #3
Manish Jethani
Dave, can you take a quick look at the test file?
Aug. 7, 2018, 3:04 p.m. (2018-08-07 15:04:37 UTC) #4
kzar
https://codereview.adblockplus.org/29849575/diff/29849578/test/snippets.js File test/snippets.js (right): https://codereview.adblockplus.org/29849575/diff/29849578/test/snippets.js#newcode60 test/snippets.js:60: "#$#foo", "example.com#$#foo", Shouldn't we use different snippet names for ...
Aug. 8, 2018, 11:31 a.m. (2018-08-08 11:31:50 UTC) #5
Manish Jethani
Patch Set 3: Use different snippet names https://codereview.adblockplus.org/29849575/diff/29849578/test/snippets.js File test/snippets.js (right): https://codereview.adblockplus.org/29849575/diff/29849578/test/snippets.js#newcode60 test/snippets.js:60: "#$#foo", "example.com#$#foo", ...
Aug. 8, 2018, 1:56 p.m. (2018-08-08 13:56:42 UTC) #6
kzar
Aug. 8, 2018, 3:06 p.m. (2018-08-08 15:06:41 UTC) #7
LGTM

Powered by Google App Engine
This is Rietveld