Issue 5159 - Expose collapse property for BlockingFilter

Issue 5159 - Expose collapse property for BlockingFilter

[sergei, 2017-10-13 07:52:15]

LGTM

https://codereview.adblockplus.org/29574665/diff/29574666/compiled/bindings/m...
File compiled/bindings/main.cpp (right):

https://codereview.adblockplus.org/29574665/diff/29574666/compiled/bindings/m...
compiled/bindings/main.cpp:78: .property("collapse",
&BlockingFilter::GetCollapse);
JIC, it would be better to add tests that the property does work and that its
default value is true.

[Wladimir Palant, 2017-10-13 09:28:24]

Yes, this definitely needs tests.

[hub, 2017-10-13 15:41:03]

test that collapse is true on blocking filter, and undefined on another filter.

https://codereview.adblockplus.org/29574665/diff/29574666/compiled/bindings/m...
File compiled/bindings/main.cpp (right):

https://codereview.adblockplus.org/29574665/diff/29574666/compiled/bindings/m...
compiled/bindings/main.cpp:78: .property("collapse",
&BlockingFilter::GetCollapse);
On 2017/10/13 07:52:14, sergei wrote:
> JIC, it would be better to add tests that the property does work and that its
> default value is true.

Done.

[sergei, 2017-11-21 11:24:04]

https://codereview.adblockplus.org/29574665/diff/29574666/compiled/bindings/m...
File compiled/bindings/main.cpp (right):

https://codereview.adblockplus.org/29574665/diff/29574666/compiled/bindings/m...
compiled/bindings/main.cpp:78: .property("collapse",
&BlockingFilter::GetCollapse);
On 2017/10/13 15:41:03, hub wrote:
> On 2017/10/13 07:52:14, sergei wrote:
> > JIC, it would be better to add tests that the property does work and that
its
> > default value is true.
> 
> Done.

Could you please add the test for ~collapse?

https://codereview.adblockplus.org/29574665/diff/29575641/compiled/filter/Reg...
File compiled/filter/RegExpFilter.cpp (right):

https://codereview.adblockplus.org/29574665/diff/29575641/compiled/filter/Reg...
compiled/filter/RegExpFilter.cpp:221: data.mCollapse = reverse ? false : true;
I wonder whether it should be just `!reverse`.

[hub, 2017-11-21 15:15:20]

https://codereview.adblockplus.org/29574665/diff/29575641/compiled/filter/Reg...
File compiled/filter/RegExpFilter.cpp (right):

https://codereview.adblockplus.org/29574665/diff/29575641/compiled/filter/Reg...
compiled/filter/RegExpFilter.cpp:221: data.mCollapse = reverse ? false : true;
On 2017/11/21 11:24:03, sergei wrote:
> I wonder whether it should be just `!reverse`.

good point. Done.

[hub, 2017-11-21 15:18:19]

https://codereview.adblockplus.org/29574665/diff/29574666/compiled/bindings/m...
File compiled/bindings/main.cpp (right):

https://codereview.adblockplus.org/29574665/diff/29574666/compiled/bindings/m...
compiled/bindings/main.cpp:78: .property("collapse",
&BlockingFilter::GetCollapse);
On 2017/11/21 11:24:03, sergei wrote:
> On 2017/10/13 15:41:03, hub wrote:
> > On 2017/10/13 07:52:14, sergei wrote:
> > > JIC, it would be better to add tests that the property does work and that
> its
> > > default value is true.
> > 
> > Done.
> 
> Could you please add the test for ~collapse?

will do

[hub, 2017-11-21 15:37:59]

https://codereview.adblockplus.org/29574665/diff/29574666/compiled/bindings/m...
File compiled/bindings/main.cpp (right):

https://codereview.adblockplus.org/29574665/diff/29574666/compiled/bindings/m...
compiled/bindings/main.cpp:78: .property("collapse",
&BlockingFilter::GetCollapse);
On 2017/11/21 15:18:19, hub wrote:
> On 2017/11/21 11:24:03, sergei wrote:
> > On 2017/10/13 15:41:03, hub wrote:
> > > On 2017/10/13 07:52:14, sergei wrote:
> > > > JIC, it would be better to add tests that the property does work and
that
> > its
> > > > default value is true.
> > > 
> > > Done.
> > 
> > Could you please add the test for ~collapse?
> 
> will do

Done.

[sergei, 2017-11-21 16:17:01]

https://codereview.adblockplus.org/29574665/diff/29613783/test/filterClasses.js
File test/filterClasses.js (right):

https://codereview.adblockplus.org/29574665/diff/29613783/test/filterClasses....
test/filterClasses.js:106: if (filter instanceof BlockingFilter)
What about checking that `filter.collapse` and `collapse` are undefined if the
filter is not a blocking filter? (I've just seen ["@@asdf$image,~collapse",
WhitelistFilter, "whitelist", 0],).

[hub, 2017-11-21 17:42:47]

https://codereview.adblockplus.org/29574665/diff/29613783/test/filterClasses.js
File test/filterClasses.js (right):

https://codereview.adblockplus.org/29574665/diff/29613783/test/filterClasses....
test/filterClasses.js:106: if (filter instanceof BlockingFilter)
On 2017/11/21 16:17:01, sergei wrote:
> What about checking that `filter.collapse` and `collapse` are undefined if the
> filter is not a blocking filter? (I've just seen ["@@asdf$image,~collapse",
> WhitelistFilter, "whitelist", 0],).

done.

[sergei, 2017-11-21 19:20:52]

LGTM