Remove DataModelWrapper from android build.

Remove DataModelWrapper from android build.

BUG=none

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

[aruslan, 2014-01-24 22:23:14]

Thanks for doing this, Evan!
So far I see only one nit regarding &NullGetInfo vs NullGetInfo consistency, so
it's LGTM mod nits.
I'll make a thorough style pass on Monday morning if Dan wouldn't and if you're
ok to wait.

https://codereview.chromium.org/137723008/diff/200001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc
(right):

https://codereview.chromium.org/137723008/diff/200001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:98:
common::BuildInputsForSection(section, "US", &inputs);
nit: I would be so happy if we could make this gone.
Practically, this should return all possible fields that are applicable for a
given section, not just the fields matching the whatever country the user has
chosen.

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

https://codereview.chromium.org/137723008/diff/200001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:607:
RequestedTypesForSection(SECTION_SHIPPING),
nit: Ideally this shouldn't depend on the country either.
E.g. if a website asks for CEDEX and no other shipping information.
(I'm not saying this is a realistic scenario, and it all could be just a red
herring.)

Additionally, I think it is strange that the choice of the country by default
("US" in this case) comes from the fact that ComboboxModel::GetDefaultIndex()
returns 0 (assuming I followed the code through correctly).

https://codereview.chromium.org/137723008/diff/200001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:609:
base::Bind(&NullGetInfo),
nit: consistency.  it's Bind(NullGetInfo) in the Android code.

https://codereview.chromium.org/137723008/diff/200001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2975:
profile_wrapper.FillFormStructure(types, compare, &form_structure_);
Does the desktop clear _all_ fields (e.g. CEDEX, borough etc) from any
"default"/"preset" values when the user sets the US as the country?
In general, is it allowed for the web-pages to prefill say the billing address
if they know it into the shipping address?

[Evan Stade, 2014-01-25 01:08:46]

https://codereview.chromium.org/137723008/diff/200001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc
(right):

https://codereview.chromium.org/137723008/diff/200001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:98:
common::BuildInputsForSection(section, "US", &inputs);
On 2014/01/24 22:23:15, aruslan wrote:
> nit: I would be so happy if we could make this gone.
> Practically, this should return all possible fields that are applicable for a
> given section, not just the fields matching the whatever country the user has
> chosen.

yes, I agree, we should remove it, especially because it's only used on android
(with libaddressinput replacing it on desktop). But I think that's a job for
another day.

But the reason it works like this is because we generally want there to be a 1:1
mapping between fields in the dialog and fields in the page. Plus I'm pretty
sure this was necessary in order to prevent cross filling, i.e. shipping
overwriting billing fields. And I'm not sure what the win would be in trying to
let a US wallet fill in a CEDEX?

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

https://codereview.chromium.org/137723008/diff/200001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:607:
RequestedTypesForSection(SECTION_SHIPPING),
On 2014/01/24 22:23:15, aruslan wrote:
> nit: Ideally this shouldn't depend on the country either.
> E.g. if a website asks for CEDEX and no other shipping information.
> (I'm not saying this is a realistic scenario, and it all could be just a red
> herring.)
> 
> Additionally, I think it is strange that the choice of the country by default
> ("US" in this case) comes from the fact that ComboboxModel::GetDefaultIndex()
> returns 0 (assuming I followed the code through correctly).

the default country will depend on a number of heuristics, including other
addresses you have stored, your system timezone, and your UI locale. But it's
true, there should be a better way of doing this, perhaps we can look at
html_mode_ in AutofillType, which didn't exist when the code was originally
written.

https://codereview.chromium.org/137723008/diff/200001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:2975:
profile_wrapper.FillFormStructure(types, compare, &form_structure_);
On 2014/01/24 22:23:15, aruslan wrote:
> Does the desktop clear _all_ fields (e.g. CEDEX, borough etc) from any
> "default"/"preset" values when the user sets the US as the country?

no, it only sets values on the fields that make sense for the country of the
selected source.

