rAc: use libaddressinput to validate international addresses.

rAc: use libaddressinput to validate international addresses.

BUG=317839
R=estade@chromium.org

Based on https://codereview.chromium.org/105823007/ by Evan Stade <estade@chromium.org>;.

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

[Dan Beam, 2014-01-25 02:27:57]

do you want me to split this CL up?

[Evan Stade, 2014-01-27 23:11:14]

pass 1

https://codereview.chromium.org/145553009/diff/110001/chrome/browser/autofill...
File chrome/browser/autofill/address_storage_factory.cc (right):

https://codereview.chromium.org/145553009/diff/110001/chrome/browser/autofill...
chrome/browser/autofill/address_storage_factory.cc:21: Storage*
AddressStorageFactory::CreateStorage() {
make this return a scoped ptr

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

https://codereview.chromium.org/145553009/diff/110001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2661:
scoped_ptr<autofill::ChromeDownloaderImpl> downloader(
change this type then you don't have to PassAs (I think)

https://codereview.chromium.org/145553009/diff/110001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2669:
billing_country_combobox_model_.GetDefaultCountryCode());
I think you could just make this:

validator_->LoadRules(GetManager()->GetDefaultCountryCodeForNewAddress())

https://codereview.chromium.org/145553009/diff/110001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
(right):

https://codereview.chromium.org/145553009/diff/110001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:95:
Storage* storage,
revert

https://codereview.chromium.org/145553009/diff/110001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/include/libaddressinput/storage.h
(left):

https://codereview.chromium.org/145553009/diff/110001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/storage.h:54:
scoped_ptr<Callback> data_ready) const = 0;
I'd rather you didn't make this change --- this is a point of contention we
should address later.

