Issue 5870 - Implement the new ElemHideEmulation filter type

Issue 5870 - Implement the new ElemHideEmulation filter type

Applies on top:
https://codereview.adblockplus.org/29594607/

[hub, 2017-11-02 23:47:50]

Also on top of my other patches.

https://codereview.adblockplus.org/29595633/diff/29595634/compiled/filter/Ele...
File compiled/filter/ElemHideEmulationFilter.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29595634/compiled/filter/Ele...
compiled/filter/ElemHideEmulationFilter.cpp:24: OwnedString ConvertFilter(const
String& text, String::size_type at)
this is a bit rough conversion, but no RegExp. It probably needs to handle
corner cases better.

https://codereview.adblockplus.org/29595633/diff/29595634/compiled/filter/Ele...
compiled/filter/ElemHideEmulationFilter.cpp:117: : ElemHideBase(classType,
ConvertFilter(text, data.mSelectorStart), data)
I think the conversion should be done in the elemhide base for exception
filters.

[hub, 2017-11-02 23:48:52]

also we need to merge the changes to the content script here. Not exactly sure
how we proceed.

[sergei, 2018-01-30 18:09:08]

could you please rebase it?

[hub, 2018-01-30 18:16:50]

On 2018/01/30 18:09:08, sergei wrote:
> could you please rebase it?

done

[sergei, 2018-01-30 19:02:40]

I will post the comments regarding the implementation of ConvertFilter in the
next round, if they are not resolved on my side.

https://codereview.adblockplus.org/29595633/diff/29684732/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29684732/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:54: OwnedString ConvertFilter(const String&
text, String::size_type at)
I'm not sure that we need to build OwnedString for each filter, perhaps the
contructor of base classes should just support OwnedString&&. What do you think
about it? If we decide to add support of OwnedString&& in the Filter constructor
then IMO it should be in a separate commit and with the issue.

