Add country combobox to change country and rebuild address inputs.

Add country combobox to change country and rebuild address inputs.

R=estade@chromium.org
BUG=331544
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245434

[groby-ooo-7-16, 2014-01-10 01:56:37]

https://codereview.chromium.org/124533003/diff/550001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right):

https://codereview.chromium.org/124533003/diff/550001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:289: if (popup) {
If you must get a separate popup notification, I'd rather you add a
-(void)popupChanged action to all comboboxes.

https://codereview.chromium.org/124533003/diff/550001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:506: if
(shouldClobber) {
I'd prefer to not rebuild the entire layout whenever we clobber. Shouldn't this
only happen on language-triggered rebuild?

https://codereview.chromium.org/124533003/diff/550001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:507: [self
makeInputControls];
Also, you presumably want to request a relayout here.

https://codereview.chromium.org/124533003/diff/550001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:521: if
(shouldClobber)
How could shouldClobber ever be true here? You're in the else-branch of the
first shouldClobber?

https://codereview.chromium.org/124533003/diff/550001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:727:
AutofillPopUpButton* popup =
Please do that in AutofillPopUp, not here.

[Dan Beam, 2014-01-11 04:16:58]

status update: don't review yet, but... this is very close.

there's 1 small issue on views though overall the updated tests pass.

i haven't tried the new code on mac recently.

https://chromiumcodereview.appspot.com/124533003/diff/550001/chrome/browser/u...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right):

https://chromiumcodereview.appspot.com/124533003/diff/550001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:289: if (popup) {
On 2014/01/10 01:56:38, groby wrote:
> If you must get a separate popup notification, I'd rather you add a
> -(void)popupChanged action to all comboboxes.

see updated code

https://chromiumcodereview.appspot.com/124533003/diff/550001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:506: if
(shouldClobber) {
On 2014/01/10 01:56:38, groby wrote:
> I'd prefer to not rebuild the entire layout whenever we clobber. Shouldn't
this
> only happen on language-triggered rebuild?

we talked about this and will wait until it's noticeable

https://chromiumcodereview.appspot.com/124533003/diff/550001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:507: [self
makeInputControls];
On 2014/01/10 01:56:38, groby wrote:
> Also, you presumably want to request a relayout here.

Done.

https://chromiumcodereview.appspot.com/124533003/diff/550001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:521: if
(shouldClobber)
On 2014/01/10 01:56:38, groby wrote:
> How could shouldClobber ever be true here? You're in the else-branch of the
> first shouldClobber?

copy pasta, removed

https://chromiumcodereview.appspot.com/124533003/diff/550001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:727:
AutofillPopUpButton* popup =
On 2014/01/10 01:56:38, groby wrote:
> Please do that in AutofillPopUp, not here.

Done.

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

https://chromiumcodereview.appspot.com/124533003/diff/650001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1100: }
note for myself:
SuggestionsUpdated is taking a snapshot, resetting the section into (which
switches back to default country), then refilling from the snapshot

[Dan Beam, 2014-01-14 05:08:16]

estade@: logic and views are ready to review
groby@: still working on mac

[Dan Beam, 2014-01-14 05:26:52]

> groby@: still working on mac

got mac working, ptal

