Build libaddressinput in Chrome for requestAutocomplete().

Build libaddressinput in Chrome for requestAutocomplete().

This CL:
- creates a chromium-specific:
 * third_party/libaddressinput/
  * chromium/util/
   - json.cc (chromium-specific json reader)
   - json.h (matches src/cpp/src/util/json.h's interface)
  - libaddressinput.gyp (chromium-specific .gyp file)
- builds and runs a new test target named libaddressinput_unittests

R=rouslan@chromium.org
BUG=317837
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236996

[Dan Beam, 2013-11-19 23:42:31]

rouslan@: this still depends on your DEPS roll landing but I guess it doesn't
hurt to start review now.

[please use gerrit instead, 2013-11-20 00:03:09]

Looks great! A couple suggestions:

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
File third_party/libaddressinput/chromium/util/json.cc (right):

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/chromium/util/json.cc:7: #include "util/json.h"
1) Place the header that you're implementing at the top of the file and separate
from the rest of of the headers.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

2) Include-what-you-use for std::string, scoped_ptr, base::Value.

After these suggestions, the includes should look like this:

#include "util/json.h"

#include <string>

#include "base/json/json_reader.h"
#include "base/logging.h"
#include "base/memory/scoped_ptr.h"
#include "base/values.h"

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
File third_party/libaddressinput/chromium/util/json.h (right):

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/chromium/util/json.h:15: // This class provides the
same interface as ../src/cpp/src/util/json.h but with
Move this comment at the top of the file:

// Copyright 2013 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.
//
// This class provides the same interface as ../src/cpp/src/util/json.h but with
// a Chromium-specific JSON parser. This is because it didn't make much sense to
// have 2 JSON parsers in Chrome's code base, and the standalone library opted
// to use rapidjson instead of jsoncpp. See the other file for an explanation of
// what this class does.

#ifndef THIRD_PARTY_LIBADDRESSINPUT_CHROMIUM_UTIL_JSON_H_
...

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/chromium/util/json.h:26: bool ParseObject(const
std::string& json);
Include-what-you-use specifies that you should #include <string> if you have it
here. It should go before the Chrome includes and be separate from them.

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/chromium/util/json.h:31:
scoped_ptr<base::DictionaryValue> dict_;
You can forward-declare this and avoid including "base/values.h". After these
suggestions, the include section might look like this:

#include <string>

#include "base/basictypes.h"
#include "base/memory/scoped_ptr.h"

namespace base {
class DictionaryValue;
}

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:40: 'src/cpp/src/',
The files in 'cpp/src/src' use quotation marks "" instead of <> when including
the headers from the same directory. I believe that the search path for header
files starts with the current directories when using "" instead of <>. Unless
removing 'src/cpp/src' from the 'include_dirs' breaks json.h, I recommend
removing 'src/cpp/src' from this list.

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:55: 'src/cpp/src/rule.cc',
If you're including all of the header files in the 'sources' section, then
please also add 'address_field_util.h', 'region_data_constants.h', and 'rule.h'.

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:67:
'<(SHARED_INTERMEDIATE_DIR)/libaddressinput/',
Direct depending settings are settings for the autofill module that will use
libaddressinput. Technically, the autofill module should include only
'src/cpp/include' from libaddressinput. 'chromium', 'src/cpp/src/', and
<(SHARED_INTERMEDIATE_DIR)/libaddressinput/' are implementation details of
libaddressinput. Autofill should not have those includes. '<(DEPTH)' is included
by default in all Chrome code. That's not necessary.

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:87:
'<(DEPTH)/base/base.gyp:run_all_unittests',
'libaddressinput_unittests' depends on 'run_all_unittests'? I think you meant
the other way around.

[please use gerrit instead, 2013-11-20 00:04:19]

https://codereview.chromium.org/68323005/diff/200001/third_party/libaddressin...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/68323005/diff/200001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:35: 'include_dirs+': [
I think that <(DEPS) might be forced upon all targets in Chrome. Try removing
the +. It should still work.

[please use gerrit instead, 2013-11-20 00:07:57]

A couple more small comments.

https://codereview.chromium.org/68323005/diff/200001/third_party/libaddressin...
File third_party/libaddressinput/chromium/util/json.cc (right):

https://codereview.chromium.org/68323005/diff/200001/third_party/libaddressin...
third_party/libaddressinput/chromium/util/json.cc:30: if (dict_)
Simpler:

  DCHECK(dict_);
  dict_->GetWithoutPathExpansion(key, &val);

(I promise libaddressinput will not call HasStringValueForKey() if ParseObject()
returned false.)

https://codereview.chromium.org/68323005/diff/200001/third_party/libaddressin...
third_party/libaddressinput/chromium/util/json.cc:41: if (dict_)
Ditto:

  DCHECK(dict_);
  dict_->GetStringWithoutPathExpansion(key, &result);

Also, libaddressinput will not call GetStringValueForKey() if
HasStringValueForKey() returned false.

[please use gerrit instead, 2013-11-20 00:10:20]

https://codereview.chromium.org/68323005/diff/200001/third_party/libaddressin...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/68323005/diff/200001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:35: 'include_dirs+': [
On 2013/11/20 00:04:19, Rouslan Solomakhin wrote:
> I think that <(DEPS) might be forced upon all targets in Chrome. Try removing
> the +. It should still work.

