requestAutocomplete: Fill |form_structure_| from Online Wallet data (including

requestAutocomplete: Fill |form_structure_| from Online Wallet data (including
fronting card).

R=estade@chromium.org
BUG=163504
TEST=none, yet


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

[Dan Beam, 2013-03-13 01:41:07]

+estade@ for pre-review

I've left the wrapper I was going to create - I have a feeling you'll want me to
force some code into a helper or another class.  Let me know where you want this
code (FillOutputForSectionWithComparator() when IsPayingWithWallet()) to live. 
I'm mainly publishing a little bit early for ahutter@ as he needs this patch for
tomorrow.

[Dan Beam, 2013-03-13 01:43:57]

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/da...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/da...
chrome/browser/ui/autofill/data_model_wrapper.cc:40: FormStructure*
form_structure) {
or this would need to take a section so FullWalletWrapper would be able to
differentiate billing from shipping as AutofillFieldType alone isn't enough (as
we're not using the _BILLING_ variety, as we talked about today)

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/da...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/da...
chrome/browser/ui/autofill/data_model_wrapper.h:155: virtual string16
GetInfo(AutofillFieldType type) OVERRIDE;
this interface would need to differ

https://codereview.chromium.org/12815002/diff/1/components/autofill/browser/w...
File components/autofill/browser/wallet/wallet_client.cc (right):

https://codereview.chromium.org/12815002/diff/1/components/autofill/browser/w...
components/autofill/browser/wallet/wallet_client.cc:242: //   
DialogTypeToFeatureString(full_wallet_request.dialog_type));
sorry, ignore this

