set a default country in the autofill dialog.

set a default country in the autofill dialog, and sort by display name

BUG=none
TBR=thestig@chromium.org

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

[Evan Stade, 2013-02-25 23:56:57]

depends on https://codereview.chromium.org/12335026/, which is currently failing
on clang trybots.

[Ilya Sherman, 2013-02-26 00:31:13]

There is similar logic in [
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
].  It would be nice to share the logic between these two, so that they're
consistent.

Specifically:
  (1) The Autofill options includes the default country both at the top and
alphabetically inline in the list.  I believe that this was per recommendation
from Glen.
  (2) The autofill_options_handler doesn't sort the countries using l10n_util. 
Instead, they're sorted in autofill_edit_address_overlay.cc; but I'm not sure
that the sorting ends up consistent.

It would be good for these two UI-surfaced lists of countries to be consistent
with one another.

https://codereview.chromium.org/12328091/diff/3001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_models.h (right):

https://codereview.chromium.org/12328091/diff/3001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_models.h:15: #include
"chrome/browser/autofill/autofill_country.h"
nit: Can you forward-declare this?

[Evan Stade, 2013-02-26 02:12:45]

updated

this leaves us in the unpleasant situation of trying to reconcile what the empty
entry should look like. In the autofill dialog, I'm not sure we want an empty
entry, but if we do we need some way to indicate that the dropdown refers to
country. In the options overlay, it's not an issue because there are labels
instead of placeholders.

[Ilya Sherman, 2013-02-26 09:57:07]

LGTM, thanks.  I'm ok with omitting the empty entry in the dialog UI, since, the
field is required, unlike in the options UI.

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/country_combobox_model.cc (right):

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/country_combobox_model.cc:35:
countries_.insert(countries_.begin(), NULL);
nit: This is a little hard to follow, since it's in the opposite order from the
order in which the items actually appear in the vector.  IMO it would be clearer
to use a temporary vector for the countries that you want to sort, and then copy
that vector into the end of countries_.

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/autofill_options_handler.cc (right):

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/autofill_options_handler.cc:2: // Use of this
soutaxrce code is governed by a BSD-style license that can be
nit: soutaxrce, huh? ;)

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/autofill_options_handler.cc:343: ListValue*
country_list = new ListValue();
nit: I'd prefer you use a scoped_ptr here, and release() it on line 265.  (Same
applies to the other new's in this function.)

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/autofill_options_handler.h (right):

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/autofill_options_handler.h:12: #include
"chrome/browser/ui/autofill/country_combobox_model.h"
nit: Can this be moved into the implementation file?

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/autofill_options_handler.h:46: void
SetCountryData(base::DictionaryValue* localized_strings);
nit: Can this remain tucked into the anonymous namespace, rather than exposed in
the header?

[Evan Stade, 2013-02-26 19:01:40]

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/country_combobox_model.cc (right):

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/country_combobox_model.cc:35:
countries_.insert(countries_.begin(), NULL);
On 2013/02/26 09:57:07, Ilya Sherman wrote:
> nit: This is a little hard to follow, since it's in the opposite order from
the
> order in which the items actually appear in the vector.  IMO it would be
clearer
> to use a temporary vector for the countries that you want to sort, and then
copy
> that vector into the end of countries_.

Done.

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/autofill_options_handler.cc (right):

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/autofill_options_handler.cc:2: // Use of this
soutaxrce code is governed by a BSD-style license that can be
On 2013/02/26 09:57:07, Ilya Sherman wrote:
> nit: soutaxrce, huh? ;)

Done.

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/autofill_options_handler.cc:343: ListValue*
country_list = new ListValue();
On 2013/02/26 09:57:07, Ilya Sherman wrote:
> nit: I'd prefer you use a scoped_ptr here, and release() it on line 265. 
(Same
> applies to the other new's in this function.)

Done.

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/autofill_options_handler.h (right):

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/autofill_options_handler.h:12: #include
"chrome/browser/ui/autofill/country_combobox_model.h"
On 2013/02/26 09:57:07, Ilya Sherman wrote:
> nit: Can this be moved into the implementation file?

Done.

https://codereview.chromium.org/12328091/diff/4005/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/autofill_options_handler.h:46: void
SetCountryData(base::DictionaryValue* localized_strings);
On 2013/02/26 09:57:07, Ilya Sherman wrote:
> nit: Can this remain tucked into the anonymous namespace, rather than exposed
in
> the header?

Done.

[commit-bot: I haz the power, 2013-02-26 19:05:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/12328091/1013

[commit-bot: I haz the power, 2013-02-26 19:05:33]

Presubmit check for 12328091-1013 failed and returned exit status 1.

INFO:root:Found 8 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/chrome_browser_ui.gypi

Presubmit checks took 1.6s to calculate.

[commit-bot: I haz the power, 2013-02-27 00:56:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/12328091/1013

[commit-bot: I haz the power, 2013-02-27 04:39:54]

Change committed as 184875

[Lei Zhang, 2013-03-04 20:21:58]

.gypi change lgtm