Include RE2 library

Include RE2 library


BUG=112155
TEST=no
TBR=ben@chromium.org

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

[battre, 2012-06-21 18:37:28]

Hi Aaron,

what is the right way of landing this?

These files are from me:
chrome/chrome_browser.gypi
third_party/re2/.gitignore
third_party/re2/README.chromium
third_party/re2/re2.gyp
third_party/re2/re2-msvc9-chrome.patch
third_party/re2/remove-posix-option.patch

We might be able to only check in these files and then fetch the Mercurial
repository from re2 and apply the two patch files. Do you know anybody who knows
this stuff?

Best regards,
Dominic

[battre, 2012-07-02 13:37:42]

friendly ping

[Alexander Potapenko, 2012-07-02 13:45:35]

http://codereview.chromium.org/10575037/diff/15001/third_party/re2/util/valgr...
File third_party/re2/util/valgrind.cc (right):

http://codereview.chromium.org/10575037/diff/15001/third_party/re2/util/valgr...
third_party/re2/util/valgrind.cc:20: int RunningOnValgrind() {
Note that Chromium is already using RunningOnValgrind declared in
base/third_party/dynamic_annotations/dynamic_annotations.c.
It doesn't conflict with re2::RunningOnValgrind, but we certainly want to call
the existing function from re2 instead of adding another one.

[battre, 2012-07-03 15:10:55]

The diff does not work reliably to show the differences between this and the
previous version. I have added and applied
third_party/re2/remove-valgrind-code.patch and modified third_party/re2/re2.gyp.

http://codereview.chromium.org/10575037/diff/15001/third_party/re2/util/valgr...
File third_party/re2/util/valgrind.cc (right):

http://codereview.chromium.org/10575037/diff/15001/third_party/re2/util/valgr...
third_party/re2/util/valgrind.cc:20: int RunningOnValgrind() {
On 2012/07/02 13:45:35, Alexander Potapenko wrote:
> Note that Chromium is already using RunningOnValgrind declared in
> base/third_party/dynamic_annotations/dynamic_annotations.c.
> It doesn't conflict with re2::RunningOnValgrind, but we certainly want to call
> the existing function from re2 instead of adding another one.

Done.

[battre, 2012-07-19 07:56:00]

Aaron, do you know how we are moving this forward?

Best regards,
Dominic

[Mark Mentovai, 2012-07-19 16:30:06]

http://codereview.chromium.org/10575037/diff/45001/third_party/re2/README.chr...
File third_party/re2/README.chromium (right):

http://codereview.chromium.org/10575037/diff/45001/third_party/re2/README.chr...
third_party/re2/README.chromium:3: URL: http://code.google.com/p/re2/
Don’t we already have more than one RE library in Chrome? What’s the plan for
this one?

I find it interesting that when I look at this page, the project’s own home
site, the last line of the description is:

“If you absolutely need backreferences and generalized assertions, then RE2 is
not for you, but you might be interested in irregexp, Google Chrome's regular
expression engine.”

[battre, 2012-07-19 16:39:06]

http://codereview.chromium.org/10575037/diff/45001/third_party/re2/README.chr...
File third_party/re2/README.chromium (right):

http://codereview.chromium.org/10575037/diff/45001/third_party/re2/README.chr...
third_party/re2/README.chromium:3: URL: http://code.google.com/p/re2/
On 2012/07/19 16:30:06, Mark Mentovai wrote:
> Don’t we already have more than one RE library in Chrome? What’s the plan for
> this one?
> 
> I find it interesting that when I look at this page, the project’s own home
> site, the last line of the description is:
> 
> “If you absolutely need backreferences and generalized assertions, then RE2 is
> not for you, but you might be interested in irregexp, Google Chrome's regular
> expression engine.”

We have already 2 RE libraries (ICU and one in V8). The key difference of this
one is:

- the algorithm is guaranteed to be linear in the input size (not possible to
generate DoS attacks by extension supplied RegEx)
- the engine does not generate byte code (a safety concern that allows us to
execute extension supplied RegEx in the browser process)

I talked to the V8 guys about using the V8 RegEx implementation and it requires
a significant setup (it does not work without a V8 context) and has to run in a
sandboxed process. Both are significant disadvantages four our use case (using
it for extensions; in particular the declarative web request API).

[Mark Mentovai, 2012-07-19 16:41:37]

What will this one be used for?

[battre, 2012-07-19 16:46:08]

On 2012/07/19 16:41:37, Mark Mentovai wrote:
> What will this one be used for?

For the Declarative WebRequest API
http://code.google.com/chrome/extensions/dev/declarativeWebRequest.html

For example HTTPS Everywhere needs it for their rules
https://www.eff.org/https-everywhere/rulesets. Also AdBlock might need it in
order to switch from the old webRequest API to the declarative webRequest API.

Overall we want to migrate extensions from the old webRequeest API to the new
one because it will be significantly faster (no message round trips between the
browser and the renderers for each URLRequest) and remove the necessity of a
background page which consumes memory. This is part of the Extensions vNext
effort.

[Aaron Boodman, 2012-07-24 00:16:43]

I'm not the right person to review this. Perhaps Mark knows who is?

Mark, ftr, we had a thread about whether to include this already (from both a
size/security perspective) with chrome-security, and decided to go ahead. I can
forward it to you if you like.

[Aaron Boodman, 2012-07-24 00:18:00]

On second thought, Mark seems like the right person, since the main thing here
is the gyp changes.

[Mark Mentovai, 2012-07-24 14:56:29]

Yes, I can review this.

http://codereview.chromium.org/10575037/diff/45001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10575037/diff/45001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:61: '../third_party/re2/re2.gyp:re2',
Also list in all.gyp.

http://codereview.chromium.org/10575037/diff/45001/third_party/re2/README.chr...
File third_party/re2/README.chromium (right):

http://codereview.chromium.org/10575037/diff/45001/third_party/re2/README.chr...
third_party/re2/README.chromium:16: - Dropped POSIX configuration
(remove-posix-option.patch)
Do you want to put the patches into their own directory for better organization?

[battre, 2012-07-24 16:24:07]

http://codereview.chromium.org/10575037/diff/45001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10575037/diff/45001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:61: '../third_party/re2/re2.gyp:re2',
On 2012/07/24 14:56:29, Mark Mentovai wrote:
> Also list in all.gyp.

Done.

http://codereview.chromium.org/10575037/diff/45001/third_party/re2/README.chr...
File third_party/re2/README.chromium (right):

http://codereview.chromium.org/10575037/diff/45001/third_party/re2/README.chr...
third_party/re2/README.chromium:16: - Dropped POSIX configuration
(remove-posix-option.patch)
On 2012/07/24 14:56:29, Mark Mentovai wrote:
> Do you want to put the patches into their own directory for better
organization?

Done.

[Mark Mentovai, 2012-07-24 16:27:47]

LGTM

[commit-bot: I haz the power, 2012-07-24 16:37:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10575037/70029

[commit-bot: I haz the power, 2012-07-24 16:51:21]

Try job failure for 10575037-70029 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-25 13:21:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/battre@chromium.org/10575037/76001

[commit-bot: I haz the power, 2012-07-25 13:35:01]

Try job failure for 10575037-76001 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[battre, 2012-07-30 17:29:21]

Mark, could you please have another look? I have added re2-android.patch to fix
some compilation problems on Android (lack of tr1::unordered_set and isdigit()).

Thanks,
Dominic

[battre, 2012-07-31 14:25:09]

TBRing ben for chrome/OWNERS approval

[Mark Mentovai, 2012-07-31 23:01:49]

LGTM

[Ben Goodger (Google), 2012-08-01 15:41:02]

lgtm

[Nico, 2012-12-12 00:31:16]

Why is this not pulled through DEPS? (CL description doesn't say, and I didn't
find it in the review thread either.)

[battre, 2012-12-12 15:14:28]

On 2012/12/12 00:31:16, Nico wrote:
> Why is this not pulled through DEPS? (CL description doesn't say, and I didn't
> find it in the review thread either.)

I think the reasons were:
- RE2 is in a Mercurial repository
- my lack of knowledge of gyp
- the need to patch the upstream code (see "my lack of knowledge of gyp")

[Nico, 2013-01-10 01:49:55]

On Tue, Dec 11, 2012 at 4:31 PM,  <thakis@chromium.org> wrote:
> Why is this not pulled through DEPS? (CL description doesn't say, and I
> didn't
> find it in the review thread either.)

Ping.

>
> https://chromiumcodereview.appspot.com/10575037/

[battre, 2013-01-10 09:09:29]

On 2013/01/10 01:49:55, Nico wrote:
> On Tue, Dec 11, 2012 at 4:31 PM,  <mailto:thakis@chromium.org> wrote:
> > Why is this not pulled through DEPS? (CL description doesn't say, and I
> > didn't
> > find it in the review thread either.)
> 
> Ping.
> 
> >
> > https://chromiumcodereview.appspot.com/10575037/

See https://chromiumcodereview.appspot.com/10575037/#msg24

[Nico, 2013-01-10 19:02:16]

On Thu, Jan 10, 2013 at 1:09 AM,  <battre@chromium.org> wrote:
> On 2013/01/10 01:49:55, Nico wrote:
>
>> On Tue, Dec 11, 2012 at 4:31 PM,  <mailto:thakis@chromium.org> wrote:
>> > Why is this not pulled through DEPS? (CL description doesn't say, and I
>> > didn't
>> > find it in the review thread either.)
>
>
>> Ping.
>
>
>> >
>> > https://chromiumcodereview.appspot.com/10575037/
>
>
> See https://chromiumcodereview.appspot.com/10575037/#msg24
>
> https://chromiumcodereview.appspot.com/10575037/

Sorry, that didn't make it to my mailbox somehow.

I would think that the infra guys could set up an svn or git mirror of
a hg repo.

Your changes mostly look like stuff that the upstream repo should
accept, have you tried upstreaming them?

I understand that adding this stuff to the tree is a lot easier in the
short term, but it makes it much much harder to update it in the
future. (I speak from experience.)

[battre, 2013-01-11 16:26:07]

On 2013/01/10 19:02:16, Nico wrote:
> On Thu, Jan 10, 2013 at 1:09 AM,  <mailto:battre@chromium.org> wrote:
> > On 2013/01/10 01:49:55, Nico wrote:
> >
> >> On Tue, Dec 11, 2012 at 4:31 PM,  <mailto:thakis@chromium.org> wrote:
> >> > Why is this not pulled through DEPS? (CL description doesn't say, and I
> >> > didn't
> >> > find it in the review thread either.)
> >
> >
> >> Ping.
> >
> >
> >> >
> >> > https://chromiumcodereview.appspot.com/10575037/
> >
> >
> > See https://chromiumcodereview.appspot.com/10575037/#msg24
> >
> > https://chromiumcodereview.appspot.com/10575037/
> 
> Sorry, that didn't make it to my mailbox somehow.
> 
> I would think that the infra guys could set up an svn or git mirror of
> a hg repo.
> 
> Your changes mostly look like stuff that the upstream repo should
> accept, have you tried upstreaming them?
> 
> I understand that adding this stuff to the tree is a lot easier in the
> short term, but it makes it much much harder to update it in the
> future. (I speak from experience.)

I pinged Russ and CC'ed you.