https://codereview.adblockplus.org/29595633/diff/29684732/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:189: emulation = scanner.skipOne(u'?');
It's actually different from "Filter.elemhideRegExp =
/^([^/*|@"!]*?)#([@?])?#(.+)$/;"
https://github.com/adblockplus/adblockpluscore/blob/ec65d0a80470fc2eacd9ef83e...
where it allows only one character @ or ? but here we allow "#@?#". I think it
should be fixed.

https://codereview.adblockplus.org/29595633/diff/29684732/test/elemHideEmulat...
File test/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29595633/diff/29684732/test/elemHideEmulat...
test/elemHideEmulation.js:145: let filter = Filter.fromText(old);
`filter` should be released.

[hub, 2018-01-30 22:53:13]

https://codereview.adblockplus.org/29595633/diff/29684732/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29684732/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:54: OwnedString ConvertFilter(const String&
text, String::size_type at)
On 2018/01/30 19:02:40, sergei wrote:
> I'm not sure that we need to build OwnedString for each filter, perhaps the
> contructor of base classes should just support OwnedString&&. What do you
think
> about it? If we decide to add support of OwnedString&& in the Filter
constructor
> then IMO it should be in a separate commit and with the issue.

Since we copy the string at the end in the Filter constructor we could have
chain of constructors that take an OwnedString&& so that ConvertFilter does the
copy or the conversion.

https://codereview.adblockplus.org/29595633/diff/29684732/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:189: emulation = scanner.skipOne(u'?');
On 2018/01/30 19:02:40, sergei wrote:
> It's actually different from "Filter.elemhideRegExp =
> mailto:/^([^/*|@"!]*?)#([@?])?#(.+)$/;"
>
https://github.com/adblockplus/adblockpluscore/blob/ec65d0a80470fc2eacd9ef83e...
> where it allows only one character @ or ? but here we allow "#@?#". I think it
> should be fixed.

#@?# is "rejected". actually according to issue 6234 it is made to a blocking
filter. But this is correct. Same as the regexp.

Adding a test.

https://codereview.adblockplus.org/29595633/diff/29684732/test/elemHideEmulat...
File test/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29595633/diff/29684732/test/elemHideEmulat...
test/elemHideEmulation.js:145: let filter = Filter.fromText(old);
On 2018/01/30 19:02:40, sergei wrote:
> `filter` should be released.

Done.

[hub, 2018-01-30 23:12:02]

https://codereview.adblockplus.org/29595633/diff/29684741/test/compiled/Filte...
File test/compiled/Filter.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29684741/test/compiled/Filte...
test/compiled/Filter.cpp:1: 
I have an update with the license header I forgot.

https://codereview.adblockplus.org/29595633/diff/29684741/test/compiled/Filte...
test/compiled/Filter.cpp:14: }
and more comprehensive tests for this part. This was to test the constructor
chain.

[hub, 2018-02-01 20:38:20]

This should be better.

[sergei, 2018-02-05 14:51:07]

https://codereview.adblockplus.org/29595633/diff/29685641/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29685641/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:125: emulation = !exception;
Should it be an invalid filter if it's exception with emulation?

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:24: #define LENGTH_OF(x) ((sizeof(x) /
sizeof(x[0])) - 1)
I think we may use constexpr function here, and maybe move it into Utils.h

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:65: static constexpr String::value_type
NEW_PROPS_SELECTOR[] = u":-abp-properties(";
I would rather prefer to call it PROPS_SELECTOR and OLD_PROPS_SELECTOR, so later
we can drop OLD_PROPS_SELECTOR.

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:158: auto properties = selectorPos +
PROPS_SELECTOR_LEN;
should it be something like propertiesPos?

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:177: case '\\':
should they be u'\\', u'"', etc? Since we keep in mind the possibility to switch
to UTF-8 I also think about some macro like in Windows `_T(x)` (or _TEXT) which
expands to either `Lx` or `x`, perhaps we could have something similar too.
ABP_CHAR and ABP_TEXT?

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:202: }
What if filter is malformed and `end` is equal to zero after the loop? It seems
memcpy below will be pretty dangerous.

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:202: }
It also differs from the regexp in current js implementation
(/\[-abp-properties=(["'])([^"']+)\1\]/) in particular that regexp does not
allow "[-abp-properties='quotedpart1'part2'quotedpart3']", I'm not sure how
flexible we should be here but IMO we should not allow more than is allowed
currently in JS.

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:218: OwnedString converted(length + ((delimiter
!= text.npos) ? 1 : 0) - removed);
Since removed cannot be negative (no growth), `text` is originally modifiable
and an old syntax expects quotes, which are removed now, there is no growth, so
what about returning of a DependentString? So, there will no be additional
overhead with constructing of an additional OwnedString.

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
File compiled/filter/ElemHideBase.h (right):

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.h:29: bool mNeedConversion;
Do we really need this member for each instance of ElemHideBase? What do you
think about detecting whether a conversion is required in the constructor of
ElemHideBase?

[hub, 2018-02-07 04:13:37]

Patch updated.

https://codereview.adblockplus.org/29595633/diff/29685641/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29685641/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:125: emulation = !exception;
On 2018/02/05 14:51:04, sergei wrote:
> Should it be an invalid filter if it's exception with emulation?

no. This is actually how exceptions for element hiding emulation are specified.

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:24: #define LENGTH_OF(x) ((sizeof(x) /
sizeof(x[0])) - 1)
On 2018/02/05 14:51:06, sergei wrote:
> I think we may use constexpr function here,

No. This has to be a macro because of sizeof() and the use with the argument
that is an array.

> and maybe move it into Utils.h

Done

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:65: static constexpr String::value_type
NEW_PROPS_SELECTOR[] = u":-abp-properties(";
On 2018/02/05 14:51:05, sergei wrote:
> I would rather prefer to call it PROPS_SELECTOR and OLD_PROPS_SELECTOR, so
later
> we can drop OLD_PROPS_SELECTOR.

Done.

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:158: auto properties = selectorPos +
PROPS_SELECTOR_LEN;
On 2018/02/05 14:51:06, sergei wrote:
> should it be something like propertiesPos?

Done.

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:177: case '\\':
On 2018/02/05 14:51:05, sergei wrote:
> should they be u'\\', u'"', etc? Since we keep in mind the possibility to
switch
> to UTF-8 I also think about some macro like in Windows `_T(x)` (or _TEXT)
which
> expands to either `Lx` or `x`, perhaps we could have something similar too.
> ABP_CHAR and ABP_TEXT?

yes it should be u'', but with C it works without since we are in the ASCII
realm. Will change it.

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:202: }
On 2018/02/05 14:51:06, sergei wrote:
> What if filter is malformed and `end` is equal to zero after the loop? It
seems
> memcpy below will be pretty dangerous.

end cannot be equal to zero. It will be at least equal to "properties".

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:202: }
On 2018/02/05 14:51:05, sergei wrote:
> It also differs from the regexp in current js implementation
> (/\[-abp-properties=(["'])([^"']+)\1\]/) in particular that regexp does not
> allow "[-abp-properties='quotedpart1'part2'quotedpart3']", I'm not sure how
> flexible we should be here but IMO we should not allow more than is allowed
> currently in JS.

It's actually worse: the parser doesn't like it at all. The handling of quotes
and escapes is just wrong. I will increase the test coverage and fix it.

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:218: OwnedString converted(length + ((delimiter
!= text.npos) ? 1 : 0) - removed);
On 2018/02/05 14:51:06, sergei wrote:
> Since removed cannot be negative (no growth), `text` is originally modifiable
> and an old syntax expects quotes, which are removed now, there is no growth,
so
> what about returning of a DependentString? So, there will no be additional
> overhead with constructing of an additional OwnedString.

I'm not really fond of modifying in place, but we already do that in normalize
and this helps even though the conversion will be still a corner case in the
filter loading.

Will do that, even though this quite complicate things.

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
File compiled/filter/ElemHideBase.h (right):

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.h:29: bool mNeedConversion;
On 2018/02/05 14:51:06, sergei wrote:
> Do we really need this member for each instance of ElemHideBase? What do you
> think about detecting whether a conversion is required in the constructor of
> ElemHideBase?

I can move mNeedConversion out of this and pass it to ElemHideBase::Parse().

The conversion in the constructor is a problem as it involved mutating
data.elemhide as well.

[hub, 2018-02-07 15:02:52]

There is still a test failure I missed. Will update the patch with a fix.

[hub, 2018-02-07 15:50:34]

fixed

[sergei, 2018-02-12 12:53:21]

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:24: #define LENGTH_OF(x) ((sizeof(x) /
sizeof(x[0])) - 1)
On 2018/02/07 04:13:37, hub wrote:
> On 2018/02/05 14:51:06, sergei wrote:
> > I think we may use constexpr function here,
> 
> No. This has to be a macro because of sizeof() and the use with the argument
> that is an array.
The following does work
template<typename T, size_t N>
constexpr size_t length_of(T (&)[N])
{
  return N - 1;
}

> 
> > and maybe move it into Utils.h
> 
> Done

BTW, I just released that it's a length of string not an array (N - 1 vs N),
what do you think about renaming it to something like get_str_length. In
addition I would replace "static string array" by something like "null
terminated character sequence (string)".

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/ElemHide.cpp
File compiled/ElemHide.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/ElemHide.c...
compiled/ElemHide.cpp:200: intrusive_ptr<ElemHide_SelectorList> selectors(new
ElemHide_SelectorList(), false);
Good catch, it seems unrelated change, could you please extract it to a separate
commit?

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:173: prefix.end = text.find(OLD_PROPS_SELECTOR,
at, OLD_PROPS_SELECTOR_LEN);
not important but perhaps it would be better to say
Range prefix = {at, text.find(OLD_PROPS_SELECTOR, at, OLD_PROPS_SELECTOR_LEN)};

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:174: if (prefix.end != text.npos)
What about returning DependentString(text); here in order to reduce the
indentation below?Subjectively it also improves readability, BTW `suffix` and
`length` will be declared after that `if`.

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:209: closing = true;
What about using of suffix.start instead of `closing`? We could initialize it to
zero. In addition to that if suffix.start is still zero after finishing the loop
then the filter is malformed because there is no closing ']'. Continuing without
taking it into account can result in bad sizes below.

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:221: at++;
BTW, if there is no such delimiter then is it a malformed filter string?

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:223: + PROPS_SELECTOR_LEN + 1 + at;
What about
at + prefix.len() + PROPS_SELECTOR_LEN + properties.len() + 1 /* ')' */ +
suffix.len()
?

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:224: 
Could you please add the assert here that
new_len + 1 == length || (delimiter == text.npos && new_len + 2 == length)
just in case? Despite there are tests ensuring that nothing is lost, it just
looks scaring :)).

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:246: std::memmove(converted.data() + new_len,
logically it should be memcpy but since memmove does not modify the memory
accessed by `src` argument it's fine.

The same for ELEM_HIDE_EMULATION_DELIMITER below.

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
File compiled/filter/ElemHideBase.h (right):

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.h:66: static Type Parse(DependentString& text,
ElemHideData& data, bool& needConversion);
Perhaps it makes sense just in case to add a comment that needConversion should
be initialized to false before the calling of this method.

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Fil...
File compiled/filter/Filter.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Fil...
compiled/filter/Filter.cpp:118: if (needConversion && (type ==
ElemHideException::classType || type == ElemHideEmulationFilter::classType))
Firstly I thought about moving of `(type == ElemHideException::classType || type
== ElemHideEmulationFilter::classType)` inside `ElemHideBase::Parse`, it seems a
detail of ElemHide classes, here we just check the value of needConversion and
do the conversion if there is a necessity in it. However it seems it's redundant
at all. If needConversion is set to true then the type can be either
ELEMHIDEEXCEPTION if `exception` variable in ElemHideBase::Parse is true or
ELEMHIDEEMULATION if that `exception` variable is false, therefore if
needConversion is true then `(type == ElemHideException::classType || type ==
ElemHideEmulationFilter::classType)` is always true.
I think it can be just removed.

[hub, 2018-02-12 18:14:41]

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:24: #define LENGTH_OF(x) ((sizeof(x) /
sizeof(x[0])) - 1)
On 2018/02/12 12:53:17, sergei wrote:
> On 2018/02/07 04:13:37, hub wrote:
> > On 2018/02/05 14:51:06, sergei wrote:
> > > I think we may use constexpr function here,
> > 
> > No. This has to be a macro because of sizeof() and the use with the argument
> > that is an array.
> The following does work
> template<typename T, size_t N>
> constexpr size_t length_of(T (&)[N])
> {
>   return N - 1;
> }
> 
> > 
> > > and maybe move it into Utils.h
> > 
> > Done
> 
> BTW, I just released that it's a length of string not an array (N - 1 vs N),
> what do you think about renaming it to something like get_str_length. In
> addition I would replace "static string array" by something like "null
> terminated character sequence (string)".

