Ask libaddressinput for address components to use in requestAutocomplete().

Ask libaddressinput for address components to use in requestAutocomplete().

This change will cause slightly different translations and address layout to
show when the --enable-autofill-address-i18n flag is passed.

Next up:
- pull translations from chrome rather than libaddressinput
- show country combobox and rebuild inputs on switch

Based on https://codereview.chromium.org/25620002 by Rouslan Solomakhin <rouslan@chromium.org>;.

BUG=317837
TBR=aruslan

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

[please use gerrit instead, 2013-11-18 19:22:29]

https://chromiumcodereview.appspot.com/63053003/diff/110001/third_party/libad...
File third_party/libaddressinput/README.chromium (right):

https://chromiumcodereview.appspot.com/63053003/diff/110001/third_party/libad...
third_party/libaddressinput/README.chromium:18: - Added libaddressinput.gyp for
chromium-specific gyp structure/locations.
I was under the impression that Local Modifications section is for when you
modify the code in the third party library. It's unavoidable that you will have
code for integrating the library into Chrome. I think this section is for
modifications in src/third_party/libaddressinput/src/ directory.

https://chromiumcodereview.appspot.com/63053003/diff/110001/third_party/libad...
File third_party/libaddressinput/libaddressinput.gyp (right):

https://chromiumcodereview.appspot.com/63053003/diff/110001/third_party/libad...
third_party/libaddressinput/libaddressinput.gyp:6: 'component%':
'shared_library',
Please remove the 'variables' section. This variable is defined in Chrome.

https://chromiumcodereview.appspot.com/63053003/diff/110001/third_party/libad...
third_party/libaddressinput/libaddressinput.gyp:12: 'cflags': ['-fPIC'],
Please remove the 'target_defaults' section. Chrome takes care of the -fPIC
flag, too.

[Dan Beam, 2013-11-19 05:15:11]

not quite ready for review, but just FYI

https://codereview.chromium.org/63053003/diff/110001/third_party/libaddressin...
File third_party/libaddressinput/README.chromium (right):

https://codereview.chromium.org/63053003/diff/110001/third_party/libaddressin...
third_party/libaddressinput/README.chromium:18: - Added libaddressinput.gyp for
chromium-specific gyp structure/locations.
On 2013/11/18 19:22:30, Rouslan Solomakhin wrote:
> I was under the impression that Local Modifications section is for when you
> modify the code in the third party library. It's unavoidable that you will
have
> code for integrating the library into Chrome. I think this section is for
> modifications in src/third_party/libaddressinput/src/ directory.

Done.

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

https://codereview.chromium.org/63053003/diff/110001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:6: 'component%':
'shared_library',
On 2013/11/18 19:22:30, Rouslan Solomakhin wrote:
> Please remove the 'variables' section. This variable is defined in Chrome.

Done.

https://codereview.chromium.org/63053003/diff/110001/third_party/libaddressin...
third_party/libaddressinput/libaddressinput.gyp:12: 'cflags': ['-fPIC'],
On 2013/11/18 19:22:30, Rouslan Solomakhin wrote:
> Please remove the 'target_defaults' section. Chrome takes care of the -fPIC
> flag, too.

Done.

[Dan Beam, 2013-11-22 00:15:23]

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

https://chromiumcodereview.appspot.com/63053003/diff/590001/chrome/browser/ui...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2043:
parent->AddChildViewAt(group->manual_input, add_index);
^ there are still paint issues here, but the rest should be good to review

[please use gerrit instead, 2013-11-22 00:51:23]

https://chromiumcodereview.appspot.com/63053003/diff/590001/chrome/app/genera...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/63053003/diff/590001/chrome/app/genera...
chrome/app/generated_resources.grd:10389: +      <message
name="IDS_AUTOFILL_DIALOG_PLACEHOLDER_ORGANIZATION" desc="The placeholder/label
text for organization/company in the requestAutocomplete dialog.">
I assume that we will figure out how to integrate libaddressinput's GRD file
into Chrome.

[Evan Stade, 2013-11-25 19:52:05]