> In general, is it allowed for the web-pages to prefill say the billing address
> if they know it into the shipping address?

don't understand the question. The existing values in the form will be ignored.

[aruslan, 2014-01-27 19:05:35]

lgtm & ~ (#include, gypi and Bind(&)).

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

https://codereview.chromium.org/137723008/diff/200001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:466:
std::vector<ServerFieldType> RequestedTypesForSection(DialogSection section);
nit: const?

https://codereview.chromium.org/137723008/diff/200001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://codereview.chromium.org/137723008/diff/200001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/data_model_wrapper.h:7: 
#include <vector>

https://codereview.chromium.org/137723008/diff/200001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/137723008/diff/200001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:2249:
'browser/ui/autofill/data_model_wrapper_unittest.cc',
Do we have similar exclusions for OS=="ios"?

[Dan Beam, 2014-01-27 19:37:50]

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/browser/u...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/browser/u...
chrome/browser/ui/autofill/data_model_wrapper.cc:328:
g_browser_process->GetApplicationLocale(), type);
i prefer the old API.

why have 2 different methods (Get{Billing,Shipping}Info) when you can just ask
for type.server_type()?

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/chrome_te...
File chrome/chrome_tests_unit.gypi (right):

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/chrome_te...
chrome/chrome_tests_unit.gypi:2249:
'browser/ui/autofill/data_model_wrapper_unittest.cc',
On 2014/01/27 19:05:36, aruslan wrote:
> Do we have similar exclusions for OS=="ios"?

OS=="ios" currently works because enable_autofill_dialog!=1 on iOS.

recently we've been starting to add an

  ... or OS=="ios"

so we should probably do that here.

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
File components/autofill/content/browser/wallet/full_wallet.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
components/autofill/content/browser/wallet/full_wallet.cc:158: const
AutofillType& type) {
nit: reverse the order of |app_locale| and |type| as we do |type|, |app_locale|
everywhere else

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
components/autofill/content/browser/wallet/full_wallet.cc:161: return
base::UTF8ToUTF16(GetPan());
nit: base::ASCIIToUTF16() if you assume CVN/PAN can't be non-ASCII or maybe just
put a NOTREACHED()

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
components/autofill/content/browser/wallet/full_wallet.cc:167: return
base::UTF8ToUTF16(GetCvn());
same nit: re base::ASCIIToUTF16()

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
File components/autofill/content/browser/wallet/full_wallet.h (left):

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
components/autofill/content/browser/wallet/full_wallet.h:108: const std::string&
GetPan();
^ why is revealing this method preferable?

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
File components/autofill/core/browser/form_structure.h (right):

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
components/autofill/core/browser/form_structure.h:14: #include
"base/memory/scoped_vector.h"
nit: #include "base/strings/string16.h"

[Evan Stade, 2014-01-27 19:45:40]

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

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:609:
base::Bind(&NullGetInfo),
On 2014/01/24 22:23:15, aruslan wrote:
> nit: consistency.  it's Bind(NullGetInfo) in the Android code.

Done.

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/browser/u...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/browser/u...
chrome/browser/ui/autofill/data_model_wrapper.cc:328:
g_browser_process->GetApplicationLocale(), type);
On 2014/01/27 19:37:50, Dan Beam wrote:
> i prefer the old API.
> 
> why have 2 different methods (Get{Billing,Shipping}Info) when you can just ask
> for type.server_type()?

because sometimes we're using the billing address to fill in the shipping info.

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/browser/u...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/browser/u...
chrome/browser/ui/autofill/data_model_wrapper.h:7: 
On 2014/01/27 19:05:36, aruslan wrote:
> #include <vector>

Done.

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/chrome_te...
File chrome/chrome_tests_unit.gypi (right):

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/chrome_te...
chrome/chrome_tests_unit.gypi:2249:
'browser/ui/autofill/data_model_wrapper_unittest.cc',
On 2014/01/27 19:05:36, aruslan wrote:
> Do we have similar exclusions for OS=="ios"?

