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

Issue 29548581: Issue 4128, 5138 - Add Parser and Serializer implemented in C++

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 months ago by sergei
Modified:
4 months, 1 week ago
CC:
Felix Dahlke, tlucas
Base URL:
https://github.com/adblockplus/adblockpluscore.git
Visibility:
Public.

Description

- don't expose in JS API `Subscription::Serialize` - remove `Subscription::SerializeFilters` - add JS _FilterStorage_Parser bound to C++ Parser and is aliased to require("filterStorage").Parser. - add JS _FilterStorage_Serializer bound to C++ `Serializer` and is aliased to require("filterStorage").Serializer - add C++ only `Subscription` factory method `Subscription::FromProperties(const KeyValues& properties)`, `Subscription::KeyValues` and constructor for `Subscription` classes accepting `KeyValues` allowing to construct a subscription from pairs of subscription properties according to the `url` property (either `DownloadableSubscritpion` or `UserDefinedSubscription`). - make the constructor of `Subscription` protected - add C++ only version of `Subscription::AddFilter(Filter&)` adding a filter to the end of subscription filters without notifications. - add C++ only `const Filters& Subscription::GetFilters() const`, for present it's used by `Serializer`. - rename `Subscription::Serialize` to `Subscription::SerializerProperties` because it serializes only certain subscription properties. It's kept as a member of `Subscription` for the sake of simplicity. - add `testEqualObjProperties` to test/_test-utils.js comparing properties of emscripten classes objects. - update tests The implementations of both `Parser` and `Serializer` are not complete yet in accordance with the issue. It is the first step having the core functionality, allowing to write tests and start to use them, at least partially. In particular, beside some other things, `Parser` is not connected with `FilterStroage` yet and it has to implicitly affect global `knownFilters` and `knownSubscriptions`, `Serializer` does not provide with generator-like interface, does not do anything keeping a passed subscription unmodified externally during serialization process and is not connected with `FilterEngine` either, and there is no conversion to UTF-8.

Patch Set 1 #

Total comments: 28

Patch Set 2 : rebase #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+670 lines, -106 lines) Patch
M compiled/bindings/main.cpp View 1 5 chunks +16 lines, -6 lines 0 comments Download
A compiled/storage/Parser.h View 1 chunk +65 lines, -0 lines 1 comment Download
A compiled/storage/Parser.cpp View 1 chunk +183 lines, -0 lines 1 comment Download
A compiled/storage/Serializer.h View 1 chunk +40 lines, -0 lines 0 comments Download
A compiled/storage/Serializer.cpp View 1 chunk +72 lines, -0 lines 0 comments Download
M compiled/subscription/DownloadableSubscription.h View 1 2 chunks +3 lines, -3 lines 0 comments Download
M compiled/subscription/DownloadableSubscription.cpp View 1 1 chunk +18 lines, -5 lines 0 comments Download
M compiled/subscription/Subscription.h View 1 4 chunks +33 lines, -8 lines 0 comments Download
M compiled/subscription/Subscription.cpp View 1 5 chunks +44 lines, -28 lines 0 comments Download
M compiled/subscription/UserDefinedSubscription.h View 1 2 chunks +5 lines, -3 lines 0 comments Download
M compiled/subscription/UserDefinedSubscription.cpp View 1 3 chunks +25 lines, -4 lines 0 comments Download
M lib/filterStorage.js View 2 chunks +4 lines, -1 line 0 comments Download
M meson.build View 1 1 chunk +2 lines, -0 lines 0 comments Download
M test/_test-utils.js View 1 1 chunk +35 lines, -0 lines 0 comments Download
M test/subscriptionClasses.js View 1 6 chunks +125 lines, -48 lines 0 comments Download

Messages

Total messages: 10
sergei
https://codereview.adblockplus.org/29548581/diff/29548701/compiled/subscription/DownloadableSubscription.cpp File compiled/subscription/DownloadableSubscription.cpp (left): https://codereview.adblockplus.org/29548581/diff/29548701/compiled/subscription/DownloadableSubscription.cpp#oldcode25 compiled/subscription/DownloadableSubscription.cpp:25: SetTitle(id); Why is the title set to ID only ...
10 months ago (2017-09-18 19:20:07 UTC) #1
sergei
https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Serializer.cpp File compiled/storage/Serializer.cpp (right): https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Serializer.cpp#newcode67 compiled/storage/Serializer.cpp:67: auto escapedResult = EscapeOpeningBracket(filter->GetText()); BTW, what about escaping of ...
10 months ago (2017-09-19 09:17:35 UTC) #2
hub
https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h#newcode212 compiled/String.h:212: } No overflow checking? int64_t is quite a lot ...
7 months, 4 weeks ago (2017-11-22 15:40:13 UTC) #3
hub
https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h#newcode212 compiled/String.h:212: } On 2017/11/22 15:40:12, hub wrote: > No overflow ...
7 months, 2 weeks ago (2017-11-30 21:35:37 UTC) #4
Wladimir Palant
Unfortunately, I didn't have time to finish outstanding reviews any more. I'm publishing the comments ...
6 months, 4 weeks ago (2017-12-21 10:30:35 UTC) #5
sergei
Although comments are here, please continue the discussion of string helpers in the codereview https://codereview.adblockplus.org/29712634/. ...
4 months, 2 weeks ago (2018-03-01 15:05:25 UTC) #6
sergei
So far I have merely rebased it (all tests pass). The old description was: - ...
4 months, 1 week ago (2018-03-07 12:06:43 UTC) #7
hub
Do you want to land this, without the missing bits ?
4 months, 1 week ago (2018-03-08 16:52:56 UTC) #8
sergei
On 2018/03/08 16:52:56, hub wrote: > Do you want to land this, without the missing ...
4 months, 1 week ago (2018-03-09 09:02:55 UTC) #9
hub
4 months, 1 week ago (2018-03-09 14:32:16 UTC) #10
https://codereview.adblockplus.org/29548581/diff/29716563/compiled/storage/Pa...
File compiled/storage/Parser.cpp (right):

https://codereview.adblockplus.org/29548581/diff/29716563/compiled/storage/Pa...
compiled/storage/Parser.cpp:43: return ::IsSection(value, u"subscription
filters"_str);
::IsSection() is inconsistent with above that doesn't use '::'

https://codereview.adblockplus.org/29548581/diff/29716563/compiled/storage/Pa...
File compiled/storage/Parser.h (right):

https://codereview.adblockplus.org/29548581/diff/29716563/compiled/storage/Pa...
compiled/storage/Parser.h:39: Subscription* BINDINGS_EXPORTED
SubscriptionAt(Subscriptions::size_type index);
I think this should be a const method.
Sign in to reply to this message.

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