https://codereview.chromium.org/63053003/diff/590001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/63053003/diff/590001/chrome/app/generated_res...
chrome/app/generated_resources.grd:10389: +      <message
name="IDS_AUTOFILL_DIALOG_PLACEHOLDER_ORGANIZATION" desc="The placeholder/label
text for organization/company in the requestAutocomplete dialog.">
On 2013/11/22 00:51:23, Rouslan Solomakhin wrote:
> I assume that we will figure out how to integrate libaddressinput's GRD file
> into Chrome.

This is the goal. Dan, if you want to leave it as a TODO rather than tackling it
in this CL, you should just put string literals in the code rather than
temporarily adding to this grd.

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (right):

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_common.cc:230: std::string
GetCountryCode(const base::string16& country_name) {
I don't think these 2 functions are particularly necessary

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

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1936:
base::string16 country = model->GetItemAt(index);
country_combobox_model_.countries()[index].country_code();

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2462: 
intentional?

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_types.h:199: typedef
std::map<ServerFieldType, base::string16> UserInputSnapshot;
what is the reason that both of these types need to exist? Why is
DetailOutputMap insufficient for user input snapshots? Why is
ServerFieldType->string insufficient for DetailOutputMap?

[Evan Stade, 2013-11-25 20:35:45]

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

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:80:
{ADDRESS_HOME_COUNTRY, ADDRESS_BILLING_COUNTRY},
there should be spaces inside the {} throughout this file.

But this array seems extremely fragile anyway. You should be using switch/case.

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:148: void
GetI18nFields(const std::string country_region,
const ref

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:187: int
BuildI18nInput(int row_index,
this doesn't seem like something that should be a separate function.

1) It's only used in one place
2) It has a crapton of parameters
3) It has a really strange return value

[Dan Beam, 2013-11-28 02:52:44]

https://codereview.chromium.org/63053003/diff/590001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/63053003/diff/590001/chrome/app/generated_res...
chrome/app/generated_resources.grd:10389: +      <message
name="IDS_AUTOFILL_DIALOG_PLACEHOLDER_ORGANIZATION" desc="The placeholder/label
text for organization/company in the requestAutocomplete dialog.">
On 2013/11/25 19:52:06, Evan Stade wrote:
> On 2013/11/22 00:51:23, Rouslan Solomakhin wrote:
> > I assume that we will figure out how to integrate libaddressinput's GRD file
> > into Chrome.
> 
> This is the goal. Dan, if you want to leave it as a TODO rather than tackling
it
> in this CL, you should just put string literals in the code rather than
> temporarily adding to this grd.

modified TODO()

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (right):

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_common.cc:230: std::string
GetCountryCode(const base::string16& country_name) {
On 2013/11/25 19:52:06, Evan Stade wrote:
> I don't think these 2 functions are particularly necessary

Done.

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

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1936:
base::string16 country = model->GetItemAt(index);
On 2013/11/25 19:52:06, Evan Stade wrote:
> country_combobox_model_.countries()[index].country_code();

Done.

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2462: 
On 2013/11/25 19:52:06, Evan Stade wrote:
> intentional?

nope, removed

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

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:80:
{ADDRESS_HOME_COUNTRY, ADDRESS_BILLING_COUNTRY},
On 2013/11/25 20:35:45, Evan Stade wrote:
> there should be spaces inside the {} throughout this file.
> 
> But this array seems extremely fragile anyway. You should be using
switch/case.

Done.

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:148: void
GetI18nFields(const std::string country_region,
On 2013/11/25 20:35:45, Evan Stade wrote:
> const ref

Done.

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:187: int
BuildI18nInput(int row_index,
On 2013/11/25 20:35:45, Evan Stade wrote:
> this doesn't seem like something that should be a separate function.
> 
> 1) It's only used in one place
> 2) It has a crapton of parameters
> 3) It has a really strange return value

Done.

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://codereview.chromium.org/63053003/diff/590001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_types.h:199: typedef
std::map<ServerFieldType, base::string16> UserInputSnapshot;
On 2013/11/25 19:52:06, Evan Stade wrote:
> what is the reason that both of these types need to exist? Why is
> DetailOutputMap insufficient for user input snapshots? Why is
> ServerFieldType->string insufficient for DetailOutputMap?

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

