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

Issue 29587914: Issue 5142 - Convert Element Hiding to C++ (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 months ago by hub
Modified:
8 months, 3 weeks ago
Reviewers:
sergei
CC:
Oleksandr
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 5142 - Convert Element Hiding to C++

Patch Set 1 #

Total comments: 11

Patch Set 2 : More test, cache the unconditional selectors. Various cleanup #

Total comments: 2

Patch Set 3 : A few more fixes. #

Patch Set 4 : Add proper delete for filters in tests. #

Patch Set 5 : Some style change following feedback from a different review. #

Patch Set 6 : Added OwnedStringMap to address ownership of keys #

Patch Set 7 : Rebased on master. #

Total comments: 45

Patch Set 8 : Reworked after review. #

Total comments: 14

Patch Set 9 : Re-added elemHide.js test. Fixed test failures related. Addresses review comments. #

Total comments: 2

Patch Set 10 : Fixed leaks in test. Extracted changes to string and string map. #

Patch Set 11 : remove an unused #include #

Patch Set 12 : defaultDomains is now in an anon namespace #

Patch Set 13 : Depend on #6279. Other minor fixes. #

Total comments: 21

Patch Set 14 : Use withNAD in the test. #

Patch Set 15 : Updated from feedback #

Patch Set 16 : mFiltersByDomain is now an OwnedStringMap #

Unified diffs Side-by-side diffs Delta from patch set Stats (+800 lines, -427 lines) Patch
A compiled/ElemHide.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +106 lines, -0 lines 0 comments Download
A compiled/ElemHide.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +244 lines, -0 lines 0 comments Download
M compiled/bindings/main.cpp View 1 2 3 4 5 6 7 2 chunks +15 lines, -0 lines 0 comments Download
M compiled/filter/ActiveFilter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +5 lines, -3 lines 0 comments Download
M compiled/filter/ActiveFilter.cpp View 1 2 3 4 5 6 7 1 chunk +1 line, -4 lines 0 comments Download
M lib/elemHide.js View 1 2 3 4 5 6 7 8 1 chunk +6 lines, -375 lines 0 comments Download
M lib/elemHideEmulation.js View 1 2 3 4 5 6 7 2 chunks +3 lines, -3 lines 0 comments Download
M meson.build View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
A test/elemHide.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +268 lines, -0 lines 0 comments Download
M test/elemHideEmulation.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +151 lines, -42 lines 0 comments Download

Messages

Total messages: 22
hub
12 months ago (2017-10-25 01:07:47 UTC) #1
hub
It is based on some unlanded patches. I have a few more changes to do ...
11 months, 4 weeks ago (2017-10-25 01:19:39 UTC) #2
hub
https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.cpp#newcode172 compiled/ElemHide.cpp:172: _ElemHide_SelectorList* ElemHide::GetUnconditionalSelectors() const there is also a more efficient ...
11 months, 4 weeks ago (2017-10-25 01:39:12 UTC) #3
hub
This looks better. Still depend on unlanded patches.
11 months, 4 weeks ago (2017-10-26 19:21:10 UTC) #4
hub
https://codereview.adblockplus.org/29587914/diff/29589658/compiled/ElemHide.h File compiled/ElemHide.h (right): https://codereview.adblockplus.org/29587914/diff/29589658/compiled/ElemHide.h#newcode43 compiled/ElemHide.h:43: void push_back(ElemHideBasePtr filter) I'm doing this wrong. We are ...
11 months, 4 weeks ago (2017-10-26 20:53:31 UTC) #5
hub
https://codereview.adblockplus.org/29587914/diff/29589658/compiled/ElemHide.h File compiled/ElemHide.h (right): https://codereview.adblockplus.org/29587914/diff/29589658/compiled/ElemHide.h#newcode43 compiled/ElemHide.h:43: void push_back(ElemHideBasePtr filter) On 2017/10/26 20:53:30, hub wrote: > ...
11 months, 4 weeks ago (2017-10-26 20:58:27 UTC) #6
sergei
First portion of comments mainly merely regarding the code. https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.h File compiled/ElemHide.h (right): https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.h#newcode55 compiled/ElemHide.h:55: ...
9 months, 1 week ago (2018-01-15 15:31:20 UTC) #7
hub
Patch updated. Now require https://codereview.adblockplus.org/29669631/ that was split out. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.cpp#newcode22 compiled/ElemHide.cpp:22: ...
9 months ago (2018-01-16 02:57:40 UTC) #8
sergei
So, I have not checked logic yet :) will do it next. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.cpp File compiled/ElemHide.cpp ...
9 months ago (2018-01-16 16:44:00 UTC) #9
sergei
It seems test/elemHide.js is not added, I think it's very essential here because the logic ...
9 months ago (2018-01-16 18:05:28 UTC) #10
hub
On 2018/01/16 18:05:28, sergei wrote: > It seems test/elemHide.js is not added, I think it's ...
9 months ago (2018-01-16 18:17:15 UTC) #11
hub
Sorry about that missing, led to several fixes. Patch now updated. https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.h File compiled/ElemHide.h (right): ...
9 months ago (2018-01-19 02:11:05 UTC) #12
sergei
Sorry for the delay, I have a couple of comments which are worthy already. Tests ...
9 months ago (2018-01-22 15:40:08 UTC) #13
hub
On 2018/01/22 15:40:08, sergei wrote: > Sorry for the delay, I have a couple of ...
9 months ago (2018-01-22 16:00:47 UTC) #14
hub
On 2018/01/22 16:00:47, hub wrote: > > Since it takes really too long I would ...
9 months ago (2018-01-22 16:33:44 UTC) #15
hub
https://codereview.adblockplus.org/29587914/diff/29673635/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29587914/diff/29673635/test/elemHide.js#newcode258 test/elemHide.js:258: elemHide.add(Filter.fromText("##test")); On 2018/01/22 15:40:07, sergei wrote: > there is ...
9 months ago (2018-01-22 16:49:18 UTC) #16
hub
https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.cpp#newcode38 compiled/ElemHide.cpp:38: ActiveFilter::DomainMap ElemHide::defaultDomains; On 2018/01/22 15:40:07, sergei wrote: > On ...
9 months ago (2018-01-22 16:59:16 UTC) #17
sergei
just few things https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp#newcode40 compiled/ElemHide.cpp:40: const ActiveFilter::DomainMap defaultDomains = { Could ...
8 months, 3 weeks ago (2018-01-25 14:52:42 UTC) #18
hub
https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp#newcode40 compiled/ElemHide.cpp:40: const ActiveFilter::DomainMap defaultDomains = { On 2018/01/25 14:52:40, sergei ...
8 months, 3 weeks ago (2018-01-25 16:55:11 UTC) #19
sergei
https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp#newcode145 compiled/ElemHide.cpp:145: } On 2018/01/25 16:55:08, hub wrote: > On 2018/01/25 ...
8 months, 3 weeks ago (2018-01-26 15:48:48 UTC) #20
hub
Updated patch. Require https://codereview.adblockplus.org/29680706/ to work. https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp#newcode145 compiled/ElemHide.cpp:145: } On 2018/01/26 ...
8 months, 3 weeks ago (2018-01-26 20:42:00 UTC) #21
sergei
8 months, 3 weeks ago (2018-01-29 13:47:15 UTC) #22
LGTM

