Use upstream libaddressinput in Chrome.

Use upstream libaddressinput in Chrome.

Most of the changes are mechanical switches from chromium/ to src/
directory in third_party/libadressinput/.

To simplify the switch, autofill::AddressValidator is a shim between
Chromium and i18n::addressinput::AddressValidator. The shim behaves the
same way as Chromium's version of libaddressinput. The goal is to
gradually remove the shim and make Chromium work directly with
libaddressinupt.

Upstream libaddressinput does not include suggestions, which now can be
found in third_party/libadressinput/chromium/input_suggester.{h,cc}.

A follow up patch will remove Chromium's version of libaddressinput.

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

[please use gerrit instead, 2014-05-23 14:44:52]

This file gives a good overview of things still to be done :-)

In libaddressinput:

Keghani: Error message strings.
David: Required fields from static data.
Rouslan: Suggestions.
Rouslan: Synonym resolution.

In chrome:
Rouslan: Use the address formatter.

--

Would you like any of these, Fredrik?

https://codereview.chromium.org/298863012/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/298863012/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1983:
base::string16 text = base::ASCIIToUTF16("Placeholder Error. TODO.");
Need Keghani's error message strings.

https://codereview.chromium.org/298863012/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/298863012/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:100: //
HasAllRequiredFields()) {
Need David's code to get required fields from static data.

https://codereview.chromium.org/298863012/diff/20001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://codereview.chromium.org/298863012/diff/20001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/data_model_wrapper.cc:64: //
address_data->FormatForDisplay(&lines);
Need to switch to using address_formatter.h in libaddressinput.

https://codereview.chromium.org/298863012/diff/20001/third_party/libaddressin...
File third_party/libaddressinput/chromium/preload_address_validator.cc (right):

https://codereview.chromium.org/298863012/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/preload_address_validator.cc:101: // Stub
implementation. TODO: Implement! ;-)
Need the suggestions implementation from Rouslan.

https://codereview.chromium.org/298863012/diff/20001/third_party/libaddressin...
third_party/libaddressinput/chromium/preload_address_validator.cc:107: // Stub
implementation. TODO: Implement! ;-)
Need the synonym implementation from Rouslan.

[please use gerrit instead, 2014-05-23 14:49:38]

On 2014/05/23 14:44:52, Rouslan Solomakhin wrote:
> In chrome:
> Rouslan: Use the address formatter.
Anyone: Use Lara's AddressInputHelper() to to fill in admin-area, locality, etc
for the postal code.

[roubert, 2014-06-02 20:34:58]

https://codereview.chromium.org/298863012/diff/320001/third_party/libaddressi...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/298863012/diff/320001/third_party/libaddressi...
third_party/libaddressinput/libaddressinput.gyp:52: 'chromium/override/',
You got some tab characters instead of spaces from me here by mistake ... Sorry
...

[please use gerrit instead, 2014-06-02 20:40:22]

On 2014/06/02 20:34:58, roubert wrote:
>
https://codereview.chromium.org/298863012/diff/320001/third_party/libaddressi...
> File third_party/libaddressinput/libaddressinput.gyp (right):
> 
>
https://codereview.chromium.org/298863012/diff/320001/third_party/libaddressi...
> third_party/libaddressinput/libaddressinput.gyp:52: 'chromium/override/',
> You got some tab characters instead of spaces from me here by mistake ...
Sorry
> ...

Oh, no worries :0)

I will hunt them down (be wevy wevy quiet...)

[please use gerrit instead, 2014-06-05 22:22:49]

