Autofill i18n: Set postal code and state field labels based on the selected country.

Autofill i18n: Set postal code and state field labels based on the selected country.

* Changes the country field to a <select> field.
* Restricts the possible values for the "country" field to a set of known values
* Moves the country field to the top of the Autofill dialog
* Changes the field labels according to the selected country

BUG=56599, 56602, 56604
TEST=unit_tests --gtest_filter=AddressTest (and a few others; see cl)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76117

[Ilya Sherman, 2011-02-11 11:02:53]

This still needs a good bit of work, but I'd appreciate any feedback you might
have at this point.

James: Mostly tagging you for DOMUI changes, though feel free to comment on the
other parts as well.

Things that still need to happen:
 * Only allowing autofill profiles to contain one of these country values
 * Tests

[James Hawkins, 2011-02-11 18:54:18]

The diff viewer is saying "Old chunk mismatch" and I can't see the diff that
way.

[Ilya Sherman, 2011-02-11 19:59:37]

Re-uploaded

[James Hawkins, 2011-02-11 21:40:50]

I'm concerned that the list of countries is being added to
generated_resources.grd. Please ask on the list if this is the appropriate
place.

http://codereview.chromium.org/6484022/diff/7005/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6484022/diff/7005/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7118: Do/Si:
What is this?

[Ilya Sherman, 2011-02-11 21:58:50]

On 2011/02/11 21:40:50, James Hawkins wrote:
> I'm concerned that the list of countries is being added to
> generated_resources.grd. Please ask on the list if this is the appropriate
> place.

Emailed the list.

http://codereview.chromium.org/6484022/diff/7005/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6484022/diff/7005/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7118: Do/Si:
On 2011/02/11 21:40:50, James Hawkins wrote:
> What is this?

The dataset I'm pulling this from lists this as the Korean term for "province". 
Based on further research, it looks like "province" would probably be more
appropriate though -- will fix.

[arv (Not doing code reviews), 2011-02-11 22:55:37]

http://codereview.chromium.org/6484022/diff/7005/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_country.h (right):

http://codereview.chromium.org/6484022/diff/7005/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_country.h:264: AutoFillCountry
CountryCodeToCountry(const std::string& country_code);
Please add description of all functions here

http://codereview.chromium.org/6484022/diff/7005/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_edit_address_overlay.js (right):

http://codereview.chromium.org/6484022/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_edit_address_overlay.js:123: var
country_code = $('country').value;
No underscores in JS

