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

Issue 29333474: Issue 4125 - [emscripten] Convert filter classes to C++ (Closed)

Created:
Jan. 14, 2016, 4:40 p.m. by Wladimir Palant
Modified:
Dec. 21, 2017, 1:03 p.m.
CC:
kzar
Visibility:
Public.

Description

The implementation is still hacky in many places, these can probably be improved. You can clone the code from https://bitbucket.org/palant/adblockpluscore/commits/branch/emscripten if you want to play with it.

Patch Set 1 #

Patch Set 2 : Fixed some bugs and exposed/tested more properties #

Total comments: 14

Patch Set 3 : Using embind #

Patch Set 4 : Back to manual approach for API #

Patch Set 5 : Now passing all filter matching tests (without filter options) #

Patch Set 6 : Almost complete implementation, missing CSS property filters #

Patch Set 7 : How with CSS property filters #

Patch Set 8 : Minor improvements #

Patch Set 9 : Improved performance #

Patch Set 10 : Fixed annotation for hash buffers #

Patch Set 11 : Replaced shared_ptr by boost-like intrusive_ptr #

Patch Set 12 : Call parameters in JS wrappers generated statically #

Patch Set 13 : Reworked JS binding generation #

Total comments: 8

Patch Set 14 : Addressed comments, made String class slightly more sane, slightly cleaned up bindings.cpp #

Patch Set 15 : Merged filter parsing and normalization #

Patch Set 16 : Split up String class into two, cleaned up RegExpFilter methods #

Patch Set 17 : Minor improvements #

Patch Set 18 : Optimized hash lookup performance a bit #

Total comments: 99

Patch Set 19 : Rebased, addressed comments, changed StringMap::find() return value #

Total comments: 27

Patch Set 20 : Replaced old filter classes unit tests completely #

Patch Set 21 : Addressed Sergei`s comments again and added some asserts #

Total comments: 7

Patch Set 22 : Addressed Sergei`s comments again #

Patch Set 23 : Renamed bindings.h into bindings.ipp #

Total comments: 2

Patch Set 24 : Got rid of extra output in bindings.js file #

Total comments: 92

Patch Set 25 : Updated unit test framework to the current state of the repository #

Patch Set 26 : Addressed comments from Patch Set 24 #

Total comments: 39

Patch Set 27 : Addressed comments from Patch Set 26 #

Patch Set 28 : Properly determine ref_counted offset instead of assuming that it is zero #

Total comments: 20

