Issue 5143 - Convert ElemHideEmulation to C++

Issue 5143 - Convert ElemHideEmulation to C++

Applies on top of:
https://codereview.adblockplus.org/29587914/

[hub, 2017-11-02 04:11:14]

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
File compiled/ElemHideEmulation.cpp (right):

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:43: DependentString docDomain(domain);
IsActiveOnDomain() will modify the DependentString passed to it, which makes
everything else fail. It is subtle. And IMHO it would be better if we made
IsActiveOnDomain() use an immutable domain string - (albeit it calls lowerCase()
on it which modifies the string in place)

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:44: if (!filter.is_deleted() &&
IMHO we should change StringMap<> iterator so that the deleted entries are
skipped.

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/bindings/m...
File compiled/bindings/main.cpp (left):

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/bindings/m...
compiled/bindings/main.cpp:153: .function("getException",
&ElemHide::GetException)
This is no longer needed as issue #5142 says.

[sergei, 2017-11-16 09:58:07]

I think we should separately review and address the usage of singleton pattern
and when the strings may be mutable.

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
File compiled/ElemHideEmulation.cpp (right):

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:40: auto result = new
_ElemHideEmulation_FilterList;
I would prefer to have the type of `result` intrusive_ptr.

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:43: DependentString docDomain(domain);
On 2017/11/02 04:11:13, hub wrote:
> IsActiveOnDomain() will modify the DependentString passed to it, which makes
> everything else fail. It is subtle. And IMHO it would be better if we made
> IsActiveOnDomain() use an immutable domain string - (albeit it calls
lowerCase()
> on it which modifies the string in place)

agree.

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:44: if (!filter.is_deleted() &&
On 2017/11/02 04:11:13, hub wrote:
> IMHO we should change StringMap<> iterator so that the deleted entries are
> skipped.

agree

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
File compiled/ElemHideEmulation.h (right):

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.h:26: class _ElemHideEmulation_FilterList : public
ref_counted
IMO it's not a good idea to have names starting with underscore in C++. Could
you please rename it?

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.h:39: void push_back(ElemHideBasePtr filter)
What about using r-value or l-value reference here?

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.h:53: static ElemHideEmulation* BINDINGS_EXPORTED
GetInstance()
IMO the growth of usage of the singleton pattern is getting really dangerous.
Can we stop doing it?

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

https://codereview.adblockplus.org/29594607/diff/29594608/test/elemHideEmulat...
test/elemHideEmulation.js:177: let rules =
ElemHideEmulation.getRulesForDomain(domain);
What about returning a generator object from ElemHideEmulation.getRulesForDomain
(similar to
https://github.com/adblockplus/adblockpluscore/blob/emscripten/lib/filterStor...

BTW, one should call `delete` on the result of `rules.filterAt(i)` and on
`rules`.

[hub, 2017-11-20 19:16:01]

I didn't implement the iterator. (see my comment)

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
File compiled/ElemHideEmulation.cpp (right):

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:40: auto result = new
_ElemHideEmulation_FilterList;
On 2017/11/16 09:58:06, sergei wrote:
> I would prefer to have the type of `result` intrusive_ptr.

Done.

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
File compiled/ElemHideEmulation.h (right):

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.h:26: class _ElemHideEmulation_FilterList : public
ref_counted
On 2017/11/16 09:58:07, sergei wrote:
> IMO it's not a good idea to have names starting with underscore in C++. Could
> you please rename it?

That was part of the "spec" but indeed it conflict with C++.

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.h:39: void push_back(ElemHideBasePtr filter)
On 2017/11/16 09:58:07, sergei wrote:
> What about using r-value or l-value reference here?

r-value is impossible: we don't want to move the value. (also fails to compile).
const l-value is fine.

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.h:53: static ElemHideEmulation* BINDINGS_EXPORTED
GetInstance()
On 2017/11/16 09:58:07, sergei wrote:
> IMO the growth of usage of the singleton pattern is getting really dangerous.
> Can we stop doing it?

yes.

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

https://codereview.adblockplus.org/29594607/diff/29594608/test/elemHideEmulat...
test/elemHideEmulation.js:177: let rules =
ElemHideEmulation.getRulesForDomain(domain);
On 2017/11/16 09:58:07, sergei wrote:
> What about returning a generator object from
ElemHideEmulation.getRulesForDomain
> (similar to
>
https://github.com/adblockplus/adblockpluscore/blob/emscripten/lib/filterStor...
> 
> BTW, one should call `delete` on the result of `rules.filterAt(i)` and on
> `rules`.

delete on the result of `rules.filterAt(i)` is an error with the current
implementation. I'll change filterAt() to match FilterStorage.

I am not convinced on having that iterator / generator since it involve writing
JS code.

[sergei, 2018-01-25 16:04:02]

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
File compiled/ElemHideEmulation.h (right):

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.h:39: void push_back(ElemHideBasePtr filter)
On 2017/11/20 19:16:01, hub wrote:
> On 2017/11/16 09:58:07, sergei wrote:
> > What about using r-value or l-value reference here?
> 
> r-value is impossible: we don't want to move the value. (also fails to
compile).
> const l-value is fine.

Acknowledged. Perhaps we should revisit such methods later and create both
versions.

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
File compiled/ElemHideEmulation.cpp (right):

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:38: ElemHideEmulation_FilterList*
ElemHideEmulation::GetRulesForDomain(DependentString& domain)
Just a note: it should accept elemhide now

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:40: intrusive_ptr<ElemHideEmulation_FilterList>
result(new ElemHideEmulation_FilterList);
could you please add () to the constructor call?

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:41: for (auto filter: mFilters)
perhaps it would be better to call it filterEntry because it's not exactly a
filter.

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:45: filter.second->IsActiveOnDomain(docDomain,
u""_str) &&
what do you think about having Dereferenceable iterators? E.g. *filterEntry
could be equal to filterEntry.second. Of course it's beyond the scope of the
current issue.

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
File compiled/ElemHideEmulation.h (right):

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
compiled/ElemHideEmulation.h:55: static ElemHideEmulation* mInstance;
Could you please turn it into non-singleton?

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

https://codereview.adblockplus.org/29594607/diff/29631268/test/elemHideEmulat...
test/elemHideEmulation.js:86: test.equal(selectors.selectorCount, 3);
IMO it would be better to check the actually returned selectors than merely the
size of the list.

[hub, 2018-01-25 19:05:28]

update patch

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
File compiled/ElemHideEmulation.h (right):

https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEm...
compiled/ElemHideEmulation.h:39: void push_back(ElemHideBasePtr filter)
On 2018/01/25 16:04:00, sergei wrote:
> On 2017/11/20 19:16:01, hub wrote:
> > On 2017/11/16 09:58:07, sergei wrote:
> > > What about using r-value or l-value reference here?
> > 
> > r-value is impossible: we don't want to move the value. (also fails to
> compile).
> > const l-value is fine.
> 
> Acknowledged. Perhaps we should revisit such methods later and create both
> versions.

Looking at the use case, move doesn't make sense here. We sure can revisit it if
the need arises.

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
File compiled/ElemHideEmulation.cpp (right):

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:38: ElemHideEmulation_FilterList*
ElemHideEmulation::GetRulesForDomain(DependentString& domain)
On 2018/01/25 16:04:00, sergei wrote:
> Just a note: it should accept elemhide now

It is already done. I just didn't update the review.

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:40: intrusive_ptr<ElemHideEmulation_FilterList>
result(new ElemHideEmulation_FilterList);
On 2018/01/25 16:04:01, sergei wrote:
> could you please add () to the constructor call?

Done.

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:41: for (auto filter: mFilters)
On 2018/01/25 16:04:01, sergei wrote:
> perhaps it would be better to call it filterEntry because it's not exactly a
> filter.

entry like elsewhere.

Done.

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:45: filter.second->IsActiveOnDomain(docDomain,
u""_str) &&
On 2018/01/25 16:04:01, sergei wrote:
> what do you think about having Dereferenceable iterators? E.g. *filterEntry
> could be equal to filterEntry.second. Of course it's beyond the scope of the
> current issue.

filterEntry isn't an iterator, but the entry in the hash map. I don't think it
brings anything but confusion to do that. It is not in line with std:: either.

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
File compiled/ElemHideEmulation.h (right):

https://codereview.adblockplus.org/29594607/diff/29631268/compiled/ElemHideEm...
compiled/ElemHideEmulation.h:55: static ElemHideEmulation* mInstance;
On 2018/01/25 16:04:01, sergei wrote:
> Could you please turn it into non-singleton?

Ah yes, I was meaning to the same as for ElemHide. Definitely.

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

https://codereview.adblockplus.org/29594607/diff/29631268/test/elemHideEmulat...
test/elemHideEmulation.js:86: test.equal(selectors.selectorCount, 3);
On 2018/01/25 16:04:01, sergei wrote:
> IMO it would be better to check the actually returned selectors than merely
the
> size of the list.

Done. (in the whole test, not just here).

[sergei, 2018-01-30 16:00:48]

https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEm...
File compiled/ElemHideEmulation.cpp (right):

https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:44: if (!entry.is_deleted() &&
Well, because of https://issues.adblockplus.org/ticket/6280 we should actually
check that the entry is not invalid.

https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEm...
File compiled/ElemHideEmulation.h (right):

https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEm...
compiled/ElemHideEmulation.h:57: static ElemHideEmulation* mInstance;
It's not used, could you please remove it?

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

https://codereview.adblockplus.org/29594607/diff/29679845/test/elemHideEmulat...
test/elemHideEmulation.js:90: {
It seems some changes like these are not related to the issue #5143, could you
please extract them?

[hub, 2018-01-30 17:37:59]

https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEm...
File compiled/ElemHideEmulation.cpp (right):

https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:44: if (!entry.is_deleted() &&
On 2018/01/30 16:00:47, sergei wrote:
> Well, because of https://issues.adblockplus.org/ticket/6280 we should actually
> check that the entry is not invalid.

Done.

https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEm...
File compiled/ElemHideEmulation.h (right):

https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEm...
compiled/ElemHideEmulation.h:57: static ElemHideEmulation* mInstance;
On 2018/01/30 16:00:47, sergei wrote:
> It's not used, could you please remove it?

Done.

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

https://codereview.adblockplus.org/29594607/diff/29679845/test/elemHideEmulat...
test/elemHideEmulation.js:90: {
On 2018/01/30 16:00:48, sergei wrote:
> It seems some changes like these are not related to the issue #5143, could you
> please extract them?

In a preview comment on this review you wanted to check more than just the
count. So I did. I'll extract all of that.

[sergei, 2018-01-30 17:40:57]

LGTM

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

https://codereview.adblockplus.org/29594607/diff/29679845/test/elemHideEmulat...
test/elemHideEmulation.js:90: {
On 2018/01/30 17:37:58, hub wrote:
> On 2018/01/30 16:00:48, sergei wrote:
> > It seems some changes like these are not related to the issue #5143, could
you
> > please extract them?
> 
> In a preview comment on this review you wanted to check more than just the
> count. So I did. I'll extract all of that.

Ah, OK.

[hub, 2018-01-30 17:53:29]

https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEm...
File compiled/ElemHideEmulation.cpp (right):

https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:44: if (!entry.is_deleted() &&
On 2018/01/30 17:37:58, hub wrote:
> On 2018/01/30 16:00:47, sergei wrote:
> > Well, because of https://issues.adblockplus.org/ticket/6280 we should
actually
> > check that the entry is not invalid.
> 
> Done.

I forgot to add that actually we won't get invalid entries as the iterator
exclude them. Even though I did change the code.

[sergei, 2018-01-30 17:55:31]

https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEm...
File compiled/ElemHideEmulation.cpp (right):

https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEm...
compiled/ElemHideEmulation.cpp:44: if (!entry.is_deleted() &&
On 2018/01/30 17:53:29, hub wrote:
> On 2018/01/30 17:37:58, hub wrote:
> > On 2018/01/30 16:00:47, sergei wrote:
> > > Well, because of https://issues.adblockplus.org/ticket/6280 we should
> actually
> > > check that the entry is not invalid.
> > 
> > Done.
> 
> I forgot to add that actually we won't get invalid entries as the iterator
> exclude them. Even though I did change the code.

The very first entry can be invalid :)