Done.

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/ElemHide.cpp
File compiled/ElemHide.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/ElemHide.c...
compiled/ElemHide.cpp:200: intrusive_ptr<ElemHide_SelectorList> selectors(new
ElemHide_SelectorList(), false);
On 2018/02/12 12:53:18, sergei wrote:
> Good catch, it seems unrelated change, could you please extract it to a
separate
> commit?

Done.

https://codereview.adblockplus.org/29695575/

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:173: prefix.end = text.find(OLD_PROPS_SELECTOR,
at, OLD_PROPS_SELECTOR_LEN);
On 2018/02/12 12:53:18, sergei wrote:
> not important but perhaps it would be better to say
> Range prefix = {at, text.find(OLD_PROPS_SELECTOR, at,
OLD_PROPS_SELECTOR_LEN)};

Done.

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:174: if (prefix.end != text.npos)
On 2018/02/12 12:53:19, sergei wrote:
> What about returning DependentString(text); here in order to reduce the
> indentation below?Subjectively it also improves readability, BTW `suffix` and
> `length` will be declared after that `if`.

Done.

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:209: closing = true;
On 2018/02/12 12:53:19, sergei wrote:
> What about using of suffix.start instead of `closing`? We could initialize it
to
> zero. In addition to that if suffix.start is still zero after finishing the
loop
> then the filter is malformed because there is no closing ']'. Continuing
without
> taking it into account can result in bad sizes below.