Rouslan recently added OS="ios" to the conditional above (recently enough that
you can't see it on this patch), but it's not really necessary because ios
removes almost all of browser/ui by default.

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
File components/autofill/content/browser/wallet/full_wallet.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
components/autofill/content/browser/wallet/full_wallet.cc:158: const
AutofillType& type) {
On 2014/01/27 19:37:50, Dan Beam wrote:
> nit: reverse the order of |app_locale| and |type| as we do |type|,
|app_locale|
> everywhere else

can't, this is needed for the bind() to work

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
components/autofill/content/browser/wallet/full_wallet.cc:161: return
base::UTF8ToUTF16(GetPan());
On 2014/01/27 19:37:50, Dan Beam wrote:
> nit: base::ASCIIToUTF16() if you assume CVN/PAN can't be non-ASCII or maybe
just
> put a NOTREACHED()

Done.

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
components/autofill/content/browser/wallet/full_wallet.cc:167: return
base::UTF8ToUTF16(GetCvn());
On 2014/01/27 19:37:50, Dan Beam wrote:
> same nit: re base::ASCIIToUTF16()

Done.

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
File components/autofill/content/browser/wallet/full_wallet.h (left):

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
components/autofill/content/browser/wallet/full_wallet.h:108: const std::string&
GetPan();
On 2014/01/27 19:37:50, Dan Beam wrote:
> ^ why is revealing this method preferable?

Why not expose it? It makes the code slightly shorter/easier to read.

[Evan Stade, 2014-01-27 19:51:11]

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/chrome_te...
File chrome/chrome_tests_unit.gypi (right):

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/chrome_te...
chrome/chrome_tests_unit.gypi:2249:
'browser/ui/autofill/data_model_wrapper_unittest.cc',
On 2014/01/27 19:45:41, Evan Stade wrote:
> On 2014/01/27 19:05:36, aruslan wrote:
> > Do we have similar exclusions for OS=="ios"?
> 
> Rouslan recently added OS="ios" to the conditional above (recently enough that
> you can't see it on this patch), but it's not really necessary because ios
> removes almost all of browser/ui by default.

or maybe he didn't get this one actually..

[Dan Beam, 2014-01-27 20:18:22]

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/browser/u...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/200001/chrome/browser/u...
chrome/browser/ui/autofill/data_model_wrapper.cc:328:
g_browser_process->GetApplicationLocale(), type);
On 2014/01/27 19:45:41, Evan Stade wrote:
> On 2014/01/27 19:37:50, Dan Beam wrote:
> > i prefer the old API.
> > 
> > why have 2 different methods (Get{Billing,Shipping}Info) when you can just
ask
> > for type.server_type()?
> 
> because sometimes we're using the billing address to fill in the shipping
info.

GetEquivalentBillingFieldType()/AutofillType().GetStorableType()

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
File components/autofill/content/browser/wallet/full_wallet.h (left):

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
components/autofill/content/browser/wallet/full_wallet.h:108: const std::string&
GetPan();
On 2014/01/27 19:45:41, Evan Stade wrote:
> On 2014/01/27 19:37:50, Dan Beam wrote:
> > ^ why is revealing this method preferable?
> 
> Why not expose it? It makes the code slightly shorter/easier to read.

2 ways of doing the same thing.

[Evan Stade, 2014-01-27 20:28:23]

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
File components/autofill/content/browser/wallet/full_wallet.h (left):

https://chromiumcodereview.appspot.com/137723008/diff/200001/components/autof...
components/autofill/content/browser/wallet/full_wallet.h:108: const std::string&
GetPan();
On 2014/01/27 20:18:23, Dan Beam wrote:
> On 2014/01/27 19:45:41, Evan Stade wrote:
> > On 2014/01/27 19:37:50, Dan Beam wrote:
> > > ^ why is revealing this method preferable?
> > 
> > Why not expose it? It makes the code slightly shorter/easier to read.
> 
> 2 ways of doing the same thing.

which each have their own purpose. One is easier to use when you know that you
want a pan. The other is easier to use when you have some generic type (which
may be a pan) that you need.

[Dan Beam, 2014-01-27 20:51:40]

so if you need a CVC do you ask for it via

  full_wallet_->GetInfo(AutofillType(CREDIT_CARD_VERIFICATION_CODE)),
  full_wallet_->GetBillingInfo(CREDIT_CARD_VERIFICATION_CODE), or
  full_wallet_->GetPan()?

if you need the billing city do you ask by doing:

  full_wallet_->GetInfo(AutofillType(ADDRESS_BILLING_CITY)),
  full_wallet_->GetBillingInfo(ADDRESS_BILLING_CITY), or
  full_wallet_->GetBillingInfo(ADDRESS_HOME_CITY)?

https://chromiumcodereview.appspot.com/137723008/diff/440001/chrome/browser/u...
File chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc
(right):

https://chromiumcodereview.appspot.com/137723008/diff/440001/chrome/browser/u...
chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:80: }
i don't currently see how GetBillingInfo() could be called with shipping fields
nor how GetShippingInfo() could be called with billing fields with current code
(so GetInfo() should still be viable)