[Evan Stade, 2014-01-14 17:19:38]

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (right):

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:267: if
(!IsI18nInputEnabled() &&
nit: could you make this

// TODO(dbeam): remove this entire function when i18n inputs are the default.
if (IsI18nInputEnabled())
  return base::string16();

/* existing code */

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

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:564:
ASSERT_TRUE(controller()->IsManuallyEditingSection(SECTION_SHIPPING));
nit: add comment about what the second to last item is

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1414:
EXPECT_EQ(base::string16(), view->GetTextContentsOfInput(NAME_FULL));
you should also check that NAME_FULL still has something in it after changing
data sources (L1404)

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/country_combobox_model.cc (right):

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/country_combobox_model.cc:73: if (country_code ==
GetSelectedCountryCode())
redundant imo

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/country_combobox_model.cc:108: DCHECK_GE(index, 0);
why do these dchecks come after the identity check?

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/country_combobox_model.h (right):

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/country_combobox_model.h:23: class
CountryComboboxModelObserver {
why are you inventing another observer instead of using ComboboxModelObserver?

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/country_combobox_model.h:68: int selected_index_;
combobox models aren't supposed to know the selected index. Only the view knows
that bit of state.

[groby-ooo-7-16, 2014-01-14 19:37:06]

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right):

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:429: if
(textfield) {
You can skip this if - [textfield invalid] will evaluate to 'nil' for
non-textfields, because textfield is nil.

In fact, I suspect you don't even need to cast to AutofillTextField - this
behavior is only invoked via didChange, and keeping the same behavior for
comboboxes seems correct. Wondering whether the explanatory function of the cast
is worth the extra code.

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:671: [[self view]
replaceSubview:inputs_ with:view];
I'd post requestRelayout from here.

[Dan Beam, 2014-01-15 03:10:47]

still wrestling with tests and compilation, but ptal

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (right):

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:267: if
(!IsI18nInputEnabled() &&
On 2014/01/14 17:19:38, Evan Stade wrote:
> nit: could you make this
> 
> // TODO(dbeam): remove this entire function when i18n inputs are the default.
> if (IsI18nInputEnabled())
>   return base::string16();
> 
> /* existing code */

Done.

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

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:564:
ASSERT_TRUE(controller()->IsManuallyEditingSection(SECTION_SHIPPING));
On 2014/01/14 17:19:38, Evan Stade wrote:
> nit: add comment about what the second to last item is

Done.

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:1414:
EXPECT_EQ(base::string16(), view->GetTextContentsOfInput(NAME_FULL));
On 2014/01/14 17:19:38, Evan Stade wrote:
> you should also check that NAME_FULL still has something in it after changing
> data sources (L1404)

oh, I did have that... somehow it got dropped.  added back.

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/country_combobox_model.cc (right):

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/country_combobox_model.cc:108: DCHECK_GE(index, 0);
On 2014/01/14 17:19:38, Evan Stade wrote:
> why do these dchecks come after the identity check?

selected_index_ should always be valid, so index should be valid.

moved lower in new code.

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/country_combobox_model.h (right):

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/country_combobox_model.h:23: class
CountryComboboxModelObserver {
On 2014/01/14 17:19:38, Evan Stade wrote:
> why are you inventing another observer instead of using ComboboxModelObserver?

Done. (after dropping need for selected stuff)

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/country_combobox_model.h:68: int selected_index_;
On 2014/01/14 17:19:38, Evan Stade wrote:
> combobox models aren't supposed to know the selected index. Only the view
knows
> that bit of state.

Done.

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right):

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:429: if
(textfield) {
On 2014/01/14 19:37:07, groby wrote:
> You can skip this if - [textfield invalid] will evaluate to 'nil' for
> non-textfields, because textfield is nil.
> 
> In fact, I suspect you don't even need to cast to AutofillTextField - this
> behavior is only invoked via didChange, and keeping the same behavior for
> comboboxes seems correct. Wondering whether the explanatory function of the
cast
> is worth the extra code.

we talked about this offline and will change after this lands if we need to in
interest of keeping the same behavior for now

https://codereview.chromium.org/124533003/diff/780001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:671: [[self view]
replaceSubview:inputs_ with:view];
On 2014/01/14 19:37:07, groby wrote:
> I'd post requestRelayout from here.

Done.

[groby-ooo-7-16, 2014-01-15 22:22:51]

https://codereview.chromium.org/124533003/diff/1040001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right):

https://codereview.chromium.org/124533003/diff/1040001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:700: if (input)
Q: Why can we have NULL here?

[Evan Stade, 2014-01-15 23:12:28]

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

https://codereview.chromium.org/124533003/diff/1040001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:607: //
TODO(dbeam): does SECTION_CC need to be internationalized?
I don't think so. I think you can leave out this TODO.

https://codereview.chromium.org/124533003/diff/1040001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:608:
common::BuildInputsForSection(SECTION_CC, "US", &requested_cc_fields_);
pass an empty country code to make it clear it's not used/relevant.

https://codereview.chromium.org/124533003/diff/1040001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1959:
GetManager()->RemoveObserver(this);
why is this here and not in the destructor?

why are the new RemoveObserver calls here and not in the destructor?

Can we use base::ScopedObserver instead?

https://codereview.chromium.org/124533003/diff/1040001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2190: // If
country will change while filling, rebuild inputs now.
I can't make sense of this sentence.

https://codereview.chromium.org/124533003/diff/1040001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3084: if
((is_shipping && section != SECTION_SHIPPING) ||
this is the second place in this CL I've seen so far where a
CountryComboboxModelForSection fn would come in handy (the first was in
SuggestionItemSelected)

for this case, even better would be a SectionForComboboxModel function

https://codereview.chromium.org/124533003/diff/1040001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3100: return "US";
ditto -- return "" as it's international.

https://codereview.chromium.org/124533003/diff/1040001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3111:
CountryComboboxModel* model = section == SECTION_SHIPPING ?
third place

https://codereview.chromium.org/124533003/diff/1040001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/country_combobox_model.cc (right):

https://codereview.chromium.org/124533003/diff/1040001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/country_combobox_model.cc:86: if (country_code ==
GetDefaultCountryCode())
I don't know if you missed, disagreed with, or what, but I think this check is
redundant because of the check on L108. As a performance optimization, it
doesn't really help much (in fact it's not clear it helps at all, since it adds
overhead for the != case).

[Dan Beam, 2014-01-16 01:39:12]

groby@: rebuilding on mac to answer your question

https://chromiumcodereview.appspot.com/124533003/diff/1040001/chrome/browser/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/124533003/diff/1040001/chrome/browser/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:607: //
TODO(dbeam): does SECTION_CC need to be internationalized?
On 2014/01/15 23:12:29, Evan Stade wrote:
> I don't think so. I think you can leave out this TODO.

Done.

https://chromiumcodereview.appspot.com/124533003/diff/1040001/chrome/browser/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:608:
common::BuildInputsForSection(SECTION_CC, "US", &requested_cc_fields_);
On 2014/01/15 23:12:29, Evan Stade wrote:
> pass an empty country code to make it clear it's not used/relevant.

Done.

https://chromiumcodereview.appspot.com/124533003/diff/1040001/chrome/browser/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1959:
GetManager()->RemoveObserver(this);
On 2014/01/15 23:12:29, Evan Stade wrote:
> why is this here and not in the destructor?
> 
> why are the new RemoveObserver calls here and not in the destructor?
> 
> Can we use base::ScopedObserver instead?

Done.

https://chromiumcodereview.appspot.com/124533003/diff/1040001/chrome/browser/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2190: // If
country will change while filling, rebuild inputs now.
On 2014/01/15 23:12:29, Evan Stade wrote:
> I can't make sense of this sentence.

Removed.

https://chromiumcodereview.appspot.com/124533003/diff/1040001/chrome/browser/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3084: if
((is_shipping && section != SECTION_SHIPPING) ||
On 2014/01/15 23:12:29, Evan Stade wrote:
> this is the second place in this CL I've seen so far where a
> CountryComboboxModelForSection fn would come in handy (the first was in
> SuggestionItemSelected)

Done.

> 
> for this case, even better would be a SectionForComboboxModel function

this code rebuilds 2 sections of input

https://chromiumcodereview.appspot.com/124533003/diff/1040001/chrome/browser/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3100: return "US";
On 2014/01/15 23:12:29, Evan Stade wrote:
> ditto -- return "" as it's international.

Done.

https://chromiumcodereview.appspot.com/124533003/diff/1040001/chrome/browser/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3111:
CountryComboboxModel* model = section == SECTION_SHIPPING ?
On 2014/01/15 23:12:29, Evan Stade wrote:
> third place

Done.

https://chromiumcodereview.appspot.com/124533003/diff/1040001/chrome/browser/...
File chrome/browser/ui/autofill/country_combobox_model.cc (right):

https://chromiumcodereview.appspot.com/124533003/diff/1040001/chrome/browser/...
chrome/browser/ui/autofill/country_combobox_model.cc:86: if (country_code ==
GetDefaultCountryCode())
On 2014/01/15 23:12:29, Evan Stade wrote:
> I don't know if you missed, disagreed with, or what, but I think this check is
> redundant because of the check on L108. As a performance optimization, it
> doesn't really help much (in fact it's not clear it helps at all, since it
adds
> overhead for the != case).

Done.

https://chromiumcodereview.appspot.com/124533003/diff/1040001/chrome/browser/...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right):

https://chromiumcodereview.appspot.com/124533003/diff/1040001/chrome/browser/...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:700: if (input)
On 2014/01/15 22:22:52, groby wrote:
> Q: Why can we have NULL here?

i'll find out right now

[Evan Stade, 2014-01-16 02:13:31]

lgtm

https://codereview.chromium.org/124533003/diff/1150018/chrome/browser/ui/view...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/124533003/diff/1150018/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2404: continue;
\n

https://codereview.chromium.org/124533003/diff/1150018/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2422: continue;
\n

https://codereview.chromium.org/124533003/diff/1150018/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2439: continue;
\n

https://codereview.chromium.org/124533003/diff/1150018/chrome/browser/ui/view...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2454: continue;
\n

[Dan Beam, 2014-01-16 05:09:10]

groby@: all mac issues seem resolved, ptal

https://chromiumcodereview.appspot.com/124533003/diff/1150018/chrome/browser/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/124533003/diff/1150018/chrome/browser/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2404: continue;
On 2014/01/16 02:13:31, Evan Stade wrote:
> \n

Done.

https://chromiumcodereview.appspot.com/124533003/diff/1150018/chrome/browser/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2422: continue;
On 2014/01/16 02:13:31, Evan Stade wrote:
> \n

Done.

https://chromiumcodereview.appspot.com/124533003/diff/1150018/chrome/browser/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2439: continue;
On 2014/01/16 02:13:31, Evan Stade wrote:
> \n

Done.

https://chromiumcodereview.appspot.com/124533003/diff/1150018/chrome/browser/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2454: continue;
On 2014/01/16 02:13:31, Evan Stade wrote:
> \n

Done.

[Dan Beam, 2014-01-17 02:39:57]

Ping groby@

[groby-ooo-7-16, 2014-01-17 02:56:11]

LGTM % adding a comment

https://codereview.chromium.org/124533003/diff/1520001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (right):

https://codereview.chromium.org/124533003/diff/1520001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:553: -
(void)makeInputControls {
Leave a comment indicating we do this so we don't get any more callbacks due to
deletion...

[commit-bot: I haz the power, 2014-01-17 04:11:05]

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

[commit-bot: I haz the power, 2014-01-17 06:24:13]

Change committed as 245434