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

third_party/libphonenumber/libphonenumber.gyp

+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+{
+  'variables': {
+    'protoc_out_dir': '<(SHARED_INTERMEDIATE_DIR)/protoc_out',
+  },
+  'target_defaults': {
+    'include_dirs': [
+      '../..', # add it first, so scrc/base headers are used

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

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, …

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

Done.

+               # instead of the ones brought with the library

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

Why didn’t you use all 80 columns at your disposal for this comment? You
probably could have saved a line.

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

Done.

+               # as cc files would be taken from the main
+               # chrome tree as well.
+      './cpp/src',

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

No leading ./

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

Done.

+      '<(protoc_out_dir)',
+      '../protobuf/src',
+      '../icu/public/common',
+      '../icu/public/i18n',
+    ],
+   'defines': [

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

Something’s wrong with the indentation here.

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

Done.

+      'U_USING_ICU_NAMESPACE=0',
+    ],
+   'conditions': [

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

And here.

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

Done.

+      ['OS!="win" or component=="static_library"', {
+        'defines': [
+          'U_STATIC_IMPLEMENTATION',
+        ],
+      }],
+    ],
+  },
+  'targets': [
+  {
+    'target_name': 'libphonenumber',
+    'type': '<(library)',
+    'dependencies': [
+        'phonenumber_proto',

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

Fix indentation.

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

Done.

+    ],
+    'sources': [
+        'chrome/regexp_adapter_icuregexp.cc',

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

Fix indentation.

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

Done.

+        'cpp/src/default_logger.cc',
+        'cpp/src/logger_adapter.cc',
+        # Generated by src/embed_binary_data.sh

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

And you’ve checked this in, right?

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

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

+        'cpp/src/metadata.cc',
+        'cpp/src/phonenumberutil.cc',
+        'cpp/src/stringutil.cc',
+        'cpp/src/utf/rune.c',
+        'cpp/src/utf/unicodetext.cc',
+        'cpp/src/utf/unilib.cc',
+        # Generated by phonenumber_proto.
+        '<(protoc_out_dir)/phonemetadata.pb.cc',
+        '<(protoc_out_dir)/phonenumber.pb.cc',
+    ],
+    'conditions': [
+      ['OS=="win"', {
+        'defines': [
+          'COMPILER_MSVC=1',

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

Really? Why not just #include "build/build_config.h"?

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

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

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

GeorgeY wrote:
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?

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

Done.

+          'OS_WIN=1',
+          'ARCH_CPU_X86_FAMILY=1',
+        ],
+        'action': [
+          '/wo4309',
+        ],
+      },],

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

Usually just }],. Same on line 71.

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

Done.

+      ['OS=="linux"', {
+        'defines': [
+          'COMPILER_GCC=1',

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

Same.

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

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

+          'OS_POSIX=1',
+          'OS_LINUX=1',
+        ],
+      },],
+      ['OS=="mac"', {
+        'defines': [
+          'OS_MAC=1',

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

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?

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

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)

+        ],
+      }],  # OS=="mac"
+    ],
+  },
+  {
+    # Protobuf compiler / generate rule for the phones
+    'target_name': 'phonenumber_proto',
+    'type': 'none',
+    'sources': [
+       'cpp/src/phonemetadata.proto',

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

Indentation. Go back through this file and make sure you’ve gotten everything
right.

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

Done.

+       'cpp/src/phonenumber.proto',
+    ],
+    'rules': [
+      {
+        'rule_name': 'genproto',
+        'extension': 'proto',
+        'inputs': [
+          '<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)protoc<(EXECUTABLE_SUFFIX)',
+        ],
+        'variables': {
+          # The protoc compiler requires a proto_path argument with the
+          # directory containing the .proto file.
+          # There's no generator variable that corresponds to this, so fake it.
+          'rule_input_relpath': 'cpp/src',
+        },
+        'outputs': [
+          '<(protoc_out_dir)/<(RULE_INPUT_ROOT).pb.h',
+          '<(protoc_out_dir)/<(RULE_INPUT_ROOT).pb.cc',
+        ],
+        'action': [
+          '<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)protoc<(EXECUTABLE_SUFFIX)',
+          '--proto_path=./<(rule_input_relpath)',

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

The leading ./ on this line and the next aren’t necessary.

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

Done.

+          './<(rule_input_relpath)/<(RULE_INPUT_ROOT)<(RULE_INPUT_EXT)',
+          '--cpp_out=<(protoc_out_dir)',
+        ],
+        'message': 'Generating C++ code from <(RULE_INPUT_PATH)',
+      },
+    ],
+    'dependencies': [
+      '../protobuf/protobuf.gyp:protobuf_lite',
+      '../protobuf/protobuf.gyp:protoc#host',
+    ],
+    'direct_dependent_settings': {
+      'include_dirs': [
+          '<(protoc_out_dir)',

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

Indentation!

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

Done.

+      ]
+    },
+    'export_dependent_settings': [

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

Is this needed?

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

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

+      '../../third_party/protobuf/protobuf.gyp:protobuf_lite',
+    ],
+  },
+  {
+    'target_name': 'libphonenumber_unittests',
+    'type': 'executable',
+    'sources': [
+      '../../base/test/run_all_unittests.cc',
+      'cpp/src/phonenumberutil_test.cc',
+      'cpp/src/regexp_adapter_unittest.cc',
+      'cpp/src/stringutil_test.cc',
+      'cpp/src/test_metadata.cc',
+    ],
+    'dependencies': [
+      '../icu/icu.gyp:icui18n',
+      '../icu/icu.gyp:icuuc',
+      '../protobuf/protobuf.gyp:protobuf_lite',
+      '../../base/base.gyp:base',
+      '../../base/base.gyp:test_support_base',
+      '../../testing/gmock.gyp:gmock',
+      '../../testing/gtest.gyp:gtest',
+      'libphonenumber',
+    ],
+    'conditions': [

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

As before.

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

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

+      ['OS=="win"', {
+        'defines': [
+          'COMPILER_MSVC=1',
+          'OS_WIN=1',
+          'ARCH_CPU_X86_FAMILY=1',
+        ],
+        'action': [
+          '/wo4309',
+        ],
+      },],
+      ['OS=="linux"', {
+        'defines': [
+          'COMPILER_GCC=1',
+          'OS_POSIX=1',
+          'OS_LINUX=1',
+        ],
+      },],
+      ['OS=="mac"', {
+        'defines': [
+          'OS_MAC=1',
+        ],
+      }],  # OS=="mac"
+    ],
+  },
+  ]

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

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]

Done.

+}