suffix.start is initialised to `at`. So I'll use that. And I'll return an
invalid string.

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:221: at++;
On 2018/02/12 12:53:20, sergei wrote:
> BTW, if there is no such delimiter then is it a malformed filter string?

no. #@# is valid too. We could check if it is #@# eventually but I don't think
it is part of this.

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:223: + PROPS_SELECTOR_LEN + 1 + at;
On 2018/02/12 12:53:19, sergei wrote:
> What about
> at + prefix.len() + PROPS_SELECTOR_LEN + properties.len() + 1 /* ')' */ +
> suffix.len()
> ?

Done.

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:224: 
On 2018/02/12 12:53:18, sergei wrote:
> Could you please add the assert here that
> new_len + 1 == length || (delimiter == text.npos && new_len + 2 == length)
> just in case? Despite there are tests ensuring that nothing is lost, it just
> looks scaring :)).

Done.

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:246: std::memmove(converted.data() + new_len,
On 2018/02/12 12:53:18, sergei wrote:
> logically it should be memcpy but since memmove does not modify the memory
> accessed by `src` argument it's fine.
> 
> The same for ELEM_HIDE_EMULATION_DELIMITER below.

You are right. Changing it to memcpy().

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
File compiled/filter/ElemHideBase.h (right):

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Ele...
compiled/filter/ElemHideBase.h:66: static Type Parse(DependentString& text,
ElemHideData& data, bool& needConversion);
On 2018/02/12 12:53:20, sergei wrote:
> Perhaps it makes sense just in case to add a comment that needConversion
should
> be initialized to false before the calling of this method.

Instead I'll set it to false in there.

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Fil...
File compiled/filter/Filter.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29693607/compiled/filter/Fil...
compiled/filter/Filter.cpp:118: if (needConversion && (type ==
ElemHideException::classType || type == ElemHideEmulationFilter::classType))
On 2018/02/12 12:53:20, sergei wrote:
> Firstly I thought about moving of `(type == ElemHideException::classType ||
type
> == ElemHideEmulationFilter::classType)` inside `ElemHideBase::Parse`, it seems
a
> detail of ElemHide classes, here we just check the value of needConversion and
> do the conversion if there is a necessity in it. However it seems it's
redundant
> at all. If needConversion is set to true then the type can be either
> ELEMHIDEEXCEPTION if `exception` variable in ElemHideBase::Parse is true or
> ELEMHIDEEMULATION if that `exception` variable is false, therefore if
> needConversion is true then `(type == ElemHideException::classType || type ==
> ElemHideEmulationFilter::classType)` is always true.
> I think it can be just removed.

I was being overly paranoid.

Done.

[sergei, 2018-02-13 09:12:51]

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:24: #define LENGTH_OF(x) ((sizeof(x) /
sizeof(x[0])) - 1)
On 2018/02/12 18:14:38, hub wrote:
> On 2018/02/12 12:53:17, sergei wrote:
> > On 2018/02/07 04:13:37, hub wrote:
> > > On 2018/02/05 14:51:06, sergei wrote:
> > > > I think we may use constexpr function here,
> > > 
> > > No. This has to be a macro because of sizeof() and the use with the
argument
> > > that is an array.
> > The following does work
> > template<typename T, size_t N>
> > constexpr size_t length_of(T (&)[N])
> > {
> >   return N - 1;
> > }
> > 
> > > 
> > > > and maybe move it into Utils.h
> > > 
> > > Done
> > 
> > BTW, I just released that it's a length of string not an array (N - 1 vs N),
> > what do you think about renaming it to something like get_str_length. In
> > addition I would replace "static string array" by something like "null
> > terminated character sequence (string)".
> 
> Done.

