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

- 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.

[sergei, 2017-09-18 19:20:07]

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/subscripti...
File compiled/subscription/DownloadableSubscription.cpp (left):

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/subscripti...
compiled/subscription/DownloadableSubscription.cpp:25: SetTitle(id);
Why is the title set to ID only for downloadable subscription and not for all
subscriptions in base Subscription class?

[sergei, 2017-09-19 09:17:35]

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Se...
File compiled/storage/Serializer.cpp (right):

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Se...
compiled/storage/Serializer.cpp:67: auto escapedResult =
EscapeOpeningBracket(filter->GetText());
BTW, what about escaping of '[' only when it's the first character and there is
']' at the end of the line?

[hub, 2017-11-22 15:40:13]

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#n...
compiled/String.h:212: }
No overflow checking?

int64_t is quite a lot of significant digits, but we should fail parsing if
going too far. On the 19th we should bail. (20th for uint64_t)

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h#n...
compiled/String.h:443: inline Type AdaptValue(const String& value);
I would have called this "lexical_cast<>" probably in reference to the
boost::lexical_cast module AND static_cast<>. This is definitely a cast.

CastValue() would work too. Not convinced about how obvious is "AdaptValue()"

[hub, 2017-11-30 21:35:37]

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#n...
compiled/String.h:212: }
On 2017/11/22 15:40:12, hub wrote:
> No overflow checking?
> 
> int64_t is quite a lot of significant digits, but we should fail parsing if
> going too far. On the 19th we should bail. (20th for uint64_t)
> 

std::numeric_limits<int64_t>::digit10 is the value you are looking for. And you
could combine toInt64() and toUInt64() with templates and use
std::numeric_limits<T>::is_signed() to reject when the value is unsigned.

I have my own version of this in https://codereview.adblockplus.org/29606600/ -
we should combine them. I don't mind actually making a separate patch that we
could review and land prior any of our reviews.

[Wladimir Palant, 2017-12-21 10:30:35]

Unfortunately, I didn't have time to finish outstanding reviews any more. I'm
publishing the comments I have here so far, too bad that I couldn't finish this.
Note that these comments are several months old and might be outdated to some
degree.

It seems that the implementation is missing serialization and parsing of filter
data at this point (this is mostly about the `ActiveFilter::disabled` property).

Note that conversion to and from UTF-8 is *not* an issue. This is performed
externally if necessary (on Chrome and Firefox it currently isn't), our
Emscripten code can generally assume UTF-16 data.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.cpp
File compiled/String.cpp (right):

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.cpp...
compiled/String.cpp:25: if (value[start] > u' ')
That's an unusual definition of "whitespace," but I guess that I can live with
being consistent to NormalizeWhitespace().

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.cpp...
compiled/String.cpp:28: for (; end > 0; end--)
You should check for `end > start` here. Otherwise you will end up with a
negative length for whitespace-only strings (meaning a huge length because the
values are unsigned).

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.cpp...
compiled/String.cpp:32: }
All string methods are inlined, why did you put this one into a separate file?

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#n...
compiled/String.h:200: return 0;
What about using StringScanner so that we won't need explicit length checks and
position tracking?

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h#n...
compiled/String.h:216: uint64_t toUInt64() const
Nit: This should be called toUint64().

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h#n...
compiled/String.h:226: }
General note: this should be ok as a first iteration. However, performance of
i64 arithmetics is rather suboptimal with JavaScript using 32 bit numbers. I
filed https://issues.adblockplus.org/ticket/5732 on improving that.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h#n...
compiled/String.h:443: inline Type AdaptValue(const String& value);
Can't we have that as an instance method instead? String.To<Type>() probably.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h#n...
compiled/String.h:472: DependentString{value, secondBeginPos, value.length() -
secondBeginPos}};
What is `value.length() - secondBeginPos` going to be if `secondBeginPos` is
`npos`? While I don't think that it will cause any trouble, please consider just
handling this case separately:

  if (separatorPos == String::npos)
    return {DependentString(value), DependentString()};

Also a nit: This should be more readable with a few additional newlines:

  return {
    DependentString{...},
    DependentString{...}
  };

