Autofill phone number enhancements and integration of Phone Number Util Library: part 1

Autofill phone number enhancements and integration of Phone Number Util Library: part 1
Temporarily the whole library is included, until the patch is upstreamed.

BUG=71443
TEST=Unit-tested
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=84000

[dhollowa, 2011-04-06 15:02:55]

George, the project as it appears here doesn't build.  Did you forget to post
the libphonenumber sources?

http://codereview.chromium.org/6803005/diff/5001/DEPS
File DEPS (right):

http://codereview.chromium.org/6803005/diff/5001/DEPS#newcode225
DEPS:225: (Var("googlecode_url") % "libphonenumber") + "/trunk/cpp@" +
Var("libphonenumber_revision"),
nit: 80 cols

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/R...
File third_party/libphonenumber/README.chromium (right):

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/R...
third_party/libphonenumber/README.chromium:14: 2. third_party\icu for
internationalisation, UTF8-16-32 conversion and regular expressions.
nit: 80 col

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
File third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc (right):

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:7: // Setup all of
the chrome/webkit defines
nit: s/chrome\/webkit defines/ Chromium and WebKit defines./

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:16: 
nit: remove extra space.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:28: class
IcuRegularExpressionInput : public reg_exp::RegularExpressionInput {
Please add comments for class.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:32: virtual bool
ConsumeRegExp(std::string const& reg_exp,
nit: add comment to designate interface.  i.e.  // RegularExpressionInput:

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:40: int pos()
const { return pos_; }
Please add comments.  It is not clear, upon casual inspection why this is here
or what it is used for.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:43: private:
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:48: 
nit: remove extra space.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:65: private:
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:71: :
utf8_input_(icu::UnicodeString::fromUTF8(utf8_input)),
clank may not like inlined ctor with non-trivial computation.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:99: DCHECK(false);
NOTREACHED();

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:221: 
if (U_FAILURE(status))...

[dhollowa, 2011-04-06 15:49:22]

On 2011/04/06 15:02:55, dhollowa wrote:
> George, the project as it appears here doesn't build.  Did you forget to post
> the libphonenumber sources?

Nevermind...  Pilot error with "gclient sync".  It's working for me now.

[dhollowa, 2011-04-06 15:52:21]

On 2011/04/06 15:49:22, dhollowa wrote:
> On 2011/04/06 15:02:55, dhollowa wrote:
> > George, the project as it appears here doesn't build.  Did you forget to
post
> > the libphonenumber sources?
> 
> Nevermind...  Pilot error with "gclient sync".  It's working for me now.

Except now:  error: third_party/libphonenumber/cpp/src/regexp_adapter.h: No such
file or directory.

[GeorgeY, 2011-04-07 00:00:38]

done. To compile you need to apply upstream cl (attached to e-mail) to the
library as well

http://codereview.chromium.org/6803005/diff/5001/DEPS
File DEPS (right):

http://codereview.chromium.org/6803005/diff/5001/DEPS#newcode225
DEPS:225: (Var("googlecode_url") % "libphonenumber") + "/trunk/cpp@" +
Var("libphonenumber_revision"),
On 2011/04/06 15:02:55, dhollowa wrote:
> nit: 80 cols

Fixed, but I think the limit is about source code only - there are a lot of
lines longer than 80 chars in this file.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/R...
File third_party/libphonenumber/README.chromium (right):

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/R...
third_party/libphonenumber/README.chromium:14: 2. third_party\icu for
internationalisation, UTF8-16-32 conversion and regular expressions.
On 2011/04/06 15:02:55, dhollowa wrote:
> nit: 80 col

Done.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
File third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc (right):

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:7: // Setup all of
the chrome/webkit defines
On 2011/04/06 15:02:55, dhollowa wrote:
> nit: s/chrome\/webkit defines/ Chromium and WebKit defines./

Done.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:16: 
On 2011/04/06 15:02:55, dhollowa wrote:
> nit: remove extra space.

Done.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:28: class
IcuRegularExpressionInput : public reg_exp::RegularExpressionInput {
On 2011/04/06 15:02:55, dhollowa wrote:
> Please add comments for class.

Copied comments from the libphonenumber/cpp/src/regexp_adapter.h

The classes are just implementation for abstract classes.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:32: virtual bool
ConsumeRegExp(std::string const& reg_exp,
On 2011/04/06 15:02:55, dhollowa wrote:
> nit: add comment to designate interface.  i.e.  // RegularExpressionInput:

Done.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:40: int pos()
const { return pos_; }
On 2011/04/06 15:02:55, dhollowa wrote:
> Please add comments.  It is not clear, upon casual inspection why this is here
> or what it is used for.

Done.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:43: private:
On 2011/04/06 15:02:55, dhollowa wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:48: 
On 2011/04/06 15:02:55, dhollowa wrote:
> nit: remove extra space.

Done.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:65: private:
On 2011/04/06 15:02:55, dhollowa wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:71: :
utf8_input_(icu::UnicodeString::fromUTF8(utf8_input)),
On 2011/04/06 15:02:55, dhollowa wrote:
> clank may not like inlined ctor with non-trivial computation.

Moved to the body.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:99: DCHECK(false);
On 2011/04/06 15:02:55, dhollowa wrote:
> NOTREACHED();

Done.

http://codereview.chromium.org/6803005/diff/5001/third_party/libphonenumber/c...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:221: 
On 2011/04/06 15:02:55, dhollowa wrote:
> if (U_FAILURE(status))...

Done.

[dhollowa, 2011-04-07 16:05:50]

LGTM on Chrome side.  Deferring libphonenumber patch to Shaopeng et. al.

[dhollowa, 2011-04-25 20:09:20]

http://codereview.chromium.org/6803005/diff/16001/DEPS
File DEPS (right):

http://codereview.chromium.org/6803005/diff/16001/DEPS#newcode17
DEPS:17: "libphonenumber_revision": "r156",
None of the other version numbers have the "r" prefix.  This should be "156",
not "r156".

http://codereview.chromium.org/6803005/diff/17008/build/all.gyp
File build/all.gyp (right):

http://codereview.chromium.org/6803005/diff/17008/build/all.gyp#newcode166
build/all.gyp:166:
'../third_party/libphonenumber/libphonenumber.gyp:libphonenumber_unittests',
Eventually, these should get run under "chromium_builder_dbg " target as well. 
I logged bug http://crbug.com/80484 as discussed.

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
File third_party/libphonenumber/libphonenumber.gyp (right):

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:5: {
I'm having trouble building release build on Mac.

[dhollowa, 2011-04-26 17:26:14]

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
File third_party/libphonenumber/README.chromium (right):

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:8: integration owner:
georgey@chromium.org
I don't see "integration owner" in other README.chromium files.  Is there a
reason you added it?

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:11: for C/C++
Nit: period at the end of the sentence please.

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:14: 1. base\ for types, scoped_ptr's,
etc.
nit: Use forward slashes: "base/" to match url and cpp-include style.  (And
below).

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
File third_party/libphonenumber/libphonenumber.gyp (right):

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:43: 'cpp/src/metadata.cc',        
# Generated by src/embed_binary_data.sh
This "generated" comment is at end of line, whereas the similar comment at line
49 is above.  Let's put both above, so they match.

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:49: # generated by
phonenumber_proto
s/generated/Generated/
and end sentence with a period.

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:139:
#'../protobuf/protobuf.gyp:protoc#host',
Remove this if not needed.

[dhollowa, 2011-04-26 17:27:38]

+mark for libphonenumber.gyp

Mark, gyp master, could you take a look at libphonenumber.gyp please?

[GeorgeY, 2011-04-27 00:41:45]

http://codereview.chromium.org/6803005/diff/16001/DEPS
File DEPS (right):

http://codereview.chromium.org/6803005/diff/16001/DEPS#newcode17
DEPS:17: "libphonenumber_revision": "r156",
On 2011/04/25 20:09:20, dhollowa wrote:
> None of the other version numbers have the "r" prefix.  This should be "156",
> not "r156".

AFAIK, it is passed verbatim to svn. Will check.

http://codereview.chromium.org/6803005/diff/17008/build/all.gyp
File build/all.gyp (right):

http://codereview.chromium.org/6803005/diff/17008/build/all.gyp#newcode166
build/all.gyp:166:
'../third_party/libphonenumber/libphonenumber.gyp:libphonenumber_unittests',
On 2011/04/25 20:09:20, dhollowa wrote:
> Eventually, these should get run under "chromium_builder_dbg " target as well.

> I logged bug http://crbug.com/80484 as discussed.

Thanks!

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
File third_party/libphonenumber/README.chromium (right):

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:8: integration owner:
georgey@chromium.org
On 2011/04/26 17:26:14, dhollowa wrote:
> I don't see "integration owner" in other README.chromium files.  Is there a
> reason you added it?

Removed - now we have OWNERS file

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:11: for C/C++
On 2011/04/26 17:26:14, dhollowa wrote:
> Nit: period at the end of the sentence please.

Done.

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:14: 1. base\ for types, scoped_ptr's,
etc.
On 2011/04/26 17:26:14, dhollowa wrote:
> nit: Use forward slashes: "base/" to match url and cpp-include style.  (And
> below).

Done.

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
File third_party/libphonenumber/libphonenumber.gyp (right):

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:5: {
On 2011/04/25 20:09:20, dhollowa wrote:
> I'm having trouble building release build on Mac.

Let's investigate tomorrow...

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:43: 'cpp/src/metadata.cc',        
# Generated by src/embed_binary_data.sh
On 2011/04/26 17:26:14, dhollowa wrote:
> This "generated" comment is at end of line, whereas the similar comment at
line
> 49 is above.  Let's put both above, so they match.

Done.

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:49: # generated by
phonenumber_proto
On 2011/04/26 17:26:14, dhollowa wrote:
> s/generated/Generated/
> and end sentence with a period.

Done.

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:139:
#'../protobuf/protobuf.gyp:protoc#host',
On 2011/04/26 17:26:14, dhollowa wrote:
> Remove this if not needed.

Done.

[dhollowa, 2011-04-27 16:11:57]

http://codereview.chromium.org/6803005/diff/17008/build/all.gyp
File build/all.gyp (right):

http://codereview.chromium.org/6803005/diff/17008/build/all.gyp#newcode166
build/all.gyp:166:
'../third_party/libphonenumber/libphonenumber.gyp:libphonenumber_unittests',
On second thought, we should take care of this now.  It'd be nice to get full
coverage of the bots (Valgrind, etc.) earlier rather than later.

On 2011/04/27 00:41:45, GeorgeY wrote:
> On 2011/04/25 20:09:20, dhollowa wrote:
> > Eventually, these should get run under "chromium_builder_dbg " target as
well.
> 
> > I logged bug http://crbug.com/80484 as discussed.
> 
> Thanks!

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
File third_party/libphonenumber/libphonenumber.gyp (right):

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:16: '<(protoc_out_dir)/cpp/src',
phonenumberutil.h:32 has: #include "src/cpp/phonenumber.pb.h"

but

phonenumberutil.cc has: #include "phonemetadata.pb.h"

So this is not resolving correctly.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
File third_party/libphonenumber/README.chromium (right):

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:11: This library depends on
Let's add a directory called "patches" and add the re-adaptor patch to the
directory.  We should apply the patch to the local contents as well.

Documentation for the patch should be added to this file.  Please see
third_party/icu/README.chromium and third_party/icu/patches for examples.

[Mark Mentovai, 2011-04-27 16:53:01]

http://codereview.chromium.org/6803005/diff/27001/DEPS
File DEPS (right):

http://codereview.chromium.org/6803005/diff/27001/DEPS#newcode17
DEPS:17: "libphonenumber_revision": "r156",
Get rid of the r.

I know it works with the r, but we’ve standardized on no r, and there’s no
compelling reason to break with tradition here.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
File third_party/libphonenumber/README.chromium (right):

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:8: This directory contains the source
code of international phone number library
Is “international phone number library” a proper name? Then capitalize it.

If you’re just using it as a descriptive term, this should say “source code of
an international phone number library.”

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:14: expressions.
This line could use a little line-up-with-the-one-above-it love.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:20:
chrome\regexp_adapter_icuregexp.cc
You used slashes in the text above but a backslash here. Standardize on forward
slashes.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:21: 
Get rid of the blank lines at the end of this file.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
File third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc (right):

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:7: // Setup all of
the Chromium and WebKit defines
“Set up” as a verb is two words.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:8: #include
<build/build_config.h>
These should all be "…" includes, not <…> includes. Only use <…> for system
headers like <stdio.h> and <vector>. Because these aren’t system includes, you
should put them in the same section as the base includes below.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:18: // Converts
|source| to utf8 string, returns it.
“to a UTF-8 string.”

This comment is silent on what |pos| is used for.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:20: int pos) {
This looks like it would have fit on the preceding line.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:31: // The reg exp
input class.
Don’t abbreviate. Regular expression.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:111: const char*
utf8_input)
This looks like it would fit on the preceding line.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:139: // All of the
passed Regular expressions should compile correctly.
Lowercase r.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:228: // ($0-9 for
matches). All '$' should be pre-pended with '\' as well.
prepended (no hyphen).

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:294: 
This blank line isn’t necessary.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
File third_party/libphonenumber/libphonenumber.gyp (right):

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:11: '../..', # add it first, so
scrc/base headers are used
1. In the comment, src/base, not scrc/base.

2. As in C++, put two spaces between the last non-whitespace thing on the line
and the beginning of the comment.

      '../..',  # add it first, …

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:12: # instead of the ones brought
with the library
Why didn’t you use all 80 columns at your disposal for this comment? You
probably could have saved a line.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:15: './cpp/src',
No leading ./

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:21: 'defines': [
Something’s wrong with the indentation here.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:24: 'conditions': [
And here.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:37: 'phonenumber_proto',
Fix indentation.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:40:
'chrome/regexp_adapter_icuregexp.cc',
Fix indentation.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:43: # Generated by
src/embed_binary_data.sh
And you’ve checked this in, right?

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:57: 'COMPILER_MSVC=1',
Really? Why not just #include "build/build_config.h"?

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:64: },],
Usually just }],. Same on line 71.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:67: 'COMPILER_GCC=1',
Same.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:74: 'OS_MAC=1',
Same.

Chrome code doesn’t even use OS_MAC, it’s OS_MACOSX. The Mac is also OS_POSIX.

The fact that you haven’t even gotten these #defines right makes me wonder if
you even need them. Why did you add them?

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:84: 'cpp/src/phonemetadata.proto',
Indentation. Go back through this file and make sure you’ve gotten everything
right.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:106:
'--proto_path=./<(rule_input_relpath)',
The leading ./ on this line and the next aren’t necessary.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:119: '<(protoc_out_dir)',
Indentation!

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:122: 'export_dependent_settings':
[
Is this needed?

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:146: 'conditions': [
As before.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:171: ]
When you have this at the same indentation level as the line above it, it’s a
pretty good clue that your indentation is messed up.

[GeorgeY, 2011-04-28 07:21:18]

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
File third_party/libphonenumber/libphonenumber.gyp (right):

http://codereview.chromium.org/6803005/diff/17008/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:16: '<(protoc_out_dir)/cpp/src',
On 2011/04/27 16:11:57, dhollowa wrote:
> phonenumberutil.h:32 has: #include "src/cpp/phonenumber.pb.h"
> 
> but
> 
> phonenumberutil.cc has: #include "phonemetadata.pb.h"
> 
> So this is not resolving correctly.

Not anymore! :) Fixed.

http://codereview.chromium.org/6803005/diff/27001/DEPS
File DEPS (right):

http://codereview.chromium.org/6803005/diff/27001/DEPS#newcode17
DEPS:17: "libphonenumber_revision": "r156",
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Get rid of the r.
> 
> I know it works with the r, but we’ve standardized on no r, and there’s no
> compelling reason to break with tradition here.

Done. But for now added patched library completely, until they upstream my
changes

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
File third_party/libphonenumber/README.chromium (right):

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:8: This directory contains the source
code of international phone number library
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Is “international phone number library” a proper name? Then capitalize it.
> 
> If you’re just using it as a descriptive term, this should say “source code of
> an international phone number library.”

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:11: This library depends on
On 2011/04/27 16:11:57, dhollowa wrote:
> Let's add a directory called "patches" and add the re-adaptor patch to the
> directory.  We should apply the patch to the local contents as well.
> 
> Documentation for the patch should be added to this file.  Please see
> third_party/icu/README.chromium and third_party/icu/patches for examples.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:14: expressions.
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> This line could use a little line-up-with-the-one-above-it love.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:20:
chrome\regexp_adapter_icuregexp.cc
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> You used slashes in the text above but a backslash here. Standardize on
forward
> slashes.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/README.chromium:21: 
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Get rid of the blank lines at the end of this file.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
File third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc (right):

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:7: // Setup all of
the Chromium and WebKit defines
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> “Set up” as a verb is two words.

Fixed

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:8: #include
<build/build_config.h>
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> These should all be "…" includes, not <…> includes. Only use <…> for system
> headers like <stdio.h> and <vector>. Because these aren’t system includes, you
> should put them in the same section as the base includes below.

Yes, I used their style while the file was in their subfolder, now that I moved
it to separate folder I forgot to fix it. Fixed now.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:18: // Converts
|source| to utf8 string, returns it.
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> “to a UTF-8 string.”
> 
> This comment is silent on what |pos| is used for.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:20: int pos) {
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> This looks like it would have fit on the preceding line.

Nope, short by two characters.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:31: // The reg exp
input class.
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Don’t abbreviate. Regular expression.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:111: const char*
utf8_input)
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> This looks like it would fit on the preceding line.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:139: // All of the
passed Regular expressions should compile correctly.
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Lowercase r.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:228: // ($0-9 for
matches). All '$' should be pre-pended with '\' as well.
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> prepended (no hyphen).

