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

Issue 29361640: Issue 4593 - Support regular expressions for CSS property matching (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 11 months ago by Felix Dahlke
Modified:
2 years, 11 months ago
Reviewers:
kzar, Wladimir Palant
Base URL:
https://bitbucket.org/fhd/adblockpluscore
Visibility:
Public.

Description

Issue 4593 - Support regular expressions for CSS property matching

Patch Set 1 #

Total comments: 14

Patch Set 2 : Use String.prototype.startsWith/.endsWith #

Total comments: 3

Patch Set 3 : Don't detect '/' as a regexp and speed up regexp detection #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -1 line) Patch
M lib/filterClasses.js View 1 2 1 chunk +6 lines, -1 line 1 comment Download
M test/filterClasses.js View 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 24
Felix Dahlke
2 years, 11 months ago (2016-11-03 11:40:27 UTC) #1
kzar
Really cool idea, mostly looks good. Did you already test that it works for real ...
2 years, 11 months ago (2016-11-03 12:18:16 UTC) #2
kzar
I guess as well we should update the documentation if this lands. (I can update ...
2 years, 11 months ago (2016-11-03 12:21:26 UTC) #3
kzar
I just thought what will older versions of Adblock Plus would do with these filters? ...
2 years, 11 months ago (2016-11-03 13:21:57 UTC) #4
Felix Dahlke
On 2016/11/03 12:18:16, kzar wrote: > Really cool idea, mostly looks good. Did you already ...
2 years, 11 months ago (2016-11-03 13:47:51 UTC) #5
kzar
> Yup, that's what would happen. We did explicitly say that > this syntax is ...
2 years, 11 months ago (2016-11-03 16:03:19 UTC) #6
Felix Dahlke
New patch set is up. https://codereview.adblockplus.org/29361640/diff/29361641/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361640/diff/29361641/lib/filterClasses.js#newcode1046 lib/filterClasses.js:1046: if (this.regexpSource.charAt(0) == "/" ...
2 years, 11 months ago (2016-11-03 19:08:02 UTC) #7
kzar
https://codereview.adblockplus.org/29361640/diff/29361641/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361640/diff/29361641/lib/filterClasses.js#newcode1048 lib/filterClasses.js:1048: regexp = this.regexpSource.slice(1, -1); On 2016/11/03 19:08:02, Felix Dahlke ...
2 years, 11 months ago (2016-11-04 10:06:56 UTC) #8
Felix Dahlke
https://codereview.adblockplus.org/29361640/diff/29361641/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361640/diff/29361641/lib/filterClasses.js#newcode1048 lib/filterClasses.js:1048: regexp = this.regexpSource.slice(1, -1); On 2016/11/04 10:06:56, kzar wrote: ...
2 years, 11 months ago (2016-11-04 13:43:56 UTC) #9
kzar
https://codereview.adblockplus.org/29361640/diff/29361641/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361640/diff/29361641/lib/filterClasses.js#newcode1048 lib/filterClasses.js:1048: regexp = this.regexpSource.slice(1, -1); On 2016/11/04 13:43:56, Felix Dahlke ...
2 years, 11 months ago (2016-11-04 14:29:50 UTC) #10
Felix Dahlke
https://codereview.adblockplus.org/29361640/diff/29361641/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361640/diff/29361641/lib/filterClasses.js#newcode1048 lib/filterClasses.js:1048: regexp = this.regexpSource.slice(1, -1); On 2016/11/04 14:29:50, kzar wrote: ...
2 years, 11 months ago (2016-11-04 16:11:44 UTC) #11
kzar
https://codereview.adblockplus.org/29361640/diff/29361641/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361640/diff/29361641/lib/filterClasses.js#newcode1048 lib/filterClasses.js:1048: regexp = this.regexpSource.slice(1, -1); On 2016/11/04 16:11:44, Felix Dahlke ...
2 years, 11 months ago (2016-11-04 16:18:27 UTC) #12
Felix Dahlke
Here's another try, but if we can't manage to agree let's just wait for Wladimir... ...
2 years, 11 months ago (2016-11-04 16:51:16 UTC) #13
kzar
On 2016/11/04 16:51:16, Felix Dahlke wrote: > Here's another try, but if we can't manage ...
2 years, 11 months ago (2016-11-04 16:53:20 UTC) #14
kzar
On 2016/11/04 16:53:20, kzar wrote: > Yea, sounds good to me, we're going in circles. ...
2 years, 11 months ago (2016-11-04 16:58:46 UTC) #15
Felix Dahlke
On 2016/11/04 16:58:46, kzar wrote: > On 2016/11/04 16:53:20, kzar wrote: > > Yea, sounds ...
2 years, 11 months ago (2016-11-04 17:03:01 UTC) #16
kzar
(So other than the case insensitive stuff this LGTM, I don't want to block this ...
2 years, 11 months ago (2016-11-04 17:21:52 UTC) #17
Felix Dahlke
On 2016/11/04 17:21:52, kzar wrote: > (So other than the case insensitive stuff this LGTM, ...
2 years, 11 months ago (2016-11-04 21:42:01 UTC) #18
Felix Dahlke
On 2016/11/04 21:42:01, Felix Dahlke wrote: > On 2016/11/04 17:21:52, kzar wrote: > > (So ...
2 years, 11 months ago (2016-11-04 21:58:42 UTC) #19
Wladimir Palant
Concerning case-insensitive matching: I agree with Felix that this is an entirely unrelated change and ...
2 years, 11 months ago (2016-11-08 09:56:42 UTC) #20
Felix Dahlke
https://codereview.adblockplus.org/29361640/diff/29361726/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361640/diff/29361726/lib/filterClasses.js#newcode1046 lib/filterClasses.js:1046: if (this.regexpSource.startsWith("/") && this.regexpSource.endsWith("/")) On 2016/11/08 09:56:41, Wladimir Palant ...
2 years, 11 months ago (2016-11-08 15:46:36 UTC) #21
Felix Dahlke
New patch set is up. https://codereview.adblockplus.org/29361640/diff/29361726/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361640/diff/29361726/lib/filterClasses.js#newcode1046 lib/filterClasses.js:1046: if (this.regexpSource.startsWith("/") && this.regexpSource.endsWith("/")) ...
2 years, 11 months ago (2016-11-08 17:28:39 UTC) #22
Wladimir Palant
LGTM https://codereview.adblockplus.org/29361640/diff/29362058/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29361640/diff/29362058/lib/filterClasses.js#newcode1047 lib/filterClasses.js:1047: this.regexpSource[this.regexpSource.length - 1] == "/") Well, accessing this.regexpSource ...
2 years, 11 months ago (2016-11-08 17:41:34 UTC) #23
kzar
2 years, 11 months ago (2016-11-08 19:34:51 UTC) #24
Fair enough, LGTM
Sign in to reply to this message.

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