[Evan Stade, 2013-03-13 01:54:09]

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:168: if (cvc &&
it->first->type == CREDIT_CARD_VERIFICATION_CODE) {
shouldn't you put the if (cvc) check inside this block

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:179: } else {
merge else { if into else if

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/da...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/da...
chrome/browser/ui/autofill/data_model_wrapper.h:155: virtual string16
GetInfo(AutofillFieldType type) OVERRIDE;
On 2013/03/13 01:43:57, Dan Beam wrote:
> this interface would need to differ

2 different wrappers

[Dan Beam, 2013-03-13 03:05:46]

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:168: if (cvc &&
it->first->type == CREDIT_CARD_VERIFICATION_CODE) {
On 2013/03/13 01:54:10, Evan Stade wrote:
> shouldn't you put the if (cvc) check inside this block

Done.

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:179: } else {
On 2013/03/13 01:54:10, Evan Stade wrote:
> merge else { if into else if

Done.

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/da...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://codereview.chromium.org/12815002/diff/1/chrome/browser/ui/autofill/da...
chrome/browser/ui/autofill/data_model_wrapper.h:155: virtual string16
GetInfo(AutofillFieldType type) OVERRIDE;
On 2013/03/13 01:54:10, Evan Stade wrote:
> On 2013/03/13 01:43:57, Dan Beam wrote:
> > this interface would need to differ
> 
> 2 different wrappers

Done.

[Dan Beam, 2013-03-13 18:11:04]

+ahutter@

[Dan Beam, 2013-03-14 02:19:01]

ping

[Evan Stade, 2013-03-14 02:39:38]

https://codereview.chromium.org/12815002/diff/18001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/12815002/diff/18001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:289: bool
SaveDetailsLocally();
perhaps both this one and UseBililngForShipping should be "Should[...]"

https://codereview.chromium.org/12815002/diff/18001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://codereview.chromium.org/12815002/diff/18001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/data_model_wrapper.cc:193: string16
FullWalletWrapper::GetInfo(AutofillFieldType type) {
FullWalletWrapper needn't exist. Implement GetInfo separately in
FullWalletBillingWrapper and FullWalletShippingWrapper.

https://codereview.chromium.org/12815002/diff/18001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/data_model_wrapper.cc:198: // TODO(dbeam): this could
be implemented if necessary, I just don't see where
comment too verbose. Remove.

https://codereview.chromium.org/12815002/diff/18001/components/autofill/brows...
File components/autofill/browser/wallet/full_wallet.h (right):

https://codereview.chromium.org/12815002/diff/18001/components/autofill/brows...
components/autofill/browser/wallet/full_wallet.h:50: string16
GetInfo(AutofillFieldType type, bool is_billing);
leave this file alone, do it all in data model wrapper.

[Dan Beam, 2013-03-14 04:31:51]

https://codereview.chromium.org/12815002/diff/18001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/12815002/diff/18001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:289: bool
SaveDetailsLocally();
On 2013/03/14 02:39:38, Evan Stade wrote:
> perhaps both this one and UseBililngForShipping should be "Should[...]"

Done.

https://codereview.chromium.org/12815002/diff/18001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://codereview.chromium.org/12815002/diff/18001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/data_model_wrapper.cc:193: string16
FullWalletWrapper::GetInfo(AutofillFieldType type) {
On 2013/03/14 02:39:38, Evan Stade wrote:
> FullWalletWrapper needn't exist. Implement GetInfo separately in
> FullWalletBillingWrapper and FullWalletShippingWrapper.

Done.

https://codereview.chromium.org/12815002/diff/18001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/data_model_wrapper.cc:198: // TODO(dbeam): this could
be implemented if necessary, I just don't see where
On 2013/03/14 02:39:38, Evan Stade wrote:
> comment too verbose. Remove.

Done.

https://codereview.chromium.org/12815002/diff/18001/components/autofill/brows...
File components/autofill/browser/wallet/full_wallet.h (right):

https://codereview.chromium.org/12815002/diff/18001/components/autofill/brows...
components/autofill/browser/wallet/full_wallet.h:50: string16
GetInfo(AutofillFieldType type, bool is_billing);
On 2013/03/14 02:39:38, Evan Stade wrote:
> leave this file alone, do it all in data model wrapper.

we talked about this -- the stuff that has to do with specifically credit card
stuff can go here

[Evan Stade, 2013-03-14 18:15:46]

lgtm

https://codereview.chromium.org/12815002/diff/29001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://codereview.chromium.org/12815002/diff/29001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/data_model_wrapper.cc:175: wallet::FullWallet*
full_wallet) : full_wallet_(full_wallet) {
initializer list on new line

https://codereview.chromium.org/12815002/diff/29001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/data_model_wrapper.cc:191: wallet::FullWallet*
full_wallet) : full_wallet_(full_wallet) {
initializer list on new line

https://codereview.chromium.org/12815002/diff/29001/components/autofill/brows...
File components/autofill/browser/wallet/full_wallet.h (right):

https://codereview.chromium.org/12815002/diff/29001/components/autofill/brows...
components/autofill/browser/wallet/full_wallet.h:42: const std::string&
GetPan();
since you're adding GetInfo, I don't think GetPan and GetCvn need to be public
any more.

[Dan Beam, 2013-03-14 18:36:43]

https://codereview.chromium.org/12815002/diff/29001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/data_model_wrapper.cc (right):

https://codereview.chromium.org/12815002/diff/29001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/data_model_wrapper.cc:175: wallet::FullWallet*
full_wallet) : full_wallet_(full_wallet) {
On 2013/03/14 18:15:47, Evan Stade wrote:
> initializer list on new line

Done.

https://codereview.chromium.org/12815002/diff/29001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/data_model_wrapper.cc:191: wallet::FullWallet*
full_wallet) : full_wallet_(full_wallet) {
On 2013/03/14 18:15:47, Evan Stade wrote:
> initializer list on new line

Done.

https://codereview.chromium.org/12815002/diff/29001/components/autofill/brows...
File components/autofill/browser/wallet/full_wallet.h (right):

https://codereview.chromium.org/12815002/diff/29001/components/autofill/brows...
components/autofill/browser/wallet/full_wallet.h:42: const std::string&
GetPan();
On 2013/03/14 18:15:47, Evan Stade wrote:
> since you're adding GetInfo, I don't think GetPan and GetCvn need to be public
> any more.

Done.

[Dan Beam, 2013-03-14 18:40:41]

+isherman@ for components/autofill/browser/wallet/OWNERS

[Dan Beam, 2013-03-14 19:36:43]

isherman@: hold off on this, I'm hitting DCHECK()s, will ping again when I fix

[Dan Beam, 2013-03-14 20:20:33]

OK, I've fixed the issues, feel free to nit pick away again! :P

[Ilya Sherman, 2013-03-14 21:59:08]

LGTM with nits

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:109: bool
CreditCardType(AutofillFieldType type) {
nit: IsCreditCardType

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:178:
DCHECK(profile);
These don't seem very helpful, since you dereference the pointers within this
method.

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:187:
cvc->assign(trimmed);
Just curious, why did you change this?

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:633: }
nit: Some docs for the code flow logic here would be helpful.  It's not
completely obvious why "!did_submit_ || !full_wallet_.get()" is an interesting
case.

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1363: if (section
== SECTION_CC)
Should this apply to SECTION_CC_BILLING as well?

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1395: } else {
Can you DCHECK that the section is the one you expect here?

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1540: return
!IsPayingWithWallet() && view_->SaveDetailsLocally();
nit: Please include a comment explaining why we need more than just the second
check.  This code looks a little surprising as it is.

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:238: // a field.
NOTE: |compare| is ignored when |section| is SECTION_CC_BILLING.
Please document that this method has the side-effect of saving Autofill data to
the PDM.

https://codereview.chromium.org/12815002/diff/42001/components/autofill/brows...
File components/autofill/browser/wallet/full_wallet.cc (right):

https://codereview.chromium.org/12815002/diff/42001/components/autofill/brows...
components/autofill/browser/wallet/full_wallet.cc:142: case
CREDIT_CARD_EXP_DATE_4_DIGIT_YEAR:
As I mentioned in the other CL, this isn't how EXP_DATE_4_DIGIT_YEAR works --
that should be a month+year.

[Dan Beam, 2013-03-14 23:49:34]

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:109: bool
CreditCardType(AutofillFieldType type) {
On 2013/03/14 21:59:08, Ilya Sherman wrote:
> nit: IsCreditCardType

Done.

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:178:
DCHECK(profile);
On 2013/03/14 21:59:08, Ilya Sherman wrote:
> These don't seem very helpful, since you dereference the pointers within this
> method.

Done.

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:187:
cvc->assign(trimmed);
On 2013/03/14 21:59:08, Ilya Sherman wrote:
> Just curious, why did you change this?

seemed like this is the intended API, akin to saying .size() == 0 vs. .empty()

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:633: }
On 2013/03/14 21:59:08, Ilya Sherman wrote:
> nit: Some docs for the code flow logic here would be helpful.  It's not
> completely obvious why "!did_submit_ || !full_wallet_.get()" is an interesting
> case.

Done.

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1363: if (section
== SECTION_CC)
On 2013/03/14 21:59:08, Ilya Sherman wrote:
> Should this apply to SECTION_CC_BILLING as well?

No, we need to get the CVC from |full_wallet_|; added comment.

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1395: } else {
On 2013/03/14 21:59:08, Ilya Sherman wrote:
> Can you DCHECK that the section is the one you expect here?

This hunk of the diff was reverted and now it'd be SECTION_EMAIL,
SECTION_BILLING, or SECTION_SHIPPING. Is DCHECK()'ing that useful?

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1540: return
!IsPayingWithWallet() && view_->SaveDetailsLocally();
On 2013/03/14 21:59:08, Ilya Sherman wrote:
> nit: Please include a comment explaining why we need more than just the second
> check.  This code looks a little surprising as it is.

Done.

[Dan Beam, 2013-03-14 23:50:36]

so I updated this to use |full_wallet_| in the case that a user has edited or
added an instrument and saved it to wallet -- we need to use only what we get
back as card number and CVC are auto-generated (so we can't get this info from
the view).

[Dan Beam, 2013-03-15 02:10:58]

https://codereview.chromium.org/12815002/diff/42001/components/autofill/brows...
File components/autofill/browser/wallet/full_wallet.cc (right):

https://codereview.chromium.org/12815002/diff/42001/components/autofill/brows...
components/autofill/browser/wallet/full_wallet.cc:142: case
CREDIT_CARD_EXP_DATE_4_DIGIT_YEAR:
On 2013/03/14 21:59:08, Ilya Sherman wrote:
> As I mentioned in the other CL, this isn't how EXP_DATE_4_DIGIT_YEAR works --
> that should be a month+year.

Delorted.

[Dan Beam, 2013-03-15 02:15:01]

sorry I missed the last 2

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/12815002/diff/42001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:238: // a field.
NOTE: |compare| is ignored when |section| is SECTION_CC_BILLING.
On 2013/03/14 21:59:08, Ilya Sherman wrote:
> Please document that this method has the side-effect of saving Autofill data
to
> the PDM.

Done.

[Evan Stade, 2013-03-15 03:04:24]

slgtm

[commit-bot: I haz the power, 2013-03-15 03:07:06]

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

[commit-bot: I haz the power, 2013-03-15 09:13:29]

Change committed as 188312