https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp
File compiled/ElemHide.cpp (right):

https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c...
compiled/ElemHide.cpp:145: }
On 2018/01/26 20:41:59, hub wrote:
> On 2018/01/26 15:48:47, sergei wrote:
> 
> 
> > > list is StringMap<> and erase() on it doesn't decrease size(). Until we
fix
> > > this, we can't know.
> > 
> > I think we should remove it. Despite it's not supported now, it makes sense
to
> > not "leak" the memory for domains which are not used anymore, although it
can
> be
> > a rare case.
> 
> erase will release the filter (it resets to the default value). When we fix
> StringMap we can choose to remove the entry in mFiltersByDomain.
> 
> > 
> > In addition to that I think there is a tricky issue. Let's say we add a
> > `filter1` with "domain.value", then we add `filter2` with the the same
> > "domain-value", then we remove `filter1`, so the key, which is used in the
> > mFiltersByDomain, which is a `DependentString` pointing to a string held by
> > `filter1`, so it is a dangling `DependentString` now and when it is used
next
> > time in `Map`, in particular as `entry->equals(some-key)` there will be
access
> > violation. Could we do something with this?
> 
> Good call.
> I should turn them into OwnedStringMap.
OK, I also thought that maybe we could just update the key (DependentString) to
the first filter from the "value" and if the size of "value" is zero then just
remove the entry, but the approach with OwnedStringMap looks also fine because
it seems harder to make a mistake :).
> 
> I'll need https://codereview.adblockplus.org/29680706/ for this to work
though.
Sign in to reply to this message.

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