https://chromiumcodereview.appspot.com/137723008/diff/440001/chrome/browser/u...
chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:88:
g_browser_process->GetApplicationLocale());
nit:

  form_structure.FillFields(types,
                            compare,
                            get_info,
                            g_browser_process->GetApplicationLocale());

https://chromiumcodereview.appspot.com/137723008/diff/440001/chrome/browser/u...
chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:362:
SECTION_SHIPPING, form_structure_, full_wallet.get(), email);
where is "Use billing for shipping" on Android implemented?

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

https://chromiumcodereview.appspot.com/137723008/diff/440001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:794:
base::string16 cc_number = base::ASCIIToUTF16(full_wallet_->GetPan());
if you need this as a string16 anyways i'd just leave GetPan() private

https://chromiumcodereview.appspot.com/137723008/diff/440001/components/autof...
File components/autofill/content/browser/wallet/full_wallet.h (left):

https://chromiumcodereview.appspot.com/137723008/diff/440001/components/autof...
components/autofill/content/browser/wallet/full_wallet.h:55: base::string16
GetInfo(const AutofillType& type);
i think |type| was sufficient to determine whether billing or shipping
information was necessary

https://chromiumcodereview.appspot.com/137723008/diff/440001/components/autof...
File components/autofill/content/browser/wallet/full_wallet_unittest.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/440001/components/autof...
components/autofill/content/browser/wallet/full_wallet_unittest.cc:527:
full_wallet->GetBillingInfo("",
^ i don't see the changes of this file as a win

[Evan Stade, 2014-01-27 21:00:53]

On 2014/01/27 20:51:40, Dan Beam wrote:
> so if you need a CVC do you ask for it via
> 
>   full_wallet_->GetInfo(AutofillType(CREDIT_CARD_VERIFICATION_CODE)),
>   full_wallet_->GetBillingInfo(CREDIT_CARD_VERIFICATION_CODE), or
>   full_wallet_->GetPan()?

well, obviously not via GetPan(), and GetInfo no longer exists...

> 
> if you need the billing city do you ask by doing:
> 
>   full_wallet_->GetInfo(AutofillType(ADDRESS_BILLING_CITY)),

doesn't exist

>   full_wallet_->GetBillingInfo(ADDRESS_BILLING_CITY), or
>   full_wallet_->GetBillingInfo(ADDRESS_HOME_CITY)?

I'd recommend full_wallet_->billing_address()->locality_name()

[Evan Stade, 2014-01-27 22:36:50]

brought back FullWallet::GetInfo

https://chromiumcodereview.appspot.com/137723008/diff/440001/components/autof...
File components/autofill/content/browser/wallet/full_wallet_unittest.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/440001/components/autof...
components/autofill/content/browser/wallet/full_wallet_unittest.cc:527:
full_wallet->GetBillingInfo("",
On 2014/01/27 20:51:40, Dan Beam wrote:
> ^ i don't see the changes of this file as a win

I don't write code with the intent of making unit tests beautiful

[Dan Beam, 2014-01-27 23:11:26]

lgtm w/nits

https://chromiumcodereview.appspot.com/137723008/diff/440001/chrome/browser/u...
File chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc
(right):

https://chromiumcodereview.appspot.com/137723008/diff/440001/chrome/browser/u...
chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:88:
g_browser_process->GetApplicationLocale());
On 2014/01/27 20:51:40, Dan Beam wrote:
> nit:
> 
>   form_structure.FillFields(types,
>                             compare,
>                             get_info,
>                             g_browser_process->GetApplicationLocale());

this

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

https://chromiumcodereview.appspot.com/137723008/diff/460001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:794:
base::string16 cc_number = base::ASCIIToUTF16(full_wallet_->GetPan());
nit: i still see

  full_wallet_->GetInfo(AutofillType(CREDIT_CARD_NUMBER));

as more self-documenting than

  full_wallet_->GetPan()

because PAN (Primary Account Number) is not used anywhere else in our code

https://chromiumcodereview.appspot.com/137723008/diff/460001/chrome/browser/u...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/460001/chrome/browser/u...
chrome/browser/ui/autofill/data_model_wrapper.cc:356: type,
g_browser_process->GetApplicationLocale());
why can't this be

  full_wallet_->GetInfo(g_browser_process->GetApplicationLocale(), type);

