Issue 29613805: Issue 6034 - :-abp-contains() accept a regular expression (Closed)
|
Created:
Nov. 21, 2017, 9 p.m. by hub Modified:
Feb. 22, 2018, 5:39 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 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, #
MessagesTotal messages: 31
https://codereview.adblockplus.org/29613805/diff/29613806/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29613806/lib/content/elemHid... lib/content/elemHideEmulation.js:134: if (text.length >= 2 && text[0] == "/" && Technically // is not a valid regexp, even though you can call new RegExp() with an empty string as an argument. I'd use "> 2" instead of ">= 2" here. https://codereview.adblockplus.org/29613805/diff/29613806/lib/content/elemHid... lib/content/elemHideEmulation.js:258: return element.textContent.match(this._regexp); Might be better to use this._regexp.test() since it returns boolean true/false.
https://codereview.adblockplus.org/29613805/diff/29613806/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29613806/lib/content/elemHid... lib/content/elemHideEmulation.js:134: if (text.length >= 2 && text[0] == "/" && On 2017/11/22 09:06:19, lainverse wrote: > Technically // is not a valid regexp, even though you can call new RegExp() with > an empty string as an argument. I'd use "> 2" instead of ">= 2" here. this is just what was done before for :-abp-properties(). https://codereview.adblockplus.org/29613805/diff/29613806/lib/content/elemHid... lib/content/elemHideEmulation.js:258: return element.textContent.match(this._regexp); On 2017/11/22 09:06:19, lainverse wrote: > Might be better to use this._regexp.test() since it returns boolean true/false. good point.
This LGTM but I'd like to get another pair of eyes on it as well before you push, please could you take a look Manish? https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... lib/content/elemHideEmulation.js:128: * @return {string} an unescaped RegExp string. null if it is not a Shouldn't this be `{?string}`? https://codereview.adblockplus.org/29613805/diff/29615623/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/test/browser/elemHi... test/browser/elemHideEmulation.js:444: function runTestPseudClassContains(test, selector, expectations) Nit: Typo "Pseudo"
https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... lib/content/elemHideEmulation.js:247: this._regexp = new RegExp(regexpString); Probably a dumb question, but how come with the PropsSelector constructor we use filterToRegExp to convert the text into a RegExp if it isn't already one, but with the ContainsSelector we're instead storing the text?
https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... lib/content/elemHideEmulation.js:128: * @return {string} an unescaped RegExp string. null if it is not a On 2018/01/08 15:29:35, kzar wrote: > Shouldn't this be `{?string}`? The reference I use for JSDoc doesn't mention this syntax. http://usejsdoc.org/tags-returns.html Nor have I seen it used. https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... lib/content/elemHideEmulation.js:247: this._regexp = new RegExp(regexpString); On 2018/01/08 15:35:01, kzar wrote: > Probably a dumb question, but how come with the PropsSelector constructor we use > filterToRegExp to convert the text into a RegExp if it isn't already one, but > with the ContainsSelector we're instead storing the text? PropsSelector search for the CSS properties using regexp. So we have to construct a Regexp from a plain text string if needed. ContainsSelector doesn't use Regexp if it can avoid it (a simple substring match), so we take a different approach. https://codereview.adblockplus.org/29613805/diff/29615623/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/test/browser/elemHi... test/browser/elemHideEmulation.js:444: function runTestPseudClassContains(test, selector, expectations) On 2018/01/08 15:29:35, kzar wrote: > Nit: Typo "Pseudo" Done.
https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... lib/content/elemHideEmulation.js:128: * @return {string} an unescaped RegExp string. null if it is not a On 2018/01/08 16:29:51, hub wrote: > On 2018/01/08 15:29:35, kzar wrote: > > Shouldn't this be `{?string}`? > > The reference I use for JSDoc doesn't mention this syntax. > http://usejsdoc.org/tags-returns.html > > Nor have I seen it used. Well the page you linked says this "@returns [{type}] [description]" and the page which documents the {type} tag[1] mentions a Nullable Type which uses the "?" prefix syntax. (Check it still lints though.) [1] - http://usejsdoc.org/tags-type.html
https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... lib/content/elemHideEmulation.js:128: * @return {string} an unescaped RegExp string. null if it is not a On 2018/01/08 16:56:06, kzar wrote: > On 2018/01/08 16:29:51, hub wrote: > > On 2018/01/08 15:29:35, kzar wrote: > > > Shouldn't this be `{?string}`? > > > > The reference I use for JSDoc doesn't mention this syntax. > > http://usejsdoc.org/tags-returns.html > > > > Nor have I seen it used. > > Well the page you linked says this "@returns [{type}] [description]" and the > page which documents the {type} tag[1] mentions a Nullable Type which uses the > "?" prefix syntax. (Check it still lints though.) > > [1] - http://usejsdoc.org/tags-type.html Done
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:127: * @param {string} text argument. This should be: @param {string} text the text argument Right? https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHid... lib/content/elemHideEmulation.js:138: .replace("\\7B ", "{").replace("\\7D ", "}"); Why is this needed? If it's for legacy reasons, maybe we can drop it now for -abp-contains https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHid... lib/content/elemHideEmulation.js:247: this._regexp = new RegExp(regexpString); We have to consider what happens if the regular expression is invalid. For example /(foo/ which contains an unterminated group. There are some options for how to deal with this, but I think we should just catch the error here and set this._regexp to null. Since this._text is null too, it'll evaluate to false in this.match.
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:247: this._regexp = new RegExp(regexpString); I think it should be either "RegExp(regexpString, 'i')" to be case-insensitive or checkRegExpParameter should support regexp flags defined right in the filter and return new RegExp instead of just a string. https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHid... lib/content/elemHideEmulation.js:260: return element.textContent.includes(this._text); I wasn't quite sure that "string.includes()" actually better than "regexp.test()" and made this test to check: https://jsperf.com/compare-regexp-test-and-string-include/1 ...and it looks like "string.includes()" 30~40 percent slower both in Google Chrome 63 and Firefox 58 DE on my system. So, it might be a good idea to construct new RegExp even from a simple string and use .test() in all cases. And drop this function.
On 2018/01/09 10:49:23, lainverse wrote: > ...and it looks like "string.includes()" 30~40 percent slower both in Google > Chrome 63 and Firefox 58 DE on my system. Usually difference is less prominent in Firefox and ends up around 10~20 percent. Still in favor of regexp.test(), though.
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:127: * @param {string} text argument. On 2018/01/09 07:53:02, Manish Jethani wrote: > This should be: > > @param {string} text the text argument > > Right? Done. https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHid... lib/content/elemHideEmulation.js:138: .replace("\\7B ", "{").replace("\\7D ", "}"); 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. https://codereview.adblockplus.org/29613805/diff/29659612/lib/content/elemHid... lib/content/elemHideEmulation.js:260: return element.textContent.includes(this._text); On 2018/01/09 10:49:22, lainverse wrote: > I wasn't quite sure that "string.includes()" actually better than > "regexp.test()" and made this test to check: > https://jsperf.com/compare-regexp-test-and-string-include/1 > > ...and it looks like "string.includes()" 30~40 percent slower both in Google > Chrome 63 and Firefox 58 DE on my system. > > So, it might be a good idea to construct new RegExp even from a simple string > and use .test() in all cases. And drop this function. I'll rewrite this part taking the comments above into account aswell. Looks like I'll use regexp matching even for plain text.
Update patch. It doesn't currently parse the regexp to allow flags. Assumes /regexp/ format.
This also takes into account: https://issues.adblockplus.org/ticket/6034#comment:9 Should be good now for review.
https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29615623/lib/content/elemHid... lib/content/elemHideEmulation.js:247: this._regexp = new RegExp(regexpString); On 2018/01/08 16:29:51, hub wrote: > On 2018/01/08 15:35:01, kzar wrote: > > Probably a dumb question, but how come with the PropsSelector constructor we > use > > filterToRegExp to convert the text into a RegExp if it isn't already one, but > > with the ContainsSelector we're instead storing the text? > > PropsSelector search for the CSS properties using regexp. So we have to > construct a Regexp from a plain text string if needed. > ContainsSelector doesn't use Regexp if it can avoid it (a simple substring > match), so we take a different approach. The newer revision of the patch changed this as regexp are faster than String.includes(). So yes we use filterToRegexp in both case.
https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... lib/content/elemHideEmulation.js:276: yield null; BTW, while we are at it why do we even want to yield null? As I understand makeSelector will skip it anyway. Similar yield in the other getElements function.
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. Acknowledged. https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... lib/content/elemHideEmulation.js:125: const regexpRegexp = /^\/(.*)\/([gimuy]*)$/; We don't have to support all these flags, some of them don't make any sense anyway. g - no need, we only want one match anyway i - OK m - OK u - no need, Unicode is supported in filters, i.e. you can use a Unicode character directly instead of using sequences like "\u{1F47B}" y - doesn't make sense for a filter Also see: https://github.com/gorhill/uBlock/issues/3372#issuecomment-354576078 https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... lib/content/elemHideEmulation.js:135: function makeRegExpParameter(text, flag) Shouldn't the parameter be called "flags" (plural)? https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... lib/content/elemHideEmulation.js:142: if (!flag) This changes the syntax of PropsSelector so that now it supports flags even though the flags are ignored (always "i"). Instead we should just use two separate regular expressions for ContainsSelector and PropsSelector. https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... lib/content/elemHideEmulation.js:148: return new RegExp(regexpString, flag); We still need to handle the error if any.
https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... lib/content/elemHideEmulation.js:125: const regexpRegexp = /^\/(.*)\/([gimuy]*)$/; On 2018/01/10 11:22:15, Manish Jethani wrote: > We don't have to support all these flags, some of them don't make any sense > anyway. > > g - no need, we only want one match anyway > i - OK > m - OK > u - no need, Unicode is supported in filters, i.e. you can use a Unicode > character directly instead of using sequences like "\u{1F47B}" > y - doesn't make sense for a filter > > Also see: > > https://github.com/gorhill/uBlock/issues/3372#issuecomment-354576078 let's restrict to "im" for the flags. https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... lib/content/elemHideEmulation.js:135: function makeRegExpParameter(text, flag) On 2018/01/10 11:22:14, Manish Jethani wrote: > Shouldn't the parameter be called "flags" (plural)? Done. https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... lib/content/elemHideEmulation.js:142: if (!flag) On 2018/01/10 11:22:15, Manish Jethani wrote: > This changes the syntax of PropsSelector so that now it supports flags even > though the flags are ignored (always "i"). Instead we should just use two > separate regular expressions for ContainsSelector and PropsSelector. It is just more permissive to allow the regexp syntax with flags, which will be ignored anyway. I don't think we need to make things more complex by having more RegExp to parse. https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... lib/content/elemHideEmulation.js:148: return new RegExp(regexpString, flag); On 2018/01/10 11:22:14, Manish Jethani wrote: > We still need to handle the error if any. Done.
https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29661555/lib/content/elemHid... lib/content/elemHideEmulation.js:276: yield null; On 2018/01/10 09:14:02, lainverse wrote: > BTW, while we are at it why do we even want to yield null? As I understand > makeSelector will skip it anyway. Similar yield in the other getElements > function. Checked. makeSelectors returns null if null is passed as selector (and that's what getSelectors does), then getSelectors yields an array [null, subtree], "evaluate" yields null when encounters null there later on and processPatterns skips all null selectors because what else could it do with them anyway? I can make an assumption that on some sites it leads to hundreds of useless calls just to do one or two right.
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
Removed the escaping for :-abp-contains().
On 2018/02/08 17:26:05, hub wrote: > Removed the escaping for :-abp-contains(). Sorry about the long delay. Has this been rebased? It would have been nice if the rebase was a separate patch set on its own (then we can get the actual diff between two patch sets).
https://codereview.adblockplus.org/29613805/diff/29692566/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29692566/lib/content/elemHid... lib/content/elemHideEmulation.js:180: * @param {boolean} escape indicate whether we escape curly braces. escape could be made optional ("?boolean"), then there's no need to pass a value for the -abp-contains case below. https://codereview.adblockplus.org/29613805/diff/29692566/lib/content/elemHid... lib/content/elemHideEmulation.js:197: regexpString = filterToRegExp(text); This doesn't make sense for the contains filter, does it? It changes the behavior even. Previously "Adblock*" would not match the string "Adblock Plus" but rather only the literal string "Adblock*", but now it does. If there's no slashes around the text, then it's not a regular expression, nor is it an ABP filter. We should either fall back to a text search like before or convert it to a regular expression without any special parsing. e.g. https://gist.github.com/mjethani/93a03a26abc557a4b365 So if it doesn't match regexpRegexp then it should just be regexEscape(text).
https://codereview.adblockplus.org/29613805/diff/29692566/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29692566/lib/content/elemHid... lib/content/elemHideEmulation.js:180: * @param {boolean} escape indicate whether we escape curly braces. On 2018/02/20 10:47:10, Manish Jethani wrote: > escape could be made optional ("?boolean"), then there's no need to pass a value > for the -abp-contains case below. Good point, but this is moot in the current patch. https://codereview.adblockplus.org/29613805/diff/29692566/lib/content/elemHid... lib/content/elemHideEmulation.js:197: regexpString = filterToRegExp(text); On 2018/02/20 10:47:10, Manish Jethani wrote: > This doesn't make sense for the contains filter, does it? It changes the > behavior even. Previously "Adblock*" would not match the string "Adblock Plus" > but rather only the literal string "Adblock*", but now it does. If there's no > slashes around the text, then it's not a regular expression, nor is it an ABP > filter. We should either fall back to a text search like before or convert it to > a regular expression without any special parsing. > > e.g. https://gist.github.com/mjethani/93a03a26abc557a4b365 > > So if it doesn't match regexpRegexp then it should just be regexEscape(text). You are totally right. The new patch fixes that.
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#newco... lib/common.js:20: /** I would call this function "textToRegExp" to match "filterToRegExp" It's just like the latter, but instead of treating its input as an ABP filter it just treats it as a normal string with no special meaning. Also then we can rename the parameter from "s" to "text". The comment could read "Converts raw text into regular expression string", and basically s/filter/raw/g in the comment. https://codereview.adblockplus.org/29613805/diff/29703629/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29703629/lib/content/elemHid... lib/content/elemHideEmulation.js:180: * it overrides flags passed in the text argument. This line got left out. https://codereview.adblockplus.org/29613805/diff/29703629/lib/content/elemHid... lib/content/elemHideEmulation.js:188: if (match) So we're having a discussion on #eyeo-js about modernizing our use of JS a little. This could be written as: let [, regexpString, flags] = regexpRegexp.exec(text) || [, escapeRegExp(text)]; That's 10 lines of code reduced to one. https://codereview.adblockplus.org/29613805/diff/29703629/lib/content/elemHid... lib/content/elemHideEmulation.js:351: if (regexp && regexp.test(style.style)) This check is no longer necessary? (i.e. this._regexp is always non-null)
https://codereview.adblockplus.org/29613805/diff/29703629/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29703629/lib/content/elemHid... lib/content/elemHideEmulation.js:188: if (match) On 2018/02/21 14:41:59, Manish Jethani wrote: > So we're having a discussion on #eyeo-js about modernizing our use of JS a > little. This could be written as: > > let [, regexpString, flags] = regexpRegexp.exec(text) || [, > escapeRegExp(text)]; > > That's 10 lines of code reduced to one. That spills over the 80 character line length, but we can just rename it to "pattern" which is what the RegExp documentation calls it anyway. So: let [, pattern, flags] = regexpRegexp.exec(text) || [, escapeRegExp(text)]; try { return new RegExp(pattern, flags); } catch (e) { } return null;
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#newco... lib/common.js:20: /** On 2018/02/21 14:41:59, Manish Jethani wrote: > I would call this function "textToRegExp" to match "filterToRegExp" > > It's just like the latter, but instead of treating its input as an ABP filter it > just treats it as a normal string with no special meaning. > > Also then we can rename the parameter from "s" to "text". > > The comment could read "Converts raw text into regular expression string", and > basically s/filter/raw/g in the comment. Done. https://codereview.adblockplus.org/29613805/diff/29703629/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29613805/diff/29703629/lib/content/elemHid... lib/content/elemHideEmulation.js:180: * it overrides flags passed in the text argument. On 2018/02/21 14:41:59, Manish Jethani wrote: > This line got left out. Done. https://codereview.adblockplus.org/29613805/diff/29703629/lib/content/elemHid... lib/content/elemHideEmulation.js:188: if (match) On 2018/02/21 14:46:38, Manish Jethani wrote: > On 2018/02/21 14:41:59, Manish Jethani wrote: > > So we're having a discussion on #eyeo-js about modernizing our use of JS a > > little. This could be written as: > > > > let [, regexpString, flags] = regexpRegexp.exec(text) || [, > > escapeRegExp(text)]; > > > > That's 10 lines of code reduced to one. > > That spills over the 80 character line length, but we can just rename it to > "pattern" which is what the RegExp documentation calls it anyway. > > So: > > let [, pattern, flags] = regexpRegexp.exec(text) || [, escapeRegExp(text)]; > try > { > return new RegExp(pattern, flags); > } > catch (e) > { > } > return null; Done. https://codereview.adblockplus.org/29613805/diff/29703629/lib/content/elemHid... lib/content/elemHideEmulation.js:351: if (regexp && regexp.test(style.style)) On 2018/02/21 14:41:59, Manish Jethani wrote: > This check is no longer necessary? (i.e. this._regexp is always non-null) Done.
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#newco... lib/common.js:21: * Convert raw text into a regular expression string Sorry, for consistency, can we say "Converts" instead of "Convert"? (Or let's change the comment for filterToRegExp, but let's be consistent.) https://codereview.adblockplus.org/29613805/diff/29704609/lib/common.js#newco... lib/common.js:22: * @param {string} text the string to convert. Nit: seems like we generally don't add periods at the end https://codereview.adblockplus.org/29613805/diff/29704609/lib/common.js#newco... lib/common.js:23: * @return {string} regular expression representation of string match. "string match" seems to imply something, do you think it might read better if we just called it "the text"? i.e. regular expression representation of the text 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)]; The undefined here is unnecessary, it can be left out just like on the left hand side. Right?
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#newco... lib/common.js:21: * Convert raw text into a regular expression string On 2018/02/22 07:08:27, Manish Jethani wrote: > Sorry, for consistency, can we say "Converts" instead of "Convert"? (Or let's > change the comment for filterToRegExp, but let's be consistent.) Done. https://codereview.adblockplus.org/29613805/diff/29704609/lib/common.js#newco... lib/common.js:22: * @param {string} text the string to convert. On 2018/02/22 07:08:27, Manish Jethani wrote: > Nit: seems like we generally don't add periods at the end Done. https://codereview.adblockplus.org/29613805/diff/29704609/lib/common.js#newco... lib/common.js:23: * @return {string} regular expression representation of string match. On 2018/02/22 07:08:28, Manish Jethani wrote: > "string match" seems to imply something, do you think it might read better if we > just called it "the text"? i.e. regular expression representation of the text Done. 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 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.
LGTM 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 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.
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 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
