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

Issue 29595633: Issue 5870 - Implement the new ElemHideEmulation filter type (Closed)

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

Description

Issue 5870 - Implement the new ElemHideEmulation filter type Applies on top: https://codereview.adblockplus.org/29594607/

Patch Set 1 #

Total comments: 2

Patch Set 2 : properly handle exception filters. some test fixes. #

Patch Set 3 : Rebased patch. Fix assertion. #

Patch Set 4 : Rebased #

Patch Set 5 : Rebased #

Total comments: 6

Patch Set 6 : Review comments #

Total comments: 2

Patch Set 7 : Reworked the logic and added tests #

Total comments: 2

Patch Set 8 : Reworked the strings const. String::find() from a String::value_type* #

Patch Set 9 : Refactor consts #

Total comments: 21

Patch Set 10 : Address review comment. Reworked the conversion #

Patch Set 11 : Addressed failing test: selector index was improperly adjusted. #

Patch Set 12 : Fix a few memory leaks (add ref issues) #

Patch Set 13 : Added missing ABP_NS macros #

Total comments: 20

Patch Set 14 : Addressed review comments. #

Total comments: 4

Patch Set 15 : Review comments #

Patch Set 16 : Last comment addressed. #

Total comments: 5

Patch Set 17 : Deal with ill formed filters. #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+374 lines, -68 lines) Patch
M compiled/String.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +9 lines, -4 lines 0 comments Download
M compiled/Utils.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +7 lines, -0 lines 0 comments Download
M compiled/filter/ElemHideBase.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -1 line 0 comments Download
M compiled/filter/ElemHideBase.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +152 lines, -3 lines 3 comments Download
M compiled/filter/Filter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +16 lines, -5 lines 0 comments Download
M meson.build View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
A test/compiled/Filter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +100 lines, -0 lines 0 comments Download
M test/elemHideEmulation.js View 1 2 3 4 5 6 7 8 9 2 chunks +29 lines, -13 lines 0 comments Download
M test/filterClasses.js View 1 2 3 4 5 6 5 chunks +53 lines, -41 lines 0 comments Download
M test/subscriptionClasses.js View 1 2 3 4 5 1 chunk +5 lines, -1 line 0 comments Download

Messages

