Update Autofill Private API to provide a list of available countries.

Update Autofill Private API to provide a list of available countries.

BUG=607347
Committed: https://crrev.com/132cb2dc5d841b0b25ad5580d02794614ba561f2
Cr-Commit-Position: refs/heads/master@{#399306}

[hcarmona, 2016-06-01 02:34:09]

Patchset #1 (id:1) has been deleted

[hcarmona, 2016-06-01 02:35:54]

hcarmona@chromium.org changed reviewers:
+ stevenjb@chromium.org

[hcarmona, 2016-06-01 02:35:54]

PTAL, in the old settings this was in the loadTimeData but it's not
necessary until later, so I've added it to the private API.

[stevenjb, 2016-06-08 03:43:23]

Thanks for adding this to the API.

lgtm with one suggestion.

https://codereview.chromium.org/2028903002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
(right):

https://codereview.chromium.org/2028903002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/autofill_private/autofill_private_api.cc:295:
DCHECK(personal_data && personal_data->IsDataLoaded());
Assuming data is loaded asynchronously, it would be extremely unlikely but not
strictly impossible for IsDataLoaded to be false. In that unlikely edge case I
would just return an empty list.

[hcarmona, 2016-06-08 14:47:14]

Patchset #2 (id:40001) has been deleted

[hcarmona, 2016-06-08 15:05:50]

+Devlin for .idl
+Ilya for histograms

https://codereview.chromium.org/2028903002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
(right):

https://codereview.chromium.org/2028903002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/autofill_private/autofill_private_api.cc:295:
DCHECK(personal_data && personal_data->IsDataLoaded());
On 2016/06/08 03:43:23, stevenjb wrote:
> Assuming data is loaded asynchronously, it would be extremely unlikely but not
> strictly impossible for IsDataLoaded to be false. In that unlikely edge case I
> would just return an empty list.

Done.

[hcarmona, 2016-06-10 18:12:08]

hcarmona@chromium.org changed reviewers:
+ isherman@chromium.org, rdevlin.cronin@chromium.org

[hcarmona, 2016-06-10 18:12:09]

Last message I wrote:

+Devlin for .idl
+Ilya for histograms

and forgot to actually add as reviewers.

[Devlin, 2016-06-10 18:25:12]

lgtm

https://codereview.chromium.org/2028903002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
(right):

https://codereview.chromium.org/2028903002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/autofill_private/autofill_private_api.cc:303:
extensions::autofill_util::GenerateCountryList(*personal_data);
nitty nit: no extensions::

https://codereview.chromium.org/2028903002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/autofill_private/autofill_private_apitest.cc
(right):

https://codereview.chromium.org/2028903002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/autofill_private/autofill_private_apitest.cc:50:
EXPECT_TRUE(RunAutofillSubtest("getCountryList")) << message_;
semi-drive-by - do all these really need separate browser tests?  Browser tests
are expensive, and most extension APIs are designed to be able to run multiple
tests in a single browsertest to avoid startup/shutdown costs.  Alternatively,
and even better, it looks like a lot of these could just be unittests.

I don't wanna block this CL on that (unless you're up for it ;)), but as food
for thought, and maybe a TODO.

[Ilya Sherman, 2016-06-10 20:35:21]

histograms.xml lgtm

[hcarmona, 2016-06-10 20:42:51]

https://codereview.chromium.org/2028903002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
(right):

https://codereview.chromium.org/2028903002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/autofill_private/autofill_private_api.cc:303:
extensions::autofill_util::GenerateCountryList(*personal_data);
On 2016/06/10 18:25:11, Devlin wrote:
> nitty nit: no extensions::

Done.

https://codereview.chromium.org/2028903002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/autofill_private/autofill_private_apitest.cc
(right):

https://codereview.chromium.org/2028903002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/autofill_private/autofill_private_apitest.cc:50:
EXPECT_TRUE(RunAutofillSubtest("getCountryList")) << message_;
On 2016/06/10 18:25:11, Devlin wrote:
> semi-drive-by - do all these really need separate browser tests?  Browser
tests
> are expensive, and most extension APIs are designed to be able to run multiple
> tests in a single browsertest to avoid startup/shutdown costs.  Alternatively,
> and even better, it looks like a lot of these could just be unittests.
> 
> I don't wanna block this CL on that (unless you're up for it ;)), but as food
> for thought, and maybe a TODO.

Sounds good, I've added a TODO at the top of this file to investigate
converting to unittests.

[hcarmona, 2016-06-10 20:43:03]

The CQ bit was checked by hcarmona@chromium.org

[hcarmona, 2016-06-10 20:43:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, isherman@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2028903002/#ps80001
(title: "nits")

[commit-bot: I haz the power, 2016-06-10 20:43:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028903002/80001

[commit-bot: I haz the power, 2016-06-10 22:50:57]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-06-10 22:51:02]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 22:52:49]

Description was changed from

==========
Update Autofill Private API to provide a list of available countries.

BUG=607347
==========

to

==========
Update Autofill Private API to provide a list of available countries.

BUG=607347

Committed: https://crrev.com/132cb2dc5d841b0b25ad5580d02794614ba561f2
Cr-Commit-Position: refs/heads/master@{#399306}
==========

[commit-bot: I haz the power, 2016-06-10 22:52:51]

Patchset 3 (id:??) landed as
https://crrev.com/132cb2dc5d841b0b25ad5580d02794614ba561f2
Cr-Commit-Position: refs/heads/master@{#399306}