s/DEPS/DEPTH/g

[Dan Beam, 2013-11-20 00:53:47]

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
File third_party/libaddressinput/chromium/util/json.cc (right):

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/chromium/util/json.cc:7: #include "util/json.h"
On 2013/11/20 00:03:10, Rouslan Solomakhin wrote:
> 1) Place the header that you're implementing at the top of the file and
separate
> from the rest of of the headers.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...
> 
> 2) Include-what-you-use for std::string, scoped_ptr, base::Value.
> 
> After these suggestions, the includes should look like this:
> 
> #include "util/json.h"
> 
> #include <string>
> 
> #include "base/json/json_reader.h"
> #include "base/logging.h"
> #include "base/memory/scoped_ptr.h"
> #include "base/values.h"

Done, however we don't (in Chrome code) re-include the same headers included in
the interface.  For example, scoped_ptr.h and values.h are included in
util/json.h so they're not re-included here.

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:40: 'src/cpp/src/',
On 2013/11/20 00:03:10, Rouslan Solomakhin wrote:
> The files in 'cpp/src/src' use quotation marks "" instead of <> when including
> the headers from the same directory. I believe that the search path for header
> files starts with the current directories when using "" instead of <>. Unless
> removing 'src/cpp/src' from the 'include_dirs' breaks json.h, I recommend
> removing 'src/cpp/src' from this list.

Done.

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:55: 'src/cpp/src/rule.cc',
On 2013/11/20 00:03:10, Rouslan Solomakhin wrote:
> If you're including all of the header files in the 'sources' section, then
> please also add 'address_field_util.h', 'region_data_constants.h', and
'rule.h'.

Done.

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:67:
'<(SHARED_INTERMEDIATE_DIR)/libaddressinput/',
On 2013/11/20 00:03:10, Rouslan Solomakhin wrote:
> Direct depending settings are settings for the autofill module that will use
> libaddressinput. Technically, the autofill module should include only
> 'src/cpp/include' from libaddressinput. 'chromium', 'src/cpp/src/', and
> <(SHARED_INTERMEDIATE_DIR)/libaddressinput/' are implementation details of
> libaddressinput. Autofill should not have those includes. '<(DEPTH)' is
included
> by default in all Chrome code. That's not necessary.

Done.

https://codereview.chromium.org/68323005/diff/130001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:87:
'<(DEPTH)/base/base.gyp:run_all_unittests',
On 2013/11/20 00:03:10, Rouslan Solomakhin wrote:
> 'libaddressinput_unittests' depends on 'run_all_unittests'? I think you meant
> the other way around.

look up run_all_unittests

https://codereview.chromium.org/68323005/diff/200001/third_party/libaddressin...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/68323005/diff/200001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:35: 'include_dirs+': [
On 2013/11/20 00:10:21, Rouslan Solomakhin wrote:
> On 2013/11/20 00:04:19, Rouslan Solomakhin wrote:
> > I think that <(DEPS) might be forced upon all targets in Chrome. Try
removing
> > the +. It should still work.
> 
> s/DEPS/DEPTH/g

there's no more use of <(DEPTH) in this rule

[Dan Beam, 2013-11-20 00:58:45]

https://codereview.chromium.org/68323005/diff/200001/third_party/libaddressin...
File third_party/libaddressinput/chromium/util/json.cc (right):

https://codereview.chromium.org/68323005/diff/200001/third_party/libaddressin...
third_party/libaddressinput/chromium/util/json.cc:30: if (dict_)
On 2013/11/20 00:07:58, Rouslan Solomakhin wrote:
> Simpler:
> 
>   DCHECK(dict_);
>   dict_->GetWithoutPathExpansion(key, &val);
> 
> (I promise libaddressinput will not call HasStringValueForKey() if
ParseObject()
> returned false.)

scoped_ptr::operator->() has a DCHECK() so i'll just remove the NULL handling

https://codereview.chromium.org/68323005/diff/200001/third_party/libaddressin...
third_party/libaddressinput/chromium/util/json.cc:41: if (dict_)
On 2013/11/20 00:07:58, Rouslan Solomakhin wrote:
> Ditto:
> 
>   DCHECK(dict_);
>   dict_->GetStringWithoutPathExpansion(key, &result);
> 
> Also, libaddressinput will not call GetStringValueForKey() if
> HasStringValueForKey() returned false.

Done.

[Dan Beam, 2013-11-20 00:59:46]

all comments addressed, ptal at your convenience

[please use gerrit instead, 2013-11-20 01:08:47]

LGTM % nits relayed offline. This patch cannot land until DEPS are rolled. We
are waiting on the go-ahead from the security review for libaddressinput.

[Dan Beam, 2013-11-20 04:08:30]

+darin for OWNERS

[darin (slow to review), 2013-11-20 05:57:16]

LGTM

[Dan Beam, 2013-11-22 22:08:25]

This is just waiting on rolling libaddressinput to r166 in DEPS.

[please use gerrit instead, 2013-11-22 22:12:28]

Rolling deps in https://codereview.chromium.org/83633003/.

[commit-bot: I haz the power, 2013-11-24 00:44:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/68323005/1350001

[commit-bot: I haz the power, 2013-11-24 19:08:50]

Change committed as 236996