Tell it to VS spell-checker :). Fixed.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:294: 
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> This blank line isn’t necessary.

It is, otherwise Lint would report the lack of the line as an error.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
File third_party/libphonenumber/libphonenumber.gyp (right):

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:11: '../..', # add it first, so
scrc/base headers are used
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> 1. In the comment, src/base, not scrc/base.
> 
> 2. As in C++, put two spaces between the last non-whitespace thing on the line
> and the beginning of the comment.
> 
>       '../..',  # add it first, …

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:12: # instead of the ones brought
with the library
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Why didn’t you use all 80 columns at your disposal for this comment? You
> probably could have saved a line.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:15: './cpp/src',
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> No leading ./

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:21: 'defines': [
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Something’s wrong with the indentation here.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:24: 'conditions': [
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> And here.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:37: 'phonenumber_proto',
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Fix indentation.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:40:
'chrome/regexp_adapter_icuregexp.cc',
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Fix indentation.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:43: # Generated by
src/embed_binary_data.sh
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> And you’ve checked this in, right?

They did - removed comment, as it is a remnant from their makefile.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:57: 'COMPILER_MSVC=1',
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Really? Why not just #include "build/build_config.h"?

Unfortunately they do not include it everywhere it actually needed. I think I'll
add that and upstream it as well.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:64: },],
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Usually just }],. Same on line 71.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:67: 'COMPILER_GCC=1',
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Same.