Patch Set 29 : Addressed comments from Patch Set 28 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4749 lines, -300 lines) Patch
M .gitignore View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +4 lines, -0 lines 0 comments Download
M .hgignore View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +6 lines, -0 lines 0 comments Download
M README.md View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +62 lines, -9 lines 0 comments Download
A compile View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +79 lines, -0 lines 0 comments Download
A compiled/ActiveFilter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +53 lines, -0 lines 0 comments Download
A compiled/ActiveFilter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +190 lines, -0 lines 0 comments Download
A compiled/BlockingFilter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +9 lines, -0 lines 0 comments Download
A compiled/BlockingFilter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +7 lines, -0 lines 0 comments Download
A compiled/CSSPropertyFilter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +51 lines, -0 lines 0 comments Download
A compiled/CSSPropertyFilter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +13 lines, -0 lines 0 comments Download
A compiled/CommentFilter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +11 lines, -0 lines 0 comments Download
A compiled/CommentFilter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +23 lines, -0 lines 0 comments Download
A compiled/ElemHideBase.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +54 lines, -0 lines 0 comments Download
A compiled/ElemHideBase.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +155 lines, -0 lines 0 comments Download
A compiled/ElemHideException.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +12 lines, -0 lines 0 comments Download
A compiled/ElemHideException.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +7 lines, -0 lines 0 comments Download
A compiled/ElemHideFilter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +12 lines, -0 lines 0 comments Download
A compiled/ElemHideFilter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +6 lines, -0 lines 0 comments Download
A compiled/Filter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +46 lines, -0 lines 0 comments Download
A compiled/Filter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +144 lines, -0 lines 0 comments Download
A compiled/InvalidFilter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +15 lines, -0 lines 0 comments Download
A compiled/InvalidFilter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +7 lines, -0 lines 0 comments Download
A compiled/RegExpFilter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +96 lines, -0 lines 0 comments Download
A compiled/RegExpFilter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +372 lines, -0 lines 0 comments Download
A compiled/String.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +343 lines, -0 lines 0 comments Download
A compiled/StringMap.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +335 lines, -0 lines 0 comments Download
A compiled/StringScanner.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +51 lines, -0 lines 0 comments Download
A compiled/WhitelistFilter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +9 lines, -0 lines 0 comments Download
A compiled/WhitelistFilter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +7 lines, -0 lines 0 comments Download
A compiled/bindings.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +71 lines, -0 lines 0 comments Download
A compiled/bindings.ipp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +727 lines, -0 lines 0 comments Download
A compiled/debug.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +66 lines, -0 lines 0 comments Download
A compiled/intrusive_ptr.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +200 lines, -0 lines 0 comments Download
A compiled/shell.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +63 lines, -0 lines 0 comments Download
A compiled/traceInit.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +18 lines, -0 lines 0 comments Download
M lib/filterClasses.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +2 lines, -0 lines 0 comments Download
A lib/filterClassesNew.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +9 lines, -0 lines 0 comments Download
M lib/filterNotifier.js View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M lib/subscriptionClasses.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +2 lines, -0 lines 0 comments Download
M package.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +2 lines, -2 lines 0 comments Download
A test/_common.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +434 lines, -0 lines 0 comments Download
A test/domainRestrictions.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +209 lines, -0 lines 0 comments Download
M test/filterClasses.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +331 lines, -283 lines 0 comments Download
A test/regexpFilters_matching.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +359 lines, -0 lines 0 comments Download
A test/stub-modules/info.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +8 lines, -0 lines 0 comments Download
M test/subscriptionClasses.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +19 lines, -4 lines 0 comments Download
M test_runner.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +48 lines, -2 lines 0 comments Download

Messages

