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

Issue 29613805: Issue 6034 - :-abp-contains() accept a regular expression (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months ago by hub
Modified:
2 months ago
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 6034 - :-abp-contains() accept a regular expression

Patch Set 1 #

Total comments: 4

Patch Set 2 : Use RegExp.test() #

Total comments: 9

Patch Set 3 : Addressed nit. (also rebased) #

Patch Set 4 : Fix a test breakage from previous change. Address jsdoc comment. #

Total comments: 10

Patch Set 5 : Do all matches with RegExp. #

Patch Set 6 : Parse the regexp flag. #

Total comments: 10

Patch Set 7 : Refine regexp handling. #

Patch Set 8 : Check regexp isn't null #

Patch Set 9 : Remove the escaping of curly braces for :-abp-contains() #

Total comments: 4

Patch Set 10 : Address the regexp "escaping" #

Total comments: 9

Patch Set 11 : Review comments #

Total comments: 10

Patch Set 12 : Some comments editing, #

Unified diffs Side-by-side diffs Delta from patch set Stats (+138 lines, -29 lines) Patch
M lib/common.js View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +12 lines, -0 lines 0 comments Download
M lib/content/elemHideEmulation.js View 1 2 3 4 5 6 7 8 9 10 4 chunks +26 lines, -3 lines 0 comments Download
M test/browser/elemHideEmulation.js View 1 2 3 4 5 6 7 8 9 3 chunks +100 lines, -26 lines 0 comments Download

Messages

Total messages: 31
hub
5 months ago (2017-11-21 21:00:14 UTC) #1
lainverse
https://codereview.adblockplus.org/29613805/diff/29613806/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29613806/lib/content/elemHideEmulation.js#newcode134 lib/content/elemHideEmulation.js:134: if (text.length >= 2 && text[0] == "/" && ...
5 months ago (2017-11-22 09:06:19 UTC) #2
hub
https://codereview.adblockplus.org/29613805/diff/29613806/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29613806/lib/content/elemHideEmulation.js#newcode134 lib/content/elemHideEmulation.js:134: if (text.length >= 2 && text[0] == "/" && ...
5 months ago (2017-11-22 14:34:59 UTC) #3
kzar
This LGTM but I'd like to get another pair of eyes on it as well ...
3 months, 2 weeks ago (2018-01-08 15:29:36 UTC) #4
kzar
https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHideEmulation.js#newcode247 lib/content/elemHideEmulation.js:247: this._regexp = new RegExp(regexpString); Probably a dumb question, but ...
3 months, 2 weeks ago (2018-01-08 15:35:02 UTC) #5
hub
https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHideEmulation.js#newcode128 lib/content/elemHideEmulation.js:128: * @return {string} an unescaped RegExp string. null if ...
3 months, 2 weeks ago (2018-01-08 16:29:52 UTC) #6
kzar
https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHideEmulation.js#newcode128 lib/content/elemHideEmulation.js:128: * @return {string} an unescaped RegExp string. null if ...
3 months, 2 weeks ago (2018-01-08 16:56:06 UTC) #7
hub
https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHideEmulation.js#newcode128 lib/content/elemHideEmulation.js:128: * @return {string} an unescaped RegExp string. null if ...
3 months, 2 weeks ago (2018-01-08 17:53:13 UTC) #8
Manish Jethani
https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHideEmulation.js#newcode127 lib/content/elemHideEmulation.js:127: * @param {string} text argument. This should be: @param ...
3 months, 2 weeks ago (2018-01-09 07:53:03 UTC) #9
lainverse
https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHideEmulation.js#newcode247 lib/content/elemHideEmulation.js:247: this._regexp = new RegExp(regexpString); I think it should be ...
3 months, 2 weeks ago (2018-01-09 10:49:23 UTC) #10
lainverse
On 2018/01/09 10:49:23, lainverse wrote: > ...and it looks like "string.includes()" 30~40 percent slower both ...
3 months, 2 weeks ago (2018-01-09 15:18:10 UTC) #11
hub
https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHideEmulation.js#newcode127 lib/content/elemHideEmulation.js:127: * @param {string} text argument. On 2018/01/09 07:53:02, Manish ...
3 months, 2 weeks ago (2018-01-09 18:33:06 UTC) #12
hub
Update patch. It doesn't currently parse the regexp to allow flags. Assumes /regexp/ format.
3 months, 2 weeks ago (2018-01-09 22:26:44 UTC) #13
hub
This also takes into account: https://issues.adblockplus.org/ticket/6034#comment:9 Should be good now for review.
3 months, 2 weeks ago (2018-01-10 06:03:11 UTC) #14
hub
https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHideEmulation.js#newcode247 lib/content/elemHideEmulation.js:247: this._regexp = new RegExp(regexpString); On 2018/01/08 16:29:51, hub wrote: ...
3 months, 2 weeks ago (2018-01-10 06:05:17 UTC) #15
lainverse
https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHideEmulation.js#newcode276 lib/content/elemHideEmulation.js:276: yield null; BTW, while we are at it why ...
3 months, 2 weeks ago (2018-01-10 09:14:02 UTC) #16
Manish Jethani
https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHideEmulation.js#newcode138 lib/content/elemHideEmulation.js:138: .replace("\\7B ", "{").replace("\\7D ", "}"); On 2018/01/09 18:33:06, hub ...
3 months, 2 weeks ago (2018-01-10 11:22:15 UTC) #17
hub
https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHideEmulation.js#newcode125 lib/content/elemHideEmulation.js:125: const regexpRegexp = /^\/(.*)\/([gimuy]*)$/; On 2018/01/10 11:22:15, Manish Jethani ...
3 months, 2 weeks ago (2018-01-10 18:27:22 UTC) #18
lainverse
https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHideEmulation.js#newcode276 lib/content/elemHideEmulation.js:276: yield null; On 2018/01/10 09:14:02, lainverse wrote: > BTW, ...
3 months, 2 weeks ago (2018-01-10 19:10:22 UTC) #19
Manish Jethani
https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHideEmulation.js#newcode138 lib/content/elemHideEmulation.js:138: .replace("\\7B ", "{").replace("\\7D ", "}"); On 2018/01/09 18:33:06, hub ...
3 months, 1 week ago (2018-01-16 12:00:22 UTC) #20
hub
Removed the escaping for :-abp-contains().
2 months, 2 weeks ago (2018-02-08 17:26:05 UTC) #21
Manish Jethani
On 2018/02/08 17:26:05, hub wrote: > Removed the escaping for :-abp-contains(). Sorry about the long ...
2 months ago (2018-02-20 10:41:07 UTC) #22
Manish Jethani
https://codereview.adblockplus.org/29613805/diff/29692566/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29692566/lib/content/elemHideEmulation.js#newcode180 lib/content/elemHideEmulation.js:180: * @param {boolean} escape indicate whether we escape curly ...
2 months ago (2018-02-20 10:47:10 UTC) #23
hub
https://codereview.adblockplus.org/29613805/diff/29692566/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29692566/lib/content/elemHideEmulation.js#newcode180 lib/content/elemHideEmulation.js:180: * @param {boolean} escape indicate whether we escape curly ...
2 months ago (2018-02-20 17:50:18 UTC) #24
Manish Jethani
https://codereview.adblockplus.org/29613805/diff/29703629/lib/common.js File lib/common.js (right): https://codereview.adblockplus.org/29613805/diff/29703629/lib/common.js#newcode20 lib/common.js:20: /** I would call this function "textToRegExp" to match ...
2 months ago (2018-02-21 14:41:59 UTC) #25
Manish Jethani
https://codereview.adblockplus.org/29613805/diff/29703629/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29703629/lib/content/elemHideEmulation.js#newcode188 lib/content/elemHideEmulation.js:188: if (match) On 2018/02/21 14:41:59, Manish Jethani wrote: > ...
2 months ago (2018-02-21 14:46:39 UTC) #26
hub
https://codereview.adblockplus.org/29613805/diff/29703629/lib/common.js File lib/common.js (right): https://codereview.adblockplus.org/29613805/diff/29703629/lib/common.js#newcode20 lib/common.js:20: /** On 2018/02/21 14:41:59, Manish Jethani wrote: > I ...
2 months ago (2018-02-21 21:38:01 UTC) #27
Manish Jethani
https://codereview.adblockplus.org/29613805/diff/29704609/lib/common.js File lib/common.js (right): https://codereview.adblockplus.org/29613805/diff/29704609/lib/common.js#newcode21 lib/common.js:21: * Convert raw text into a regular expression string ...
2 months ago (2018-02-22 07:08:28 UTC) #28
hub
https://codereview.adblockplus.org/29613805/diff/29704609/lib/common.js File lib/common.js (right): https://codereview.adblockplus.org/29613805/diff/29704609/lib/common.js#newcode21 lib/common.js:21: * Convert raw text into a regular expression string ...
2 months ago (2018-02-22 14:07:50 UTC) #29
Manish Jethani
LGTM https://codereview.adblockplus.org/29613805/diff/29704609/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29704609/lib/content/elemHideEmulation.js#newcode185 lib/content/elemHideEmulation.js:185: regexpRegexp.exec(text) || [undefined, textToRegExp(text)]; On 2018/02/22 14:07:50, hub ...
2 months ago (2018-02-22 15:21:56 UTC) #30
Manish Jethani
2 months ago (2018-02-22 15:37:10 UTC) #31
https://codereview.adblockplus.org/29613805/diff/29704609/lib/content/elemHid...
File lib/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29613805/diff/29704609/lib/content/elemHid...
lib/content/elemHideEmulation.js:185: regexpRegexp.exec(text) || [undefined,
textToRegExp(text)];
On 2018/02/22 15:21:55, Manish Jethani wrote:
> On 2018/02/22 14:07:50, hub wrote:
> > On 2018/02/22 07:08:28, Manish Jethani wrote:
> > > The undefined here is unnecessary, it can be left out just like on the
left
> > hand
> > > side. Right?
> > 
> > eslint complains. "no sparse array", so I have to have something.
> 
> OK, if you feel like, you can change it to:
> 
>   let match = regexpRegexp.exec(text);
>   let [pattern, flags] = match ? match.slice(1) : [textToRegExp(text)];
> 
> But I'll also file an issue for changing the ESList config to allow this
> pattern.

I have filed an issue for this:

https://issues.adblockplus.org/ticket/6411
Sign in to reply to this message.

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