They use them in their makefile, instead of including build/build_config.h
everywhere.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:74: 'OS_MAC=1',
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Same.
> 
> Chrome code doesn’t even use OS_MAC, it’s OS_MACOSX. The Mac is also OS_POSIX.
> 
> The fact that you haven’t even gotten these #defines right makes me wonder if
> you even need them. Why did you add them?

They use them in their makefile, instead of including build/build_config.h.

As I haven't tried MAC yet - I missed that. (The change is not going to go in
until successful on every platform, of course)

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:84: 'cpp/src/phonemetadata.proto',
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Indentation. Go back through this file and make sure you’ve gotten everything
> right.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:106:
'--proto_path=./<(rule_input_relpath)',
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> The leading ./ on this line and the next aren’t necessary.

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:119: '<(protoc_out_dir)',
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Indentation!

Done.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:122: 'export_dependent_settings':
[
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> Is this needed?

Yes, it is needed - otherwise they are generated incorrectly.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:146: 'conditions': [
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> As before.

As before - I need to modify their protobuf files to include build_config.h,
which I will do with the next upstreamed patch.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:171: ]
On 2011/04/27 16:53:01, Mark Mentovai wrote:
> When you have this at the same indentation level as the line above it, it’s a
> pretty good clue that your indentation is messed up.

Done.

[Mark Mentovai, 2011-04-28 14:27:03]

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
File third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc (right):

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:294: 
GeorgeY wrote:
> It is, otherwise Lint would report the lack of the line as an error.

lint reports an error if the last line in the file doesn’t have an LF at the end
of it. In other words, if there’s anything at all in the file, it needs to end
in an LF.

This file ends in two LFs, meaning that you have a blank line at EOF. lint
doesn’t require that.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
File third_party/libphonenumber/libphonenumber.gyp (right):

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:57: 'COMPILER_MSVC=1',
GeorgeY wrote:
> Unfortunately they do not include it everywhere it actually needed. I think
I'll
> add that and upstream it as well.

The macro definitions in this file are wrong. Still.

I strongly advise you to instead add the proper #include in files that use these
macros instead of duplicating their configurations here. You said that our copy
is the “patched” version now anyway, right? Why can’t we just stage the proper
change in our copy of the source and avoid this mess altogether?

[dhollowa, 2011-04-28 22:51:28]

http://codereview.chromium.org/6803005/diff/23083/build/all.gyp
File build/all.gyp (right):

http://codereview.chromium.org/6803005/diff/23083/build/all.gyp#newcode165
build/all.gyp:165:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation_unittests',
Please don't forget to add libphonenumber_unittests to the other targets (where
cacheinvalidation_unittests are included).  Let's get maximal coverage as soon
as we can.

