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

Issue 29594607: Issue 5143 - Convert ElemHideEmulation to C++ (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 months, 3 weeks ago by hub
Modified:
8 months, 3 weeks ago
Reviewers:
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: 17

Patch Set 2 : Improvements from review. #

Patch Set 3 : Rebased. #

Total comments: 12

Patch Set 4 : Rebase. Review comments addressed. #

Total comments: 9

Patch Set 5 : Review comments addressed #

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

Messages

Total messages: 11
hub
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 1 week ago (2017-11-16 09:58:07 UTC) #3
hub
I didn't implement the iterator. (see my comment) 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#newcode40 compiled/ElemHideEmulation.cpp:40: auto ...
11 months ago (2017-11-20 19:16:01 UTC) #4
sergei
https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEmulation.h File compiled/ElemHideEmulation.h (right): https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEmulation.h#newcode39 compiled/ElemHideEmulation.h:39: void push_back(ElemHideBasePtr filter) On 2017/11/20 19:16:01, hub wrote: > ...
8 months, 3 weeks ago (2018-01-25 16:04:02 UTC) #5
hub
update patch https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEmulation.h File compiled/ElemHideEmulation.h (right): https://codereview.adblockplus.org/29594607/diff/29594608/compiled/ElemHideEmulation.h#newcode39 compiled/ElemHideEmulation.h:39: void push_back(ElemHideBasePtr filter) On 2018/01/25 16:04:00, sergei ...
8 months, 3 weeks ago (2018-01-25 19:05:28 UTC) #6
sergei
https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEmulation.cpp File compiled/ElemHideEmulation.cpp (right): https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEmulation.cpp#newcode44 compiled/ElemHideEmulation.cpp:44: if (!entry.is_deleted() && Well, because of https://issues.adblockplus.org/ticket/6280 we should ...
8 months, 3 weeks ago (2018-01-30 16:00:48 UTC) #7
hub
https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEmulation.cpp File compiled/ElemHideEmulation.cpp (right): https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEmulation.cpp#newcode44 compiled/ElemHideEmulation.cpp:44: if (!entry.is_deleted() && On 2018/01/30 16:00:47, sergei wrote: > ...
8 months, 3 weeks ago (2018-01-30 17:37:59 UTC) #8
sergei
LGTM https://codereview.adblockplus.org/29594607/diff/29679845/test/elemHideEmulation.js File test/elemHideEmulation.js (right): https://codereview.adblockplus.org/29594607/diff/29679845/test/elemHideEmulation.js#newcode90 test/elemHideEmulation.js:90: { On 2018/01/30 17:37:58, hub wrote: > On ...
8 months, 3 weeks ago (2018-01-30 17:40:57 UTC) #9
hub
https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEmulation.cpp File compiled/ElemHideEmulation.cpp (right): https://codereview.adblockplus.org/29594607/diff/29679845/compiled/ElemHideEmulation.cpp#newcode44 compiled/ElemHideEmulation.cpp:44: if (!entry.is_deleted() && On 2018/01/30 17:37:58, hub wrote: > ...
8 months, 3 weeks ago (2018-01-30 17:53:29 UTC) #10
sergei
8 months, 3 weeks ago (2018-01-30 17:55:31 UTC) #11
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 :)
Sign in to reply to this message.

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