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

Issue 29594607: Issue 5143 - Convert ElemHideEmulation to C++

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by hub
Modified:
1 week, 1 day ago
Reviewers:
Wladimir Palant, sergei
CC:
Oleksandr
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 5143 - Convert ElemHideEmulation to C++ Applies on top of: https://codereview.adblockplus.org/29587914/

Patch Set 1 #

Total comments: 15

Patch Set 2 : Improvements from review. #

Patch Set 3 : Rebased. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+153 lines, -67 lines) Patch
M compiled/ElemHide.h View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
A compiled/ElemHideEmulation.h View 1 1 chunk +67 lines, -0 lines 0 comments Download
A compiled/ElemHideEmulation.cpp View 1 1 chunk +51 lines, -0 lines 0 comments Download
M compiled/bindings/main.cpp View 1 2 2 chunks +11 lines, -1 line 0 comments Download
M lib/elemHideEmulation.js View 1 chunk +1 line, -60 lines 0 comments Download
M meson.build View 1 chunk +1 line, -0 lines 0 comments Download
M test/elemHideEmulation.js View 1 2 2 chunks +20 lines, -4 lines 0 comments Download

Messages

Total messages: 4
hub
1 month, 1 week ago (2017-11-01 14:42:38 UTC) #1
hub
https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEmulation.cpp File compiled/ElemHideEmulation.cpp (right): https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEmulation.cpp#newcode43 compiled/ElemHideEmulation.cpp:43: DependentString docDomain(domain); IsActiveOnDomain() will modify the DependentString passed to ...
1 month, 1 week ago (2017-11-02 04:11:14 UTC) #2
sergei
I think we should separately review and address the usage of singleton pattern and when ...
4 weeks ago (2017-11-16 09:58:07 UTC) #3
hub
3 weeks, 2 days ago (2017-11-20 19:16:01 UTC) #4
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.
Sign in to reply to this message.

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