[GeorgeY, 2011-05-02 23:36:59]

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
File third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc (right):

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:294: 
On 2011/04/28 14:27:03, Mark Mentovai wrote:
> GeorgeY wrote:
> > It is, otherwise Lint would report the lack of the line as an error.
> 
> lint reports an error if the last line in the file doesn’t have an LF at the
end
> of it. In other words, if there’s anything at all in the file, it needs to end
> in an LF.
> 
> This file ends in two LFs, meaning that you have a blank line at EOF. lint
> doesn’t require that.

Apparently Lint does require empty line at the end - removing it produces the
following error:
+} // namespace reg_exp
Could not find a newline character at the end of the file.

Oh, and other files *do* have it.

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
File third_party/libphonenumber/libphonenumber.gyp (right):

http://codereview.chromium.org/6803005/diff/27001/third_party/libphonenumber/...
third_party/libphonenumber/libphonenumber.gyp:57: 'COMPILER_MSVC=1',
On 2011/04/28 14:27:03, Mark Mentovai wrote:
> GeorgeY wrote:
> > Unfortunately they do not include it everywhere it actually needed. I think
> I'll
> > add that and upstream it as well.
> 
> The macro definitions in this file are wrong. Still.
> 
> I strongly advise you to instead add the proper #include in files that use
these
> macros instead of duplicating their configurations here. You said that our
copy
> is the “patched” version now anyway, right? Why can’t we just stage the proper
> change in our copy of the source and avoid this mess altogether?

