Support redirects by regular expression in declarative WebRequest API

Support redirects by regular expression in declarative WebRequest API


BUG=112155
TEST=no

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=142182

[battre, 2012-05-30 12:02:49]

Hi Matt,

please review this CL.

Thanks,
Dominic

[Matt Perry, 2012-05-30 18:55:50]

https://chromiumcodereview.appspot.com/10449069/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

https://chromiumcodereview.appspot.com/10449069/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:74:
icu::RegexPattern::compile(icu::UnicodeString(from.c_str(), from.size()),
I thought the consensus on the thread was to evaluate the regex in the
originating renderer?

[battre, 2012-05-30 19:06:30]

On 2012/05/30 18:55:50, Matt Perry wrote:
>
https://chromiumcodereview.appspot.com/10449069/diff/1/chrome/browser/extensi...
> File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/10449069/diff/1/chrome/browser/extensi...
> chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:74:
> icu::RegexPattern::compile(icu::UnicodeString(from.c_str(), from.size()),
> I thought the consensus on the thread was to evaluate the regex in the
> originating renderer?

I thought the current consensus was:
- We use RE2 and then can use it in the browser.
- We use ICU temporarily to expose the API to the public but switch to RE2
before the feature is enabled on beta branch.

+aa to comment on this.

[Matt Perry, 2012-05-30 19:18:26]

http://codereview.chromium.org/10449069/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

http://codereview.chromium.org/10449069/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:419:
to_pattern_(to_pattern.c_str(), to_pattern.size()) {}
nit: prefer data() instead of c_str() when you're passing a size, since c_str()
might end up reallocating the string to add a null terminator.

http://codereview.chromium.org/10449069/diff/1/chrome/common/extensions/api/d...
File chrome/common/extensions/api/declarative_web_request.json (right):

http://codereview.chromium.org/10449069/diff/1/chrome/common/extensions/api/d...
chrome/common/extensions/api/declarative_web_request.json:70: "description":
"Redirects a request by applying a regular expression on the URL. The regular
expressions support temporarily the <a
href=\"http://www.unicode.org/reports/tr18/\">ICU syntax</a> with $1 to
reference capture group 1. This will be replaced with the <a
href=\"http://code.google.com/p/re2/wiki/Syntax\">RE2 syntax</a> which uses \\1
to reference capture group 1 before the API is released into the beta release of
Google Chrome.",
Can we just do a string.replaceAll('\\', '$') for the time being, to avoid
having to mention this caveat? (or maybe it has to be replaceAll("\N", "$N") for
N=1..9)

[Aaron Boodman, 2012-05-30 19:24:29]

On Wed, May 30, 2012 at 12:06 PM,  <battre@chromium.org> wrote:
> On 2012/05/30 18:55:50, Matt Perry wrote:
>
>
https://chromiumcodereview.appspot.com/10449069/diff/1/chrome/browser/extensi...
>>
>> File
>> chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
>> (right):
>
>
>
>
https://chromiumcodereview.appspot.com/10449069/diff/1/chrome/browser/extensi...
>>
>>
>>
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:74:
>> icu::RegexPattern::compile(icu::UnicodeString(from.c_str(), from.size()),
>> I thought the consensus on the thread was to evaluate the regex in the
>> originating renderer?
>
>
> I thought the current consensus was:
> - We use RE2 and then can use it in the browser.
> - We use ICU temporarily to expose the API to the public but switch to RE2
> before the feature is enabled on beta branch.

Yeah, my thought was that as long as we are only in dev, this is fine.
Am I missing something?

- a

[Matt Perry, 2012-05-30 19:28:55]

On 2012/05/30 19:24:29, Aaron Boodman wrote:
> On Wed, May 30, 2012 at 12:06 PM,  <mailto:battre@chromium.org> wrote:
> > On 2012/05/30 18:55:50, Matt Perry wrote:
> >
> >
>
https://chromiumcodereview.appspot.com/10449069/diff/1/chrome/browser/extensi...
> >>
> >> File
> >> chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
> >> (right):
> >
> >
> >
> >
>
https://chromiumcodereview.appspot.com/10449069/diff/1/chrome/browser/extensi...
> >>
> >>
> >>
> chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:74:
> >> icu::RegexPattern::compile(icu::UnicodeString(from.c_str(), from.size()),
> >> I thought the consensus on the thread was to evaluate the regex in the
> >> originating renderer?
> >
> >
> > I thought the current consensus was:
> > - We use RE2 and then can use it in the browser.
> > - We use ICU temporarily to expose the API to the public but switch to RE2
> > before the feature is enabled on beta branch.
> 
> Yeah, my thought was that as long as we are only in dev, this is fine.
> Am I missing something?
> 
> - a

Looking back through the thread, I don't see a clear consensus. Aaron, is it
your stance that RE2 would be fine to use in the browser, post-dev?

[Matt Perry, 2012-05-30 21:35:13]

On 2012/05/30 19:28:55, Matt Perry wrote:
> On 2012/05/30 19:24:29, Aaron Boodman wrote:
> > On Wed, May 30, 2012 at 12:06 PM,  <mailto:battre@chromium.org> wrote:
> > > On 2012/05/30 18:55:50, Matt Perry wrote:
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10449069/diff/1/chrome/browser/extensi...
> > >>
> > >> File
> > >> chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
> > >> (right):
> > >
> > >
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/10449069/diff/1/chrome/browser/extensi...
> > >>
> > >>
> > >>
> >
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:74:
> > >> icu::RegexPattern::compile(icu::UnicodeString(from.c_str(), from.size()),
> > >> I thought the consensus on the thread was to evaluate the regex in the
> > >> originating renderer?
> > >
> > >
> > > I thought the current consensus was:
> > > - We use RE2 and then can use it in the browser.
> > > - We use ICU temporarily to expose the API to the public but switch to RE2
> > > before the feature is enabled on beta branch.
> > 
> > Yeah, my thought was that as long as we are only in dev, this is fine.
> > Am I missing something?
> > 
> > - a
> 
> Looking back through the thread, I don't see a clear consensus. Aaron, is it
> your stance that RE2 would be fine to use in the browser, post-dev?

OK it sounds like we're OK with RE2. Dominic, your plan sounds good to me.

[battre, 2012-05-31 16:01:32]

http://codereview.chromium.org/10449069/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

http://codereview.chromium.org/10449069/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:74:
icu::RegexPattern::compile(icu::UnicodeString(from.c_str(), from.size()),
On 2012/05/30 18:55:50, Matt Perry wrote:
> I thought the consensus on the thread was to evaluate the regex in the
> originating renderer?

As discussed, we will change this to RE2 before releasing this API into the beta
branch and consider RE2 sufficiently safe for execution in the browser.

http://codereview.chromium.org/10449069/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:419:
to_pattern_(to_pattern.c_str(), to_pattern.size()) {}
On 2012/05/30 19:18:26, Matt Perry wrote:
> nit: prefer data() instead of c_str() when you're passing a size, since
c_str()
> might end up reallocating the string to add a null terminator.

Done.

http://codereview.chromium.org/10449069/diff/1/chrome/common/extensions/api/d...
File chrome/common/extensions/api/declarative_web_request.json (right):

http://codereview.chromium.org/10449069/diff/1/chrome/common/extensions/api/d...
chrome/common/extensions/api/declarative_web_request.json:70: "description":
"Redirects a request by applying a regular expression on the URL. The regular
expressions support temporarily the <a
href=\"http://www.unicode.org/reports/tr18/\">ICU syntax</a> with $1 to
reference capture group 1. This will be replaced with the <a
href=\"http://code.google.com/p/re2/wiki/Syntax\">RE2 syntax</a> which uses \\1
to reference capture group 1 before the API is released into the beta release of
Google Chrome.",
On 2012/05/30 19:18:26, Matt Perry wrote:
> Can we just do a string.replaceAll('\\', '$') for the time being, to avoid
> having to mention this caveat? (or maybe it has to be replaceAll("\N", "$N")
for
> N=1..9)

I looked up the individual capture group definitions and have done it properly.
You may notice that I have written some code that is currently not used (but
unit tested). I figured it would be easier to write the code now and just delete
some code once we migrate to RE2.

I am planning to leave both options as input syntax because the $ notation is
pretty common in the extension world because JavaScript RegExes use $. Note,
though, that JavaScript's RegExes are more powerful. I am not sure whether we
should mention "Perl" because it is significantly more powerful.

[Matt Perry, 2012-05-31 20:22:17]

http://codereview.chromium.org/10449069/diff/11/chrome/common/extensions/api/...
File chrome/common/extensions/api/declarative_web_request.json (right):

http://codereview.chromium.org/10449069/diff/11/chrome/common/extensions/api/...
chrome/common/extensions/api/declarative_web_request.json:85:
"perlCaptureGroupStyle": {
I don't think we should support this. We should just stick to one capture group
style. Probably RE2, since that is what we're planning on using, and we can get
rid of the conversion step once we switch.

[battre, 2012-05-31 21:30:40]

http://codereview.chromium.org/10449069/diff/11/chrome/common/extensions/api/...
File chrome/common/extensions/api/declarative_web_request.json (right):

http://codereview.chromium.org/10449069/diff/11/chrome/common/extensions/api/...
chrome/common/extensions/api/declarative_web_request.json:85:
"perlCaptureGroupStyle": {
On 2012/05/31 20:22:17, Matt Perry wrote:
> I don't think we should support this. We should just stick to one capture
group
> style. Probably RE2, since that is what we're planning on using, and we can
get
> rid of the conversion step once we switch.

This will create a certain burden for HTTPS Everywhere who cannot use their
default rules which all contain $ for capture groups (which is the default for
JavaScript RegEx, RE2 is the outlier). If I don't hear anything contrary by
tomorrow, I will remove this feature. I have a slight preference for leaving it
in, though.

[Matt Perry, 2012-05-31 21:38:56]

lgtm

http://codereview.chromium.org/10449069/diff/11/chrome/common/extensions/api/...
File chrome/common/extensions/api/declarative_web_request.json (right):

http://codereview.chromium.org/10449069/diff/11/chrome/common/extensions/api/...
chrome/common/extensions/api/declarative_web_request.json:85:
"perlCaptureGroupStyle": {
On 2012/05/31 21:30:40, battre wrote:
> On 2012/05/31 20:22:17, Matt Perry wrote:
> > I don't think we should support this. We should just stick to one capture
> group
> > style. Probably RE2, since that is what we're planning on using, and we can
> get
> > rid of the conversion step once we switch.
> 
> This will create a certain burden for HTTPS Everywhere who cannot use their
> default rules which all contain $ for capture groups (which is the default for
> JavaScript RegEx, RE2 is the outlier). If I don't hear anything contrary by
> tomorrow, I will remove this feature. I have a slight preference for leaving
it
> in, though.

I see... Maybe we should default to $ syntax, then? Though I presume RE2 syntax
is different in other ways as well, right?

My order of preferences is:
1. Use $ always, with a converter when we switch to RE2.
2. Leave this option (keep patch as-is).

But I don't feel that strongly, so I'll leave it up to you.

[Aaron Boodman, 2012-06-08 18:46:03]

On Thu, May 31, 2012 at 2:38 PM,  <mpcomplete@chromium.org> wrote:
> lgtm
>
>
>
>
>
>
http://codereview.chromium.org/10449069/diff/11/chrome/common/extensions/api/...
> File chrome/common/extensions/api/declarative_web_request.json (right):
>
>
http://codereview.chromium.org/10449069/diff/11/chrome/common/extensions/api/...
> chrome/common/extensions/api/declarative_web_request.json:85:
> "perlCaptureGroupStyle": {
> On 2012/05/31 21:30:40, battre wrote:
>>
>> On 2012/05/31 20:22:17, Matt Perry wrote:
>> > I don't think we should support this. We should just stick to one
>
> capture
>>
>> group
>> > style. Probably RE2, since that is what we're planning on using, and
>
> we can
>>
>> get
>> > rid of the conversion step once we switch.
>
>
>> This will create a certain burden for HTTPS Everywhere who cannot use
>
> their
>>
>> default rules which all contain $ for capture groups (which is the
>
> default for
>>
>> JavaScript RegEx, RE2 is the outlier). If I don't hear anything
>
> contrary by
>>
>> tomorrow, I will remove this feature. I have a slight preference for
>
> leaving it
>>
>> in, though.
>
>
> I see... Maybe we should default to $ syntax, then? Though I presume RE2
> syntax is different in other ways as well, right?
>
> My order of preferences is:
> 1. Use $ always, with a converter when we switch to RE2.
> 2. Leave this option (keep patch as-is).

Whichever one we do, can we put it in a more general place so that the
downloads API can use it too?

- a

[battre, 2012-06-14 12:48:09]

http://codereview.chromium.org/10449069/diff/11/chrome/common/extensions/api/...
File chrome/common/extensions/api/declarative_web_request.json (right):

http://codereview.chromium.org/10449069/diff/11/chrome/common/extensions/api/...
chrome/common/extensions/api/declarative_web_request.json:85:
"perlCaptureGroupStyle": {
On 2012/05/31 21:38:56, Matt Perry wrote:
> On 2012/05/31 21:30:40, battre wrote:
> > On 2012/05/31 20:22:17, Matt Perry wrote:
> > > I don't think we should support this. We should just stick to one capture
> > group
> > > style. Probably RE2, since that is what we're planning on using, and we
can
> > get
> > > rid of the conversion step once we switch.
> > 
> > This will create a certain burden for HTTPS Everywhere who cannot use their
> > default rules which all contain $ for capture groups (which is the default
for
> > JavaScript RegEx, RE2 is the outlier). If I don't hear anything contrary by
> > tomorrow, I will remove this feature. I have a slight preference for leaving
> it
> > in, though.
> 
> I see... Maybe we should default to $ syntax, then? Though I presume RE2
syntax
> is different in other ways as well, right?
> 
> My order of preferences is:
> 1. Use $ always, with a converter when we switch to RE2.
> 2. Leave this option (keep patch as-is).
> 
> But I don't feel that strongly, so I'll leave it up to you.

Done. I have followed your preference 1.

I did not move the conversion from $ to \\ syntax in a central place, yet,
because the Downloads API does not use RegEx for substitutions.

[Matt Perry, 2012-06-14 18:39:13]

lgtm

http://codereview.chromium.org/10449069/diff/15002/chrome/browser/extensions/...
File chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
(right):

http://codereview.chromium.org/10449069/diff/15002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:463: }
else if (*i == '$'){
nit: space before {

http://codereview.chromium.org/10449069/diff/15002/chrome/browser/extensions/...
chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc:465: }
else if (*i == '\\'){
ditto