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

Issue 29580558: Issue 5174 - Allow '{' and '}' in selectors (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by hub
Modified:
1 year, 12 months ago
Reviewers:
Wladimir Palant, sergei
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 5174 - Allow '{' and '}' in selectors

Patch Set 1 #

Patch Set 2 : Fix a bug. Elememulation test case. #

Total comments: 5

Patch Set 3 : Preallocate the destination #

Total comments: 14

Patch Set 4 : adressed comments #

Total comments: 2

Patch Set 5 : Use the proper escape sequence #

Total comments: 2

Patch Set 6 : Indent namespace content. #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+69 lines, -15 lines) Patch
M compiled/filter/ElemHideBase.h View 1 chunk +1 line, -5 lines 0 comments Download
M compiled/filter/ElemHideBase.cpp View 1 2 3 4 5 2 chunks +52 lines, -9 lines 3 comments Download
M test/filterClasses.js View 1 2 3 4 2 chunks +16 lines, -1 line 0 comments Download

Messages

Total messages: 15
hub
2 years, 1 month ago (2017-10-16 17:10:35 UTC) #1
hub
https://codereview.adblockplus.org/29580558/diff/29580599/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29580558/diff/29580599/test/filterClasses.js#newcode391 test/filterClasses.js:391: filter = Filter.fromText("foo.com##[-abp-properties='/margin: [3-4]{2}/']"); we don't handle the new ...
2 years, 1 month ago (2017-10-16 18:39:43 UTC) #2
Wladimir Palant
https://codereview.adblockplus.org/29580558/diff/29580599/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29580599/compiled/filter/ElemHideBase.cpp#newcode150 compiled/filter/ElemHideBase.cpp:150: result.append(str.data() + start, i - start); This is performing ...
2 years, 1 month ago (2017-10-17 10:24:34 UTC) #3
hub
https://codereview.adblockplus.org/29580558/diff/29580599/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29580599/compiled/filter/ElemHideBase.cpp#newcode150 compiled/filter/ElemHideBase.cpp:150: result.append(str.data() + start, i - start); On 2017/10/17 10:24:33, ...
2 years, 1 month ago (2017-10-17 19:22:18 UTC) #4
Wladimir Palant
https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/ElemHideBase.cpp#newcode121 compiled/filter/ElemHideBase.cpp:121: namespace { Please put the opening curly on the ...
2 years, 1 month ago (2017-10-18 09:24:03 UTC) #5
hub
https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/ElemHideBase.cpp#newcode121 compiled/filter/ElemHideBase.cpp:121: namespace { On 2017/10/18 09:24:02, Wladimir Palant wrote: > ...
2 years, 1 month ago (2017-10-18 12:53:48 UTC) #6
sergei
LGTM https://codereview.adblockplus.org/29580558/diff/29582624/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29582624/compiled/filter/ElemHideBase.cpp#newcode128 compiled/filter/ElemHideBase.cpp:128: static constexpr String::size_type CURLY_REPLACEMENT_SIZE = sizeof(OPENING_CURLY_REPLACEMENT) / sizeof(OPENING_CURLY_REPLACEMENT[0]) ...
2 years, 1 month ago (2017-10-18 14:01:37 UTC) #7
hub
Fixed following. https://issues.adblockplus.org/ticket/5174#comment
2 years, 1 month ago (2017-10-18 18:38:34 UTC) #8
hub
On 2017/10/18 18:38:34, hub wrote: > Fixed following. > > https://issues.adblockplus.org/ticket/5174#comment I meant https://issues.adblockplus.org/ticket/5174#comment:7
2 years, 1 month ago (2017-10-18 18:44:06 UTC) #9
sergei
LGTM
2 years, 1 month ago (2017-10-18 19:21:42 UTC) #10
Wladimir Palant
https://codereview.adblockplus.org/29580558/diff/29582685/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29582685/compiled/filter/ElemHideBase.cpp#newcode125 compiled/filter/ElemHideBase.cpp:125: Nit: This newline is unnecessary. And the block contents ...
2 years, 1 month ago (2017-10-19 07:59:28 UTC) #11
hub
https://codereview.adblockplus.org/29580558/diff/29582685/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29582685/compiled/filter/ElemHideBase.cpp#newcode125 compiled/filter/ElemHideBase.cpp:125: On 2017/10/19 07:59:28, Wladimir Palant wrote: > Nit: This ...
2 years, 1 month ago (2017-10-19 12:48:26 UTC) #12
sergei
https://codereview.adblockplus.org/29580558/diff/29583579/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29583579/compiled/filter/ElemHideBase.cpp#newcode132 compiled/filter/ElemHideBase.cpp:132: OwnedString result(str.length() + replacementCount * (CURLY_REPLACEMENT_SIZE - 1)); I ...
1 year, 12 months ago (2017-11-21 11:42:10 UTC) #13
hub
https://codereview.adblockplus.org/29580558/diff/29583579/compiled/filter/ElemHideBase.cpp File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29583579/compiled/filter/ElemHideBase.cpp#newcode132 compiled/filter/ElemHideBase.cpp:132: OwnedString result(str.length() + replacementCount * (CURLY_REPLACEMENT_SIZE - 1)); On ...
1 year, 12 months ago (2017-11-21 15:58:21 UTC) #14
sergei
1 year, 12 months ago (2017-11-21 16:18:00 UTC) #15
LGTM

https://codereview.adblockplus.org/29580558/diff/29583579/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29580558/diff/29583579/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:132: OwnedString result(str.length() +
replacementCount * (CURLY_REPLACEMENT_SIZE - 1));
On 2017/11/21 15:58:21, hub wrote:
> On 2017/11/21 11:42:10, sergei wrote:
> > I think we should not subtract 1 here because it's already done above.
> 
> the -1 is because we are replacing. str.length() already counts for the '{' or
> '}' as CURLY_REPLACEMENT_SIZE is the length of the string (sizeof() returns
byte
> size with the terminating 0 hence the -1) which is 4. So we end up just adding
3
> per replacement.

Acknowledged.
Sign in to reply to this message.

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