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

Issue 29581602: Issue 5141 - [emscripten] Convert filter matcher to C++

Created:
Oct. 17, 2017, 12:31 p.m. by Wladimir Palant
Modified:
Dec. 21, 2017, 10:57 a.m.
Reviewers:
sergei, hub
CC:
Felix Dahlke
Base URL:
https://hg.adblockplus.org/adblockpluscore
Visibility:
Public.

Description

These changes rely on code currently under review in https://codereview.adblockplus.org/29574591/, https://codereview.adblockplus.org/29581588/ and https://codereview.adblockplus.org/29572731/. COLLABORATOR=sergei@adblockplus.org

Patch Set 1 #

Total comments: 28
Unified diffs Side-by-side diffs Delta from patch set Stats (+404 lines, -450 lines) Patch
M compiled/Map.h View 1 chunk +5 lines, -0 lines 0 comments Download
M compiled/bindings/main.cpp View 2 chunks +7 lines, -0 lines 0 comments Download
M compiled/filter/RegExpFilter.h View 2 chunks +7 lines, -12 lines 1 comment Download
M compiled/filter/RegExpFilter.cpp View 2 chunks +5 lines, -3 lines 1 comment Download
A compiled/matcher/Matcher.h View 1 chunk +42 lines, -0 lines 5 comments Download
A compiled/matcher/Matcher.cpp View 1 chunk +175 lines, -0 lines 14 comments Download
M lib/matcher.js View 1 chunk +2 lines, -435 lines 2 comments Download
M meson.build View 1 chunk +1 line, -0 lines 0 comments Download
A test/matcher.js View 1 chunk +160 lines, -0 lines 5 comments Download

Messages

Total messages: 8
Wladimir Palant
https://codereview.adblockplus.org/29581602/diff/29581603/compiled/filter/RegExpFilter.h File compiled/filter/RegExpFilter.h (left): https://codereview.adblockplus.org/29581602/diff/29581603/compiled/filter/RegExpFilter.h#oldcode33 compiled/filter/RegExpFilter.h:33: }; The matcher might need to access the pattern ...
Oct. 17, 2017, 12:51 p.m. (2017-10-17 12:51:22 UTC) #1
Felix Dahlke
It looks like you've also based this on https://codereview.adblockplus.org/29527808/ (introducing Meson). I don't think we ...
Oct. 17, 2017, 1:42 p.m. (2017-10-17 13:42:02 UTC) #2
Wladimir Palant
On 2017/10/17 13:42:02, Felix Dahlke wrote: > It looks like you've also based this on ...
Oct. 17, 2017, 2:35 p.m. (2017-10-17 14:35:23 UTC) #3
sergei
I have not tried it yet but in general good, merely some thought regarding the ...
Oct. 18, 2017, 10:58 a.m. (2017-10-18 10:58:44 UTC) #4
sergei
https://codereview.adblockplus.org/29581602/diff/29581603/test/matcher.js File test/matcher.js (right): https://codereview.adblockplus.org/29581602/diff/29581603/test/matcher.js#newcode137 test/matcher.js:137: checkMatch(test, ["abc$domain=foo.com", "abc$domain=bar.com", "abc$domain=~foo.com|~bar.com"], "http://abc/def", "IMAGE", "foo.com", false, null, ...
Nov. 20, 2017, 4:30 p.m. (2017-11-20 16:30:50 UTC) #5
hub
https://codereview.adblockplus.org/29581602/diff/29581603/compiled/matcher/Matcher.cpp File compiled/matcher/Matcher.cpp (right): https://codereview.adblockplus.org/29581602/diff/29581603/compiled/matcher/Matcher.cpp#newcode52 compiled/matcher/Matcher.cpp:52: int32_t PrefixLength() IMHO this should be const method. https://codereview.adblockplus.org/29581602/diff/29581603/compiled/matcher/Matcher.cpp#newcode57 ...
Nov. 20, 2017, 5:38 p.m. (2017-11-20 17:38:33 UTC) #6
sergei
https://codereview.adblockplus.org/29581602/diff/29581603/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29581602/diff/29581603/lib/matcher.js#newcode21 lib/matcher.js:21: exports.Matcher = Matcher; this Matcher is used in lib/notifications.js, ...
Dec. 20, 2017, 9:23 a.m. (2017-12-20 09:23:23 UTC) #7
Wladimir Palant
Dec. 21, 2017, 10:57 a.m. (2017-12-21 10:57:00 UTC) #8
Since I won't finish this change, I've added Sergei as collaborator - this
should allow you to update this review if necessary.

Powered by Google App Engine
This is Rietveld