Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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

Created:
April 8, 2018, 9:35 a.m. by Manish Jethani
Modified:
April 19, 2018, 9:34 a.m.
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
April 8, 2018, 9:35 a.m. (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] = ...
April 8, 2018, 9:40 a.m. (2018-04-08 09:40:09 UTC) #2
hub
test/filterClasses.js should be updated to test this.
April 9, 2018, 4:03 a.m. (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 ...
April 9, 2018, 10:38 a.m. (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 ...
April 9, 2018, 3:02 p.m. (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 ...
April 9, 2018, 3:29 p.m. (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 ...
April 10, 2018, 12:59 p.m. (2018-04-10 12:59:11 UTC) #7
hub
LGTM
April 10, 2018, 2:04 p.m. (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 ...
April 11, 2018, 11:29 a.m. (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#?# # ...
April 11, 2018, 11:50 a.m. (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 ...
April 11, 2018, 12:04 p.m. (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 ...
April 11, 2018, 5:34 p.m. (2018-04-11 17:34:54 UTC) #12
hub
LGTM (since technically it hasn't changed since last time)
April 12, 2018, 7:18 a.m. (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, ...
April 18, 2018, 11:17 a.m. (2018-04-18 11:17:44 UTC) #14
Manish Jethani
Patch Set 5: Add tests for incorrect syntax Note that the behavior between "# ?##" ...
April 18, 2018, 12:43 p.m. (2018-04-18 12:43:24 UTC) #15
kzar
April 19, 2018, 8:35 a.m. (2018-04-19 08:35:17 UTC) #16
Nice thanks, I'm really happy with this change now. LGTM.

Powered by Google App Engine
This is Rietveld