https://codereview.chromium.org/145553009/diff/110001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/145553009/diff/110001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:185: "^(" +
rule.GetPostalCodeFormat() + ").*")) {
this should be a separate cl

[Dan Beam, 2014-01-28 02:22:36]

https://codereview.chromium.org/145553009/diff/110001/chrome/browser/autofill...
File chrome/browser/autofill/address_storage_factory.cc (right):

https://codereview.chromium.org/145553009/diff/110001/chrome/browser/autofill...
chrome/browser/autofill/address_storage_factory.cc:21: Storage*
AddressStorageFactory::CreateStorage() {
On 2014/01/27 23:11:15, Evan Stade wrote:
> make this return a scoped ptr

Done.

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

https://codereview.chromium.org/145553009/diff/110001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2661:
scoped_ptr<autofill::ChromeDownloaderImpl> downloader(
On 2014/01/27 23:11:15, Evan Stade wrote:
> change this type then you don't have to PassAs (I think)

Done.

https://codereview.chromium.org/145553009/diff/110001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2669:
billing_country_combobox_model_.GetDefaultCountryCode());
On 2014/01/27 23:11:15, Evan Stade wrote:
> I think you could just make this:
> 
> validator_->LoadRules(GetManager()->GetDefaultCountryCodeForNewAddress())

Done.

https://codereview.chromium.org/145553009/diff/110001/third_party/libaddressi...
File
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h
(right):

https://codereview.chromium.org/145553009/diff/110001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/address_validator.h:95:
Storage* storage,
On 2014/01/27 23:11:15, Evan Stade wrote:
> revert

Done.

https://codereview.chromium.org/145553009/diff/110001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/include/libaddressinput/storage.h
(left):

https://codereview.chromium.org/145553009/diff/110001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/include/libaddressinput/storage.h:54:
scoped_ptr<Callback> data_ready) const = 0;
On 2014/01/27 23:11:15, Evan Stade wrote:
> I'd rather you didn't make this change --- this is a point of contention we
> should address later.

Done.

https://codereview.chromium.org/145553009/diff/110001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/src/address_validator.cc (right):

https://codereview.chromium.org/145553009/diff/110001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/src/address_validator.cc:185: "^(" +
rule.GetPostalCodeFormat() + ").*")) {
On 2014/01/27 23:11:15, Evan Stade wrote:
> this should be a separate cl

Done.

[Evan Stade, 2014-01-28 19:23:11]

https://codereview.chromium.org/145553009/diff/170001/chrome/browser/autofill...
File chrome/browser/autofill/address_storage_factory.h (right):

https://codereview.chromium.org/145553009/diff/170001/chrome/browser/autofill...
chrome/browser/autofill/address_storage_factory.h:25: class
AddressStorageFactory {
s/AddressStorageFactory/ValidationRulesStorageFactory

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

https://codereview.chromium.org/145553009/diff/170001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1724: if
(!i18ninput::Enabled() && !value.empty() &&
I feel like all the address stuff should be removed in the i18n case, even
"field required messages"

https://codereview.chromium.org/145553009/diff/170001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1766:
FieldValueMap values = inputs;
why do you need to make this copy?

https://codereview.chromium.org/145553009/diff/170001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1773:
address_data.administrative_area = UTF16ToUTF8(
DataModelWrapper already has code that creates an AddressData from
ServerFieldTypes, I think you should reuse that (this will require a slight
refactor of course, similar to the FillFormFields one)

https://codereview.chromium.org/145553009/diff/170001/third_party/libaddressi...
File third_party/libaddressinput/chromium/cpp/test/retriever_test.cc (left):

https://codereview.chromium.org/145553009/diff/170001/third_party/libaddressi...
third_party/libaddressinput/chromium/cpp/test/retriever_test.cc:63: data_() {
leave this as is, it's a libaddressinput style thing

[Evan Stade, 2014-01-28 19:25:08]

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

https://codereview.chromium.org/145553009/diff/170001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2659: scoped_ptr<
::i18n::addressinput::Downloader> downloader(
move this all to show

[Dan Beam, 2014-01-30 04:51:35]

https://chromiumcodereview.appspot.com/145553009/diff/170001/chrome/browser/a...
File chrome/browser/autofill/address_storage_factory.h (right):

https://chromiumcodereview.appspot.com/145553009/diff/170001/chrome/browser/a...
chrome/browser/autofill/address_storage_factory.h:25: class
AddressStorageFactory {
On 2014/01/28 19:23:12, Evan Stade wrote:
> s/AddressStorageFactory/ValidationRulesStorageFactory

Done.

https://chromiumcodereview.appspot.com/145553009/diff/170001/chrome/browser/u...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/145553009/diff/170001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1724: if
(!i18ninput::Enabled() && !value.empty() &&
On 2014/01/28 19:23:12, Evan Stade wrote:
> I feel like all the address stuff should be removed in the i18n case, even
> "field required messages"

why not just remove when we axe the flag?

https://chromiumcodereview.appspot.com/145553009/diff/170001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1766:
FieldValueMap values = inputs;
On 2014/01/28 19:23:12, Evan Stade wrote:
> why do you need to make this copy?

was to create empty base::string16() by non-const operator[] but no longer
needed in updated code

https://chromiumcodereview.appspot.com/145553009/diff/170001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1773:
address_data.administrative_area = UTF16ToUTF8(
On 2014/01/28 19:23:12, Evan Stade wrote:
> DataModelWrapper already has code that creates an AddressData from
> ServerFieldTypes, I think you should reuse that (this will require a slight
> refactor of course, similar to the FillFormFields one)

Done.

https://chromiumcodereview.appspot.com/145553009/diff/170001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2659: scoped_ptr<
::i18n::addressinput::Downloader> downloader(
On 2014/01/28 19:25:09, Evan Stade wrote:
> move this all to show

Done.

https://chromiumcodereview.appspot.com/145553009/diff/170001/third_party/liba...
File third_party/libaddressinput/chromium/cpp/test/retriever_test.cc (left):

https://chromiumcodereview.appspot.com/145553009/diff/170001/third_party/liba...
third_party/libaddressinput/chromium/cpp/test/retriever_test.cc:63: data_() {
On 2014/01/28 19:23:12, Evan Stade wrote:
> leave this as is, it's a libaddressinput style thing

Done.

[Evan Stade, 2014-01-30 04:58:20]

https://chromiumcodereview.appspot.com/145553009/diff/170001/chrome/browser/u...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/145553009/diff/170001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1724: if
(!i18ninput::Enabled() && !value.empty() &&
On 2014/01/30 04:51:36, Dan Beam wrote:
> On 2014/01/28 19:23:12, Evan Stade wrote:
> > I feel like all the address stuff should be removed in the i18n case, even
> > "field required messages"
> 
> why not just remove when we axe the flag?

because then the behavior will be wrong until we axe the flag? I'm not ready to
half-axe it right now.

[Dan Beam, 2014-01-30 06:28:58]

https://chromiumcodereview.appspot.com/145553009/diff/170001/chrome/browser/u...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/145553009/diff/170001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1724: if
(!i18ninput::Enabled() && !value.empty() &&
On 2014/01/30 04:58:20, Evan Stade wrote:
> On 2014/01/30 04:51:36, Dan Beam wrote:
> > On 2014/01/28 19:23:12, Evan Stade wrote:
> > > I feel like all the address stuff should be removed in the i18n case, even
> > > "field required messages"
> > 
> > why not just remove when we axe the flag?
> 
> because then the behavior will be wrong until we axe the flag? I'm not ready
to
> half-axe it right now.

Done.

[Evan Stade, 2014-01-30 17:25:38]

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

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:169:
get_info.Run(AutofillType(HTML_TYPE_COUNTRY_CODE, HTML_MODE_NONE)));
HTML_MODE_NONE is used for AutofillTypes that don't have an HTML type, not for
"either shipping or billing"

see https://codereview.chromium.org/148273010/

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:181: AutofillType
address_lines_type(HTML_TYPE_STREET_ADDRESS, HTML_MODE_NONE);
ADDRESS_HOME_STREET_ADDRESS

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.h (right):

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.h:54:
::i18n::addressinput::AddressData CreateAddressData(
instead or returning, fill it in as an outparam imo

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/field_map_wrapper.h (right):

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/field_map_wrapper.h:16: class FieldMapWrapper {
I do not think this class is worthwhile. You can inline FillInputs and you can
write GetInfo[FromFieldValueMap]() as a local static function.

[Dan Beam, 2014-01-30 17:43:49]

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

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:169:
get_info.Run(AutofillType(HTML_TYPE_COUNTRY_CODE, HTML_MODE_NONE)));
On 2014/01/30 17:25:39, Evan Stade wrote:
> HTML_MODE_NONE is used for AutofillTypes that don't have an HTML type, not for
> "either shipping or billing"
> 
> see https://codereview.chromium.org/148273010/

Done.

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:181: AutofillType
address_lines_type(HTML_TYPE_STREET_ADDRESS, HTML_MODE_NONE);
On 2014/01/30 17:25:39, Evan Stade wrote:
> ADDRESS_HOME_STREET_ADDRESS

Done.

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.h (right):

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.h:54:
::i18n::addressinput::AddressData CreateAddressData(
On 2014/01/30 17:25:39, Evan Stade wrote:
> instead or returning, fill it in as an outparam imo

Done.

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/field_map_wrapper.h (right):

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/field_map_wrapper.h:16: class FieldMapWrapper {
On 2014/01/30 17:25:39, Evan Stade wrote:
> I do not think this class is worthwhile. You can inline FillInputs and you can
> write GetInfo[FromFieldValueMap]() as a local static function.

i did this originally but it was harder to test and added more LOC to the
controller.  you sure you want me to change?

[Evan Stade, 2014-01-30 17:56:28]

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/field_map_wrapper.h (right):

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/field_map_wrapper.h:16: class FieldMapWrapper {
On 2014/01/30 17:43:50, Dan Beam wrote:
> On 2014/01/30 17:25:39, Evan Stade wrote:
> > I do not think this class is worthwhile. You can inline FillInputs and you
can
> > write GetInfo[FromFieldValueMap]() as a local static function.
> 
> i did this originally but it was harder to test and added more LOC to the
> controller.  you sure you want me to change?

yes

[Dan Beam, 2014-01-30 20:34:48]

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/field_map_wrapper.h (right):

https://codereview.chromium.org/145553009/diff/350001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/field_map_wrapper.h:16: class FieldMapWrapper {
On 2014/01/30 17:56:29, Evan Stade wrote:
> On 2014/01/30 17:43:50, Dan Beam wrote:
> > On 2014/01/30 17:25:39, Evan Stade wrote:
> > > I do not think this class is worthwhile. You can inline FillInputs and you
> can
> > > write GetInfo[FromFieldValueMap]() as a local static function.
> > 
> > i did this originally but it was harder to test and added more LOC to the
> > controller.  you sure you want me to change?
> 
> yes

Done.

[Dan Beam, 2014-01-30 23:54:29]

+thestig@ for chrome/common/chrome_paths.*

[Lei Zhang, 2014-01-31 00:26:40]

https://codereview.chromium.org/145553009/diff/420001/chrome/common/chrome_pa...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/145553009/diff/420001/chrome/common/chrome_pa...
chrome/common/chrome_paths.cc:411: case chrome::FILE_LIBADDRESSINPUT_CACHE:
Instead, put the file name in chrome/common/chrome_constants.cc and just have a
helper function in autofill to get the path. See
CRLSetFetcher::GetCRLSetFilePath() as an example for "Certificate Revocation
Lists"

[Dan Beam, 2014-01-31 01:34:28]

thestig@: fyi, no changes in chrome/common anymore...

https://codereview.chromium.org/145553009/diff/420001/chrome/common/chrome_pa...
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/145553009/diff/420001/chrome/common/chrome_pa...
chrome/common/chrome_paths.cc:411: case chrome::FILE_LIBADDRESSINPUT_CACHE:
On 2014/01/31 00:26:41, Lei Zhang wrote:
> Instead, put the file name in chrome/common/chrome_constants.cc and just have
a
> helper function in autofill to get the path. See
> CRLSetFetcher::GetCRLSetFilePath() as an example for "Certificate Revocation
> Lists"

made local but kept same location like we all discussed

[Lei Zhang, 2014-01-31 02:07:10]

https://codereview.chromium.org/145553009/diff/440001/chrome/browser/autofill...
File chrome/browser/autofill/validation_rules_storage_factory.cc (right):

https://codereview.chromium.org/145553009/diff/440001/chrome/browser/autofill...
chrome/browser/autofill/validation_rules_storage_factory.cc:34:
user_data_dir.Append(FILE_PATH_LITERAL("libaddressinput"));
Do you want to give it a more descriptive name? You can do AppendASCII() instead
of Append(FPL())

[Evan Stade, 2014-01-31 04:09:51]

https://codereview.chromium.org/145553009/diff/440001/chrome/browser/autofill...
File chrome/browser/autofill/validation_rules_storage_factory.cc (right):

https://codereview.chromium.org/145553009/diff/440001/chrome/browser/autofill...
chrome/browser/autofill/validation_rules_storage_factory.cc:34:
user_data_dir.Append(FILE_PATH_LITERAL("libaddressinput"));
On 2014/01/31 02:07:11, Lei Zhang wrote:
> Do you want to give it a more descriptive name? You can do AppendASCII()
instead
> of Append(FPL())

do you have a suggestion for the name? It holds "all data that libaddressinput
might want to save to disk".

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

https://codereview.chromium.org/145553009/diff/460001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1679: return
base::string16();
add TODO to remove this and all ADDRESS_ stuff below

https://codereview.chromium.org/145553009/diff/460001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1788:
AddressProblems problems;
declare as low as possible

https://codereview.chromium.org/145553009/diff/460001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1796: &problems)
== AddressValidator::SUCCESS) {
so if not successful, we just assume the address is valid?

you need to disable the submit button in this case and wait for the
LoadRulesDelegate callback to fire (at least add a TODO here to do this)

https://codereview.chromium.org/145553009/diff/460001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1812: if
(!messages.GetMessageOrDefault(type).text.empty())
I don't think you need this with the changes in
https://codereview.chromium.org/141843005/

just a heads up in case that CL lands first. If not, I can try to remember to
remove this.

https://codereview.chromium.org/145553009/diff/460001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3197: if
(GetValidator())
instead of if (GetValidator()), how about if (i18n::Enabled())? eventually we
shouldn't have to make any check at all.

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

https://codereview.chromium.org/145553009/diff/460001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3032:
base::Bind(&AutofillDialogControllerImpl::GetInfoFromInputs,
I feel like you should be able to test this function without exposing it in the
header. Can't you set some input text then switch countries? Or run
InputsAreValid on some FieldValueMap, and make sure the address passed to
mock_validator has the country you expect?

[Lei Zhang, 2014-01-31 05:02:14]

https://codereview.chromium.org/145553009/diff/440001/chrome/browser/autofill...
File chrome/browser/autofill/validation_rules_storage_factory.cc (right):

https://codereview.chromium.org/145553009/diff/440001/chrome/browser/autofill...
chrome/browser/autofill/validation_rules_storage_factory.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
nit: it's 2014.

https://codereview.chromium.org/145553009/diff/440001/chrome/browser/autofill...
chrome/browser/autofill/validation_rules_storage_factory.cc:34:
user_data_dir.Append(FILE_PATH_LITERAL("libaddressinput"));
On 2014/01/31 04:09:52, Evan Stade wrote:
> On 2014/01/31 02:07:11, Lei Zhang wrote:
> > Do you want to give it a more descriptive name? You can do AppendASCII()
> instead
> > of Append(FPL())
> 
> do you have a suggestion for the name? It holds "all data that libaddressinput
> might want to save to disk".

"Libaddress Cache" ?

[Dan Beam, 2014-01-31 19:24:54]

https://codereview.chromium.org/145553009/diff/440001/chrome/browser/autofill...
File chrome/browser/autofill/validation_rules_storage_factory.cc (right):

https://codereview.chromium.org/145553009/diff/440001/chrome/browser/autofill...
chrome/browser/autofill/validation_rules_storage_factory.cc:34:
user_data_dir.Append(FILE_PATH_LITERAL("libaddressinput"));
On 2014/01/31 05:02:15, Lei Zhang wrote:
> On 2014/01/31 04:09:52, Evan Stade wrote:
> > On 2014/01/31 02:07:11, Lei Zhang wrote:
> > > Do you want to give it a more descriptive name? You can do AppendASCII()
> > instead
> > > of Append(FPL())
> > 
> > do you have a suggestion for the name? It holds "all data that
libaddressinput
> > might want to save to disk".
> 
> "Libaddress Cache" ?

there is no libaddress.

if you want to change it, "Address Validation" or "Address Validation Cache"
sound the best to me

[Dan Beam, 2014-02-01 00:43:21]

https://codereview.chromium.org/145553009/diff/440001/chrome/browser/autofill...
File chrome/browser/autofill/validation_rules_storage_factory.cc (right):

https://codereview.chromium.org/145553009/diff/440001/chrome/browser/autofill...
chrome/browser/autofill/validation_rules_storage_factory.cc:34:
user_data_dir.Append(FILE_PATH_LITERAL("libaddressinput"));
On 2014/01/31 19:24:55, Dan Beam wrote:
> On 2014/01/31 05:02:15, Lei Zhang wrote:
> > On 2014/01/31 04:09:52, Evan Stade wrote:
> > > On 2014/01/31 02:07:11, Lei Zhang wrote:
> > > > Do you want to give it a more descriptive name? You can do AppendASCII()
> > > instead
> > > > of Append(FPL())
> > > 
> > > do you have a suggestion for the name? It holds "all data that
> libaddressinput
> > > might want to save to disk".
> > 
> > "Libaddress Cache" ?
> 
> there is no libaddress.
> 
> if you want to change it, "Address Validation" or "Address Validation Cache"
> sound the best to me

changed to "Address Validation Rules"

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

https://codereview.chromium.org/145553009/diff/460001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1679: return
base::string16();
On 2014/01/31 04:09:52, Evan Stade wrote:
> add TODO to remove this and all ADDRESS_ stuff below

Done.

https://codereview.chromium.org/145553009/diff/460001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1788:
AddressProblems problems;
On 2014/01/31 04:09:52, Evan Stade wrote:
> declare as low as possible

Done.

https://codereview.chromium.org/145553009/diff/460001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1796: &problems)
== AddressValidator::SUCCESS) {
On 2014/01/31 04:09:52, Evan Stade wrote:
> so if not successful, we just assume the address is valid?
> 
> you need to disable the submit button in this case and wait for the
> LoadRulesDelegate callback to fire (at least add a TODO here to do this)

added TODOs

https://codereview.chromium.org/145553009/diff/460001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1812: if
(!messages.GetMessageOrDefault(type).text.empty())
On 2014/01/31 04:09:52, Evan Stade wrote:
> I don't think you need this with the changes in
> https://codereview.chromium.org/141843005/
> 
> just a heads up in case that CL lands first. If not, I can try to remember to
> remove this.

Will resolve when yours lands.

https://codereview.chromium.org/145553009/diff/460001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3197: if
(GetValidator())
On 2014/01/31 04:09:52, Evan Stade wrote:
> instead of if (GetValidator()), how about if (i18n::Enabled())? eventually we
> shouldn't have to make any check at all.

Done.

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

https://codereview.chromium.org/145553009/diff/460001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:3032:
base::Bind(&AutofillDialogControllerImpl::GetInfoFromInputs,
On 2014/01/31 04:09:52, Evan Stade wrote:
> I feel like you should be able to test this function without exposing it in
the
> header. Can't you set some input text then switch countries? Or run
> InputsAreValid on some FieldValueMap, and make sure the address passed to
> mock_validator has the country you expect?

Done.

[Evan Stade, 2014-02-01 00:56:30]

lgtm

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

https://codereview.chromium.org/145553009/diff/520001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3208: 
^H

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

https://codereview.chromium.org/145553009/diff/520001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:727: scoped_ptr<
::i18n::addressinput::AddressValidator> validator_;
why is this here in the middle of various state bools?

[Dan Beam, 2014-02-01 01:14:53]

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

https://codereview.chromium.org/145553009/diff/520001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3208: 
On 2014/02/01 00:56:31, Evan Stade wrote:
> ^H

Done.

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

https://codereview.chromium.org/145553009/diff/520001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:727: scoped_ptr<
::i18n::addressinput::AddressValidator> validator_;
On 2014/02/01 00:56:31, Evan Stade wrote:
> why is this here in the middle of various state bools?

Done.

[Dan Beam, 2014-02-01 01:15:03]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-02-01 01:15:09]

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

[commit-bot: I haz the power, 2014-02-01 01:49:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-01 01:49:29]

Retried try job too often on linux_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2014-02-01 01:49:32]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 01:49:35]

CQ bit was unchecked on CL. Ignoring.

[Dan Beam, 2014-02-01 02:02:24]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-02-01 02:02:47]

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

[commit-bot: I haz the power, 2014-02-01 02:45:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-01 02:45:39]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, events_unittests, google_apis_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2014-02-01 02:45:43]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 02:45:45]

CQ bit was unchecked on CL. Ignoring.

[Dan Beam, 2014-02-01 02:46:35]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-02-01 02:46:57]

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

[commit-bot: I haz the power, 2014-02-01 03:23:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-01 03:23:45]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, events_unittests, google_apis_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2014-02-01 03:23:53]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 03:23:55]

CQ bit was unchecked on CL. Ignoring.

[Dan Beam, 2014-02-01 03:30:00]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2014-02-01 03:30:24]

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

[commit-bot: I haz the power, 2014-02-01 14:46:33]

Change committed as 248365

[commit-bot: I haz the power, 2014-02-01 14:46:40]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-01 14:46:40]

CQ bit was unchecked on CL. Ignoring.