Done.

http://codereview.chromium.org/6803005/diff/23083/build/all.gyp
File build/all.gyp (right):

http://codereview.chromium.org/6803005/diff/23083/build/all.gyp#newcode165
build/all.gyp:165:
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation_unittests',
On 2011/04/28 22:51:28, dhollowa wrote:
> Please don't forget to add libphonenumber_unittests to the other targets
(where
> cacheinvalidation_unittests are included).  Let's get maximal coverage as soon
> as we can.

Done.

[GeorgeY, 2011-05-03 19:40:44]

On 2011/04/28 22:51:28, dhollowa wrote:
> http://codereview.chromium.org/6803005/diff/23083/build/all.gyp
> File build/all.gyp (right):
> 
> http://codereview.chromium.org/6803005/diff/23083/build/all.gyp#newcode165
> build/all.gyp:165:
>
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation_unittests',
> Please don't forget to add libphonenumber_unittests to the other targets
(where
> cacheinvalidation_unittests are included).  Let's get maximal coverage as soon
> as we can.

done as well :)

[Mark Mentovai, 2011-05-03 20:52:59]

LG otherwise

http://codereview.chromium.org/6803005/diff/31038/third_party/libphonenumber/...
File third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc (right):

http://codereview.chromium.org/6803005/diff/31038/third_party/libphonenumber/...
third_party/libphonenumber/chrome/regexp_adapter_icuregexp.cc:300: 
Blank line at the end of this guy too.

[dhollowa, 2011-05-03 20:54:24]

LGTM.  Thanks.

[Nico, 2011-11-28 19:30:43]

For posterity, the bug to unfork libphonenumber is
http://code.google.com/p/chromium/issues/detail?id=105577