This matches the spacing we are using for object literals in JavaScript.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h#n...
compiled/String.h:473: }
Why aren't these helpers methods of the String class? String.trim() and
String.split() would make more sense.

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

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Pa...
compiled/storage/Parser.cpp:43: return ::IsSection(value, u"subscription
filters"_str);
Does :: make sense here? You don't use it above.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Pa...
compiled/storage/Parser.cpp:173: OwnedString lineCopy{line};
I assume that you create a copy so that DecodeOpeningBracket doesn't mess with
the input buffer? We should rather require a non-const input buffer that we can
mess with, this will prevent the temporary allocation here. For me, this would
produce 200k temporary allocations, that isn't cheap and will increase memory
fragmentation massively (note that the temporary buffer is only released after
memory for the filter and its copy of the text has been allocated).

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Pa...
compiled/storage/Parser.cpp:175: DecodeOpeningBracket(lineForFilter);
Copying all data around unconditionally seems wasteful, it isn't the fastest
operation and in 99.9% of the cases it should be unnecessary. Maybe call this
conditionally: `if (lineForFilter.find(u'\\') != String::npos)`

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

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Pa...
compiled/storage/Parser.h:50: {
I assume that this is supposed to do something if DEBUG is defined?

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Se...
File compiled/storage/Serializer.cpp (right):

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Se...
compiled/storage/Serializer.cpp:53: mStream.append(u"# Adblock Plus
preferences\nversion=5\n"_str);
I hope that this is part of your "not complete yet" comment. This approach will
produce a horrible number of reallocations which should render it way too slow
for any realistic data amounts.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Se...
compiled/storage/Serializer.cpp:67: auto escapedResult =
EscapeOpeningBracket(filter->GetText());
On 2017/09/19 09:17:35, sergei wrote:
> BTW, what about escaping of '[' only when it's the first character and there
is
> ']' at the end of the line?

That should be fine. For now we still have to unescape it everywhere when
parsing however, for backwards compatibility.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/subscripti...
File compiled/subscription/DownloadableSubscription.cpp (left):

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/subscripti...
compiled/subscription/DownloadableSubscription.cpp:25: SetTitle(id);
On 2017/09/18 19:20:07, sergei wrote:
> Why is the title set to ID only for downloadable subscription and not for all
> subscriptions in base Subscription class?

The ID is only meaningful for downloadable subscriptions. Special subscriptions
(a.k.a. custom filter groups) use randomly generated IDs. Here it makes more
sense to leave the title empty, the UI will display generic titles then.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/subscripti...
File compiled/subscription/DownloadableSubscription.h (right):

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/subscripti...
compiled/subscription/DownloadableSubscription.h:28: explicit
DownloadableSubscription(const String& id, const KeyValues& properties);
I'd rather not have properties as an additional constructor parameter. It should
be ok to apply the properties in a separate method after construction.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/subscripti...
File compiled/subscription/Subscription.h (right):

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/subscripti...
compiled/subscription/Subscription.h:125: OwnedString DoSerializeProperties()
const;
Why is this method called DoSerializeProperties() here but SerializeProperties()
in DownloadableSubscription?

[sergei, 2018-03-01 15:05:25]

Although comments are here, please continue the discussion of string helpers in
the codereview https://codereview.adblockplus.org/29712634/.

I'm going to extract casting functions (include toUInt) into a separate
codereview too.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.cpp
File compiled/String.cpp (right):

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.cpp...
compiled/String.cpp:25: if (value[start] > u' ')
On 2017/12/21 10:30:36, Wladimir Palant wrote:
> That's an unusual definition of "whitespace," but I guess that I can live with
> being consistent to NormalizeWhitespace().

It's not about whitespaces, it's only for spaces, it can be extended later as we
need it.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.cpp...
compiled/String.cpp:28: for (; end > 0; end--)
On 2017/12/21 10:30:35, Wladimir Palant wrote:
> You should check for `end > start` here. Otherwise you will end up with a
> negative length for whitespace-only strings (meaning a huge length because the
> values are unsigned).

Done, it's covered in tests.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.cpp...
compiled/String.cpp:32: }
On 2017/12/21 10:30:35, Wladimir Palant wrote:
> All string methods are inlined, why did you put this one into a separate file?

I moved SplitString into cpp too because when it's inlined it generates a lot of
code. Since it's easier to use it in the development when it's in cpp file,
let's hope that the linker is smart enough to optimize it.

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#n...
compiled/String.h:472: DependentString{value, secondBeginPos, value.length() -
secondBeginPos}};
On 2017/12/21 10:30:36, Wladimir Palant wrote:
> What is `value.length() - secondBeginPos` going to be if `secondBeginPos` is
> `npos`? While I don't think that it will cause any trouble, please consider
just
> handling this case separately:
> 
>   if (separatorPos == String::npos)
>     return {DependentString(value), DependentString()};
> 
It's handled by the constructor of DependentString.
> Also a nit: This should be more readable with a few additional newlines:
> 
>   return {
>     DependentString{...},
>     DependentString{...}
>   };
> 
> This matches the spacing we are using for object literals in JavaScript.
Done.

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h#n...
compiled/String.h:473: }
On 2017/12/21 10:30:37, Wladimir Palant wrote:
> Why aren't these helpers methods of the String class? String.trim() and
> String.split() would make more sense.

These functions are rather helper functions than methods of String class, so
they are just functions.

[sergei, 2018-03-07 12:06:43]

So far I have merely rebased it (all tests pass).


The old description was:

- move the check whether a template parameter of `intrusive_ptr` inherits
`ref_counted` in `static_assert` in the class definition in order to use it with
yet incomplete class, e.g. `class Subscription; typedef
intrusive_ptr<Subscription> SubscriptionPtr;`
- add `addRef = true` argument to `intrusive_ptr::reset`, so it has the same
functionality as the constructor.

- add `String::toInt64` and `String::toUInt64` parsing the string into an
integer
- add a very basic `template<typename Type> Type AdaptValue(const String&
value)` function allowing to easily convert a string value into a desired
`Type`.
- add `DependentString TrimSpaces(const String& value)` removing ASCII space
characters on both sides of the argument
- add `std::pair<DependentString, DependentString> SplitString(const String&
value, String::size_type separatorPos)` splitting the string into two parts
without a separator character

- 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 the const version of `Subscription::As()`

- add test/_test-utils.js with `withNAD` reducing the amount of code working
with objects from emscripten and with `testEqualObjProperties` 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.

[hub, 2018-03-08 16:52:56]

Do you want to land this, without the missing bits ?

[sergei, 2018-03-09 09:02:55]

On 2018/03/08 16:52:56, hub wrote:
> Do you want to land this, without the missing bits ?

yeah, I think I would like to land it without some missing parts. If there are
some questions/comments/etc regarding the existing code, could you please post
them, I will address them here before landing.

Additionally FYI, I'm working on https://issues.adblockplus.org/ticket/6452, and
so far that code is getting more, IMO unnecessary, complicated :) (more
abstractions) because all filter and subscription classes will go into
emscripten implementation and there will be the corresponding impl in
libadblockplus which is using currently available in adblockpluscore[master] JS
filter and subscriptions classes.

[hub, 2018-03-09 14:32:16]

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.