Issue 6886 - Return snippet filters instead of just scripts

Issue 6886 - Return snippet filters instead of just scripts

[hub, 2018-08-27 03:08:50]

We need the filter for logging. There is a followup patch for adblockpluschrome.

[Manish Jethani, 2018-08-27 05:40:11]

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#new...
lib/snippets.js:68: getSnippetsForDomain(domain)
Let's just call this `getFiltersForDomain`. Currently we have
`ElemHide.getSelectorsForDomain` (returns selectors),
`ElemHideEmulation.getRulesForDomain` (returns filters), and
`Snippets.getScriptsForDomain` (returns scripts). We will have to merge these at
some point and the only generic name one can come up with is
`getFiltersForDomain`, where the name of the module (i.e. `Snippets` in this
case) implies the kinds of filters that are returned.

https://codereview.adblockplus.org/29865583/diff/29865584/test/snippets.js
File test/snippets.js (right):

https://codereview.adblockplus.org/29865583/diff/29865584/test/snippets.js#ne...
test/snippets.js:51: let matches = Snippets.getSnippetsForDomain(domain);
I think it would be cleaner to keep the call to `test.deepEqual` free of this
additional mapping, so perhaps we could do this:

  let matches = Snippets.getFiltersForDomain(domain).map(({script}) => script);

[Manish Jethani, 2018-08-27 05:46:58]

Nit: "Issue 6843 - Return snippet filters matching instead of just script": I
would write this as "Issue 6843 - Return snippet filters instead of just
scripts"

BTW we need to have a separate Trac issue for core for this change, if I
understand the process correctly.

[Manish Jethani, 2018-08-27 05:56:10]

Filed a core issue for this:

https://issues.adblockplus.org/ticket/6886

[Manish Jethani, 2018-08-27 05:59:13]

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#new...
lib/snippets.js:66: * @return {Filter[]}
As in the rest of the file we can be more specific here. Let's make it `@returns
{Array.<SnippetFilter>}` here.

[hub, 2018-08-27 12:14:36]

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#new...
lib/snippets.js:66: * @return {Filter[]}
On 2018/08/27 05:59:13, Manish Jethani wrote:
> As in the rest of the file we can be more specific here. Let's make it
`@returns
> {Array.<SnippetFilter>}` here.

Done.

https://codereview.adblockplus.org/29865583/diff/29865584/lib/snippets.js#new...
lib/snippets.js:68: getSnippetsForDomain(domain)
On 2018/08/27 05:40:10, Manish Jethani wrote:
> Let's just call this `getFiltersForDomain`. Currently we have
> `ElemHide.getSelectorsForDomain` (returns selectors),
> `ElemHideEmulation.getRulesForDomain` (returns filters), and
> `Snippets.getScriptsForDomain` (returns scripts). We will have to merge these
at
> some point and the only generic name one can come up with is
> `getFiltersForDomain`, where the name of the module (i.e. `Snippets` in this
> case) implies the kinds of filters that are returned.

Done.

https://codereview.adblockplus.org/29865583/diff/29865584/test/snippets.js
File test/snippets.js (right):

https://codereview.adblockplus.org/29865583/diff/29865584/test/snippets.js#ne...
test/snippets.js:51: let matches = Snippets.getSnippetsForDomain(domain);
On 2018/08/27 05:40:10, Manish Jethani wrote:
> I think it would be cleaner to keep the call to `test.deepEqual` free of this
> additional mapping, so perhaps we could do this:
> 
>   let matches = Snippets.getFiltersForDomain(domain).map(({script}) =>
script);

Done.

[hub, 2018-08-27 12:19:03]

patch wasn't updated. please standby

[hub, 2018-08-27 12:21:51]

this time the patch was updated.

[Manish Jethani, 2018-08-27 12:36:21]

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#ne...
test/snippets.js:51: let matches = Snippets.getFiltersForDomain(domain).map(elem
=> elem.script);
Nit: Can we keep it `filter => filter.script` similar to the way it was
previously in `compareRules`? I feel like more explicit naming is usually
better. We could just split it into two lines if required.

[hub, 2018-08-27 12:53:36]

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#ne...
test/snippets.js:51: let matches = Snippets.getFiltersForDomain(domain).map(elem
=> elem.script);
On 2018/08/27 12:36:21, Manish Jethani wrote:
> Nit: Can we keep it `filter => filter.script` similar to the way it was
> previously in `compareRules`? I feel like more explicit naming is usually
> better. We could just split it into two lines if required.

Done.

[Manish Jethani, 2018-08-27 13:14:38]

LGTM