Some notes to myself on what needs to be done to be ready for a review.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1717:
Validate(CountryCodeMatcher("DE"), _, _)).Times(2).
To reduce reviewer burden: change Validate() back to ValidateAddress().

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1727:
controller()->AddressValidationRulesLoaded(true, "FR", 0);
To reduce reviewer burden: AddressValidationRulesLoaded(bool, string, int) back
to OnAddressValidationRulesLoaded(string, bool).

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1981: iter->first,
To reduce reviewer burden: add comment on what is iter->first (field) and
iter->second (problem).

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (left):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:82: public
::i18n::addressinput::LoadRulesDelegate {
To reduce reviewer burden: Bring back LoadRulesDelegate, but in autofill
namespace.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:843:
address_validation_rules_loaded_;
To reduce reviewer burden: Use autofill::LoadRulesDelegate instead of
PreloadAddressValidator::Callback address_validation_rules_loaded_.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3224:
LoadRules("US", _)).Times(AtLeast(1));
To reduce reviewer burden: Make LoadRules() not take a callback but instead call
LoadRulesDelegate when finished.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (left):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:97:
HasAllRequiredFields()) {
To reduce reviewer burden: make a HasAllRequiredFields() static helper in
third_party/libaddressinput/chromium/has_all_required_fields.{h,cc}.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:209: return true;
Investigate: this might have to return false due to no field for this
|server_type|.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:225: std::string
best_address_language;
To reduce reviewer burden: put this into a function BuildCompnents(country-code,
&best_address_langauge) in anonymous namespace.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc (left):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc:34:
BuildAddressInputs(common::ADDRESS_TYPE_BILLING, "US", &inputs, NULL);
To reduce reviewer burden: allow NULL for BuildAddressInputs() language code
parameter.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/country_combobox_model_unittest.cc (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/country_combobox_model_unittest.cc:57: country_code,
localization, &best_address_language);
Reduce reviewer burden: NULL should be OK here.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/data_model_wrapper.cc:78: //     GetLanguageCode());
Use ::i18n::addressinput::GetStreetAddressLinesAsSingleLine() here!

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/mock_address_validator.cc (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/mock_address_validator.cc:12: ON_CALL(*this,
LoadRules(_, _)).WillByDefault(Return());
To reduce reviewer burden: Single parameter.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/mock_address_validator.cc:13: ON_CALL(*this,
Validate(_, _, _)).WillByDefault(Return(SUCCESS));
To reduce reviewer burden: ValidateAddress.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/mock_address_validator.h (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/mock_address_validator.h:26: void(const std::string&
region_code, const Callback& loaded));
To reduce reviewer burden: No callback.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/mock_address_validator.h:28:
MOCK_CONST_METHOD3(Validate,
To reduce reviewer burden: ValidateAddress.

https://codereview.chromium.org/298863012/diff/510001/components/autofill/cor...
File components/autofill/core/browser/autofill_country.cc (right):

https://codereview.chromium.org/298863012/diff/510001/components/autofill/cor...
components/autofill/core/browser/autofill_country.cc:1058: }
Investigate: This change be unnecessary after the AN and XK changes in the enum
above.

https://codereview.chromium.org/298863012/diff/510001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (left):

https://codereview.chromium.org/298863012/diff/510001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:301: if
(!address_data->HasAllRequiredFields())
To reduce reviewer burden: Use the has_all_required_fileds.{h,cc} helper in
third_party/libaddressinput/chromium.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/README.chromium (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/README.chromium:22: - Copy src/cpp/res/messages.grdp
to ../../chrome/app/address_input_strings.grdp.
To reduce reviewer burden: Last line is not necessary.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/canonicalize_string.cc (left):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/canonicalize_string.cc:1: // Copyright 2014
The Chromium Authors. All rights reserved.
To reduce reviewer burden: Don't delete all unused files in this patch. Do it in
a follow up.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/json.cc (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/json.cc:5: // Use
"base/memory/scoped_ptr.h" instead.
Investigate: No longer necessary do this, is it?

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/json.cc:14: // Must be before json.h to be
used in the Json class.
Remove this comment.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/localization.cc (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/localization.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
Remove this localization.cc file! (It's not used in the build.)

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/preload_address_validator_unittest.cc
(left):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/preload_address_validator_unittest.cc:254:
validator_->GetSuggestions(address, COUNTRY, 1, NULL));
Add back this test! Handle NULL in PreloadAddressValdiator::GetSuggestions().

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/preload_address_validator_unittest.cc
(right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/preload_address_validator_unittest.cc:566: 
Add a test SuggestionsAreCleared!

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/suggestions.cc (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/suggestions.cc:119: for
(RegionMap::const_iterator region_it = tries_.begin();
Investigate: can we put RegionMap into its own struct that takes care of
insertion, iteration, and destruction?

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/suggestions.cc:151: if (focused_field !=
POSTAL_CODE &&
Remove special handling for POSTAL CODE!

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/suggestions.cc:198: scoped_ptr<RegionIds>
precise_regions;
Use more data structures that hide the complexity of the maps inside of
themselves!

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/suggestions.h (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/suggestions.h:56: // If the index is not
built, then builds it.
Reduce reviewer burden: Remove extra newline.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/suggestions_unittest.cc (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/suggestions_unittest.cc:2: // Use of this
source code is governed by a BSD-style license that can be
Remove this file! (PreloadAddressValidatorTests have all the tests.)

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/trie.cc (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/trie.cc:23: }  // namespace i18n
No need for closing namespace comment on a forward declaration.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/libaddressinput.gyp:7: 'cflags!':
['-fvisibility=hidden'],
Note: This may not work on some platforms, so would need to make it
platform-dependent or make the library static like some other third party
libraries in Chromium.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/libaddressinput.gyp:52: 'chromium/override/',
Remove tab!

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/libaddressinput.gyp:57:
'I18N_ADDRESSINPUT_USE_BASICTYPES_OVERRIDE=1',
Remove tab!

[roubert, 2014-06-06 14:17:03]

One thing worth thinking about is those changes to make the change easier to
review: The decision I took when writing the PreloadAddressValidator glue class
was to keep behaviour unchanged but change names and mechanisms
(AddressValidator -> Validator; LoadRulesDelegate -> Callback) to use
libaddressinput conventions. That seemed like a good balance between both
keeping the set of integration changes small and the glue class simple.

[please use gerrit instead, 2014-06-06 16:53:55]

On 2014/06/06 14:17:03, roubert wrote:
> One thing worth thinking about is those changes to make the change easier to
> review: The decision I took when writing the PreloadAddressValidator glue
class
> was to keep behaviour unchanged but change names and mechanisms
> (AddressValidator -> Validator; LoadRulesDelegate -> Callback) to use
> libaddressinput conventions. That seemed like a good balance between both
> keeping the set of integration changes small and the glue class simple.

Makes sense. I will try to avoid making the glue class unwieldy as well. The
glue class should eventually go away in favor of using the actual
libaddressinput API. Do think that's a good idea?

[please use gerrit instead, 2014-06-09 23:28:18]

Some things accomplished.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1717:
Validate(CountryCodeMatcher("DE"), _, _)).Times(2).
On 2014/06/05 22:22:47, Rouslan Solomakhin wrote:
> To reduce reviewer burden: change Validate() back to ValidateAddress().

Done.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1981: iter->first,
On 2014/06/05 22:22:47, Rouslan Solomakhin wrote:
> To reduce reviewer burden: add comment on what is iter->first (field) and
> iter->second (problem).

Done.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3224:
LoadRules("US", _)).Times(AtLeast(1));
On 2014/06/05 22:22:47, Rouslan Solomakhin wrote:
> To reduce reviewer burden: Make LoadRules() not take a callback but instead
call
> LoadRulesDelegate when finished.

Done.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (left):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:97:
HasAllRequiredFields()) {
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> To reduce reviewer burden: make a HasAllRequiredFields() static helper in
> third_party/libaddressinput/chromium/has_all_required_fields.{h,cc}.

Done.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:209: return true;
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> Investigate: this might have to return false due to no field for this
> |server_type|.

Done.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:225: std::string
best_address_language;
On 2014/06/05 22:22:47, Rouslan Solomakhin wrote:
> To reduce reviewer burden: put this into a function
BuildCompnents(country-code,
> &best_address_langauge) in anonymous namespace.

Done.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc (left):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc:34:
BuildAddressInputs(common::ADDRESS_TYPE_BILLING, "US", &inputs, NULL);
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> To reduce reviewer burden: allow NULL for BuildAddressInputs() language code
> parameter.

Done.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/country_combobox_model_unittest.cc (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/country_combobox_model_unittest.cc:57: country_code,
localization, &best_address_language);
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> Reduce reviewer burden: NULL should be OK here.

Nope, that's a libaddressinput function.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/data_model_wrapper.cc:78: //     GetLanguageCode());
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> Use ::i18n::addressinput::GetStreetAddressLinesAsSingleLine() here!

Done.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/mock_address_validator.cc (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/mock_address_validator.cc:12: ON_CALL(*this,
LoadRules(_, _)).WillByDefault(Return());
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> To reduce reviewer burden: Single parameter.

Done.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/mock_address_validator.cc:13: ON_CALL(*this,
Validate(_, _, _)).WillByDefault(Return(SUCCESS));
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> To reduce reviewer burden: ValidateAddress.

Done.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/mock_address_validator.h (right):

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/mock_address_validator.h:26: void(const std::string&
region_code, const Callback& loaded));
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> To reduce reviewer burden: No callback.

Done.

https://codereview.chromium.org/298863012/diff/510001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/mock_address_validator.h:28:
MOCK_CONST_METHOD3(Validate,
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> To reduce reviewer burden: ValidateAddress.

Done.

https://codereview.chromium.org/298863012/diff/510001/components/autofill/cor...
File components/autofill/core/browser/autofill_country.cc (right):

https://codereview.chromium.org/298863012/diff/510001/components/autofill/cor...
components/autofill/core/browser/autofill_country.cc:1058: }
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> Investigate: This change be unnecessary after the AN and XK changes in the
enum
> above.

Done.

https://codereview.chromium.org/298863012/diff/510001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (left):

https://codereview.chromium.org/298863012/diff/510001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:301: if
(!address_data->HasAllRequiredFields())
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> To reduce reviewer burden: Use the has_all_required_fileds.{h,cc} helper in
> third_party/libaddressinput/chromium.

Done.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/README.chromium (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/README.chromium:22: - Copy src/cpp/res/messages.grdp
to ../../chrome/app/address_input_strings.grdp.
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> To reduce reviewer burden: Last line is not necessary.

Done.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/json.cc (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/json.cc:5: // Use
"base/memory/scoped_ptr.h" instead.
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> Investigate: No longer necessary do this, is it?

Done.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/json.cc:14: // Must be before json.h to be
used in the Json class.
On 2014/06/05 22:22:48, Rouslan Solomakhin wrote:
> Remove this comment.

Done.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/localization.cc (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/localization.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/06/05 22:22:49, Rouslan Solomakhin wrote:
> Remove this localization.cc file! (It's not used in the build.)

Done.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/preload_address_validator_unittest.cc
(left):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/preload_address_validator_unittest.cc:254:
validator_->GetSuggestions(address, COUNTRY, 1, NULL));
On 2014/06/05 22:22:49, Rouslan Solomakhin wrote:
> Add back this test! Handle NULL in PreloadAddressValdiator::GetSuggestions().

Done.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/preload_address_validator_unittest.cc
(right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/preload_address_validator_unittest.cc:566: 
On 2014/06/05 22:22:49, Rouslan Solomakhin wrote:
> Add a test SuggestionsAreCleared!

Done.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/suggestions.cc (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/suggestions.cc:151: if (focused_field !=
POSTAL_CODE &&
On 2014/06/05 22:22:49, Rouslan Solomakhin wrote:
> Remove special handling for POSTAL CODE!

Simply removing handling of postal code will break functionality. This change
would require a lot of re-thinking of logic of this code.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/suggestions.h (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/suggestions.h:56: // If the index is not
built, then builds it.
On 2014/06/05 22:22:49, Rouslan Solomakhin wrote:
> Reduce reviewer burden: Remove extra newline.

Done.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/suggestions_unittest.cc (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/suggestions_unittest.cc:2: // Use of this
source code is governed by a BSD-style license that can be
On 2014/06/05 22:22:49, Rouslan Solomakhin wrote:
> Remove this file! (PreloadAddressValidatorTests have all the tests.)

Done.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/chromium/trie.cc (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/chromium/trie.cc:23: }  // namespace i18n
On 2014/06/05 22:22:49, Rouslan Solomakhin wrote:
> No need for closing namespace comment on a forward declaration.

Done.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/libaddressinput.gyp:52: 'chromium/override/',
On 2014/06/05 22:22:49, Rouslan Solomakhin wrote:
> Remove tab!

Done.

https://codereview.chromium.org/298863012/diff/510001/third_party/libaddressi...
third_party/libaddressinput/libaddressinput.gyp:57:
'I18N_ADDRESSINPUT_USE_BASICTYPES_OVERRIDE=1',
On 2014/06/05 22:22:49, Rouslan Solomakhin wrote:
> Remove tab!

Done.

[please use gerrit instead, 2014-06-10 00:05:09]

Evan: PTAL Patch Set 13.

I'm still figuring out how to make the suggestions.{h,cc}. implementation easier
to follow, but all of the moving parts are working correctly and passing our
numerous tests.

This patch needs https://codereview.chromium.org/304223010/ to land, which is
stuck on android_aosp bot at the moment. I'm investigating how to get it
unstuck.

[Evan Stade, 2014-06-12 00:18:09]

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1967:
l10n_util::GetStringUTF8, g_browser_process->GetApplicationLocale());
I don't understand the purpose of passing the application locale, and this
comment doesn't help me:

"The |language_tag| parameter is used only for information purposes here."

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1978: /*field*/
iter->first,
nit: remove inline comments

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:52: 
^H^H

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:104: if
(!HasAllRequiredFields(*i18n::CreateAddressDataFromAutofillProfile(
HasAllRequiredFields probably should have a namespace

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc (right):

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc:82: bool
should_be_supported = !(regions[i] == "KR" ||
you can just leave this alone, it's about to get deleted

https://codereview.chromium.org/322453003/

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/data_model_wrapper_unittest.cc (right):

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/data_model_wrapper_unittest.cc:102:
ASCIIToUTF16("John H. Doe, 666 Erebus St., Apt 8, Elysium CA 91111\n"
this breaks the test

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/data_model_wrapper_unittest.cc:110: "Elysium CA
91111\n"
ditto

https://codereview.chromium.org/298863012/diff/980054/components/autofill.gypi
File components/autofill.gypi (right):

https://codereview.chromium.org/298863012/diff/980054/components/autofill.gyp...
components/autofill.gypi:100:
'../third_party/libaddressinput/libaddressinput.gyp:libaddressinput',
why this change?

we specifically made a separate target so android etc. doesn't have to pull in
all of libaddressinput

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/chrome_downloader_impl.cc (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/chrome_downloader_impl.cc:72:
request->second->callback(ok, request->second->url, data);
why add a copy operation?

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/chrome_storage_impl.h (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/chrome_storage_impl.h:30: virtual void
Put(const std::string& key, const std::string& data) OVERRIDE;
why add a copy operation

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/preload_address_validator.h (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/preload_address_validator.h:40: //
different directories) gracefully. This class uses preloaded validation rules
what? so chrome doesn't load validation rules on demand any more?

I think that's false and "preload" is a bad name.

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/storage_test_runner.h (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/storage_test_runner.h:18: class
StorageTestRunner {
class level comment

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/suggestions.h (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/suggestions.h:23: class Suggestions {
class level comment

better class name needed. Most class names describe what they do, not what they
return. Hence InputSuggester, InputAssistant, etc.

[please use gerrit instead, 2014-06-13 19:22:09]

Evan: PTAL Patch Set 15. Thank you for the review!

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1967:
l10n_util::GetStringUTF8, g_browser_process->GetApplicationLocale());
On 2014/06/12 00:18:08, Evan Stade wrote:
> I don't understand the purpose of passing the application locale, and this
> comment doesn't help me:
> 
> "The |language_tag| parameter is used only for information purposes here."

Improved the comment in https://codereview.appspot.com/109940045/.

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1978: /*field*/
iter->first,
On 2014/06/12 00:18:08, Evan Stade wrote:
> nit: remove inline comments

I removed the inline comments, but replaced them with constants, because it's
difficult to read localization.GetErrorMessage(*address_data, iter->second,
true, false), IMHO.

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:52: 
On 2014/06/12 00:18:08, Evan Stade wrote:
> ^H^H

Done.

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:104: if
(!HasAllRequiredFields(*i18n::CreateAddressDataFromAutofillProfile(
On 2014/06/12 00:18:08, Evan Stade wrote:
> HasAllRequiredFields probably should have a namespace

How about "addressinput" namespace?

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc (right):

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc:82: bool
should_be_supported = !(regions[i] == "KR" ||
On 2014/06/12 00:18:08, Evan Stade wrote:
> you can just leave this alone, it's about to get deleted
> 
> https://codereview.chromium.org/322453003/

Done.

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/data_model_wrapper_unittest.cc (right):

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/data_model_wrapper_unittest.cc:102:
ASCIIToUTF16("John H. Doe, 666 Erebus St., Apt 8, Elysium CA 91111\n"
On 2014/06/12 00:18:08, Evan Stade wrote:
> this breaks the test

Neither
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libadd...
nor https://i18napis.appspot.com/ssl-address/data/US have a comma between city
and state for US. We have three options: (1) live with it, (2) fix Google's
data, and (3) workaround in Chromium. I vote for (1) :-D. WDYT?

https://codereview.chromium.org/298863012/diff/980054/components/autofill.gypi
File components/autofill.gypi (right):

https://codereview.chromium.org/298863012/diff/980054/components/autofill.gyp...
components/autofill.gypi:100:
'../third_party/libaddressinput/libaddressinput.gyp:libaddressinput',
On 2014/06/12 00:18:09, Evan Stade wrote:
> why this change?
> 
> we specifically made a separate target so android etc. doesn't have to pull in
> all of libaddressinput

Good point, I originally renamed libaddressinput -> libadressinput_chromium and
libaddressinput_util -> libaddressinput. That was not the best choice in
retrospect. I've changed it back.

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/chrome_downloader_impl.cc (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/chrome_downloader_impl.cc:72:
request->second->callback(ok, request->second->url, data);
On 2014/06/12 00:18:09, Evan Stade wrote:
> why add a copy operation?

Done.

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/chrome_storage_impl.h (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/chrome_storage_impl.h:30: virtual void
Put(const std::string& key, const std::string& data) OVERRIDE;
On 2014/06/12 00:18:09, Evan Stade wrote:
> why add a copy operation

I don't think switching from scoped_ptr::Pass() to const-ref is much of an
impact. If you want to switch to scoped_ptr::Pass(), I can submit a patch to
https://code.google.com/p/libaddressinput/ after this patch lands.

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/preload_address_validator.h (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/preload_address_validator.h:40: //
different directories) gracefully. This class uses preloaded validation rules
On 2014/06/12 00:18:09, Evan Stade wrote:
> what? so chrome doesn't load validation rules on demand any more?
> 
> I think that's false and "preload" is a bad name.

Chrome loads all rules for a country when the user switches to that country in
the drop-down. In libaddressinput parlance, that's preloading. We can rename
preload_address_validator.h into something more comprehensible. Do you have
suggestions?

Libaddressinput also provides an alternative: on-demand loading of individual
rules at state, city, and dependent-locality level. This is how libaddresinput
is used internally, on the server side.

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/storage_test_runner.h (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/storage_test_runner.h:18: class
StorageTestRunner {
On 2014/06/12 00:18:09, Evan Stade wrote:
> class level comment

Done.

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/suggestions.h (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/suggestions.h:23: class Suggestions {
On 2014/06/12 00:18:09, Evan Stade wrote:
> class level comment
> 
> better class name needed. Most class names describe what they do, not what
they
> return. Hence InputSuggester, InputAssistant, etc.

Done.

[Evan Stade, 2014-06-13 20:07:09]

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1967:
l10n_util::GetStringUTF8, g_browser_process->GetApplicationLocale());
On 2014/06/13 19:22:09, Rouslan Solomakhin wrote:
> On 2014/06/12 00:18:08, Evan Stade wrote:
> > I don't understand the purpose of passing the application locale, and this
> > comment doesn't help me:
> > 
> > "The |language_tag| parameter is used only for information purposes here."
> 
> Improved the comment in https://codereview.appspot.com/109940045/.

still don't get it. You don't need the locale here at all because you never call
BuildComponents() on this Localization instance (right?).

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1978: /*field*/
iter->first,
On 2014/06/13 19:22:08, Rouslan Solomakhin wrote:
> On 2014/06/12 00:18:08, Evan Stade wrote:
> > nit: remove inline comments
> 
> I removed the inline comments, but replaced them with constants, because it's
> difficult to read localization.GetErrorMessage(*address_data, iter->second,
> true, false), IMHO.

if you must document the parameters here, you can use C++ style comments after
the |true| and |false|

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:104: if
(!HasAllRequiredFields(*i18n::CreateAddressDataFromAutofillProfile(
On 2014/06/13 19:22:09, Rouslan Solomakhin wrote:
> On 2014/06/12 00:18:08, Evan Stade wrote:
> > HasAllRequiredFields probably should have a namespace
> 
> How about "addressinput" namespace?

ok

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/data_model_wrapper_unittest.cc (right):

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/data_model_wrapper_unittest.cc:102:
ASCIIToUTF16("John H. Doe, 666 Erebus St., Apt 8, Elysium CA 91111\n"
On 2014/06/13 19:22:09, Rouslan Solomakhin wrote:
> On 2014/06/12 00:18:08, Evan Stade wrote:
> > this breaks the test
> 
> Neither
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libadd...
> nor https://i18napis.appspot.com/ssl-address/data/US have a comma between city
> and state for US. We have three options: (1) live with it, (2) fix Google's
> data, and (3) workaround in Chromium. I vote for (1) :-D. WDYT?

I believe we've had this discussion before, and you know that option (1), aka
break a product that previously worked, is not OK. Google doesn't always get
this wrong, see [1]. Hence I vote for (2), but irreconcilable differences may
force us to go with (3).

[1] http://i.imgur.com/XdHR4N1.png

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/chrome_storage_impl.h (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/chrome_storage_impl.h:30: virtual void
Put(const std::string& key, const std::string& data) OVERRIDE;
On 2014/06/13 19:22:09, Rouslan Solomakhin wrote:
> On 2014/06/12 00:18:09, Evan Stade wrote:
> > why add a copy operation
> 
> I don't think switching from scoped_ptr::Pass() to const-ref is much of an
> impact. If you want to switch to scoped_ptr::Pass(), I can submit a patch to
> https://code.google.com/p/libaddressinput/ after this patch lands.

I think you should fix upstream before landing this patch, because it would
greatly reduce the size of this patch

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/preload_address_validator.h (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/preload_address_validator.h:40: //
different directories) gracefully. This class uses preloaded validation rules
On 2014/06/13 19:22:09, Rouslan Solomakhin wrote:
> On 2014/06/12 00:18:09, Evan Stade wrote:
> > what? so chrome doesn't load validation rules on demand any more?
> > 
> > I think that's false and "preload" is a bad name.
> 
> Chrome loads all rules for a country when the user switches to that country in
> the drop-down. In libaddressinput parlance, that's preloading. We can rename
> preload_address_validator.h into something more comprehensible. Do you have
> suggestions?
> 
> Libaddressinput also provides an alternative: on-demand loading of individual
> rules at state, city, and dependent-locality level. This is how libaddresinput
> is used internally, on the server side.

It does load the rules on demand (saying it doesn't implies to me that it has a
stock set of rules that's never updated). It's just not as lazy as it could be
and doesn't reload for every request. You could just call the file
chromium_address_validator I guess, or rule_caching_address_validator

https://codereview.chromium.org/298863012/diff/1040001/third_party/libaddress...
File third_party/libaddressinput/chromium/chrome_storage_impl.cc (left):

https://codereview.chromium.org/298863012/diff/1040001/third_party/libaddress...
third_party/libaddressinput/chromium/chrome_storage_impl.cc:24:
string_value->GetString()->swap(*data);
not a copy...

https://codereview.chromium.org/298863012/diff/1040001/third_party/libaddress...
File third_party/libaddressinput/chromium/chrome_storage_impl.cc (right):

https://codereview.chromium.org/298863012/diff/1040001/third_party/libaddress...
third_party/libaddressinput/chromium/chrome_storage_impl.cc:23:
scoped_ptr<base::StringValue> string_value(new base::StringValue(data));
...to copy

[please use gerrit instead, 2014-06-16 19:02:24]

Evan: PTAL Patch Set 16. (Note that it does not build until libaddressinput has
scoped_ptr for Downloader and Storage, which is work in progress on i18n team's
end.)

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/298863012/diff/980054/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1978: /*field*/
iter->first,
On 2014/06/13 20:07:08, Evan Stade wrote:
> On 2014/06/13 19:22:08, Rouslan Solomakhin wrote:
> > On 2014/06/12 00:18:08, Evan Stade wrote:
> > > nit: remove inline comments
> > 
> > I removed the inline comments, but replaced them with constants, because
it's
> > difficult to read localization.GetErrorMessage(*address_data, iter->second,
> > true, false), IMHO.
> 
> if you must document the parameters here, you can use C++ style comments after
> the |true| and |false|

Done.

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
File third_party/libaddressinput/chromium/preload_address_validator.h (right):

https://codereview.chromium.org/298863012/diff/980054/third_party/libaddressi...
third_party/libaddressinput/chromium/preload_address_validator.h:40: //
different directories) gracefully. This class uses preloaded validation rules
On 2014/06/13 20:07:08, Evan Stade wrote:
> On 2014/06/13 19:22:09, Rouslan Solomakhin wrote:
> > On 2014/06/12 00:18:09, Evan Stade wrote:
> > > what? so chrome doesn't load validation rules on demand any more?
> > > 
> > > I think that's false and "preload" is a bad name.
> > 
> > Chrome loads all rules for a country when the user switches to that country
in
> > the drop-down. In libaddressinput parlance, that's preloading. We can rename
> > preload_address_validator.h into something more comprehensible. Do you have
> > suggestions?
> > 
> > Libaddressinput also provides an alternative: on-demand loading of
individual
> > rules at state, city, and dependent-locality level. This is how
libaddresinput
> > is used internally, on the server side.
> 
> It does load the rules on demand (saying it doesn't implies to me that it has
a
> stock set of rules that's never updated). It's just not as lazy as it could be
> and doesn't reload for every request. You could just call the file
> chromium_address_validator I guess, or rule_caching_address_validator

Changed to chrome_address_validator to be similar to chrome_storage_impl and
chrome_downloader_impl.

https://codereview.chromium.org/298863012/diff/1040001/third_party/libaddress...
File third_party/libaddressinput/chromium/chrome_storage_impl.cc (left):

https://codereview.chromium.org/298863012/diff/1040001/third_party/libaddress...
third_party/libaddressinput/chromium/chrome_storage_impl.cc:24:
string_value->GetString()->swap(*data);
On 2014/06/13 20:07:09, Evan Stade wrote:
> not a copy...

Now I see. Thank you for pointing it out.

https://codereview.chromium.org/298863012/diff/1040001/third_party/libaddress...
File third_party/libaddressinput/chromium/chrome_storage_impl.cc (right):

https://codereview.chromium.org/298863012/diff/1040001/third_party/libaddress...
third_party/libaddressinput/chromium/chrome_storage_impl.cc:23:
scoped_ptr<base::StringValue> string_value(new base::StringValue(data));
On 2014/06/13 20:07:09, Evan Stade wrote:
> ...to copy

i18n team is changing this to scoped_ptr.

[please use gerrit instead, 2014-06-16 23:37:57]

Evan: PTAL Patch Set 19. It uses the latest revision of libaddressinput that
allocates strings on the heap for Downloader and Storage. Thus Chrome
implementations of these interfaces can avoid copying strings.

[Evan Stade, 2014-06-19 00:38:00]

https://codereview.chromium.org/298863012/diff/1230001/third_party/libaddress...
File third_party/libaddressinput/chromium/chrome_downloader_impl.cc (right):

https://codereview.chromium.org/298863012/diff/1230001/third_party/libaddress...
third_party/libaddressinput/chromium/chrome_downloader_impl.cc:72:
request->second->callback(ok, request->second->url, data.release());
pretty lame that libaddressinput doesn't use scoped_ptr...

https://codereview.chromium.org/298863012/diff/1230001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1230001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:62: bool TestBit(Bitset
bitset, int bit) {
this is not the idiomatic way to deal with bitmasks. RegionField should look
like:

enum RegionField {
  KEY = 1 << 0,
  NAME = 1 << 1,
};

Then you can do normal bit operations

https://codereview.chromium.org/298863012/diff/1230001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:83: class
ParentedRegionCollector {
you created this entire class so you could use std::for_each?

https://codereview.chromium.org/298863012/diff/1230001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:325: // Cleanup.
please do not try to handle bare pointers, use stl_util

https://codereview.chromium.org/298863012/diff/1230001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:345: typedef
std::map<RegionField, RegionDataTrie*> IdMap;
serious? you need a map for this? RegionField only has two possible values...
how about a pair or a struct?

[please use gerrit instead, 2014-06-20 21:09:15]

Evan: PTAL Patch Set 23.

I switched InputSuggester from maps and bitmaps to simple structs and classes.
I'm still working on fixing a few corner-case tests and improving documentation,
etc, but this is very close the final version of InputSuggester.

https://codereview.chromium.org/298863012/diff/1230001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1230001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:62: bool TestBit(Bitset
bitset, int bit) {
On 2014/06/19 00:38:00, Evan Stade wrote:
> this is not the idiomatic way to deal with bitmasks. RegionField should look
> like:
> 
> enum RegionField {
>   KEY = 1 << 0,
>   NAME = 1 << 1,
> };
> 
> Then you can do normal bit operations

Got rid of bitmaps all together.

https://codereview.chromium.org/298863012/diff/1230001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:83: class
ParentedRegionCollector {
On 2014/06/19 00:38:00, Evan Stade wrote:
> you created this entire class so you could use std::for_each?

Removed and came up with a better implementation.

https://codereview.chromium.org/298863012/diff/1230001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:325: // Cleanup.
On 2014/06/19 00:38:00, Evan Stade wrote:
> please do not try to handle bare pointers, use stl_util

Done.

https://codereview.chromium.org/298863012/diff/1230001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:345: typedef
std::map<RegionField, RegionDataTrie*> IdMap;
On 2014/06/19 00:38:00, Evan Stade wrote:
> serious? you need a map for this? RegionField only has two possible values...
> how about a pair or a struct?

Switched to structs and classes.

[Evan Stade, 2014-06-20 22:32:27]

On 2014/06/20 21:09:15, Rouslan Solomakhin wrote:
> Evan: PTAL Patch Set 23.
> 
> I switched InputSuggester from maps and bitmaps to simple structs and classes.
> I'm still working on fixing a few corner-case tests and improving
documentation,
> etc, but this is very close the final version of InputSuggester.

InputSuggester just ballooned from 484 lines to almost 700 in the latest patch.
The code is long and hard for me to understand. Is there no way to shorten it?
The fact that it was a third shorter before suggests it may be possible.

Also, documentation is at its most useful during code review, so please write it
before sending for review.

https://codereview.chromium.org/298863012/diff/1350001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1350001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:258: // This algorithm
assumes that iterating over a set is sorted.
comment doesn't make sense (to me at least)

[roubert, 2014-06-24 15:12:56]

https://codereview.chromium.org/298863012/diff/1490001/third_party/libaddress...
File third_party/libaddressinput/chromium/string_compare.cc (right):

https://codereview.chromium.org/298863012/diff/1490001/third_party/libaddress...
third_party/libaddressinput/chromium/string_compare.cc:22: return
const_cast<Impl*>(this)->NaturalCompare(a, b) == 0;
Is this const_cast<> here to make it possible to call NaturalCompare() which
isn't const because collator_ isn't declared mutable? If so, it would be better
to just declare collator_ mutable and NaturalCompare() const instead.

[please use gerrit instead, 2014-06-24 21:23:43]

Evan: PTAL Patch Set 28.

https://codereview.chromium.org/298863012/diff/1350001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1350001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:258: // This algorithm
assumes that iterating over a set is sorted.
On 2014/06/20 22:32:27, Evan Stade wrote:
> comment doesn't make sense (to me at least)

Improved the comments and reduced the number of lines.

https://codereview.chromium.org/298863012/diff/1490001/third_party/libaddress...
File third_party/libaddressinput/chromium/string_compare.cc (right):

https://codereview.chromium.org/298863012/diff/1490001/third_party/libaddress...
third_party/libaddressinput/chromium/string_compare.cc:22: return
const_cast<Impl*>(this)->NaturalCompare(a, b) == 0;
On 2014/06/24 15:12:55, roubert wrote:
> Is this const_cast<> here to make it possible to call NaturalCompare() which
> isn't const because collator_ isn't declared mutable? If so, it would be
better
> to just declare collator_ mutable and NaturalCompare() const instead.

Done.

[Evan Stade, 2014-06-25 03:37:16]

InputSuggester looks better, thanks

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:85: const bool
region_name_matches;
this isn't used anywhere

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:247:
std::map<AddressField, ScopedVector<Suggestion>*> suggestions_;
why ScopedVector* instead of ScopedVector?

Why ScopedVector instead of std::vector? Suggestion is just 4 PODs so copying it
around a little bit is ok.

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:277: std::string
Canonicalize(const std::string& original) const {
this function makes me queasy...

[please use gerrit instead, 2014-06-25 20:41:16]

Evan: PTAL Patch Set 32.

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:85: const bool
region_name_matches;
On 2014/06/25 03:37:16, Evan Stade wrote:
> this isn't used anywhere

Removed this var and expanded the description of "region_key_matches" above.

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:247:
std::map<AddressField, ScopedVector<Suggestion>*> suggestions_;
On 2014/06/25 03:37:16, Evan Stade wrote:
> why ScopedVector* instead of ScopedVector?
> 
> Why ScopedVector instead of std::vector? Suggestion is just 4 PODs so copying
it
> around a little bit is ok.

I'd rather not copy anything to avoid accidentally introducing perf bottlenecks.
Perf has room to improve here. The code is not too complex in this
configuration, is it?

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:277: std::string
Canonicalize(const std::string& original) const {
On 2014/06/25 03:37:16, Evan Stade wrote:
> this function makes me queasy...

Changed to maintain a buffer between calls to Canonicalize() and double the
buffer size when it's too small. This results in logarithmic number of buffer
allocations.

Btw, this is essentially CountryNames::GetSortKey() from Chromium:

https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

ICU does not use STL, which makes my life harder here.

[please use gerrit instead, 2014-06-26 00:35:16]

Notes on changes to autofill_country.cc.

https://codereview.chromium.org/298863012/diff/1740001/components/autofill/co...
File components/autofill/core/browser/autofill_country.cc (left):

https://codereview.chromium.org/298863012/diff/1740001/components/autofill/co...
components/autofill/core/browser/autofill_country.cc:71:
ADDRESS_REQUIREMENTS_UNKNOWN } },
This country no longer exists.

https://codereview.chromium.org/298863012/diff/1740001/components/autofill/co...
File components/autofill/core/browser/autofill_country.cc (right):

https://codereview.chromium.org/298863012/diff/1740001/components/autofill/co...
components/autofill/core/browser/autofill_country.cc:50: ADDRESS_REQUIRES_CITY }
},
These countries are added because they are in libaddressinput.

https://codereview.chromium.org/298863012/diff/1740001/components/autofill/co...
components/autofill/core/browser/autofill_country.cc:920:
common_names_.insert(std::make_pair(iso3_country_code, country_code));
ICU uses data from CLDR, but is out-of-date from time to time. For example, CLDR
(and libaddressinput) have the country "AC", but ICU does not. ICU would return
and empty ISO3 country code for "AC". We don't want an empty string as a key in
|common_names_| map, because that would cause all countryless profiles to be get
"AC" country code.

[Evan Stade, 2014-06-26 00:46:15]

I'm not sure if it's worth doing now, but I'm pretty sure you could break this
code review into smaller chunks to make reviewing easier (and it would probably
expedite the overall process).

For example, you could:
a) add new files in one CL, then switch existing code to use the new files in
another CL
b) develop behind a flag
c) change downstream libaddressinput to match upstream piecemeal (one CL =
smallest chunk that can compile), changing the appropriate parts of Chrome's
code as needed, before finally switching to directly using upstream
libaddressinput. I wouldn't spend a lot of time on this, but if you can make the
APIs match without much difficulty (e.g. you can just change a scoped_ptr.Pass()
to a scoped_ptr.Release()), that would be one way to carve off a smaller piece
of this change.

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:247:
std::map<AddressField, ScopedVector<Suggestion>*> suggestions_;
On 2014/06/25 20:41:15, Rouslan Solomakhin wrote:
> On 2014/06/25 03:37:16, Evan Stade wrote:
> > why ScopedVector* instead of ScopedVector?
> > 
> > Why ScopedVector instead of std::vector? Suggestion is just 4 PODs so
copying
> it
> > around a little bit is ok.
> 
> I'd rather not copy anything to avoid accidentally introducing perf
bottlenecks.
> Perf has room to improve here. The code is not too complex in this
> configuration, is it?

Yes, the code is more complex (explicit memory management). No, the perf is not
any better (copying 9 bytes instead of 4 is not going to make a measurable
impact).

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:277: std::string
Canonicalize(const std::string& original) const {
On 2014/06/25 20:41:15, Rouslan Solomakhin wrote:
> On 2014/06/25 03:37:16, Evan Stade wrote:
> > this function makes me queasy...
> 
> Changed to maintain a buffer between calls to Canonicalize() and double the
> buffer size when it's too small. This results in logarithmic number of buffer
> allocations.
> 
> Btw, this is essentially CountryNames::GetSortKey() from Chromium:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
> 
> ICU does not use STL, which makes my life harder here.

Allocating the buffer isn't what worried me. It's mainly the casting between
types. Also using buffer_size in the string constructor would add a little more
safety.

[please use gerrit instead, 2014-06-26 22:49:50]

Evan: PTAL Patch Set 36.

I'll be sending your smaller CLs now following plan (c) for data structures:
overwrite address_field.h and address_data.h in chromium/ with files from src/.
Then it may become possible to put a command-line flag in
chrome_address_validator.cc to use either of the two libaddressinput
implementations.

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:247:
std::map<AddressField, ScopedVector<Suggestion>*> suggestions_;
On 2014/06/26 00:46:15, Evan Stade wrote:
> On 2014/06/25 20:41:15, Rouslan Solomakhin wrote:
> > On 2014/06/25 03:37:16, Evan Stade wrote:
> > > why ScopedVector* instead of ScopedVector?
> > > 
> > > Why ScopedVector instead of std::vector? Suggestion is just 4 PODs so
> copying
> > it
> > > around a little bit is ok.
> > 
> > I'd rather not copy anything to avoid accidentally introducing perf
> bottlenecks.
> > Perf has room to improve here. The code is not too complex in this
> > configuration, is it?
> 
> Yes, the code is more complex (explicit memory management). No, the perf is
not
> any better (copying 9 bytes instead of 4 is not going to make a measurable
> impact).

Done.

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:277: std::string
Canonicalize(const std::string& original) const {
On 2014/06/26 00:46:15, Evan Stade wrote:
> On 2014/06/25 20:41:15, Rouslan Solomakhin wrote:
> > On 2014/06/25 03:37:16, Evan Stade wrote:
> > > this function makes me queasy...
> > 
> > Changed to maintain a buffer between calls to Canonicalize() and double the
> > buffer size when it's too small. This results in logarithmic number of
buffer
> > allocations.
> > 
> > Btw, this is essentially CountryNames::GetSortKey() from Chromium:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
> > 
> > ICU does not use STL, which makes my life harder here.
> 
> Allocating the buffer isn't what worried me. It's mainly the casting between
> types. Also using buffer_size in the string constructor would add a little
more
> safety.

Done.

[Evan Stade, 2014-06-27 01:26:59]

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:194: // object is not
usable after this call due to using the swap() operation.
nit: s/this object/|this|

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:203:
result.swap(*suggestions);
nit: suggestions->swap(result), imo, is easier to read.

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:239:
StringCanonicalizer() : buffer_size_(0) {
should be initialized to something greater than 0

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:466: if
(!canonicalizer_) {
nit: no curlies

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:512:
validator_.Validate(address_copy,
these two short circuits (continue and return) --- I believe they can come at
the beginning of the loop body

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.h (right):

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.h:97:
scoped_ptr<StringCanonicalizer> canonicalizer_;
why is this a scoped_ptr

[please use gerrit instead, 2014-06-27 08:43:38]

In patch set 39.

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:194: // object is not
usable after this call due to using the swap() operation.
On 2014/06/27 01:26:58, Evan Stade wrote:
> nit: s/this object/|this|

Done.

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:203:
result.swap(*suggestions);
On 2014/06/27 01:26:58, Evan Stade wrote:
> nit: suggestions->swap(result), imo, is easier to read.

Done.

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:239:
StringCanonicalizer() : buffer_size_(0) {
On 2014/06/27 01:26:58, Evan Stade wrote:
> should be initialized to something greater than 0

Done.

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:466: if
(!canonicalizer_) {
On 2014/06/27 01:26:58, Evan Stade wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:512:
validator_.Validate(address_copy,
On 2014/06/27 01:26:58, Evan Stade wrote:
> these two short circuits (continue and return) --- I believe they can come at
> the beginning of the loop body

THank you for bring this to my attention! I noticed a bug here: |address_copy|
should be |address|. (I added a test and fixed the bug.)

To respond to your original comment: this is as high as the "continue" can be in
the loop, because the code-block validates the object that was created
immediately above.

Also, this is as high as the "return" can be in the loop, because the code-block
clears "suggestions" if their size grows above a threshold, which we can't know
for sure until we check that the next suggested address will be valid
(immediately above).

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.h (right):

https://codereview.chromium.org/298863012/diff/1800001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.h:97:
scoped_ptr<StringCanonicalizer> canonicalizer_;
On 2014/06/27 01:26:58, Evan Stade wrote:
> why is this a scoped_ptr

Changed to not be a scoped_ptr.

[Evan Stade, 2014-06-27 16:42:33]

https://codereview.chromium.org/298863012/diff/1850001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1850001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:215:
std::map<AddressField, RegionsMatchInput*> regions_match_input_;
why not std::map<AddressField, RegionsMatchInput>?

https://codereview.chromium.org/298863012/diff/1850001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:325: const FieldTries*
field_tries = field_tries_.find(address_field)->second;
why not field_tries_[address_field]?

https://codereview.chromium.org/298863012/diff/1850001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:346: // "AK",
weird line breaking here

https://codereview.chromium.org/298863012/diff/1850001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:393:
std::map<AddressField, FieldTries*> field_tries_;
why not std::map<AddressField, FieldTries>?

https://codereview.chromium.org/298863012/diff/1850001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:456: sub_region_data_it
= sub_regions_.find(&region_data);
if you move the SubRegionData constructor params to an Init() function, and make
sub_regions_ a map to SubRegionData (no *) this is simplified:

SubRegionData* sub_region_data = &sub_regions_[region_data];
sub_region_data->InitIfNecessary(region_data, canonicalizer_);
[...]
sub_region_data->BuildSuggestions([...]);

[please use gerrit instead, 2014-06-27 19:34:39]

In patch set 40.

https://codereview.chromium.org/298863012/diff/1850001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1850001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:215:
std::map<AddressField, RegionsMatchInput*> regions_match_input_;
On 2014/06/27 16:42:33, Evan Stade wrote:
> why not std::map<AddressField, RegionsMatchInput>?

Done.

https://codereview.chromium.org/298863012/diff/1850001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:325: const FieldTries*
field_tries = field_tries_.find(address_field)->second;
On 2014/06/27 16:42:33, Evan Stade wrote:
> why not field_tries_[address_field]?

Done.

https://codereview.chromium.org/298863012/diff/1850001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:346: // "AK",
On 2014/06/27 16:42:33, Evan Stade wrote:
> weird line breaking here

Fixed.

https://codereview.chromium.org/298863012/diff/1850001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:393:
std::map<AddressField, FieldTries*> field_tries_;
On 2014/06/27 16:42:33, Evan Stade wrote:
> why not std::map<AddressField, FieldTries>?

Done.

https://codereview.chromium.org/298863012/diff/1850001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:456: sub_region_data_it
= sub_regions_.find(&region_data);
On 2014/06/27 16:42:33, Evan Stade wrote:
> if you move the SubRegionData constructor params to an Init() function, and
make
> sub_regions_ a map to SubRegionData (no *) this is simplified:
> 
> SubRegionData* sub_region_data = &sub_regions_[region_data];
> sub_region_data->InitIfNecessary(region_data, canonicalizer_);
> [...]
> sub_region_data->BuildSuggestions([...]);

Done:

if (!sub_region_data.is_initialized())
  sub_region_data.Initialize(region_data, canonicalizer_);

--------------------------------
(I prefer to not have methods that may or may not do something.)

[Evan Stade, 2014-06-27 22:44:38]

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
File third_party/libaddressinput/chromium/trie.h (right):

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/trie.h:35: void AddDataForKey(const
std::string& key, const T& data_item);
this was a netter api

https://codereview.chromium.org/298863012/diff/1890001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1890001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:456: SubRegionData&
sub_region_data = sub_regions_[&region_data];
I don't think we use non-const references

https://codereview.chromium.org/298863012/diff/1890001/third_party/libaddress...
File third_party/libaddressinput/chromium/trie.h (right):

https://codereview.chromium.org/298863012/diff/1890001/third_party/libaddress...
third_party/libaddressinput/chromium/trie.h:32: void AddDataForKey(const
uint8_t* key, int32_t key_size, const T& data_item);
a) sizes are usually size_t
b) this is a really confusing and dangerous api (does it make a copy? does it
rely on |key| outliving |this|? etc.)

[please use gerrit instead, 2014-06-28 22:05:21]

In patch set 42.

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
File third_party/libaddressinput/chromium/trie.h (right):

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/trie.h:35: void AddDataForKey(const
std::string& key, const T& data_item);
On 2014/06/27 22:44:37, Evan Stade wrote:
> this was a netter api

Changed to "const std::vector<uint8_t>& key" and noted that the data should be 0
terminated in the comment.

https://codereview.chromium.org/298863012/diff/1890001/third_party/libaddress...
File third_party/libaddressinput/chromium/input_suggester.cc (right):

https://codereview.chromium.org/298863012/diff/1890001/third_party/libaddress...
third_party/libaddressinput/chromium/input_suggester.cc:456: SubRegionData&
sub_region_data = sub_regions_[&region_data];
On 2014/06/27 22:44:38, Evan Stade wrote:
> I don't think we use non-const references

Changed to pointer.

https://codereview.chromium.org/298863012/diff/1890001/third_party/libaddress...
File third_party/libaddressinput/chromium/trie.h (right):

https://codereview.chromium.org/298863012/diff/1890001/third_party/libaddress...
third_party/libaddressinput/chromium/trie.h:32: void AddDataForKey(const
uint8_t* key, int32_t key_size, const T& data_item);
On 2014/06/27 22:44:38, Evan Stade wrote:
> a) sizes are usually size_t
> b) this is a really confusing and dangerous api (does it make a copy? does it
> rely on |key| outliving |this|? etc.)

Using std::vector<uint8_t> now.

[Evan Stade, 2014-06-30 18:49:39]

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
File third_party/libaddressinput/chromium/trie.h (right):

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/trie.h:35: void AddDataForKey(const
std::string& key, const T& data_item);
On 2014/06/28 22:05:21, Rouslan Solomakhin wrote:
> On 2014/06/27 22:44:37, Evan Stade wrote:
> > this was a netter api
> 
> Changed to "const std::vector<uint8_t>& key" and noted that the data should be
0
> terminated in the comment.

in case you were wondering, s/netter/nicer

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
File third_party/libaddressinput/chromium/load_rules_delegate.h (right):

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
third_party/libaddressinput/chromium/load_rules_delegate.h:14: class
LoadRulesDelegate {
nit: LoadRulesObserver or LoadRulesListener

A "delegate" does something on behalf of |this|, but that's somewhat inverted
from the relationship here.

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
File third_party/libaddressinput/chromium/string_compare.cc (right):

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
third_party/libaddressinput/chromium/string_compare.cc:15: class
StringCompare::Impl {
Why is this class an inner class of StringCompare as opposed to say a
LazyInstance?

You create collator_ lazily but impl_ is not lazy. If you avoid using impl_ and
make a file-level static LazyInstance you can initialize the collator in the
constructor. This also means all StringCompare instances share a single
collator, which saves a little space (if multiple StringCompare objects are even
constructed).

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
third_party/libaddressinput/chromium/string_compare.cc:52:
StringCompare::StringCompare() : impl_(new Impl) {
nit: {} on one line

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
third_party/libaddressinput/chromium/string_compare.cc:55:
StringCompare::~StringCompare() {
nit: {} on one line

[Evan Stade, 2014-06-30 18:51:06]

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
File third_party/libaddressinput/chromium/has_all_required_fields.cc (right):

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
third_party/libaddressinput/chromium/has_all_required_fields.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
this file should probably be called libaddressinput_util or something generic,
not named after one of its functions

[please use gerrit instead, 2014-07-01 04:53:50]

In patch set 44.

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
File third_party/libaddressinput/chromium/trie.h (right):

https://codereview.chromium.org/298863012/diff/1680001/third_party/libaddress...
third_party/libaddressinput/chromium/trie.h:35: void AddDataForKey(const
std::string& key, const T& data_item);
On 2014/06/30 18:49:38, Evan Stade wrote:
> On 2014/06/28 22:05:21, Rouslan Solomakhin wrote:
> > On 2014/06/27 22:44:37, Evan Stade wrote:
> > > this was a netter api
> > 
> > Changed to "const std::vector<uint8_t>& key" and noted that the data should
be
> 0
> > terminated in the comment.
> 
> in case you were wondering, s/netter/nicer

Thanks :-) (I too typo tometimes)

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
File third_party/libaddressinput/chromium/has_all_required_fields.cc (right):

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
third_party/libaddressinput/chromium/has_all_required_fields.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/06/30 18:51:06, Evan Stade wrote:
> this file should probably be called libaddressinput_util or something generic,
> not named after one of its functions

Renamed to required_fields.h because it contains HasAllRequiredFields and
ValidateRequiredFields.

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
File third_party/libaddressinput/chromium/load_rules_delegate.h (right):

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
third_party/libaddressinput/chromium/load_rules_delegate.h:14: class
LoadRulesDelegate {
On 2014/06/30 18:49:38, Evan Stade wrote:
> nit: LoadRulesObserver or LoadRulesListener
> 
> A "delegate" does something on behalf of |this|, but that's somewhat inverted
> from the relationship here.

Done.

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
File third_party/libaddressinput/chromium/string_compare.cc (right):

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
third_party/libaddressinput/chromium/string_compare.cc:15: class
StringCompare::Impl {
On 2014/06/30 18:49:38, Evan Stade wrote:
> Why is this class an inner class of StringCompare as opposed to say a
> LazyInstance?
> 
> You create collator_ lazily but impl_ is not lazy. If you avoid using impl_
and
> make a file-level static LazyInstance you can initialize the collator in the
> constructor. This also means all StringCompare instances share a single
> collator, which saves a little space (if multiple StringCompare objects are
even
> constructed).

Done.

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
third_party/libaddressinput/chromium/string_compare.cc:52:
StringCompare::StringCompare() : impl_(new Impl) {
On 2014/06/30 18:49:38, Evan Stade wrote:
> nit: {} on one line

Done. (I have to remember to fix this after every clang-format, but sometimes
forget.)

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
third_party/libaddressinput/chromium/string_compare.cc:55:
StringCompare::~StringCompare() {
On 2014/06/30 18:49:38, Evan Stade wrote:
> nit: {} on one line

Done.

[Evan Stade, 2014-07-01 19:03:31]

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
File third_party/libaddressinput/chromium/has_all_required_fields.cc (right):

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
third_party/libaddressinput/chromium/has_all_required_fields.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/07/01 04:53:50, Rouslan Solomakhin wrote:
> On 2014/06/30 18:51:06, Evan Stade wrote:
> > this file should probably be called libaddressinput_util or something
generic,
> > not named after one of its functions
> 
> Renamed to required_fields.h because it contains HasAllRequiredFields and
> ValidateRequiredFields.

That's not generic enough.

https://codereview.chromium.org/298863012/diff/1990001/third_party/libaddress...
File third_party/libaddressinput/chromium/string_compare.cc (right):

https://codereview.chromium.org/298863012/diff/1990001/third_party/libaddress...
third_party/libaddressinput/chromium/string_compare.cc:18: class
StringCompareImpl {
instead of StringCompareImpl, how about calling it something like
IcuStringComparer or CaseAndDiacriticInsensitiveStringCompare?

[please use gerrit instead, 2014-07-01 20:43:46]

In patch set 45.

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
File third_party/libaddressinput/chromium/has_all_required_fields.cc (right):

https://codereview.chromium.org/298863012/diff/1970001/third_party/libaddress...
third_party/libaddressinput/chromium/has_all_required_fields.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/07/01 19:03:31, Evan Stade wrote:
> On 2014/07/01 04:53:50, Rouslan Solomakhin wrote:
> > On 2014/06/30 18:51:06, Evan Stade wrote:
> > > this file should probably be called libaddressinput_util or something
> generic,
> > > not named after one of its functions
> > 
> > Renamed to required_fields.h because it contains HasAllRequiredFields and
> > ValidateRequiredFields.
> 
> That's not generic enough.

libaddressinput_util it is.

https://codereview.chromium.org/298863012/diff/1990001/third_party/libaddress...
File third_party/libaddressinput/chromium/string_compare.cc (right):

https://codereview.chromium.org/298863012/diff/1990001/third_party/libaddress...
third_party/libaddressinput/chromium/string_compare.cc:18: class
StringCompareImpl {
On 2014/07/01 19:03:31, Evan Stade wrote:
> instead of StringCompareImpl, how about calling it something like
> IcuStringComparer or CaseAndDiacriticInsensitiveStringCompare?

IcuStringComparer

[Evan Stade, 2014-07-07 21:59:09]

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
File third_party/libaddressinput/chromium/chrome_address_validator.h (right):

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/chrome_address_validator.h:31: class
LoadRulesListener;
you can define LoadRulesListener in this file

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
File third_party/libaddressinput/chromium/json.cc (right):

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:24: //
<libaddressinput/util/scoped_ptr.h>
remove this comment

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:31: explicit JsonImpl(const
base::DictionaryValue* do_not_take_ownership)
why not a const ref?

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:64:
dictionaries_.insert(std::make_pair(key, dictionary)).second;
this whole function can be distilled down to:

if (!dictionaries_.insert(std::make_pair(key, dictionary)).second)
  NOTREACHED();

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:82: const base::DictionaryValue*
const not_owned_;
why not a const ref?

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:125: bool found =
impl_->GetStringValueForKey(key, &value);
ditto

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:149: // The value returned by
FindDictionary() is owned by impl_.
I think it would be a bit less confusing if all the functionality were
concentrated in JsonImpl and the Json functions were pass-throughs

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:156: bool found =
impl_->GetDictionaryValueForKey(key, &value);
ditto

[please use gerrit instead, 2014-07-08 22:47:59]

In patch set 46.

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
File third_party/libaddressinput/chromium/chrome_address_validator.h (right):

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/chrome_address_validator.h:31: class
LoadRulesListener;
On 2014/07/07 21:59:07, Evan Stade wrote:
> you can define LoadRulesListener in this file

Done.

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
File third_party/libaddressinput/chromium/json.cc (right):

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:24: //
<libaddressinput/util/scoped_ptr.h>
On 2014/07/07 21:59:08, Evan Stade wrote:
> remove this comment

Done.

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:31: explicit JsonImpl(const
base::DictionaryValue* do_not_take_ownership)
On 2014/07/07 21:59:08, Evan Stade wrote:
> why not a const ref?

Done.

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:64:
dictionaries_.insert(std::make_pair(key, dictionary)).second;
On 2014/07/07 21:59:08, Evan Stade wrote:
> this whole function can be distilled down to:
> 
> if (!dictionaries_.insert(std::make_pair(key, dictionary)).second)
>   NOTREACHED();

This function is no longer necessary after moving all logic to JsonImpl and
making Json methods pass through to JsonImpl equivalents.

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:82: const base::DictionaryValue*
const not_owned_;
On 2014/07/07 21:59:07, Evan Stade wrote:
> why not a const ref?

Done.

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:125: bool found =
impl_->GetStringValueForKey(key, &value);
On 2014/07/07 21:59:08, Evan Stade wrote:
> ditto

Done.

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:149: // The value returned by
FindDictionary() is owned by impl_.
On 2014/07/07 21:59:08, Evan Stade wrote:
> I think it would be a bit less confusing if all the functionality were
> concentrated in JsonImpl and the Json functions were pass-throughs

Done.

https://codereview.chromium.org/298863012/diff/2010001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:156: bool found =
impl_->GetDictionaryValueForKey(key, &value);
On 2014/07/07 21:59:08, Evan Stade wrote:
> ditto

Done.

[Evan Stade, 2014-07-09 23:35:31]

https://codereview.chromium.org/298863012/diff/2050001/third_party/libaddress...
File third_party/libaddressinput/chromium/json.cc (right):

https://codereview.chromium.org/298863012/diff/2050001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:73: std::string
GetStringValueForKey(const std::string& key) const {
this adds a copy operation. The json data may be large, so I think this is
unwise.

https://codereview.chromium.org/298863012/diff/2050001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:96: const Json*
FindDictionary(const std::string& key) {
this function needs vertical whitespace

[please use gerrit instead, 2014-07-09 23:38:55]

https://codereview.chromium.org/298863012/diff/2050001/third_party/libaddress...
File third_party/libaddressinput/chromium/json.cc (right):

https://codereview.chromium.org/298863012/diff/2050001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:73: std::string
GetStringValueForKey(const std::string& key) const {
On 2014/07/09 23:35:31, Evan Stade wrote:
> this adds a copy operation. The json data may be large, so I think this is
> unwise.

We can send a CL upstream to improve json.h interface. I suggest we do this
after this patch lands. WDYT?

[Evan Stade, 2014-07-09 23:45:32]

https://codereview.chromium.org/298863012/diff/2050001/third_party/libaddress...
File third_party/libaddressinput/chromium/json.cc (right):

https://codereview.chromium.org/298863012/diff/2050001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:73: std::string
GetStringValueForKey(const std::string& key) const {
On 2014/07/09 23:38:55, Rouslan Solomakhin wrote:
> On 2014/07/09 23:35:31, Evan Stade wrote:
> > this adds a copy operation. The json data may be large, so I think this is
> > unwise.
> 
> We can send a CL upstream to improve json.h interface. I suggest we do this
> after this patch lands. WDYT?

Regardless of the json.h interface, JsonImpl doesn't need to make a copy.

[please use gerrit instead, 2014-07-09 23:47:30]

https://codereview.chromium.org/298863012/diff/2050001/third_party/libaddress...
File third_party/libaddressinput/chromium/json.cc (right):

https://codereview.chromium.org/298863012/diff/2050001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:96: const Json*
FindDictionary(const std::string& key) {
On 2014/07/09 23:35:31, Evan Stade wrote:
> this function needs vertical whitespace

Done.

[please use gerrit instead, 2014-07-09 23:53:22]

In patch set 49.

https://codereview.chromium.org/298863012/diff/2050001/third_party/libaddress...
File third_party/libaddressinput/chromium/json.cc (right):

https://codereview.chromium.org/298863012/diff/2050001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:73: std::string
GetStringValueForKey(const std::string& key) const {
On 2014/07/09 23:45:32, Evan Stade wrote:
> On 2014/07/09 23:38:55, Rouslan Solomakhin wrote:
> > On 2014/07/09 23:35:31, Evan Stade wrote:
> > > this adds a copy operation. The json data may be large, so I think this is
> > > unwise.
> > 
> > We can send a CL upstream to improve json.h interface. I suggest we do this
> > after this patch lands. WDYT?
> 
> Regardless of the json.h interface, JsonImpl doesn't need to make a copy.

Done.

[Evan Stade, 2014-07-10 00:08:33]

lgtm

https://codereview.chromium.org/298863012/diff/2090001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/298863012/diff/2090001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2026: true,     //
Enable examples.
I had to go to the localization.h file to figure out what this was, so I don't
think this comment is helpful. Since comments are not compiled and often become
obsolete, I think it may even be harmful.

https://codereview.chromium.org/298863012/diff/2090001/third_party/libaddress...
File third_party/libaddressinput/chromium/json.cc (right):

https://codereview.chromium.org/298863012/diff/2090001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:134: DCHECK(impl_);
chrome style says don't dcheck things right before derefing them

https://codereview.chromium.org/298863012/diff/2090001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:139: std::string
Json::GetStringValueForKey(const std::string& key) const {
I do think the json interface should be changed such that HasStringValueForKey
and GetStringValueForKey are not separate functions and a copy isn't made. (Same
for HasDictionary). Can you file a bug?

[please use gerrit instead, 2014-07-10 00:19:47]

Lei: OWNERS PTAL chrome/tools/build/repack_locales.py

[please use gerrit instead, 2014-07-10 01:07:34]

Evan, please see the question inline.

https://codereview.chromium.org/298863012/diff/2090001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/298863012/diff/2090001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2026: true,     //
Enable examples.
On 2014/07/10 00:08:33, Evan Stade wrote:
> I had to go to the localization.h file to figure out what this was, so I don't
> think this comment is helpful. Since comments are not compiled and often
become
> obsolete, I think it may even be harmful.

Removed.

https://codereview.chromium.org/298863012/diff/2090001/third_party/libaddress...
File third_party/libaddressinput/chromium/json.cc (right):

https://codereview.chromium.org/298863012/diff/2090001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:134: DCHECK(impl_);
On 2014/07/10 00:08:33, Evan Stade wrote:
> chrome style says don't dcheck things right before derefing them

I couldn't find this in the guide and also thought that a DCHECK() is easier to
debug than a flat-out crash, WDYT?

https://codereview.chromium.org/298863012/diff/2090001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:139: std::string
Json::GetStringValueForKey(const std::string& key) const {
On 2014/07/10 00:08:33, Evan Stade wrote:
> I do think the json interface should be changed such that HasStringValueForKey
> and GetStringValueForKey are not separate functions and a copy isn't made.
(Same
> for HasDictionary). Can you file a bug?

Filed: https://code.google.com/p/libaddressinput/issues/detail?id=8

[Lei Zhang, 2014-07-10 01:36:44]

lgtm stamp

[please use gerrit instead, 2014-07-10 16:46:23]

Sending to CQ.

https://codereview.chromium.org/298863012/diff/2090001/third_party/libaddress...
File third_party/libaddressinput/chromium/json.cc (right):

https://codereview.chromium.org/298863012/diff/2090001/third_party/libaddress...
third_party/libaddressinput/chromium/json.cc:134: DCHECK(impl_);
On 2014/07/10 01:07:34, Rouslan Solomakhin wrote:
> On 2014/07/10 00:08:33, Evan Stade wrote:
> > chrome style says don't dcheck things right before derefing them
> 
> I couldn't find this in the guide and also thought that a DCHECK() is easier
to
> debug than a flat-out crash, WDYT?

Removed the DCHECKs per our conversation in chat.

[please use gerrit instead, 2014-07-10 16:51:09]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2014-07-10 16:52:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rouslan@chromium.org/298863012/2170001

[please use gerrit instead, 2014-07-10 17:41:04]

https://codereview.appspot.com/112940044/ - "Avoid string copying in json.cc."
(in upstream)

[commit-bot: I haz the power, 2014-07-10 20:04:46]

Change committed as 282408