[rac] Use i18n address inputs with --enable-autofill-address-i18n flag

[rac] Use i18n address inputs with --enable-autofill-address-i18n flag

If Chrome has --enable-autofill-address-i18n flag, then rac dialog will display
an i18n-capable address input form. The form is not yet refreshed after the
country/region selection changes.

The visible change from this CL is that --enable-autofill-address-i18n flag will
cause rac dialog to display a country dropdown for address input.

BUG=247202

[please use gerrit instead, 2013-10-01 22:42:19]

Evan: PTAL.

[Evan Stade, 2013-10-02 00:57:18]

https://codereview.chromium.org/25620002/diff/5001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/25620002/diff/5001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:751:
scoped_ptr<common::AutofillDialogInput> inputs_builder_;
I don't see any reason for this to be a member variable. Instead of creating an
interface, can you expose the BuildInputs methods as static utilities, with the
check inside each one proxying to other static functions?

https://codereview.chromium.org/25620002/diff/5001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_input.h (right):

https://codereview.chromium.org/25620002/diff/5001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_input.h:28: static
AutofillDialogInput* BuildInstance();
nit: factory function should be named Create()

https://codereview.chromium.org/25620002/diff/5001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_input.h:31: // must be non-null. Does
not take ownership of |inputs|.
nit: just mention that |inputs| is an out-param; not taking ownership is
assumed.

https://codereview.chromium.org/25620002/diff/5001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_input_i18n.cc (right):

https://codereview.chromium.org/25620002/diff/5001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_input_i18n.cc:59: ServerFieldType
AutofillDialogInputI18n::GetFieldType(
AutofillType::GetEquivalentBillingFieldType

[please use gerrit instead, 2013-10-02 18:21:49]

https://codereview.chromium.org/25620002/diff/5001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/25620002/diff/5001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:751:
scoped_ptr<common::AutofillDialogInput> inputs_builder_;
On 2013/10/02 00:57:19, Evan Stade wrote:
> I don't see any reason for this to be a member variable. Instead of creating
an
> interface, can you expose the BuildInputs methods as static utilities, with
the
> check inside each one proxying to other static functions?

I've decided to make AutofillDialogControllerImpl own the AutofillDialogInput
object because AutofillDialogInputI18n will own the libaddressinput context. It
makes sense to keep one libaddressinput context per address input form.
AutofillDialogControllerImpl has two address input forms, actually, so I think
that it needs two instances of AutofillDialogInput. I prefer to not have a
single static libaddressinput context be used by multiple address forms.

[Evan Stade, 2013-10-02 20:27:18]

https://codereview.chromium.org/25620002/diff/5001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/25620002/diff/5001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:751:
scoped_ptr<common::AutofillDialogInput> inputs_builder_;
On 2013/10/02 18:21:50, Rouslan Solomakhin wrote:
> On 2013/10/02 00:57:19, Evan Stade wrote:
> > I don't see any reason for this to be a member variable. Instead of creating
> an
> > interface, can you expose the BuildInputs methods as static utilities, with
> the
> > check inside each one proxying to other static functions?
> 
> I've decided to make AutofillDialogControllerImpl own the AutofillDialogInput
> object because AutofillDialogInputI18n will own the libaddressinput context.
It
> makes sense to keep one libaddressinput context per address input form.
> AutofillDialogControllerImpl has two address input forms, actually, so I think
> that it needs two instances of AutofillDialogInput. I prefer to not have a
> single static libaddressinput context be used by multiple address forms.

what state is the object going to hold?

[please use gerrit instead, 2013-10-02 20:38:41]

On 2013/10/02 20:27:18, Evan Stade wrote:
> what state is the object going to hold?

The object will hold the last-entered address and the validation rules for it.
The object will download the validation rules asynchronously, validate the
address, and send the validation results to Chrome through a callback.

[please use gerrit instead, 2013-10-04 22:39:53]

Evan: PTAL.

The change increases the size of address_dialog_common.cc significantly. Perhaps
I should split out the i18n-related things into address_dialog_i18_input.cc
instead?

[Evan Stade, 2013-10-07 19:39:30]

On 2013/10/04 22:39:53, Rouslan Solomakhin wrote:
> Evan: PTAL.
> 
> The change increases the size of address_dialog_common.cc significantly.
Perhaps
> I should split out the i18n-related things into address_dialog_i18_input.cc
> instead?

I think that makes sense.

[please use gerrit instead, 2013-10-08 22:25:06]

Evan: PTAL.

[Evan Stade, 2013-10-08 23:44:51]

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:15: // GuessCountry()
uses only the application locale and should be improved to use
link to the bug

also, we should probably look at the countries for (verified) autofill profiles,
if any exist.

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:96:
{ADDRESS_HOME_COUNTRY, ADDRESS_BILLING_COUNTRY},
space after { and before }

also, this seems to be in the wrong order (if you're modeling after US for now)

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:105: // A mapping of
AddressFieldIndex and AddressType to string identifiers for
you should make this a map from autofill field type to placeholder string id

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:107: const int
kPlaceHolderStringIds[kMaxNumberOfInputFields]
s/PlaceHolder/Placeholder

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:140: return
kStreetAddressLineIndex[street_address_line];
2 returns in a row?

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:152: void
GetI18nFields(const std::string country_region,
const ref?

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:155:
std::vector<i18n::addressinput::LengthHint>* length_hints) {
I'd prefer if this were a single vector, of either std::pair or a struct with 2
members

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:175: 
^H

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:185: prev_length_hint
!= length_hint) {
optional nit: imo, this is easier to grok as

if (length_hint == i18n::addressinput::LENGTH_HINT_LONG ||
    prev_length_hint == i18n::addressinput::LENGTH_HINT_LONG)

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:192: //
kMaxNumberOfStreetAddressLines inputs if |field_type| is STREET_ADDRESS.
can you document the other params? I don't understand prev_length_hint.

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:193: int
BuildI18nInput(int row_index,
instead of passing row_index around, I think you can check
inputs->back().row_index

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:199: int field_index =
AddressFieldToIndex(field_type, STREET_ADDRESS_LINE_1);
this is confusing because most of the time STREET_ADDRESS_LINE_1 is irrelevant
to field_type. I'd just as soon you pretend there's only 1 address line for now,
since this is placeholder code anyhow.

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:219: return
locale.substr(0, locale.find('-'));
AutofillCountry::CountryCodeForLocale

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.h (right):

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.h:25: // Returns the best
guess for the current country.
"Returns the best guess for the user's home country code/region."

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc (right):

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc:19: const
size_t kNumberOfAdderssLinesUS = 7;
spelling of this variable

[Evan Stade, 2013-11-18 19:39:46]

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/25620002/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:15: // GuessCountry()
uses only the application locale and should be improved to use
On 2013/10/08 23:44:51, Evan Stade wrote:
> link to the bug
> 
> also, we should probably look at the countries for (verified) autofill
profiles,
> if any exist.

note that I've been working on this, the code is LGTM'd but not yet committed.