http://codereview.chromium.org/6484022/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_edit_address_overlay.js:136: for
(country_code in countries) {
missing var

http://codereview.chromium.org/6484022/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_edit_address_overlay.js:162:
self.countryChanged_();
this.countryChanged_();

[Ilya Sherman, 2011-02-12 07:33:02]

Transitioned to use ICU country names.

http://codereview.chromium.org/6484022/diff/7005/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_edit_address_overlay.js (right):

http://codereview.chromium.org/6484022/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_edit_address_overlay.js:123: var
country_code = $('country').value;
On 2011/02/11 22:55:37, arv wrote:
> No underscores in JS

Done.

http://codereview.chromium.org/6484022/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_edit_address_overlay.js:136: for
(country_code in countries) {
On 2011/02/11 22:55:37, arv wrote:
> missing var

Done.

http://codereview.chromium.org/6484022/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_edit_address_overlay.js:162:
self.countryChanged_();
On 2011/02/11 22:55:37, arv wrote:
> this.countryChanged_();

Done.  When should "self" be used?  I was mirroring connectInputEvents_().

[James Hawkins, 2011-02-12 20:32:45]

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_country.cc (right):

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.cc:12: #include
"base/utf_string_conversions.h"
Is this header used?

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.cc:16: #include
"third_party/icu/public/common/unicode/locid.h"
't' before 'u'.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.cc:1237: typedef std::map<std::string,
AutoFillCountryData> MapType;
Document this type.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.cc:1244: friend struct
DefaultSingletonTraits<AutoFillCountries>;
friend declarations should be first in the section.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_country.h (right):

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.h:14: class AutoFillCountry {
Document the class.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.h:20: // Tries to return the country
data most appropriate for the given |locale|.
Ambiguities like 'tries' are best removed. The method is going to return
something no matter what, so 'tries' is not appropriate.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.h:37: std::string country_code_;
It's up to you, but I think these members could use documentation.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
File chrome/browser/resources/options/autofill_edit_address_overlay.html
(right):

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:4: <div
class="input">
Why are you moving the position of the country input?

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:5: <label
id="countryLabel">
IDs use dash-form. I realize that this file is using the old style (camelCase). 
I'll fix that after you commit this.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:8: <option
value=""></option>
It appears you're filling the select element in the JS. Remove this option.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:49: <label
id="postalCodeLabel" for="postalCode">
dash-form ID

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:62: <input
type="text" class="form" id="postalCode">
dash-form ID

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
File chrome/browser/resources/options/autofill_edit_address_overlay.js (right):

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:122:
countryChanged_: function(event) {
Document parameter.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:124: if
(countryCode == '')
if (!countryCode)

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:141: var
countryData = templateData.autofillCountryData;
localStrings.getString('autofillCountryData');

[arv (Not doing code reviews), 2011-02-14 18:36:41]

http://codereview.chromium.org/6484022/diff/7005/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_edit_address_overlay.js (right):

http://codereview.chromium.org/6484022/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_edit_address_overlay.js:162:
self.countryChanged_();
On 2011/02/12 07:33:02, Ilya Sherman wrote:
> On 2011/02/11 22:55:37, arv wrote:
> > this.countryChanged_();
> 
> Done.  When should "self" be used?  I was mirroring connectInputEvents_().

Only when you need to capture a reference to this in the closure. For example:

var self = this;
... function() {
  self.doSomething();
|

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:129: /**
Use i18n-content attribute instead

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:130: *
Populates the country <select> list.
same here

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:146: *
@private
Use literals where possible.

var countries = [];

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:154:
$('state').value = '';
Skip the quotes?

The Google3 style rule is to use quoted property access when you are dealing
with external data.

var country = {
  countryCode: countryCode,
  name: countryData[countryCode]['name']
};

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:157:
$('phone').value = '';
countries.push(country)

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:162:
self.countryChanged_();
ws around ?

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:162:
self.countryChanged_();
a.name < b.name ? -1 : 1

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:174: },
i++ is the norm in js

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:175: 
or

var option = new Option(text, value);

[Ilya Sherman, 2011-02-16 10:27:18]

Still needs tests, but otherwise hopefully feature-complete.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_country.cc (right):

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.cc:12: #include
"base/utf_string_conversions.h"
On 2011/02/12 20:32:45, James Hawkins wrote:
> Is this header used?

It was, then it wasn't, and now it is again.  Well spotted though!

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.cc:16: #include
"third_party/icu/public/common/unicode/locid.h"
On 2011/02/12 20:32:45, James Hawkins wrote:
> 't' before 'u'.

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.cc:1237: typedef std::map<std::string,
AutoFillCountryData> MapType;
On 2011/02/12 20:32:45, James Hawkins wrote:
> Document this type.

Done, though not sure how much value-add the comment really provides.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.cc:1244: friend struct
DefaultSingletonTraits<AutoFillCountries>;
On 2011/02/12 20:32:45, James Hawkins wrote:
> friend declarations should be first in the section.

That doesn't seem to be the style used elsewhere -- if anything, friend
declarations tend to occur toward the bottom of the section.  This particular
pattern I copied from singleton.h, though.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_country.h (right):

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.h:14: class AutoFillCountry {
On 2011/02/12 20:32:45, James Hawkins wrote:
> Document the class.

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.h:20: // Tries to return the country
data most appropriate for the given |locale|.
On 2011/02/12 20:32:45, James Hawkins wrote:
> Ambiguities like 'tries' are best removed. The method is going to return
> something no matter what, so 'tries' is not appropriate.

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.h:37: std::string country_code_;
On 2011/02/12 20:32:45, James Hawkins wrote:
> It's up to you, but I think these members could use documentation.

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
File chrome/browser/resources/options/autofill_edit_address_overlay.html
(right):

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:4: <div
class="input">
On 2011/02/12 20:32:45, James Hawkins wrote:
> Why are you moving the position of the country input?

Different countries have different address formats.  This extends beyond just
distinctions like "postal code" vs "zip code" -- the choice of country affects
which order the address fields should be presented in (http://crbug.com/56605). 
David and I thought such changes would feel more natural if the controlling
element -- the country <select> field -- were above the remaining address
elements.

It's less important for now, with just field labels changing, but similar
reasoning applies -- it feels a little off if an element toward the bottom of
the page changes the elements above it.

That said, I'm not sure if it's better to have the country <select> at the very
top of the form, or immediately above the other address fields.  I switched to
the latter for this revision.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:5: <label
id="countryLabel">
On 2011/02/12 20:32:45, James Hawkins wrote:
> IDs use dash-form. I realize that this file is using the old style
(camelCase). 
> I'll fix that after you commit this.

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:8: <option
value=""></option>
On 2011/02/12 20:32:45, James Hawkins wrote:
> It appears you're filling the select element in the JS. Remove this option.

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:49: <label
id="postalCodeLabel" for="postalCode">
On 2011/02/12 20:32:45, James Hawkins wrote:
> dash-form ID

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:62: <input
type="text" class="form" id="postalCode">
On 2011/02/12 20:32:45, James Hawkins wrote:
> dash-form ID

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
File chrome/browser/resources/options/autofill_edit_address_overlay.js (right):

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:122:
countryChanged_: function(event) {
On 2011/02/12 20:32:45, James Hawkins wrote:
> Document parameter.

Removed.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:124: if
(countryCode == '')
On 2011/02/12 20:32:45, James Hawkins wrote:
> if (!countryCode)

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:129:
postal.textContent = details['postalCodeLabel'];
On 2011/02/14 18:36:41, arv wrote:
> Use i18n-content attribute instead

As discussed on irc, can't use i18n-content because this content is dynamic.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:141: var
countryData = templateData.autofillCountryData;
On 2011/02/12 20:32:45, James Hawkins wrote:
> localStrings.getString('autofillCountryData');

It's not actually a string, so getString() doesn't seem appropriate.  I'm not
sure if there's a more transparent way to pass the country data to the WebUI
layer -- the current approach mirrors how the "add language" overlay loads is
data.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:146: var
countries = new Array();
On 2011/02/14 18:36:41, arv wrote:
> Use literals where possible.
> 
> var countries = [];

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:154:
'countryCode': countryCode,
On 2011/02/14 18:36:41, arv wrote:
> Skip the quotes?
> 
> The Google3 style rule is to use quoted property access when you are dealing
> with external data.
> 
> var country = {
>   countryCode: countryCode,
>   name: countryData[countryCode]['name']
> };

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:157:
countries[countries.length] = country;
On 2011/02/14 18:36:41, arv wrote:
> countries.push(country)

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:162: return
a['name'] < b['name']? -1 : 1;
On 2011/02/14 18:36:41, arv wrote:
> ws around ?

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:162: return
a['name'] < b['name']? -1 : 1;
On 2011/02/14 18:36:41, arv wrote:
> a.name < b.name ? -1 : 1

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:174: for (var
i = 0; i < countries.length; ++i) {
On 2011/02/14 18:36:41, arv wrote:
> i++ is the norm in js

Done.

http://codereview.chromium.org/6484022/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:175: var
option = document.createElement('option');
On 2011/02/14 18:36:41, arv wrote:
> or
> 
> var option = new Option(text, value);

Done.

[James Hawkins, 2011-02-16 19:49:01]

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/add...
File chrome/browser/autofill/address.cc (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/add...
chrome/browser/autofill/address.cc:9: #include "base/utf_string_conversions.h"
Where is it used?

[dhollowa, 2011-02-17 00:55:28]

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_country.cc (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.cc:396: // TODO(isherman): Add other
synonyms
We should do this now.  It need not be exhaustive for all countries, but we
should cover the common cases for the "majors" so that data migration works from
older profile data.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_profile.h (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.h:49: virtual string16 Label() const;
Please put the |const| designation back.  See item 3 of Effective-C++ for
details.  Quote: "...reduce incidence of client errors without giving up on
safety or efficiency".

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.h:56: std::string CountryCode() const;
const std::string CountryCode() const;

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/cre...
File chrome/browser/autofill/credit_card.h (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/cre...
chrome/browser/autofill/credit_card.h:35: virtual string16 Label() const;
Same here... please put |const| back.  And in implementation, of course.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
File chrome/browser/autofill/select_control_handler.cc (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
chrome/browser/autofill/select_control_handler.cc:140: webkit_glue::FormField*
field) {
A DCHECK(!value.empty()) might be worthwhile here since you've moved the
responsibility to the caller.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
chrome/browser/autofill/select_control_handler.cc:160: bool
FillCountrySelectControl(const FormGroup& form_group,
This looks very much like |SetSelectControlValue|.  But it does not try both the
country code and full country string.  I believe it should.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
chrome/browser/autofill/select_control_handler.cc:180: webkit_glue::FormField*
field) {
A DCHECK(!value.empty()) might be worthwhile here since you've moved the
responsibility to the caller.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
chrome/browser/autofill/select_control_handler.cc:181: int index = 0;
This no longer tries both the and full month string and the alternates.  I
believe it should.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/autofill_options_handler.cc (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/autofill_options_handler.cc:161:
localized_strings->SetString("country-label",
As discussed... country should follow other address fields in UI and in code.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/autofill_edit_address_overlay.html
(right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:10: <div
class="input">
As discussed, country should follow other address fields.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:17: <label
id="companyNameLabel">
The new id and i18n-content values seem inconsistent with the old ones.  Camel
case or hyphens throughout?

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/autofill_edit_address_overlay.js (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:68: address[2]
= $('country').value;
Country down below.

[Ilya Sherman, 2011-02-17 23:09:11]

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/add...
File chrome/browser/autofill/address.cc (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/add...
chrome/browser/autofill/address.cc:9: #include "base/utf_string_conversions.h"
On 2011/02/16 19:49:01, James Hawkins wrote:
> Where is it used?

Removed.  I wish cpplint.py would tell me about things like this though!

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_country.cc (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.cc:396: // TODO(isherman): Add other
synonyms
On 2011/02/17 00:55:28, dhollowa wrote:
> We should do this now.  It need not be exhaustive for all countries, but we
> should cover the common cases for the "majors" so that data migration works
from
> older profile data.

Do you have a datasource in mind for this?

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_profile.h (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.h:49: virtual string16 Label() const;
On 2011/02/17 00:55:28, dhollowa wrote:
> Please put the |const| designation back.  See item 3 of Effective-C++ for
> details.  Quote: "...reduce incidence of client errors without giving up on
> safety or efficiency".

Done.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_profile.h:56: std::string CountryCode() const;
On 2011/02/17 00:55:28, dhollowa wrote:
> const std::string CountryCode() const;

Done.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/cre...
File chrome/browser/autofill/credit_card.h (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/cre...
chrome/browser/autofill/credit_card.h:35: virtual string16 Label() const;
On 2011/02/17 00:55:28, dhollowa wrote:
> Same here... please put |const| back.  And in implementation, of course.

Done.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
File chrome/browser/autofill/select_control_handler.cc (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
chrome/browser/autofill/select_control_handler.cc:140: webkit_glue::FormField*
field) {
On 2011/02/17 00:55:28, dhollowa wrote:
> A DCHECK(!value.empty()) might be worthwhile here since you've moved the
> responsibility to the caller.

Nothing will go wrong if value is empty -- it'll just be a little slower to
return "false".

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
chrome/browser/autofill/select_control_handler.cc:160: bool
FillCountrySelectControl(const FormGroup& form_group,
On 2011/02/17 00:55:28, dhollowa wrote:
> This looks very much like |SetSelectControlValue|.  But it does not try both
the
> country code and full country string.  I believe it should.

The caller already tries the full country string.  This method actually does
something pretty neat: it canonicalizes each <option> value to a country code,
and checks to see if that matches the profile's country code.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
chrome/browser/autofill/select_control_handler.cc:180: webkit_glue::FormField*
field) {
On 2011/02/17 00:55:28, dhollowa wrote:
> A DCHECK(!value.empty()) might be worthwhile here since you've moved the
> responsibility to the caller.

An empty value would just trip the "return false;" in the if-stmt below, so I
don't think the DCHECK is all that necessary.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
chrome/browser/autofill/select_control_handler.cc:181: int index = 0;
On 2011/02/17 00:55:28, dhollowa wrote:
> This no longer tries both the and full month string and the alternates.  I
> believe it should.

The caller already tries the full month value.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/dom_ui/optio...
File chrome/browser/dom_ui/options/autofill_options_handler.cc (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/dom_ui/optio...
chrome/browser/dom_ui/options/autofill_options_handler.cc:161:
localized_strings->SetString("country-label",
On 2011/02/17 00:55:28, dhollowa wrote:
> As discussed... country should follow other address fields in UI and in code.

Done.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/autofill_edit_address_overlay.html
(right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:10: <div
class="input">
On 2011/02/17 00:55:28, dhollowa wrote:
> As discussed, country should follow other address fields.

Done.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:17: <label
id="companyNameLabel">
On 2011/02/17 00:55:28, dhollowa wrote:
> The new id and i18n-content values seem inconsistent with the old ones.  Camel
> case or hyphens throughout?

Hyphens, but James seemed to want to change the rest in a separate CL.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/autofill_edit_address_overlay.js (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.js:68: address[2]
= $('country').value;
On 2011/02/17 00:55:28, dhollowa wrote:
> Country down below.

Done.

[dhollowa, 2011-02-17 23:36:47]

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_country.cc (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country.cc:396: // TODO(isherman): Add other
synonyms
Not really.  You could ask the geo team for synonyms for the US, UK, etc.

On 2011/02/17 23:09:11, Ilya Sherman wrote:
> On 2011/02/17 00:55:28, dhollowa wrote:
> > We should do this now.  It need not be exhaustive for all countries, but we
> > should cover the common cases for the "majors" so that data migration works
> from
> > older profile data.
> 
> Do you have a datasource in mind for this?

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
File chrome/browser/autofill/select_control_handler.cc (right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
chrome/browser/autofill/select_control_handler.cc:160: bool
FillCountrySelectControl(const FormGroup& form_group,
Makes sense, ya.

On 2011/02/17 23:09:11, Ilya Sherman wrote:
> On 2011/02/17 00:55:28, dhollowa wrote:
> > This looks very much like |SetSelectControlValue|.  But it does not try both
> the
> > country code and full country string.  I believe it should.
> 
> The caller already tries the full country string.  This method actually does
> something pretty neat: it canonicalizes each <option> value to a country code,
> and checks to see if that matches the profile's country code.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
chrome/browser/autofill/select_control_handler.cc:180: webkit_glue::FormField*
field) {
Ok.

On 2011/02/17 23:09:11, Ilya Sherman wrote:
> On 2011/02/17 00:55:28, dhollowa wrote:
> > A DCHECK(!value.empty()) might be worthwhile here since you've moved the
> > responsibility to the caller.
> 
> An empty value would just trip the "return false;" in the if-stmt below, so I
> don't think the DCHECK is all that necessary.

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/autofill/sel...
chrome/browser/autofill/select_control_handler.cc:181: int index = 0;
Makes sense, ya.

On 2011/02/17 23:09:11, Ilya Sherman wrote:
> On 2011/02/17 00:55:28, dhollowa wrote:
> > This no longer tries both the and full month string and the alternates.  I
> > believe it should.
> 
> The caller already tries the full month value.

http://codereview.chromium.org/6484022/diff/28001/chrome/browser/autofill/aut...
File chrome/browser/autofill/autofill_country_unittest.cc (right):

http://codereview.chromium.org/6484022/diff/28001/chrome/browser/autofill/aut...
chrome/browser/autofill/autofill_country_unittest.cc:17: #if defined(OS_MACOSX)
Ooo, these should go.  The colon variants for the Mac was for the native dialog.
 We can purge these strings now that we have DOMUI.

[Ilya Sherman, 2011-02-18 00:57:09]

Added support for 3-letter country codes, and a few oddball country name
synonyms.  Removed Mac-only strings that differ only by colon.

[dhollowa, 2011-02-18 01:18:16]

LGTM.  Thanks!

[arv (Not doing code reviews), 2011-02-21 03:09:27]

FYI

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/autofill_edit_address_overlay.html
(right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:42: <label
id="state-label" for="state"></label>
You should probably fix the other cases too or this gets too inconsistent.

[Ilya Sherman, 2011-02-23 01:17:15]

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
File chrome/browser/resources/options/autofill_edit_address_overlay.html
(right):

http://codereview.chromium.org/6484022/diff/20001/chrome/browser/resources/op...
chrome/browser/resources/options/autofill_edit_address_overlay.html:42: <label
id="state-label" for="state"></label>
On 2011/02/21 03:09:27, arv wrote:
> You should probably fix the other cases too or this gets too inconsistent.

Split out to http://codereview.chromium.org/6552002/

[Ilya Sherman, 2011-02-25 10:01:41]

James: lg?

[James Hawkins, 2011-02-25 18:18:54]

LGTM