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

Issue 29746563: Noissue - Trim spaces around element hiding emulation selectors (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 1 week ago by Manish Jethani
Modified:
3 months ago
Reviewers:
kzar, hub
CC:
sergei
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Patch Set 1 #

Total comments: 4

Patch Set 2 : Add tests #

Total comments: 4

Patch Set 3 : Update elemhideRegExp #

Patch Set 4 : Revert Patch Set 3 #

Patch Set 5 : Add tests for incorrect syntax #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -1 line) Patch
M lib/filterClasses.js View 3 1 chunk +1 line, -1 line 0 comments Download
M test/filterClasses.js View 1 2 3 4 1 chunk +25 lines, -0 lines 0 comments Download

Messages

Total messages: 16
Manish Jethani
3 months, 1 week ago (2018-04-08 09:35:32 UTC) #1
Manish Jethani
Patch Set 1 https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.js#newcode191 lib/filterClasses.js:191: let [, domain, separator, selector] = ...
3 months, 1 week ago (2018-04-08 09:40:09 UTC) #2
hub
test/filterClasses.js should be updated to test this.
3 months, 1 week ago (2018-04-09 04:03:10 UTC) #3
kzar
Well spotted Manish. > test/filterClasses.js should be updated to test this. Agreed. https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.js File lib/filterClasses.js ...
3 months, 1 week ago (2018-04-09 10:38:30 UTC) #4
hub
https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.js#newcode191 lib/filterClasses.js:191: let [, domain, separator, selector] = /^(.*?)(#[@?]?#?)(.*)$/.exec(text); On 2018/04/09 ...
3 months, 1 week ago (2018-04-09 15:02:07 UTC) #5
kzar
https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29746564/lib/filterClasses.js#newcode191 lib/filterClasses.js:191: let [, domain, separator, selector] = /^(.*?)(#[@?]?#?)(.*)$/.exec(text); On 2018/04/09 ...
3 months, 1 week ago (2018-04-09 15:29:46 UTC) #6
Manish Jethani
Patch Set 2: Add tests On 2018/04/09 04:03:10, hub wrote: > test/filterClasses.js should be updated ...
3 months, 1 week ago (2018-04-10 12:59:11 UTC) #7
hub
LGTM
3 months, 1 week ago (2018-04-10 14:04:48 UTC) #8
kzar
https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.js#newcode435 test/filterClasses.js:435: test.equal(Filter.normalize(" domain.c om#?# # sele ctor "), What about ...
3 months, 1 week ago (2018-04-11 11:29:49 UTC) #9
Manish Jethani
Patch Set 3: Update elemhideRegExp https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.js#newcode435 test/filterClasses.js:435: test.equal(Filter.normalize(" domain.c om#?# # ...
3 months, 1 week ago (2018-04-11 11:50:19 UTC) #10
kzar
Copying in Sergei since we're now talking about filter syntax. https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.js#newcode435 ...
3 months, 1 week ago (2018-04-11 12:04:10 UTC) #11
Manish Jethani
Patch Set 4 https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29746563/diff/29748555/test/filterClasses.js#newcode435 test/filterClasses.js:435: test.equal(Filter.normalize(" domain.c om#?# # sele ctor ...
3 months, 1 week ago (2018-04-11 17:34:54 UTC) #12
hub
LGTM (since technically it hasn't changed since last time)
3 months, 1 week ago (2018-04-12 07:18:34 UTC) #13
kzar
On 2018/04/11 17:34:54, Manish Jethani wrote: > Therefore the test you suggested is not valid, ...
3 months ago (2018-04-18 11:17:44 UTC) #14
Manish Jethani
Patch Set 5: Add tests for incorrect syntax Note that the behavior between "# ?##" ...
3 months ago (2018-04-18 12:43:24 UTC) #15
kzar
3 months ago (2018-04-19 08:35:17 UTC) #16
Nice thanks, I'm really happy with this change now. LGTM.
Sign in to reply to this message.

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