Issue 6868 - Enable capturing of surrounding wildcards for rewrite filters

[Manish Jethani, 2018-08-24 16:45:24]

Patch Set 1
Patch Set 2: Add comment in test file

[hub, 2018-08-28 22:02:55]

https://codereview.adblockplus.org/29863579/diff/29863584/lib/common.js
File lib/common.js (right):

https://codereview.adblockplus.org/29863579/diff/29863584/lib/common.js#newco...
lib/common.js:43: text = text.replace(/\*+/g, "*");
I think we should keep this one unconditionally like it was before, but run it
before this block.

[Manish Jethani, 2018-08-29 13:25:25]

https://codereview.adblockplus.org/29863579/diff/29863584/lib/common.js
File lib/common.js (right):

https://codereview.adblockplus.org/29863579/diff/29863584/lib/common.js#newco...
lib/common.js:43: text = text.replace(/\*+/g, "*");
On 2018/08/28 22:02:55, hub wrote:
> I think we should keep this one unconditionally like it was before, but run it
> before this block.

Since the `optimize` argument is supposed to skip all optimizations, shouldn't
we skip this one too? It doesn't make any difference to the matching behavior of
the regular expression, it only makes it match faster, and one can argue that
it's not important for rewrite filters (why would anyone even use two or more
consecutive "*").

[hub, 2018-08-29 16:23:50]

https://codereview.adblockplus.org/29863579/diff/29863584/lib/common.js
File lib/common.js (right):

https://codereview.adblockplus.org/29863579/diff/29863584/lib/common.js#newco...
lib/common.js:38: function filterToRegExp(text, optimize = true)
also, on second thought, I'm not certain that `optimize` is the best naming for
this option. Maybe "capture"? Since the use case is to capture what the regexp
match.

https://codereview.adblockplus.org/29863579/diff/29863584/lib/common.js#newco...
lib/common.js:43: text = text.replace(/\*+/g, "*");
On 2018/08/29 13:25:24, Manish Jethani wrote:
> On 2018/08/28 22:02:55, hub wrote:
> > I think we should keep this one unconditionally like it was before, but run
it
> > before this block.
> 
> Since the `optimize` argument is supposed to skip all optimizations, shouldn't
> we skip this one too? It doesn't make any difference to the matching behavior
of
> the regular expression, it only makes it match faster, and one can argue that
> it's not important for rewrite filters (why would anyone even use two or more
> consecutive "*").

this is more about correctness IMHO. There is an expectation to remove duplicate
'*'

[Manish Jethani, 2018-09-01 14:57:52]

Patch Set 3: Rebase
Patch Set 4: Rename to captureAll

https://codereview.adblockplus.org/29863579/diff/29863584/lib/common.js
File lib/common.js (right):

https://codereview.adblockplus.org/29863579/diff/29863584/lib/common.js#newco...
lib/common.js:38: function filterToRegExp(text, optimize = true)
On 2018/08/29 16:23:50, hub wrote:
> also, on second thought, I'm not certain that `optimize` is the best naming
for
> this option. Maybe "capture"? Since the use case is to capture what the regexp
> match.

OK, it already captures everything else, just not the stuff that's leading and
trailing. How about `captureAll` in that case?

https://codereview.adblockplus.org/29863579/diff/29863584/lib/common.js#newco...
lib/common.js:43: text = text.replace(/\*+/g, "*");
On 2018/08/29 16:23:50, hub wrote:
> On 2018/08/29 13:25:24, Manish Jethani wrote:
> > On 2018/08/28 22:02:55, hub wrote:
> > > I think we should keep this one unconditionally like it was before, but
run
> it
> > > before this block.
> > 
> > Since the `optimize` argument is supposed to skip all optimizations,
shouldn't
> > we skip this one too? It doesn't make any difference to the matching
behavior
> of
> > the regular expression, it only makes it match faster, and one can argue
that
> > it's not important for rewrite filters (why would anyone even use two or
more
> > consecutive "*").
> 
> this is more about correctness IMHO. There is an expectation to remove
duplicate
> '*'

Alright, if we go with the renaming then it makes sense.

Done.

[hub, 2018-09-04 15:30:46]

LGTM