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

Issue 29865583: Issue 6886 - Return snippet filters instead of just scripts (Closed)

Created:
Aug. 27, 2018, 3:08 a.m. by hub
Modified:
Aug. 27, 2018, 2:08 p.m.
Reviewers:
Manish Jethani, kzar
CC:
Sebastian Noack
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 6886 - Return snippet filters instead of just scripts

Patch Set 1 #

Total comments: 6

Patch Set 2 : rename it getFiltersForDomain #

Patch Set 3 : rename it getFiltersForDomain (for real) #

Total comments: 2

Patch Set 4 : Fixed nit in test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -7 lines) Patch
M lib/snippets.js View 1 2 1 chunk +4 lines, -4 lines 0 comments Download
M test/snippets.js View 1 2 3 2 chunks +4 lines, -3 lines 0 comments Download

Messages

Total messages: 12
hub
Aug. 27, 2018, 3:08 a.m. (2018-08-27 03:08:15 UTC) #1
hub
We need the filter for logging. There is a followup patch for adblockpluschrome.
Aug. 27, 2018, 3:08 a.m. (2018-08-27 03:08:50 UTC) #2
Manish Jethani
https://codereview.adblockplus.org/29865583/diff/29865584/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29865583/diff/29865584/lib/snippets.js#newcode68 lib/snippets.js:68: getSnippetsForDomain(domain) Let's just call this `getFiltersForDomain`. Currently we have ...
Aug. 27, 2018, 5:40 a.m. (2018-08-27 05:40:11 UTC) #3
Manish Jethani
Nit: "Issue 6843 - Return snippet filters matching instead of just script": I would write ...
Aug. 27, 2018, 5:46 a.m. (2018-08-27 05:46:58 UTC) #4
Manish Jethani
Filed a core issue for this: https://issues.adblockplus.org/ticket/6886
Aug. 27, 2018, 5:56 a.m. (2018-08-27 05:56:10 UTC) #5
Manish Jethani
https://codereview.adblockplus.org/29865583/diff/29865584/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29865583/diff/29865584/lib/snippets.js#newcode66 lib/snippets.js:66: * @return {Filter[]} As in the rest of the ...
Aug. 27, 2018, 5:59 a.m. (2018-08-27 05:59:13 UTC) #6
hub
Updated patch https://codereview.adblockplus.org/29865583/diff/29865584/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29865583/diff/29865584/lib/snippets.js#newcode66 lib/snippets.js:66: * @return {Filter[]} On 2018/08/27 05:59:13, Manish ...
Aug. 27, 2018, 12:14 p.m. (2018-08-27 12:14:36 UTC) #7
hub
patch wasn't updated. please standby
Aug. 27, 2018, 12:19 p.m. (2018-08-27 12:19:03 UTC) #8
hub
this time the patch was updated.
Aug. 27, 2018, 12:21 p.m. (2018-08-27 12:21:51 UTC) #9
Manish Jethani
LGTM except nits https://codereview.adblockplus.org/29865583/diff/29866561/test/snippets.js File test/snippets.js (right): https://codereview.adblockplus.org/29865583/diff/29866561/test/snippets.js#newcode51 test/snippets.js:51: let matches = Snippets.getFiltersForDomain(domain).map(elem => elem.script); ...
Aug. 27, 2018, 12:36 p.m. (2018-08-27 12:36:21 UTC) #10
hub
updated patch https://codereview.adblockplus.org/29865583/diff/29866561/test/snippets.js File test/snippets.js (right): https://codereview.adblockplus.org/29865583/diff/29866561/test/snippets.js#newcode51 test/snippets.js:51: let matches = Snippets.getFiltersForDomain(domain).map(elem => elem.script); On ...
Aug. 27, 2018, 12:53 p.m. (2018-08-27 12:53:36 UTC) #11
Manish Jethani
Aug. 27, 2018, 1:14 p.m. (2018-08-27 13:14:38 UTC) #12
LGTM

Powered by Google App Engine
This is Rietveld