and maybe we can remove |shipping_address()| and/or |billing_address()|

https://chromiumcodereview.appspot.com/137723008/diff/460001/components/autof...
File components/autofill/content/browser/wallet/full_wallet_unittest.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/460001/components/autof...
components/autofill/content/browser/wallet/full_wallet_unittest.cc:459:
full_wallet.GetInfo("", AutofillType(CREDIT_CARD_NUMBER)));
nit: aren't we supposed to prefer std::string()?  i guess it depends on if small
string optimizations are on everywhere?

https://chromiumcodereview.appspot.com/137723008/diff/460001/components/autof...
components/autofill/content/browser/wallet/full_wallet_unittest.cc:462:
AutofillType(CREDIT_CARD_VERIFICATION_CODE)));
nit: 

  EXPECT_EQ(ASCIIToUTF16("989"),
            full_wallet.GetInfo(
                "", AutofillType(CREDIT_CARD_VERIFICATION_CODE)));

or

  EXPECT_EQ(ASCIIToUTF16("989"),
            full_wallet.GetInfo(
                "",
                AutofillType(CREDIT_CARD_VERIFICATION_CODE)));

[Evan Stade, 2014-01-28 00:07:58]

https://chromiumcodereview.appspot.com/137723008/diff/440001/chrome/browser/u...
File chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc
(right):

https://chromiumcodereview.appspot.com/137723008/diff/440001/chrome/browser/u...
chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:88:
g_browser_process->GetApplicationLocale());
On 2014/01/27 23:11:27, Dan Beam wrote:
> On 2014/01/27 20:51:40, Dan Beam wrote:
> > nit:
> > 
> >   form_structure.FillFields(types,
> >                             compare,
> >                             get_info,
> >                             g_browser_process->GetApplicationLocale());
> 
> this

Done.

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

https://chromiumcodereview.appspot.com/137723008/diff/460001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:794:
base::string16 cc_number = base::ASCIIToUTF16(full_wallet_->GetPan());
On 2014/01/27 23:11:27, Dan Beam wrote:
> nit: i still see
> 
>   full_wallet_->GetInfo(AutofillType(CREDIT_CARD_NUMBER));
> 
> as more self-documenting than
> 
>   full_wallet_->GetPan()
> 
> because PAN (Primary Account Number) is not used anywhere else in our code

are you asking me to change the name of GetPan() to GetCardNumber() or similar?

