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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 4 weeks ago by hub
Modified:
3 days, 18 hours 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+107 lines, -39 lines) Patch
M lib/content/elemHideEmulation.js View 1 2 3 4 5 6 7 3 chunks +37 lines, -14 lines 0 comments Download
M test/browser/elemHideEmulation.js View 1 2 3 4 5 6 3 chunks +70 lines, -25 lines 0 comments Download

Messages

Total messages: 20
hub
1 month, 4 weeks 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] == "/" && ...
1 month, 4 weeks 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] == "/" && ...
1 month, 4 weeks 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 ...
1 week, 4 days 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 ...
1 week, 4 days 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 ...
1 week, 4 days 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 ...
1 week, 4 days 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 ...
1 week, 4 days 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 ...
1 week, 3 days 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 ...
1 week, 3 days 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 ...
1 week, 3 days 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 ...
1 week, 3 days 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.
1 week, 3 days 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.
1 week, 3 days 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: ...
1 week, 3 days 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 ...
1 week, 2 days 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 ...
1 week, 2 days 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 ...
1 week, 2 days 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, ...
1 week, 2 days ago (2018-01-10 19:10:22 UTC) #19
Manish Jethani
3 days, 18 hours ago (2018-01-16 12:00:22 UTC) #20
https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHid...
File lib/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHid...
lib/content/elemHideEmulation.js:138: .replace("\\7B ", "{").replace("\\7D ",
"}");
On 2018/01/09 18:33:06, hub wrote:
> On 2018/01/09 07:53:02, Manish Jethani wrote:
> > Why is this needed? If it's for legacy reasons, maybe we can drop it now for
> > -abp-contains
> 
> We still want to have a valid CSS selector syntax.

Why does this have to be valid CSS selector syntax, since we're not actually
generating a CSS selector here? Unlike -abp-has (which does take a selector),
this only takes a string.

Imagine CSS had a contains pseudo-class [1]:

  div:contains("Hello {}") {
    display: none !important;
  }

Even if the quotes were optional, the parser should have no trouble parsing the
"{" correctly. The escaping is only required for CSS identifiers, not for plain
strings. [2]

If we do support escape sequences, we should support them properly [3], though
like I said it shouldn't be required in this case. We can simply accept the
string as a JavaScript regular expression without any CSS escaping rules applied
to it.

For what it's worth, a backslash in a regular expression would have to be
escaped with another backslash, so we would be looking at "\\\\7B " rather than
"\\7B "

[1]:
https://www.w3.org/Style/CSS/Test/CSS3/Selectors/20011105/html/full/flat/css3...
[2]: https://www.w3.org/TR/css3-values/#string-value
[3]: https://mathiasbynens.be/notes/css-escapes
Sign in to reply to this message.

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