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

Issue 29574665: Issue 5159 - Expose collapse property for BlockingFilter

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 1 day ago by hub
Modified:
1 week ago
Reviewers:
sergei, Wladimir Palant
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 5159 - Expose collapse property for BlockingFilter

Patch Set 1 #

Total comments: 2

Patch Set 2 : Added test for collapse #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -4 lines) Patch
M compiled/bindings/main.cpp View 1 chunk +3 lines, -1 line 0 comments Download
M compiled/filter/BlockingFilter.h View 1 chunk +4 lines, -0 lines 0 comments Download
M compiled/filter/RegExpFilter.h View 1 chunk +1 line, -1 line 0 comments Download
M compiled/filter/RegExpFilter.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M test/filterClasses.js View 1 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 4
hub
1 week, 1 day ago (2017-10-12 18:41:44 UTC) #1
sergei
LGTM https://codereview.adblockplus.org/29574665/diff/29574666/compiled/bindings/main.cpp File compiled/bindings/main.cpp (right): https://codereview.adblockplus.org/29574665/diff/29574666/compiled/bindings/main.cpp#newcode78 compiled/bindings/main.cpp:78: .property("collapse", &BlockingFilter::GetCollapse); JIC, it would be better to ...
1 week ago (2017-10-13 07:52:15 UTC) #2
Wladimir Palant
Yes, this definitely needs tests.
1 week ago (2017-10-13 09:28:24 UTC) #3
hub
1 week ago (2017-10-13 15:41:03 UTC) #4
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.
Sign in to reply to this message.

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