https://chromiumcodereview.appspot.com/137723008/diff/460001/chrome/browser/u...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/460001/chrome/browser/u...
chrome/browser/ui/autofill/data_model_wrapper.cc:356: type,
g_browser_process->GetApplicationLocale());
On 2014/01/27 23:11:27, Dan Beam wrote:
> why can't this be
> 
>   full_wallet_->GetInfo(g_browser_process->GetApplicationLocale(), type);
> 
> and maybe we can remove |shipping_address()| and/or |billing_address()|

I happen to like the code better as is. I am not changing it in this CL, so I
think it falls outside the scope of this review.

https://chromiumcodereview.appspot.com/137723008/diff/460001/components/autof...
File components/autofill/content/browser/wallet/full_wallet_unittest.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/460001/components/autof...
components/autofill/content/browser/wallet/full_wallet_unittest.cc:459:
full_wallet.GetInfo("", AutofillType(CREDIT_CARD_NUMBER)));
On 2014/01/27 23:11:27, Dan Beam wrote:
> nit: aren't we supposed to prefer std::string()?  i guess it depends on if
small
> string optimizations are on everywhere?

never heard of that, but in tests? officially do not care (except that "" is
shorter)

https://chromiumcodereview.appspot.com/137723008/diff/460001/components/autof...
components/autofill/content/browser/wallet/full_wallet_unittest.cc:462:
AutofillType(CREDIT_CARD_VERIFICATION_CODE)));
On 2014/01/27 23:11:27, Dan Beam wrote:
> nit: 
> 
>   EXPECT_EQ(ASCIIToUTF16("989"),
>             full_wallet.GetInfo(
>                 "", AutofillType(CREDIT_CARD_VERIFICATION_CODE)));
> 
> or
> 
>   EXPECT_EQ(ASCIIToUTF16("989"),
>             full_wallet.GetInfo(
>                 "",
>                 AutofillType(CREDIT_CARD_VERIFICATION_CODE)));

Done.

[Dan Beam, 2014-01-28 00:18:45]

slgtm

https://chromiumcodereview.appspot.com/137723008/diff/460001/chrome/browser/u...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/460001/chrome/browser/u...
chrome/browser/ui/autofill/data_model_wrapper.cc:356: type,
g_browser_process->GetApplicationLocale());
On 2014/01/28 00:07:59, Evan Stade wrote:
> On 2014/01/27 23:11:27, Dan Beam wrote:
> > why can't this be
> > 
> >   full_wallet_->GetInfo(g_browser_process->GetApplicationLocale(), type);
> > 
> > and maybe we can remove |shipping_address()| and/or |billing_address()|
> 
> I happen to like the code better as is. I am not changing it in this CL, so I
> think it falls outside the scope of this review.

that's fine

https://chromiumcodereview.appspot.com/137723008/diff/460001/components/autof...
File components/autofill/content/browser/wallet/full_wallet_unittest.cc (right):

https://chromiumcodereview.appspot.com/137723008/diff/460001/components/autof...
components/autofill/content/browser/wallet/full_wallet_unittest.cc:459:
full_wallet.GetInfo("", AutofillType(CREDIT_CARD_NUMBER)));
On 2014/01/28 00:07:59, Evan Stade wrote:
> On 2014/01/27 23:11:27, Dan Beam wrote:
> > nit: aren't we supposed to prefer std::string()?  i guess it depends on if
> small
> > string optimizations are on everywhere?
> 
> never heard of that, but in tests? officially do not care (except that "" is
> shorter)

I also officially don't care, just something to consider:
https://plus.google.com/106498078576352695438/posts/QPjyX4f9MZp
https://codereview.chromium.org/13145003

[commit-bot: I haz the power, 2014-01-28 19:34:52]

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

[commit-bot: I haz the power, 2014-01-28 21:23:24]

Retried try job too often on mac_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-29 01:09:30]

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

[commit-bot: I haz the power, 2014-01-29 02:21:52]

Change committed as 247593