[please use gerrit instead, 2013-12-02 16:51:00]

Dan: Android is not compiling because the DEPS file omits downloading
libaddressinput on Android. (I originally thought that Android will use the Java
version of the library.)

[please use gerrit instead, 2013-12-02 16:51:45]

See
https://code.google.com/p/chromium/codesearch#chromium/src/DEPS&q=deps&sq=pac...

[Evan Stade, 2013-12-02 17:57:32]

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (left):

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:98: // We don't allow the
user to change the country: http://crbug.com/247518
leave this comment

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

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:138: const DetailInput
kShippingPhoneInputs[] = {
Might as well just have one phone input struct array for both billing and
shipping. (The row ID overlap doesn't matter)

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

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:38: switch
(component.field) {
imo, make this switch its own function and remove the address_line_2 handling.
The rest of the functionality here can be inlined.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:77: void
IncrementRowIdForComponent(
2/3 of the callsites will always increment the row id, so I think you can get
rid of this function.

I also think that in the loop below you don't need the prev_length_hint. You can
just do:

for (...) {
  if (component.length_hint == LONG)
    row_id++;
  inputs->push_back(... row_id ...);
  if (component.length_hint == LONG)
    row_id++;
}

(|row_id|s needn't be consecutive)

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:90: 
// static

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:108: int row_id,
you don't need row_id to be a parameter. It's ok to always start at 0. |row_id|s
are only used within a single section.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:132: if
(component.field == i18n::addressinput::STREET_ADDRESS) {
ugh.. This isn't so great because some countries will have more than 2 lines of
street address. I think the ideal would be to have a multiline text area for
address, but I don't think Views has such a control. The second best scenario
would be to automatically add extra inputs as the visible ones are used. This is
OK for the time being but please file a bug/add a comment.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:133:
IncrementRowIdForComponent(length_hint, prev_length_hint, &row_id);
why bother calling this? just row_id++

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:149: if (!has_country)
{
when is there no country?

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:151:
IncrementRowIdForComponent(
why bother calling this? just row_id++

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_types.h:39: base::string16
placeholder_text;
I acknowledge the necessity of this change, but are you sure this change doesn't
trip a presubmit check somewhere?

Perhaps it's changed, but in the past there was a presubmit check that
disallowed structs with too much stuff in them, and non-PODs count for a lot
more "stuff" than PODs do.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1538:
details_container_->set_ignore_layouts(!details_container_needs_layout_);
instead of this, explicitly call layout on the container. The problem this hack
is skirting around is that details_container_ is getting /multiple/ layout calls
here for no particular reason.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2045: if
(clobber_inputs) {
will the rest of the address get clobbered if you change the country? that seems
no bueno. If I start typing my French address then realize it's on the wrong
country and change it, I don't want to have to start over again.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2048:
parent->RemoveChildView(group->manual_input);
I believe you're leaking group->manual_input.

Anyway I'd prefer you change InitInputsView so that it updates
group->manual_input (clears children, stuffs new child views into it, etc.)
instead of returning a view. So the initial call would be something like:

  group->manual_input = new views::View();
  InitInputsView(section);
  info_view->AddChildView(group->manual_input);

then this block of code becomes a bit simpler and less leak-prone.

[Dan Beam, 2013-12-02 19:37:00]

On 2013/12/02 16:51:45, Rouslan Solomakhin wrote:
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/DEPS&q=deps&sq=pac...

the question was not "why doesn't Android compile", it was "should we include
the library or exclude this code" and we've chosen to exclude.

[Dan Beam, 2013-12-02 22:37:53]

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (left):

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:98: // We don't allow the
user to change the country: http://crbug.com/247518
On 2013/12/02 17:57:33, Evan Stade wrote:
> leave this comment

Done.

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

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:138: const DetailInput
kShippingPhoneInputs[] = {
On 2013/12/02 17:57:33, Evan Stade wrote:
> Might as well just have one phone input struct array for both billing and
> shipping. (The row ID overlap doesn't matter)

the ServerFieldType differs

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

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:38: switch
(component.field) {
On 2013/12/02 17:57:33, Evan Stade wrote:
> imo, make this switch its own function and remove the address_line_2 handling.
> The rest of the functionality here can be inlined.

Done.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:77: void
IncrementRowIdForComponent(
On 2013/12/02 17:57:33, Evan Stade wrote:
> 2/3 of the callsites will always increment the row id, so I think you can get
> rid of this function.
> 
> I also think that in the loop below you don't need the prev_length_hint. You
can
> just do:
> 
> for (...) {
>   if (component.length_hint == LONG)
>     row_id++;
>   inputs->push_back(... row_id ...);
>   if (component.length_hint == LONG)
>     row_id++;
> }
> 
> (|row_id|s needn't be consecutive)

^ yes, they do

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:90: 
On 2013/12/02 17:57:33, Evan Stade wrote:
> // static

it's just in a namespace, no?

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:108: int row_id,
On 2013/12/02 17:57:33, Evan Stade wrote:
> you don't need row_id to be a parameter. It's ok to always start at 0.
|row_id|s
> are only used within a single section.

Done.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:132: if
(component.field == i18n::addressinput::STREET_ADDRESS) {
On 2013/12/02 17:57:33, Evan Stade wrote:
> ugh.. This isn't so great because some countries will have more than 2 lines
of
> street address. I think the ideal would be to have a multiline text area for
> address, but I don't think Views has such a control. The second best scenario
> would be to automatically add extra inputs as the visible ones are used. This
is
> OK for the time being but please file a bug/add a comment.

Done.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:133:
IncrementRowIdForComponent(length_hint, prev_length_hint, &row_id);
On 2013/12/02 17:57:33, Evan Stade wrote:
> why bother calling this? just row_id++

Done.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:149: if (!has_country)
{
On 2013/12/02 17:57:33, Evan Stade wrote:
> when is there no country?

i guess always, removed the country check

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:151:
IncrementRowIdForComponent(
On 2013/12/02 17:57:33, Evan Stade wrote:
> why bother calling this? just row_id++

Done.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_types.h:39: base::string16
placeholder_text;
On 2013/12/02 17:57:33, Evan Stade wrote:
> I acknowledge the necessity of this change, but are you sure this change
doesn't
> trip a presubmit check somewhere?
> 
> Perhaps it's changed, but in the past there was a presubmit check that
> disallowed structs with too much stuff in them, and non-PODs count for a lot
> more "stuff" than PODs do.

clang nor git cl presubmit are whining.  i think the main issue was requiring an
explicit, out-of-line constructor/destructor in the .cc file, right?  i'm not
getting that error even though there isn't a ctor/dtor in
autofill_dialog_types.cc.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1538:
details_container_->set_ignore_layouts(!details_container_needs_layout_);
On 2013/12/02 17:57:33, Evan Stade wrote:
> instead of this, explicitly call layout on the container. The problem this
hack
> is skirting around is that details_container_ is getting /multiple/ layout
calls
> here for no particular reason.

Done.

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2045: if
(clobber_inputs) {
On 2013/12/02 17:57:33, Evan Stade wrote:
> will the rest of the address get clobbered if you change the country? that
seems
> no bueno. If I start typing my French address then realize it's on the wrong
> country and change it, I don't want to have to start over again.

it's preserved by the same mechanism as changing a data source :D

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2048:
parent->RemoveChildView(group->manual_input);
On 2013/12/02 17:57:33, Evan Stade wrote:
> I believe you're leaking group->manual_input.

you're right, this was hiding a segfault that i've attempt to skirt around in
ui/views/controls/combobox/combobox.cc

> 
> Anyway I'd prefer you change InitInputsView so that it updates
> group->manual_input (clears children, stuffs new child views into it, etc.)
> instead of returning a view. So the initial call would be something like:
> 
>   group->manual_input = new views::View();
>   InitInputsView(section);
>   info_view->AddChildView(group->manual_input);
> 
> then this block of code becomes a bit simpler and less leak-prone.

Done.

[Dan Beam, 2013-12-02 23:10:31]

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

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:77: void
IncrementRowIdForComponent(
On 2013/12/02 22:37:54, Dan Beam wrote:
> On 2013/12/02 17:57:33, Evan Stade wrote:
> > 2/3 of the callsites will always increment the row id, so I think you can
get
> > rid of this function.
> > 
> > I also think that in the loop below you don't need the prev_length_hint. You
> can
> > just do:
> > 
> > for (...) {
> >   if (component.length_hint == LONG)
> >     row_id++;
> >   inputs->push_back(... row_id ...);
> >   if (component.length_hint == LONG)
> >     row_id++;
> > }
> > 
> > (|row_id|s needn't be consecutive)
> 
> ^ yes, they do

whoops, published without changing this -- sorry, Done.

[Evan Stade, 2013-12-02 23:30:33]

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

https://codereview.chromium.org/63053003/diff/1000001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:90: 
On 2013/12/02 22:37:54, Dan Beam wrote:
> On 2013/12/02 17:57:33, Evan Stade wrote:
> > // static
> 
> it's just in a namespace, no?

oh, ok.

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

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:173:
i18ninput::BuildI18nInputs(i18ninput::ADDRESS_TYPE_SHIPPING,
this still won't compile on android :/

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

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1932:
ui::ComboboxModel* model,
this should take a ServerFieldType rather than model and section.

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

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:69: std::string
GuessCountry() {
don't add this function; use PersonalDataManager::GetDefaultCountryCode()

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:88: int row_id = 100;
I don't really have an objection to the extra knee room here, except that when I
read

  int row_id = 0;

my brain skims over it without looking for extra meaning, and when I read

  int row_id = 100;

I start to wonder what black magic could be at work.

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:107: ++row_id;
\n

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1920: views::View*
inputs_container = CreateInputsContainer(section);
nit (although you didn't change this), please inline this instead of saving to a
local var

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2047:
details_container_->Layout();
This isn't what I meant, I meant for you to call Layout() on the container in
AutofillDialogViews::Layout().

I don't think this works because it won't update the size of the container to
match its new size request.

https://codereview.chromium.org/63053003/diff/1060001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/63053003/diff/1060001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:365:
'browser/ui/autofill/autofill_dialog_i18n_input.cc',
this needs to be excluded on android

[Dan Beam, 2013-12-03 02:11:18]

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

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:173:
i18ninput::BuildI18nInputs(i18ninput::ADDRESS_TYPE_SHIPPING,
On 2013/12/02 23:30:34, Evan Stade wrote:
> this still won't compile on android :/

Done.

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

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1932:
ui::ComboboxModel* model,
On 2013/12/02 23:30:34, Evan Stade wrote:
> this should take a ServerFieldType rather than model and section.

Done.

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

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:69: std::string
GuessCountry() {
On 2013/12/02 23:30:34, Evan Stade wrote:
> don't add this function; use PersonalDataManager::GetDefaultCountryCode()

Done.

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:88: int row_id = 100;
On 2013/12/02 23:30:34, Evan Stade wrote:
> I don't really have an objection to the extra knee room here, except that when
I
> read
> 
>   int row_id = 0;
> 
> my brain skims over it without looking for extra meaning, and when I read
> 
>   int row_id = 100;
> 
> I start to wonder what black magic could be at work.

Added comments.

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:107: ++row_id;
On 2013/12/02 23:30:34, Evan Stade wrote:
> \n

Done.

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1920: views::View*
inputs_container = CreateInputsContainer(section);
On 2013/12/02 23:30:34, Evan Stade wrote:
> nit (although you didn't change this), please inline this instead of saving to
a
> local var

Done.

https://codereview.chromium.org/63053003/diff/1060001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/63053003/diff/1060001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:365:
'browser/ui/autofill/autofill_dialog_i18n_input.cc',
On 2013/12/02 23:30:34, Evan Stade wrote:
> this needs to be excluded on android

Done.

[Evan Stade, 2013-12-03 02:32:47]

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

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:88: int row_id = 100;
On 2013/12/03 02:11:19, Dan Beam wrote:
> On 2013/12/02 23:30:34, Evan Stade wrote:
> > I don't really have an objection to the extra knee room here, except that
when
> I
> > read
> > 
> >   int row_id = 0;
> > 
> > my brain skims over it without looking for extra meaning, and when I read
> > 
> >   int row_id = 100;
> > 
> > I start to wonder what black magic could be at work.
> 
> Added comments.

discussed offline. Best to remove the whole row id crap. SHORT/LONG enum is a
better way.

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

https://codereview.chromium.org/63053003/diff/1090001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:107: //  >= 200: visible
fields after address inputs (e.g. email, phone number)
no magic numbers

[Dan Beam, 2013-12-03 07:38:06]

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

https://codereview.chromium.org/63053003/diff/1060001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:88: int row_id = 100;
On 2013/12/03 02:32:48, Evan Stade wrote:
> On 2013/12/03 02:11:19, Dan Beam wrote:
> > On 2013/12/02 23:30:34, Evan Stade wrote:
> > > I don't really have an objection to the extra knee room here, except that
> when
> > I
> > > read
> > > 
> > >   int row_id = 0;
> > > 
> > > my brain skims over it without looking for extra meaning, and when I read
> > > 
> > >   int row_id = 100;
> > > 
> > > I start to wonder what black magic could be at work.
> > 
> > Added comments.
> 
> discussed offline. Best to remove the whole row id crap. SHORT/LONG enum is a
> better way.

Done.

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

https://codereview.chromium.org/63053003/diff/1090001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:107: //  >= 200: visible
fields after address inputs (e.g. email, phone number)
On 2013/12/03 02:32:49, Evan Stade wrote:
> no magic numbers

Done.

[Evan Stade, 2013-12-05 01:45:30]

could you upload just the changes that are part of this CL and not include
dependent changes?

[Dan Beam, 2013-12-05 02:15:56]

On 2013/12/05 01:45:30, Evan Stade wrote:
> could you upload just the changes that are part of this CL and not include
> dependent changes?

Done.

[Dan Beam, 2013-12-05 02:45:02]

+groby@ for chrome/browser/ui/cocoa
+aruslan@ for chrome/browser/ui/android

[groby-ooo-7-16, 2013-12-05 02:56:09]

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

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:106: // Create a
view with all inputs requested by |delegate_| and resets |input_|.
Should probably mention that it replaces the old inputs in the view hierarchy,
too. 

That's why I'd rather return a LayoutView - this function starts doing too many
things.

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:634: [self
updateFieldIcons];
This must be called _after_ updating inputs_ - updateFieldIcons updates the
fields in inputs_

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:667: const
autofill::DetailInput* input = [self detailInputForType:type];
Why can this now be zero?

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_section_container_unittest.mm
(right):

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container_unittest.mm:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
Think you could add a test for the field replacement? (Happy to help with it if
Cocoa tests are confuzzling :)

[Evan Stade, 2013-12-05 03:09:16]

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (left):

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:212: AutofillCountry
country("US", g_browser_process->GetApplicationLocale());
seems like you're breaking the non-i18nized case (?)

I would say you should leave this function alone, and make the if() stmt check
if we're not using i18n addresses.

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

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1096:
it->initial_value = common::GetInitialValueForType(it->type, country_code);
I think it's simpler if you leave this code alone (see my comment about
GetHardcodedValueForType), but make sure ComboboxModel::GetDefaultIndex() works
the way you want it to for CountryComboboxModel and the view respects that.

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1935: if (model !=
&country_combobox_model_ || model->IsItemSeparatorAt(index))
are you sure the isitemseparatorat is necessary? seems that should never be
selected

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2033: input.length ==
DetailInput::SHORT_EOL) {
would be nice to test this stuff works. Maybe a TODO to add a test.

[Dan Beam, 2013-12-05 08:55:57]

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_common.cc (left):

https://codereview.chromium.org/63053003/diff/1390001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.cc:212: AutofillCountry
country("US", g_browser_process->GetApplicationLocale());
On 2013/12/05 03:09:18, Evan Stade wrote:
> seems like you're breaking the non-i18nized case (?)
> 
> I would say you should leave this function alone, and make the if() stmt check
> if we're not using i18n addresses.

We should just add back i18ninput::GuessCountry() or some equivalent as there
_was_ no way this could change from US until we started asking the personal data
manager its default country (which is why it required no special code for the
non-i18n case).

[Dan Beam, 2013-12-31 02:05:38]

ready to review again.

i'm all ears as to how to split this up if desired.

[Dan Beam, 2014-01-02 19:55:23]

specific responsibilities:

groby@: chrome/browser/ui/cocoa/autofill
aruslan@: chrome/browser/ui/android/autofill
estade@: everything else

[Evan Stade, 2014-01-02 22:59:24]

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

https://codereview.chromium.org/63053003/diff/2620001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1121:
it->initial_value = common::GetInitialValueForType(it->type, country_code);
Isn't the purpose of setting the default index on the combobox model so that you
don't need to do this?

https://codereview.chromium.org/63053003/diff/2620001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1968: if
(IsCountryComboboxModel(model))
imo avoid the cast by doing:

if (type == ADDRESS_HOME_COUNTRY) {
  shipping_country_combobox_model_.SetDefaultIndex(index);
} else if (type == ADDRESS_BILLING_COUNTRY) {
  ...
}

https://codereview.chromium.org/63053003/diff/2620001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1969:
static_cast<CountryComboboxModel*>(model)->SetDefaultIndex(index);
can you call OnComboboxModelChanged from here instead of setting up |this| as an
observer?

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

https://codereview.chromium.org/63053003/diff/2620001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:325: bool
IsCountryComboboxModel(const ui::ComboboxModel* model) const;
why is this protected instead of private? I can't find a usage outside of
AutofillDialogControllerImpl

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

https://codereview.chromium.org/63053003/diff/2620001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/data_model_wrapper.cc:35: input->initial_value =
common::GetInitialValueForType(input->type,
I was hoping we could get rid of this

[Dan Beam, 2014-01-03 22:09:46]

so i've gone ahead and split this CL up a bit.  same reviewers as before just
much less to review.  ptal at updated description.

screenshots here: http://imgur.com/a/zXgtC

estade@: all comments from pre-split patchset addressed locally (will upload
soon).

[Dan Beam, 2014-01-03 22:31:51]

> screenshots here: http://imgur.com/a/zXgtC

whoops, to visually describe what you're looking at with awesome ASCII art:

  [ City                      ]
  [ State      ] [ Zip        ] => no flag


  [ City  ] [ State ] [ Zip   ] => with --enable-autofill-address-i18n

[Dan Beam, 2014-01-04 00:04:14]

> estade@: all comments from pre-split patchset addressed locally (will upload
> soon).

... and by this i mean all comments you mentioned earlier are no longer relevant
to _this_ CL and will be uploaded as a separate CL in the near future (when it's
crash-free and pretty).

[Evan Stade, 2014-01-04 00:18:04]

lgtm with nits

https://codereview.chromium.org/63053003/diff/2960001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_common.h (right):

https://codereview.chromium.org/63053003/diff/2960001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_common.h:46: DetailInputs* inputs,
outparams come after inparams

https://codereview.chromium.org/63053003/diff/2960001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://codereview.chromium.org/63053003/diff/2960001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_types.h:36: SHORT_EOL,  // Shares a
line but causes inputs to wrap.  [ CVC ][ Zip ] <-
the ascii art here isn't helping imo, especially because CVC and zip will never
be on the same line.

"Shares a line with other short inputs, akin to display: inline."
"Shares a line with other short inputs but terminates the line, akin to display:
inline with a newline."
"Will be given its own full line, like display: block."

https://codereview.chromium.org/63053003/diff/2960001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (left):

https://codereview.chromium.org/63053003/diff/2960001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:624:
l10n_util::GetNSStringWithFixup(input.placeholder_text_rid)];
what is WithFixup?  Are we going to suffer for not fixing up now?

[Dan Beam, 2014-01-04 00:50:47]

https://chromiumcodereview.appspot.com/63053003/diff/2960001/chrome/browser/u...
File chrome/browser/ui/autofill/autofill_dialog_common.h (right):

https://chromiumcodereview.appspot.com/63053003/diff/2960001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_common.h:46: DetailInputs* inputs,
On 2014/01/04 00:18:04, Evan Stade wrote:
> outparams come after inparams

Done.

https://chromiumcodereview.appspot.com/63053003/diff/2960001/chrome/browser/u...
File chrome/browser/ui/autofill/autofill_dialog_types.h (right):

https://chromiumcodereview.appspot.com/63053003/diff/2960001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_types.h:36: SHORT_EOL,  // Shares a
line but causes inputs to wrap.  [ CVC ][ Zip ] <-
On 2014/01/04 00:18:04, Evan Stade wrote:
> the ascii art here isn't helping imo, especially because CVC and zip will
never
> be on the same line.
> 
> "Shares a line with other short inputs, akin to display: inline."
> "Shares a line with other short inputs but terminates the line, akin to
display:
> inline with a newline."
> "Will be given its own full line, like display: block."

Done.

https://chromiumcodereview.appspot.com/63053003/diff/2960001/chrome/browser/u...
File chrome/browser/ui/cocoa/autofill/autofill_section_container.mm (left):

https://chromiumcodereview.appspot.com/63053003/diff/2960001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:624:
l10n_util::GetNSStringWithFixup(input.placeholder_text_rid)];
On 2014/01/04 00:18:04, Evan Stade wrote:
> what is WithFixup?  Are we going to suffer for not fixing up now?

the other 3 uses of base::SysUTF16ToNSString() in this file suggest no, but I
could run l10n_util::FixUpWindowsStyleLabel() on the string if it makes you
nervous...

groby@: do we need to do this?

[groby-ooo-7-16, 2014-01-04 01:15:32]

For withFixup, see comment. LGTM % comments.

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

https://chromiumcodereview.appspot.com/63053003/diff/3110001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:624:
base::SysUTF16ToNSString(input.placeholder_text)];
Unless you are 100% certain there will never be ellipses or accelerators in the
text, always use FixUpWindowsStyleLabel. I _think_ we should be fine here,
though.

https://chromiumcodereview.appspot.com/63053003/diff/3110001/chrome/browser/u...
File chrome/browser/ui/cocoa/autofill/autofill_section_container_unittest.mm
(right):

https://chromiumcodereview.appspot.com/63053003/diff/3110001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_section_container_unittest.mm:125:
l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_PLACEHOLDER_EMAIL) },
Since the string constants are never used in the test, might as well just have
string16() here. Or ASCIIToUTF16("placeholder"). No need to pull in l10nutil

[Dan Beam, 2014-01-04 01:39:23]

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

https://chromiumcodereview.appspot.com/63053003/diff/3110001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_section_container.mm:624:
base::SysUTF16ToNSString(input.placeholder_text)];
On 2014/01/04 01:15:33, groby wrote:
> Unless you are 100% certain there will never be ellipses or accelerators in
the
> text, always use FixUpWindowsStyleLabel. I _think_ we should be fine here,
> though.

meh, guess it doesn't hurt. Done.

https://chromiumcodereview.appspot.com/63053003/diff/3110001/chrome/browser/u...
File chrome/browser/ui/cocoa/autofill/autofill_section_container_unittest.mm
(right):

https://chromiumcodereview.appspot.com/63053003/diff/3110001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_section_container_unittest.mm:125:
l10n_util::GetStringUTF16(IDS_AUTOFILL_DIALOG_PLACEHOLDER_EMAIL) },
On 2014/01/04 01:15:33, groby wrote:
> Since the string constants are never used in the test, might as well just have
> string16() here. Or ASCIIToUTF16("placeholder"). No need to pull in l10nutil

Done.

[commit-bot: I haz the power, 2014-01-06 19:49:26]

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

[commit-bot: I haz the power, 2014-01-06 22:39:26]

Change committed as 243189