Total messages: 25
hub
10 months, 2 weeks ago (2017-11-02 23:42:43 UTC) #1
hub
Also on top of my other patches. https://codereview.adblockplus.org/29595633/diff/29595634/compiled/filter/ElemHideEmulationFilter.cpp File compiled/filter/ElemHideEmulationFilter.cpp (right): https://codereview.adblockplus.org/29595633/diff/29595634/compiled/filter/ElemHideEmulationFilter.cpp#newcode24 compiled/filter/ElemHideEmulationFilter.cpp:24: OwnedString ConvertFilter(const ...
10 months, 2 weeks ago (2017-11-02 23:47:50 UTC) #2
hub
also we need to merge the changes to the content script here. Not exactly sure ...
10 months, 2 weeks ago (2017-11-02 23:48:52 UTC) #3
sergei
could you please rebase it?
7 months, 3 weeks ago (2018-01-30 18:09:08 UTC) #4
hub
On 2018/01/30 18:09:08, sergei wrote: > could you please rebase it? done
7 months, 3 weeks ago (2018-01-30 18:16:50 UTC) #5
sergei
I will post the comments regarding the implementation of ConvertFilter in the next round, if ...
7 months, 3 weeks ago (2018-01-30 19:02:40 UTC) #6
hub
https://codereview.adblockplus.org/29595633/diff/29684732/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29684732/compiled/filter/ElemHideBase.cpp#newcode54 compiled/filter/ElemHideBase.cpp:54: OwnedString ConvertFilter(const String& text, String::size_type at) On 2018/01/30 19:02:40, ...
7 months, 3 weeks ago (2018-01-30 22:53:13 UTC) #7
hub
https://codereview.adblockplus.org/29595633/diff/29684741/test/compiled/Filter.cpp File test/compiled/Filter.cpp (right): https://codereview.adblockplus.org/29595633/diff/29684741/test/compiled/Filter.cpp#newcode1 test/compiled/Filter.cpp:1: I have an update with the license header I ...
7 months, 3 weeks ago (2018-01-30 23:12:02 UTC) #8
hub
This should be better.
7 months, 2 weeks ago (2018-02-01 20:38:20 UTC) #9
sergei
https://codereview.adblockplus.org/29595633/diff/29685641/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29685641/compiled/filter/ElemHideBase.cpp#newcode125 compiled/filter/ElemHideBase.cpp:125: emulation = !exception; Should it be an invalid filter ...
7 months, 2 weeks ago (2018-02-05 14:51:07 UTC) #10
hub
Patch updated. https://codereview.adblockplus.org/29595633/diff/29685641/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29685641/compiled/filter/ElemHideBase.cpp#newcode125 compiled/filter/ElemHideBase.cpp:125: emulation = !exception; On 2018/02/05 14:51:04, sergei ...
7 months, 2 weeks ago (2018-02-07 04:13:37 UTC) #11
hub
There is still a test failure I missed. Will update the patch with a fix.
7 months, 2 weeks ago (2018-02-07 15:02:52 UTC) #12
hub
fixed
7 months, 2 weeks ago (2018-02-07 15:50:34 UTC) #13
sergei
https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/ElemHideBase.cpp#newcode24 compiled/filter/ElemHideBase.cpp:24: #define LENGTH_OF(x) ((sizeof(x) / sizeof(x[0])) - 1) On 2018/02/07 ...
7 months, 1 week ago (2018-02-12 12:53:21 UTC) #14
hub
https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/ElemHideBase.cpp#newcode24 compiled/filter/ElemHideBase.cpp:24: #define LENGTH_OF(x) ((sizeof(x) / sizeof(x[0])) - 1) On 2018/02/12 ...
7 months, 1 week ago (2018-02-12 18:14:41 UTC) #15
sergei
https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/ElemHideBase.cpp#newcode24 compiled/filter/ElemHideBase.cpp:24: #define LENGTH_OF(x) ((sizeof(x) / sizeof(x[0])) - 1) On 2018/02/12 ...
7 months, 1 week ago (2018-02-13 09:12:51 UTC) #16
sergei
https://codereview.adblockplus.org/29595633/diff/29695585/test/compiled/Filter.cpp File test/compiled/Filter.cpp (right): https://codereview.adblockplus.org/29595633/diff/29695585/test/compiled/Filter.cpp#newcode18 test/compiled/Filter.cpp:18: #include "gtest/gtest.h" since these paths are not relative they ...
7 months, 1 week ago (2018-02-13 09:27:58 UTC) #17
hub
https://codereview.adblockplus.org/29595633/diff/29695585/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29695585/compiled/filter/ElemHideBase.cpp#newcode177 compiled/filter/ElemHideBase.cpp:177: On 2018/02/13 09:12:50, sergei wrote: > Do you mind ...
7 months, 1 week ago (2018-02-13 15:52:26 UTC) #18
sergei
https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/ElemHideBase.cpp#newcode24 compiled/filter/ElemHideBase.cpp:24: #define LENGTH_OF(x) ((sizeof(x) / sizeof(x[0])) - 1) On 2018/02/13 ...
7 months, 1 week ago (2018-02-13 16:05:59 UTC) #19
hub
https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29686644/compiled/filter/ElemHideBase.cpp#newcode24 compiled/filter/ElemHideBase.cpp:24: #define LENGTH_OF(x) ((sizeof(x) / sizeof(x[0])) - 1) On 2018/02/13 ...
7 months, 1 week ago (2018-02-13 16:23:37 UTC) #20
sergei
Sorry, yet one thing regarding [-abp-properties=what-happens-here"useful"and-here]. https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/ElemHideBase.cpp#newcode80 compiled/filter/ElemHideBase.cpp:80: needConversion = false; ...
7 months, 1 week ago (2018-02-14 15:59:37 UTC) #21
hub
https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/ElemHideBase.cpp#newcode80 compiled/filter/ElemHideBase.cpp:80: needConversion = false; On 2018/02/14 15:59:35, sergei wrote: > ...
7 months, 1 week ago (2018-02-14 17:05:21 UTC) #22
hub
https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29696615/compiled/filter/ElemHideBase.cpp#newcode204 compiled/filter/ElemHideBase.cpp:204: case u']': On 2018/02/14 17:05:20, hub wrote: > This ...
6 months, 4 weeks ago (2018-02-22 18:56:31 UTC) #23
sergei
LGTM https://codereview.adblockplus.org/29595633/diff/29697675/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29595633/diff/29697675/compiled/filter/ElemHideBase.cpp#newcode222 compiled/filter/ElemHideBase.cpp:222: if (suffix.start == at) Just for reference, I ...
6 months, 3 weeks ago (2018-02-27 10:56:54 UTC) #24
hub
6 months, 3 weeks ago (2018-02-27 13:32:31 UTC) #25
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.
Sign in to reply to this message.

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