What about replacing of "static string array" by something like "null
terminated character sequence (string)".

https://codereview.adblockplus.org/29595633/diff/29695585/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29695585/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:177: 
Do you mind to move `length` and `suffix` variables after that `if`?

[sergei, 2018-02-13 09:27:58]

https://codereview.adblockplus.org/29595633/diff/29695585/test/compiled/Filte...
File test/compiled/Filter.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29695585/test/compiled/Filte...
test/compiled/Filter.cpp:18: #include "gtest/gtest.h"
since these paths are not relative they actually should be in <>, not in "".

[hub, 2018-02-13 15:52:26]

https://codereview.adblockplus.org/29595633/diff/29695585/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29695585/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:177: 
On 2018/02/13 09:12:50, sergei wrote:
> Do you mind to move `length` and `suffix` variables after that `if`?

Done.

https://codereview.adblockplus.org/29595633/diff/29695585/test/compiled/Filte...
File test/compiled/Filter.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29695585/test/compiled/Filte...
test/compiled/Filter.cpp:18: #include "gtest/gtest.h"
On 2018/02/13 09:27:58, sergei wrote:
> since these paths are not relative they actually should be in <>, not in "".

Done.

I'll have to do the others for consistency sake.

[sergei, 2018-02-13 16:05:59]

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:24: #define LENGTH_OF(x) ((sizeof(x) /
sizeof(x[0])) - 1)
On 2018/02/13 09:12:49, sergei wrote:
> On 2018/02/12 18:14:38, hub wrote:
> > On 2018/02/12 12:53:17, sergei wrote:
> > > On 2018/02/07 04:13:37, hub wrote:
> > > > On 2018/02/05 14:51:06, sergei wrote:
> > > > > I think we may use constexpr function here,
> > > > 
> > > > No. This has to be a macro because of sizeof() and the use with the
> argument
> > > > that is an array.
> > > The following does work
> > > template<typename T, size_t N>
> > > constexpr size_t length_of(T (&)[N])
> > > {
> > >   return N - 1;
> > > }
> > > 
> > > > 
> > > > > and maybe move it into Utils.h
> > > > 
> > > > Done
> > > 
> > > BTW, I just released that it's a length of string not an array (N - 1 vs
N),
> > > what do you think about renaming it to something like get_str_length. In
> > > addition I would replace "static string array" by something like "null
> > > terminated character sequence (string)".
> > 
> > Done.
> 
> What about replacing of "static string array" by something like "null
> terminated character sequence (string)".