Total messages: 38
Wladimir Palant
Jan. 14, 2016, 4:41 p.m. (2016-01-14 16:41:15 UTC) #1
Felix Dahlke
So you didn't have to convert subscriptionClasses after all :) It's rough around the edges, ...
Jan. 15, 2016, 10:29 a.m. (2016-01-15 10:29:18 UTC) #2
Wladimir Palant
On 2016/01/15 10:29:18, Felix Dahlke wrote: > So you didn't have to convert subscriptionClasses after ...
Jan. 15, 2016, 12:52 p.m. (2016-01-15 12:52:09 UTC) #3
Felix Dahlke
On 2016/01/15 12:52:09, Wladimir Palant wrote: > On 2016/01/15 10:29:18, Felix Dahlke wrote: > > ...
Jan. 15, 2016, 5 p.m. (2016-01-15 17:00:36 UTC) #4
Wladimir Palant
I managed to get embind work. The good news: there is almost no boilerplate on ...
Jan. 15, 2016, 8:36 p.m. (2016-01-15 20:36:16 UTC) #5
Wladimir Palant
I realized that we need to specify NO_DYNAMIC_EXECUTION=1 flag, otherwise we won't be able to ...
Jan. 16, 2016, 2:07 p.m. (2016-01-16 14:07:37 UTC) #6
Wladimir Palant
As to WebIDL: only C-style strings are supported. This really means char*, no Unicode support ...
Jan. 16, 2016, 7:28 p.m. (2016-01-16 19:28:36 UTC) #7
Wladimir Palant
Patch Set 4 is back to forwarding API parameters manually. On the bright side, our ...
Jan. 18, 2016, 12:50 p.m. (2016-01-18 12:50:08 UTC) #8
Wladimir Palant
Patch Set 6 is almost complete now, merely CSS property filters are missing. Simplified element ...
Jan. 20, 2016, 12:11 p.m. (2016-01-20 12:11:24 UTC) #9
Wladimir Palant
Patch Set 9 has much better performance. It's using our own String class instead of ...
Jan. 28, 2016, 2:43 p.m. (2016-01-28 14:43:16 UTC) #10
Wladimir Palant
I forgot something else worth noting: constructors of various filter classes might modify the filter ...
Jan. 28, 2016, 2:53 p.m. (2016-01-28 14:53:05 UTC) #11
Wladimir Palant
Patch Set 11 got rid of std::shared_ptr which I identified as a memory waster (requires ...
Jan. 28, 2016, 7:57 p.m. (2016-01-28 19:57:11 UTC) #12
Wladimir Palant
Guess what was taking up the most time? No, not the string processing - bringing ...
Jan. 28, 2016, 9:32 p.m. (2016-01-28 21:32:06 UTC) #13
Wladimir Palant
In Patch Set 13 I moved the generation of JS bindings from Python into C++ ...
Feb. 1, 2016, 9:21 p.m. (2016-02-01 21:21:01 UTC) #14
René Jeschke
Just some nits after having a quick look. https://codereview.adblockplus.org/29333474/diff/29335189/compiled/ElemHideBase.cpp File compiled/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29333474/diff/29335189/compiled/ElemHideBase.cpp#newcode28 compiled/ElemHideBase.cpp:28: loop: ...
Feb. 2, 2016, 11:11 a.m. (2016-02-02 11:11:26 UTC) #15
Wladimir Palant
If that's all the issues you found after a quick look - I'm impressed, I ...
Feb. 2, 2016, 5:55 p.m. (2016-02-02 17:55:22 UTC) #16
Wladimir Palant
Patch Set 15 separated filter parsing and instantiation. Also, normalization which had to be called ...
Feb. 4, 2016, 3:08 p.m. (2016-02-04 15:08:11 UTC) #17
Wladimir Palant
Looking at the generated code I realized that using the same class both for strings ...
Feb. 4, 2016, 7:24 p.m. (2016-02-04 19:24:41 UTC) #18
sergei
I have not check the everything, however I would say a tremendous work is done ...
Feb. 17, 2016, 12:55 p.m. (2016-02-17 12:55:15 UTC) #19
Wladimir Palant
With Patch Set 19 I rebased the changes so they should apply on top of ...
Feb. 18, 2016, 4:03 p.m. (2016-02-18 16:03:05 UTC) #20
Wladimir Palant
With Patch Set 19 I rebased the changes so they should apply on top of ...
Feb. 18, 2016, 4:07 p.m. (2016-02-18 16:07:17 UTC) #21
sergei
https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilter.cpp File compiled/ActiveFilter.cpp (right): https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilter.cpp#newcode12 compiled/ActiveFilter.cpp:12: return std::move(OwnedString(buffer, len)); On 2016/02/18 16:06:29, Wladimir Palant wrote: ...
Feb. 22, 2016, 12:46 p.m. (2016-02-22 12:46:19 UTC) #22
Wladimir Palant
Uploaded Patch Set 21 : Addressed Sergei's comments again and added some asserts https://codereview.adblockplus.org/29333474/diff/29336116/compiled/ActiveFilter.cpp File ...
Feb. 23, 2016, 12:37 p.m. (2016-02-23 12:37:51 UTC) #23
sergei
Only a couple of comments. The next part I plan to deeply review is bindings. ...
Feb. 23, 2016, 3:07 p.m. (2016-02-23 15:07:40 UTC) #24
sergei
https://codereview.adblockplus.org/29333474/diff/29337508/compiled/debug.h File compiled/debug.h (right): https://codereview.adblockplus.org/29333474/diff/29337508/compiled/debug.h#newcode43 compiled/debug.h:43: } BTW, I would #define `assert` to nothing when ...
Feb. 23, 2016, 3:45 p.m. (2016-02-23 15:45:59 UTC) #25
Wladimir Palant
I uploaded renaming bindings.h into bindings.ipp as a separate patchset so you can see changes ...
Feb. 23, 2016, 9:35 p.m. (2016-02-23 21:35:06 UTC) #26
sergei
Next portion of comments. Please pay attention to the convention regarding working with ref_counted. - ...
June 16, 2016, 9:17 p.m. (2016-06-16 21:17:35 UTC) #27
sergei
bring it up
Nov. 2, 2016, 9:31 a.m. (2016-11-02 09:31:01 UTC) #28
Wladimir Palant
There we go, Patch Set 26. https://codereview.adblockplus.org/29333474/diff/29337615/compiled/StringScanner.h File compiled/StringScanner.h (right): https://codereview.adblockplus.org/29333474/diff/29337615/compiled/StringScanner.h#newcode47 compiled/StringScanner.h:47: String::size_type oldPos = ...
Dec. 6, 2016, 10:48 a.m. (2016-12-06 10:48:39 UTC) #29
sergei
next portion of comments. https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilter.cpp File compiled/ActiveFilter.cpp (right): https://codereview.adblockplus.org/29333474/diff/29345688/compiled/ActiveFilter.cpp#newcode75 compiled/ActiveFilter.cpp:75: done = scanner.done(); On 2016/12/06 ...
Jan. 10, 2017, 3:58 p.m. (2017-01-10 15:58:19 UTC) #30
Wladimir Palant
Addressed comments from Patch Set 26
March 13, 2017, 5:36 p.m. (2017-03-13 17:36:57 UTC) #31
Wladimir Palant
https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_ptr.h File compiled/intrusive_ptr.h (right): https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_ptr.h#newcode34 compiled/intrusive_ptr.h:34: // We need this virtual destructor, otherwise pointers to ...
March 13, 2017, 5:42 p.m. (2017-03-13 17:42:40 UTC) #32
Wladimir Palant
https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_ptr.h File compiled/intrusive_ptr.h (right): https://codereview.adblockplus.org/29333474/diff/29345688/compiled/intrusive_ptr.h#newcode34 compiled/intrusive_ptr.h:34: // We need this virtual destructor, otherwise pointers to ...
March 14, 2017, 10:23 a.m. (2017-03-14 10:23:17 UTC) #33
hub
Nothing holding back landing really, but a couple of comments about eventual Windows compatibility glitches. ...
March 20, 2017, 4:21 p.m. (2017-03-20 16:21:57 UTC) #34
sergei
https://codereview.adblockplus.org/29333474/diff/29366890/compiled/debug.h File compiled/debug.h (right): https://codereview.adblockplus.org/29333474/diff/29366890/compiled/debug.h#newcode35 compiled/debug.h:35: static console_type console; On 2017/03/13 17:42:32, Wladimir Palant wrote: ...
March 20, 2017, 5:07 p.m. (2017-03-20 17:07:27 UTC) #35
sergei
I think it may be pushed, I also agree that whatever can be fixed in ...
March 20, 2017, 5:09 p.m. (2017-03-20 17:09:08 UTC) #36
Wladimir Palant
https://codereview.adblockplus.org/29333474/diff/29383674/compile File compile (right): https://codereview.adblockplus.org/29333474/diff/29383674/compile#newcode19 compile:19: COMPILER_OUTPUT = './lib/compiled.js' On 2017/03/20 16:21:49, hub wrote: > ...
March 21, 2017, 10:10 a.m. (2017-03-21 10:10:25 UTC) #37
sergei
March 21, 2017, 11:36 a.m. (2017-03-21 11:36:39 UTC) #38
LGTM

Powered by Google App Engine
This is Rietveld