Issue 4127 - [emscripten] Convert subscription classes to C++ - Part 1

This implements the subscriptions but doesn`t connect them to filters yet. Adding filters to subscriptions and everything related will be implemented in the next part.

[Wladimir Palant, 2017-03-30 17:40:55]

Added Hubert to CC.

[hub, 2017-03-31 03:17:21]

https://codereview.adblockplus.org/29384812/diff/29395601/compiled/subscripti...
File compiled/subscription/DownloadableSubscription.cpp (right):

https://codereview.adblockplus.org/29384812/diff/29395601/compiled/subscripti...
compiled/subscription/DownloadableSubscription.cpp:43: int len = sprintf(buffer,
"%.0f", mLastCheck);
I think we should use snprintf() here. Just to be on the safe side. And below.

[Wladimir Palant, 2017-04-04 14:45:37]

https://codereview.adblockplus.org/29384812/diff/29395601/compiled/subscripti...
File compiled/subscription/DownloadableSubscription.cpp (right):

https://codereview.adblockplus.org/29384812/diff/29395601/compiled/subscripti...
compiled/subscription/DownloadableSubscription.cpp:43: int len = sprintf(buffer,
"%.0f", mLastCheck);
On 2017/03/31 03:17:21, hub wrote:
> I think we should use snprintf() here. Just to be on the safe side. And below.

Right, this won't hurt.

[Wladimir Palant, 2017-04-06 15:28:24]

Patch set 6 is rebased to apply on top of
https://codereview.adblockplus.org/29404594/ so that we can avoid using
snprintf() altogether. I also want to look into storing the properties as
int64_t rather than double.

[Wladimir Palant, 2017-04-07 07:14:58]

With Patch set 7 we have proper 64-bit properties now, the conversion to double
only happens when passing them to EM_ASM. The bindings for passing 64-bit values
are somewhat complicated however.

https://codereview.adblockplus.org/29384812/diff/29405572/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29384812/diff/29405572/compiled/String.h#n...
compiled/String.h:386: for (T i = num; i; i /= 10)
Yep, fixed a stupid mistake here.

[hub, 2017-04-10 15:30:09]

https://codereview.adblockplus.org/29384812/diff/29405572/compiled/bindings.ipp
File compiled/bindings.ipp (right):

https://codereview.adblockplus.org/29384812/diff/29405572/compiled/bindings.i...
compiled/bindings.ipp:193: signature[pos] = 0;
Nowhere we check that pos doesn't overflow. Not even here. I know 'signature'
looks kinda oversized. But still, things can go wrong.

[Wladimir Palant, 2017-04-10 18:27:49]

Uploaded Patch set 8

https://codereview.adblockplus.org/29384812/diff/29405572/compiled/bindings.ipp
File compiled/bindings.ipp (right):

https://codereview.adblockplus.org/29384812/diff/29405572/compiled/bindings.i...
compiled/bindings.ipp:193: signature[pos] = 0;
On 2017/04/10 15:30:09, hub wrote:
> Nowhere we check that pos doesn't overflow. Not even here. I know 'signature'
> looks kinda oversized. But still, things can go wrong.

Indeed, this is unnecessary. This is bindings generation, no point messing with
raw buffers.

[hub, 2017-04-11 05:40:18]

LGTM

[sergei, 2017-04-12 12:04:36]

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/String.h#n...
compiled/String.h:357: assert(source, u"Null buffer passed to
OwnedString.append()"_str);
I think it makes sense to put this assert before the check above.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/StringMap....
compiled/StringMap.h:236: size_type max_size() const
What about capacity instead of max_size, though max_size is totally fine for me.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/bindings.ipp
File compiled/bindings.ipp (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/bindings.i...
compiled/bindings.ipp:46: FLOAT,
I would rather call it DOUBLE because emscripten uses double (actually JS
number).

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/bindings.i...
compiled/bindings.ipp:180: signature += "ii";
It would be good to have a link (maybe better to some code, commit/file:line),
from which one can understand that we are passing correct values to
Runtime.dynCall.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/bindings.i...
compiled/bindings.ipp:429: // values because JavaScript operates on 32-bit
integers.
I would replace JavaScript by something more specific, like "bitwise and shift
operators".

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/bindings.i...
compiled/bindings.ipp:502: params.push_back(argName + " / 0x100000000 >>> 0");
It would be very useful to have few tests for that because it can work for some
values but break for bigger or smaller values which are not common and difficult
to catch.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/filter/Fil...
File compiled/filter/Filter.h (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/filter/Fil...
compiled/filter/Filter.h:42: MAXTYPE = 7
I think that the name reflecting the number of values would be better because
it's used as the number of different values.
Type::VALUES_NUMBER?

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
File compiled/subscription/Subscription.cpp (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/Subscription.cpp:70: unsigned seed =
knownSubscriptions.size();
it would be better to put "unsigned int" (int is added).

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/Subscription.cpp:92: if (!id.empty() && id[0] == '~')
id cannot be empty here, though maybe it's still good to check it.

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

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/Subscription.h:54: }, &action, this, mType, (double)value,
(double)oldvalue);\
It seems SUBSCRIPTION_PROPERTY is only for numbers and bool, I would like to
have a static assert using std::is_arithmetic in these methods.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/Subscription.h:73: DependentString
action(u"subscription."_str #name);\
I'm not sure that the `action` value is correct here and above. For instance,
originally the value was "subscription.title", but here it is
"subscription.mTitle".

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
File compiled/subscription/UserDefinedSubscription.h (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/UserDefinedSubscription.h:26: enum Defaults
It would be better to explain that it's about default filter types for the
instance of a subscription, maybe by renaming it to DefaultFilterTypes.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/UserDefinedSubscription.h:32: static int
filterTypeToDefaults[Filter::Type::MAXTYPE + 1];
Should it be Defaults instead of int?

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/UserDefinedSubscription.h:32: static int
filterTypeToDefaults[Filter::Type::MAXTYPE + 1];
Since it's private, what about moving it into cpp file in an anonymous namespace
and mark as const?

[hub, 2017-04-12 12:12:08]

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/String.h#n...
compiled/String.h:357: assert(source, u"Null buffer passed to
OwnedString.append()"_str);
On 2017/04/12 12:04:31, sergei wrote:
> I think it makes sense to put this assert before the check above.

Not sure what the intent is but to me it looks reasonable to firsrt check for
sourceLen = 0 like it is done to allow nullptr, 0 arguments.

[sergei, 2017-04-12 12:13:17]

Forgot to mention that I start to concern a bit more about global variables (of
course excluding all types of constants) and about such things like keeping
listeners in a global JS variable `FilterNotifier`. I don't think it makes sense
to change something in that regard until we have it fully working but I would
like to say about it now, to give more time to think about it.

[sergei, 2017-04-12 12:14:40]

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/String.h#n...
compiled/String.h:357: assert(source, u"Null buffer passed to
OwnedString.append()"_str);
On 2017/04/12 12:12:07, hub wrote:
> On 2017/04/12 12:04:31, sergei wrote:
> > I think it makes sense to put this assert before the check above.
> 
> Not sure what the intent is but to me it looks reasonable to firsrt check for
> sourceLen = 0 like it is done to allow nullptr, 0 arguments.

Ah, sorry, misread it as `if (!source)...`. It's correct.

[Wladimir Palant, 2017-04-13 13:04:44]

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/StringMap....
compiled/StringMap.h:236: size_type max_size() const
On 2017/04/12 12:04:31, sergei wrote:
> What about capacity instead of max_size, though max_size is totally fine for
me.

Ouch, I misread what std::map::max_size() does. Then let's just remove this
method, it's unused and its return value isn't too useful either.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/bindings.ipp
File compiled/bindings.ipp (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/bindings.i...
compiled/bindings.ipp:46: FLOAT,
On 2017/04/12 12:04:32, sergei wrote:
> I would rather call it DOUBLE because emscripten uses double (actually JS
> number).

I was under the impression that Emscripten doesn't really distinguish between
single and double precision floating point numbers. Turned out I was wrong - see
https://github.com/kripken/emscripten/blob/1.37.3/src/modules.js#L46. So we
should have both FLOAT and DOUBLE here.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/bindings.i...
compiled/bindings.ipp:180: signature += "ii";
On 2017/04/12 12:04:32, sergei wrote:
> It would be good to have a link (maybe better to some code, commit/file:line),
> from which one can understand that we are passing correct values to
> Runtime.dynCall.

Ok, I managed to find the correct code line, linked to it.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/bindings.i...
compiled/bindings.ipp:429: // values because JavaScript operates on 32-bit
integers.
On 2017/04/12 12:04:32, sergei wrote:
> I would replace JavaScript by something more specific, like "bitwise and shift
> operators".

This comment is gone - I found that Emscripten exposes a function to combine two
i64 parts, doing exactly the same thing that I have here. So we don't need to
reinvent the wheel. Sadly, the reverse transformation is only used internally
and not exposed as a function.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/bindings.i...
compiled/bindings.ipp:502: params.push_back(argName + " / 0x100000000 >>> 0");
On 2017/04/12 12:04:32, sergei wrote:
> It would be very useful to have few tests for that because it can work for
some
> values but break for bigger or smaller values which are not common and
difficult
> to catch.

We have a few tests in FilterNotifier further up my patch queue but I've added
better values to subscriptionClasses tests as well now.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/filter/Fil...
File compiled/filter/Filter.h (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/filter/Fil...
compiled/filter/Filter.h:42: MAXTYPE = 7
On 2017/04/12 12:04:33, sergei wrote:
> I think that the name reflecting the number of values would be better because
> it's used as the number of different values.
> Type::VALUES_NUMBER?

Done.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
File compiled/subscription/Subscription.cpp (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/Subscription.cpp:70: unsigned seed =
knownSubscriptions.size();
On 2017/04/12 12:04:33, sergei wrote:
> it would be better to put "unsigned int" (int is added).

Done.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/Subscription.cpp:92: if (!id.empty() && id[0] == '~')
On 2017/04/12 12:04:33, sergei wrote:
> id cannot be empty here, though maybe it's still good to check it.

No, it cannot - with the current logic. I just verified, the compiler knows that
as well and removes this check. It's still a good idea to have it here, this way
the code won't break should the logic change later in such a way that empty IDs
become possible.

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

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/Subscription.h:54: }, &action, this, mType, (double)value,
(double)oldvalue);\
On 2017/04/12 12:04:33, sergei wrote:
> It seems SUBSCRIPTION_PROPERTY is only for numbers and bool, I would like to
> have a static assert using std::is_arithmetic in these methods.

Done.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/Subscription.h:73: DependentString
action(u"subscription."_str #name);\
On 2017/04/12 12:04:33, sergei wrote:
> I'm not sure that the `action` value is correct here and above. For instance,
> originally the value was "subscription.title", but here it is
> "subscription.mTitle".

No, it's not correct - and neither for filter classes. This is addressed in
https://issues.adblockplus.org/ticket/5063 which is also under review.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
File compiled/subscription/UserDefinedSubscription.h (right):

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/UserDefinedSubscription.h:26: enum Defaults
On 2017/04/12 12:04:35, sergei wrote:
> It would be better to explain that it's about default filter types for the
> instance of a subscription, maybe by renaming it to DefaultFilterTypes.

Let's go with FilterCategory.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/UserDefinedSubscription.h:32: static int
filterTypeToDefaults[Filter::Type::MAXTYPE + 1];
On 2017/04/12 12:04:35, sergei wrote:
> Since it's private, what about moving it into cpp file in an anonymous
namespace
> and mark as const?

Done.

https://codereview.adblockplus.org/29384812/diff/29408763/compiled/subscripti...
compiled/subscription/UserDefinedSubscription.h:32: static int
filterTypeToDefaults[Filter::Type::MAXTYPE + 1];
On 2017/04/12 12:04:33, sergei wrote:
> Should it be Defaults instead of int?

Done.

[sergei, 2017-04-13 13:34:46]

LGTM

[hub, 2017-04-13 15:48:28]

LGTM