It seems only this is left.

[hub, 2018-02-13 16:23:37]

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:24: #define LENGTH_OF(x) ((sizeof(x) /
sizeof(x[0])) - 1)
On 2018/02/13 16:05:59, sergei wrote:
> On 2018/02/13 09:12:49, sergei wrote:
> > On 2018/02/12 18:14:38, hub wrote:
> > > On 2018/02/12 12:53:17, sergei wrote:
> > > > On 2018/02/07 04:13:37, hub wrote:
> > > > > On 2018/02/05 14:51:06, sergei wrote:
> > > > > > I think we may use constexpr function here,
> > > > > 
> > > > > No. This has to be a macro because of sizeof() and the use with the
> > argument
> > > > > that is an array.
> > > > The following does work
> > > > template<typename T, size_t N>
> > > > constexpr size_t length_of(T (&)[N])
> > > > {
> > > >   return N - 1;
> > > > }
> > > > 
> > > > > 
> > > > > > and maybe move it into Utils.h
> > > > > 
> > > > > Done
> > > > 
> > > > BTW, I just released that it's a length of string not an array (N - 1 vs
> N),
> > > > what do you think about renaming it to something like get_str_length. In
> > > > addition I would replace "static string array" by something like "null
> > > > terminated character sequence (string)".
> > > 
> > > Done.
> > 
> > What about replacing of "static string array" by something like "null
> > terminated character sequence (string)".
> 
> It seems only this is left.

oops.

Done

[sergei, 2018-02-14 15:59:37]

Sorry, yet one thing regarding
[-abp-properties=what-happens-here"useful"and-here].

https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:80: needConversion = false;
Perhaps, it should be the very first instruction of this function.

https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:204: case u']':
What about filter containing
`[-abp-properties=what-happens-here"useful"and-here]`? I'm not sure that it's
supposed to be correct and I think it's not handled here. What about setting and
checking (either " or ') the value of `quote` before the loop and expecting ]
right after the closing quote is found?

[hub, 2018-02-14 17:05:21]

https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:80: needConversion = false;
On 2018/02/14 15:59:35, sergei wrote:
> Perhaps, it should be the very first instruction of this function.

Done.

https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:204: case u']':
On 2018/02/14 15:59:35, sergei wrote:
> What about filter containing
> `[-abp-properties=what-happens-here"useful"and-here]`? I'm not sure that it's
> supposed to be correct and I think it's not handled here. What about setting
and
> checking (either " or ') the value of `quote` before the loop and expecting ]
> right after the closing quote is found?

This would definitely be invalid. I'll make sure this is rejected here as well
(and tested)

[hub, 2018-02-22 18:56:31]

https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:204: case u']':
On 2018/02/14 17:05:20, hub wrote:

> This would definitely be invalid. I'll make sure this is rejected here as well
> (and tested)

It is definitely handled in that last patch.

[sergei, 2018-02-27 10:56:54]

LGTM

https://codereview.adblockplus.org/29595633/diff/29697675/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29697675/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:222: if (suffix.start == at)
Just for reference, I think it (what the whole for-loop above is doing) can be
written in a simpler form, but since it's some sort of interim function it's
fine.

https://codereview.adblockplus.org/29595633/diff/29697675/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:232: assert2(new_len + 1 == length ||
(delimiter == text.npos && new_len + 2 == length), u"Inconsistent length in
filter conversion."_str);
not important just for reference
length == new_len + (delimiter == text.npos ? 2 : 1)
looks shorted and maybe even more logical.

[hub, 2018-02-27 13:32:31]

https://codereview.adblockplus.org/29595633/diff/29697675/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29595633/diff/29697675/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:232: assert2(new_len + 1 == length ||
(delimiter == text.npos && new_len + 2 == length), u"Inconsistent length in
filter conversion."_str);
On 2018/02/27 10:56:53, sergei wrote:
> not important just for reference
> length == new_len + (delimiter == text.npos ? 2 : 1)
> looks shorted and maybe even more logical.

Acknowledged.