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

Issue 29366747: Issue 4657 - Add Acceptable Ads API (Closed)

Created:
Dec. 2, 2016, 4:27 p.m. by sergei
Modified:
April 6, 2017, 4:45 p.m.
Reviewers:
hub
CC:
Felix Dahlke, Oleksandr, Eric
Visibility:
Public.

Patch Set 1 #

Total comments: 23

Patch Set 2 : address comments and rebase #

Total comments: 6

Patch Set 3 : address comment #

Patch Set 4 : rebase and address comment #

Total comments: 2

Patch Set 5 : fix typo #

Unified diffs Side-by-side diffs Delta from patch set Stats (+382 lines, -5 lines) Patch
M include/AdblockPlus/FilterEngine.h View 1 2 3 2 chunks +31 lines, -0 lines 0 comments Download
M lib/api.js View 1 2 3 2 chunks +32 lines, -1 line 0 comments Download
M lib/compat.js View 1 2 3 1 chunk +45 lines, -0 lines 0 comments Download
M src/FilterEngine.cpp View 1 2 3 2 chunks +20 lines, -0 lines 0 comments Download
M test/FilterEngine.cpp View 1 2 3 4 2 chunks +254 lines, -4 lines 0 comments Download

Messages

Total messages: 11
sergei
https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js File lib/api.js (right): https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js#newcode31 lib/api.js:31: // returns first element of the array that satifies ...
Dec. 2, 2016, 4:36 p.m. (2016-12-02 16:36:07 UTC) #1
sergei
https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js File lib/api.js (right): https://codereview.adblockplus.org/29366747/diff/29366748/lib/api.js#newcode31 lib/api.js:31: // returns first element of the array that satifies ...
Dec. 2, 2016, 4:38 p.m. (2016-12-02 16:38:02 UTC) #2
Eric
https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus/FilterEngine.h#newcode126 include/AdblockPlus/FilterEngine.h:126: Typo: extra column of spaces https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus/FilterEngine.h#newcode282 include/AdblockPlus/FilterEngine.h:282: * Otherwise ...
Dec. 5, 2016, 2:40 p.m. (2016-12-05 14:40:58 UTC) #3
sergei
https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29366747/diff/29366748/include/AdblockPlus/FilterEngine.h#newcode126 include/AdblockPlus/FilterEngine.h:126: On 2016/12/05 14:40:57, Eric wrote: > Typo: extra column ...
March 17, 2017, 3:55 p.m. (2017-03-17 15:55:26 UTC) #4
hub
https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus/FilterEngine.h#newcode126 include/AdblockPlus/FilterEngine.h:126: bool IsAA(); Shouldn't this be a const method? https://codereview.adblockplus.org/29366747/diff/29386653/lib/api.js ...
March 17, 2017, 8:04 p.m. (2017-03-17 20:04:15 UTC) #5
sergei
https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus/FilterEngine.h#newcode126 include/AdblockPlus/FilterEngine.h:126: bool IsAA(); On 2017/03/17 20:04:15, hub wrote: > Shouldn't ...
March 17, 2017, 10:10 p.m. (2017-03-17 22:10:07 UTC) #6
hub
As it is right now, this patch need to be rebased ; with my other ...
April 5, 2017, 1:35 p.m. (2017-04-05 13:35:03 UTC) #7
sergei
rebase and address comment https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29366747/diff/29386653/include/AdblockPlus/FilterEngine.h#newcode126 include/AdblockPlus/FilterEngine.h:126: bool IsAA(); On 2017/04/05 13:35:02, ...
April 5, 2017, 2:43 p.m. (2017-04-05 14:43:29 UTC) #8
hub
LGTM https://codereview.adblockplus.org/29366747/diff/29403741/test/FilterEngine.cpp File test/FilterEngine.cpp (right): https://codereview.adblockplus.org/29366747/diff/29403741/test/FilterEngine.cpp#newcode912 test/FilterEngine.cpp:912: auto subscruption = filterEngine->GetSubscription(kOtherSubscriptionUrl); 'nit: I didn't see ...
April 5, 2017, 2:52 p.m. (2017-04-05 14:52:47 UTC) #9
sergei
https://codereview.adblockplus.org/29366747/diff/29403741/test/FilterEngine.cpp File test/FilterEngine.cpp (right): https://codereview.adblockplus.org/29366747/diff/29403741/test/FilterEngine.cpp#newcode912 test/FilterEngine.cpp:912: auto subscruption = filterEngine->GetSubscription(kOtherSubscriptionUrl); On 2017/04/05 14:52:47, hub wrote: ...
April 5, 2017, 4:54 p.m. (2017-04-05 16:54:45 UTC) #10
hub
April 6, 2017, 1:14 p.m. (2017-04-06 13:14:18 UTC) #11
LGTM

Powered by Google App Engine
This is Rietveld