[Web Payments] Implement the CVC Unmask dialog.

[Web Payments] Implement the CVC Unmask dialog.

BUG=706511
Review-Url: https://codereview.chromium.org/2779283002
Cr-Commit-Position: refs/heads/master@{#462077}
Committed: https://chromium.googlesource.com/chromium/src/+/d23ed70ac6dd7a254422e929c8175e3db61b0bb5

[anthonyvd, 2017-03-29 19:12:53]

anthonyvd@chromium.org changed reviewers:
+ mathp@chromium.org, rouslan@chromium.org, sebsg@chromium.org

[anthonyvd, 2017-03-29 19:12:54]

Hi guys,

Since this is my first foray this deep into Autofill stuff, can you please take
an initial look at this CL while I write tests for it?

Mainly, this CL contains a lot of glue to pass things around with the ultimate
goal of making a FullCardRequest from the PaymentRequest dialog. I *think* the
way I'm doing it makes sense but it's a lot of code so maybe you have opinions.

Thanks!

[sebsg, 2017-03-29 20:53:31]

Way to go! A small comment on credit cards.

https://codereview.chromium.org/2779283002/diff/1/chrome/browser/payments/chr...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2779283002/diff/1/chrome/browser/payments/chr...
chrome/browser/payments/chrome_payment_request_delegate.h:8: #include <string>
needed?

https://codereview.chromium.org/2779283002/diff/1/components/payments/core/au...
File components/payments/core/autofill_payment_instrument.cc (right):

https://codereview.chromium.org/2779283002/diff/1/components/payments/core/au...
components/payments/core/autofill_payment_instrument.cc:58: credit_card_, cvc,
billing_profiles_, app_locale_)
You should use the data from card that you got as param from the
FullCardRequest. Reason being that you'll get the unmasked version (so all the
numbers), and the new expiration date that the user entered if the card was
expired.

See example of how it's done Java:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

[anthonyvd, 2017-03-29 20:59:10]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[anthonyvd, 2017-03-29 20:59:37]

https://codereview.chromium.org/2779283002/diff/1/chrome/browser/payments/chr...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2779283002/diff/1/chrome/browser/payments/chr...
chrome/browser/payments/chrome_payment_request_delegate.h:8: #include <string>
On 2017/03/29 at 20:53:31, sebsg wrote:
> needed?

Yeah, "include what you need" for GetApplicationLocale(). It's not related to
this patch, just a lint error I fixed in passing :)

https://codereview.chromium.org/2779283002/diff/1/components/payments/core/au...
File components/payments/core/autofill_payment_instrument.cc (right):

https://codereview.chromium.org/2779283002/diff/1/components/payments/core/au...
components/payments/core/autofill_payment_instrument.cc:58: credit_card_, cvc,
billing_profiles_, app_locale_)
On 2017/03/29 at 20:53:31, sebsg wrote:
> You should use the data from card that you got as param from the
FullCardRequest. Reason being that you'll get the unmasked version (so all the
numbers), and the new expiration date that the user entered if the card was
expired.
> 
> See example of how it's done Java:
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

Ah great insight, thanks! Done.

[commit-bot: I haz the power, 2017-03-29 21:00:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 21:03:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 21:03:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sebsg, 2017-03-29 21:12:02]

https://codereview.chromium.org/2779283002/diff/1/chrome/browser/payments/chr...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2779283002/diff/1/chrome/browser/payments/chr...
chrome/browser/payments/chrome_payment_request_delegate.h:8: #include <string>
On 2017/03/29 20:59:37, anthonyvd wrote:
> On 2017/03/29 at 20:53:31, sebsg wrote:
> > needed?
> 
> Yeah, "include what you need" for GetApplicationLocale(). It's not related to
> this patch, just a lint error I fixed in passing :)

(-_-') I actually looked for a string in the file and totally missed it.... time
to go home

https://codereview.chromium.org/2779283002/diff/1/components/payments/core/au...
File components/payments/core/autofill_payment_instrument.cc (right):

https://codereview.chromium.org/2779283002/diff/1/components/payments/core/au...
components/payments/core/autofill_payment_instrument.cc:58: credit_card_, cvc,
billing_profiles_, app_locale_)
On 2017/03/29 20:59:37, anthonyvd wrote:
> On 2017/03/29 at 20:53:31, sebsg wrote:
> > You should use the data from card that you got as param from the
> FullCardRequest. Reason being that you'll get the unmasked version (so all the
> numbers), and the new expiration date that the user entered if the card was
> expired.
> > 
> > See example of how it's done Java:
> >
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
> 
> Ah great insight, thanks! Done.

It think it would be better to replace your credit_card_ object with the one you
receive as a param, so the metrics and record logic that we'll add later will
benefit from the most up to date information.

Thanks!

[anthonyvd, 2017-03-29 22:30:03]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 22:30:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 22:44:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 22:44:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[please use gerrit instead, 2017-03-30 13:14:10]

Overall design looks good.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.cc:7: #include
"base/macros.h"
Already included in the header, so not needed in here.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:8: #include <string>
Only used when overriding PaymentRequestDelegate, so no need to include it in
this case.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:19: class
PaymentRequest;
Only used when overriding PaymentRequestDelegate, so no need to forward-declare
it in this case.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:26: 
Add a comment that this overrides PaymentRequestDelegate.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller.h (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:23: public:
Need an empty virtual destructor.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:24: virtual void
OnCvcConfirmed(const base::string16& cvc) = 0;
The CVC unmask dialog can also show an expiration date picker, in case if the
card is out-of-date.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:28:
CvcUnmaskViewController(PaymentRequestSpec* spec,
Forward declare, please.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:29:
PaymentRequestState* state,
Forward declare, please.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:30:
PaymentRequestDialogView* dialog,
Forward declare, please.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:35: void
AddObserver(Observer* observer);
Note that the observer must outlive this object.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:48:
views::Textfield* cvc_field_;
Note who owns this.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:49: base::string16
masked_card_number_;
#include "base/strings/string16.h"

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog_view.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.cc:41: explicit
CvcUnmaskUIDelegate(PaymentRequestDialogView* dialog,
Explicit keyword is used only for constructors with a single parameter.

Add a comment that the parameters must outlive this object.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:13: #include
"components/autofill/core/browser/payments/full_card_request.h"
No need include this, because it's used only to implement the
PaymentRequestDialog's interface.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:20: class
CvcUnmaskViewController;
Can these two be made private? Then in the .cc file you would write something
like:

class PaymentRequestDilaogView::CardUnmaskUIDelegate {
 public:
  CardUnmaskUIDelegate() {}
  ~CardUnmaskUIDelegate() {}
  /// etc ...
};

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:74: //
payments::PaymentRequestDialog
No need to specify the "payments" namespace in this comment, because it's inside
of the "namespace payments {}" already.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/con...
File components/payments/content/payment_request_delegate.h (right):

https://codereview.chromium.org/2779283002/diff/60001/components/payments/con...
components/payments/content/payment_request_delegate.h:13: #include
"components/payments/core/autofill_payment_instrument.h"
Why are weak_ptr, autofill_client, and autofill_payment_instrument included in
here? I do not see them being used below.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/con...
File components/payments/content/payment_request_dialog.h (right):

https://codereview.chromium.org/2779283002/diff/60001/components/payments/con...
components/payments/content/payment_request_dialog.h:23: virtual
base::WeakPtr<autofill::payments::FullCardRequest::UIDelegate>
Need to include the weakptr header.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/con...
File components/payments/content/payment_request_state.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/components/payments/con...
components/payments/content/payment_request_state.cc:14: #include
"components/payments/content/payment_request_delegate.h"
I don't think this is necessary, because you don't have the
PaymentRequestDelegate data type anywhere in the file.
GetPaymentRequestDelegate() does not count, I believe.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
File components/payments/core/autofill_payment_instrument.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
components/payments/core/autofill_payment_instrument.cc:40: DCHECK(delegate);
DCHECK(!delegate_);

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
components/payments/core/autofill_payment_instrument.cc:62:
delegate_->OnInstrumentDetailsReady(method_name(), stringified_details);
delegate_ = nullptr;

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
File components/payments/core/autofill_payment_instrument.h (right):

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
components/payments/core/autofill_payment_instrument.h:30: public:
Need an empty virtual destructor.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
components/payments/core/autofill_payment_instrument.h:43:
FullCardRequestDelegate* full_card_request_delegate);
Add a comment about ownership of full_Card_request_delegate_ and how long it
must live.

[Mathieu, 2017-03-30 14:37:32]

https://codereview.chromium.org/2779283002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc (right):

https://codereview.chromium.org/2779283002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:113:
RaiseOnCvcConfirmed();
Raise is not immediately clear, change name?

NotifyOnCvcConfirmed?

https://codereview.chromium.org/2779283002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller.h (right):

https://codereview.chromium.org/2779283002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:31: const
base::string16& masked_card_number,
CreditCard* to replace this arg and the next?

https://codereview.chromium.org/2779283002/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.cc
(right):

https://codereview.chromium.org/2779283002/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:103:
views::View* pane = new views::View;
can this be a separate change? this is to limit the width of the dialog, yea?

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.cc:61:
autofill::ContentAutofillDriverFactory* factory =
I don't like this dependence on the AutofillManager. 

Let's do a couple of things so that we don't need it:

(1) full_card_request, card_unmask_delegate and payments_client 
 should really be in components/payments/core (optional)
   (1b) You will need to remove the dependence to AutofillClient* by instead
having AutofillClient implement risk_util.h (move to components) and pass that
to FullCardRequest. May need to have risk_util_impl.h/cc in
chrome/browser/autofill

(2) In this file, dialog_->ShowCvcUnmaskPrompt(credit_card, result_delegate)

(3) Ultimately the Cvc view controller should get all the things and be able to
create and hold on to the FullCardRequest!
  (3b) When it needs to pass the RiskUtil* to FullCardRequest, it can get it
from RiskUtilImpl (in chrome/browser/autofill).

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog_view.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.cc:37: class
CvcUnmaskUIDelegate
Can CvcUnmaskViewController be the
autofill::payments::FullCardRequest::UIDelegate ? This class doesn't do much but
forward the calls to the controller.

[anthonyvd, 2017-03-30 15:42:15]

Patchset #5 (id:80001) has been deleted

[anthonyvd, 2017-03-30 15:43:41]

Thanks everyone for the input. The latest patch addresses most comments.

I talked to Math offline about his suggestions and I think I'll implement the
changes in a separate CL. I'll ping you guys when I've updated this one again.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.cc:7: #include
"base/macros.h"
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Already included in the header, so not needed in here.

Done.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
File chrome/browser/payments/chrome_payment_request_delegate.h (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:8: #include <string>
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Only used when overriding PaymentRequestDelegate, so no need to include it in
this case.

cpplint disagrees :(

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:19: class
PaymentRequest;
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Only used when overriding PaymentRequestDelegate, so no need to
forward-declare it in this case.

Done.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/payments...
chrome/browser/payments/chrome_payment_request_delegate.h:26: 
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Add a comment that this overrides PaymentRequestDelegate.

Done.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller.h (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:23: public:
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Need an empty virtual destructor.

Good call.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:24: virtual void
OnCvcConfirmed(const base::string16& cvc) = 0;
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> The CVC unmask dialog can also show an expiration date picker, in case if the
card is out-of-date.

Yes, this implementation doesn't yet however.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:28:
CvcUnmaskViewController(PaymentRequestSpec* spec,
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Forward declare, please.

Done.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:29:
PaymentRequestState* state,
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Forward declare, please.

Done.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:30:
PaymentRequestDialogView* dialog,
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Forward declare, please.

Done.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:35: void
AddObserver(Observer* observer);
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Note that the observer must outlive this object.

Done.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:48:
views::Textfield* cvc_field_;
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Note who owns this.

Done.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:49: base::string16
masked_card_number_;
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> #include "base/strings/string16.h"

Done.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog_view.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.cc:41: explicit
CvcUnmaskUIDelegate(PaymentRequestDialogView* dialog,
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Explicit keyword is used only for constructors with a single parameter.
> 
> Add a comment that the parameters must outlive this object.

Ah, there was a time when this only took one parameter :) Comment done.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:13: #include
"components/autofill/core/browser/payments/full_card_request.h"
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> No need include this, because it's used only to implement the
PaymentRequestDialog's interface.

Done.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:20: class
CvcUnmaskViewController;
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Can these two be made private? Then in the .cc file you would write something
like:
> 
> class PaymentRequestDilaogView::CardUnmaskUIDelegate {
>  public:
>   CardUnmaskUIDelegate() {}
>   ~CardUnmaskUIDelegate() {}
>   /// etc ...
> };

I'm not sure what you mean. I'm just forward declaring them because they're used
as pointers in function parameters below.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:74: //
payments::PaymentRequestDialog
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> No need to specify the "payments" namespace in this comment, because it's
inside of the "namespace payments {}" already.

Done.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/con...
File components/payments/content/payment_request_delegate.h (right):

https://codereview.chromium.org/2779283002/diff/60001/components/payments/con...
components/payments/content/payment_request_delegate.h:13: #include
"components/payments/core/autofill_payment_instrument.h"
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Why are weak_ptr, autofill_client, and autofill_payment_instrument included in
here? I do not see them being used below.

Good catch, this code changed a lot while writing it and I forgot to remove
those after they became unused.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/con...
File components/payments/content/payment_request_dialog.h (right):

https://codereview.chromium.org/2779283002/diff/60001/components/payments/con...
components/payments/content/payment_request_dialog.h:23: virtual
base::WeakPtr<autofill::payments::FullCardRequest::UIDelegate>
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> Need to include the weakptr header.

Done.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/con...
File components/payments/content/payment_request_state.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/components/payments/con...
components/payments/content/payment_request_state.cc:14: #include
"components/payments/content/payment_request_delegate.h"
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> I don't think this is necessary, because you don't have the
PaymentRequestDelegate data type anywhere in the file.
GetPaymentRequestDelegate() does not count, I believe.

Yeah it's required for the compiler to know that PaymentRequestDelegate inherits
from FullCardRequestDelegate (which is the type that the
AutofillPaymentInstrument ctor accepts).

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
File components/payments/core/autofill_payment_instrument.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
components/payments/core/autofill_payment_instrument.cc:40: DCHECK(delegate);
On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> DCHECK(!delegate_);

I don't think that's correct. For example, we could call InvokePaymentApp to
show the CVC prompt for a card, the user could hit back, then "Pay" again, and
we'd end up re-entering this function. In that case, we want to overwrite
|delegate_|.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
components/payments/core/autofill_payment_instrument.cc:62:
delegate_->OnInstrumentDetailsReady(method_name(), stringified_details);
On 2017/03/30 at 13:14:10, ಠ_ಠ wrote:
> delegate_ = nullptr;

Done.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
File components/payments/core/autofill_payment_instrument.h (right):

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
components/payments/core/autofill_payment_instrument.h:30: public:
On 2017/03/30 at 13:14:10, ಠ_ಠ wrote:
> Need an empty virtual destructor.

Done.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
components/payments/core/autofill_payment_instrument.h:43:
FullCardRequestDelegate* full_card_request_delegate);
On 2017/03/30 at 13:14:10, ಠ_ಠ wrote:
> Add a comment about ownership of full_Card_request_delegate_ and how long it
must live.

Done.

[please use gerrit instead, 2017-03-31 14:41:14]

LGTM % comments

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:20: class
CvcUnmaskViewController;
On 2017/03/30 15:43:41, anthonyvd wrote:
> On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> > Can these two be made private? Then in the .cc file you would write
something
> like:
> > 
> > class PaymentRequestDilaogView::CardUnmaskUIDelegate {
> >  public:
> >   CardUnmaskUIDelegate() {}
> >   ~CardUnmaskUIDelegate() {}
> >   /// etc ...
> > };
> 
> I'm not sure what you mean. I'm just forward declaring them because they're
used
> as pointers in function parameters below.

CvcUnmaskUIDelegate is used only as a private variable and not used by anyone
else. Therefore, you can move this forward-declaration inside of the private:
section of PaymentRequestDialogView.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
File components/payments/core/autofill_payment_instrument.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
components/payments/core/autofill_payment_instrument.cc:40: DCHECK(delegate);
On 2017/03/30 15:43:41, anthonyvd wrote:
> On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> > DCHECK(!delegate_);
> 
> I don't think that's correct. For example, we could call InvokePaymentApp to
> show the CVC prompt for a card, the user could hit back, then "Pay" again, and
> we'd end up re-entering this function. In that case, we want to overwrite
> |delegate_|. 

If the user hits back, then "delegate_ = null" code in OnFullCardRequestFailed()
will be triggered. The DHCECK(!delegate_) is needed to prevent a bug where you
call InvokePaymentApp() twice by accident without OnFullCardRequestFailed()
being called.

[anthonyvd, 2017-04-03 18:14:06]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 18:14:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 18:57:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 18:57:07]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Mathieu, 2017-04-03 19:20:53]

Hey, is this ready for a review?

https://codereview.chromium.org/2779283002/diff/120001/components/payments/co...
File components/payments/core/autofill_payment_instrument.cc (right):

https://codereview.chromium.org/2779283002/diff/120001/components/payments/co...
components/payments/core/autofill_payment_instrument.cc:49: return
credit_card_.IsValid();
Let's change this to !credit_card_.IsExpired(AutofillClock::Now()), what do you
think?

[anthonyvd, 2017-04-03 21:06:05]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 21:06:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 21:11:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 21:11:22]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[anthonyvd, 2017-04-04 13:24:33]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 13:24:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[anthonyvd, 2017-04-04 13:25:22]

Hi everyone! This should be ready for a final review now. Thanks :)

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog_view.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.cc:37: class
CvcUnmaskUIDelegate
On 2017/03/30 at 14:37:32, Mathieu wrote:
> Can CvcUnmaskViewController be the
autofill::payments::FullCardRequest::UIDelegate ? This class doesn't do much but
forward the calls to the controller.

The UIDelegate is now the CvcUnmaskViewController.

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_dialog_view.h (right):

https://codereview.chromium.org/2779283002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_dialog_view.h:20: class
CvcUnmaskViewController;
On 2017/03/31 at 14:41:14, ಠ_ಠ wrote:
> On 2017/03/30 15:43:41, anthonyvd wrote:
> > On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> > > Can these two be made private? Then in the .cc file you would write
something
> > like:
> > > 
> > > class PaymentRequestDilaogView::CardUnmaskUIDelegate {
> > >  public:
> > >   CardUnmaskUIDelegate() {}
> > >   ~CardUnmaskUIDelegate() {}
> > >   /// etc ...
> > > };
> > 
> > I'm not sure what you mean. I'm just forward declaring them because they're
used
> > as pointers in function parameters below.
> 
> CvcUnmaskUIDelegate is used only as a private variable and not used by anyone
else. Therefore, you can move this forward-declaration inside of the private:
section of PaymentRequestDialogView.

Thanks for the details, this comment is now obsolete.

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
File components/payments/core/autofill_payment_instrument.cc (right):

https://codereview.chromium.org/2779283002/diff/60001/components/payments/cor...
components/payments/core/autofill_payment_instrument.cc:40: DCHECK(delegate);
On 2017/03/31 at 14:41:14, ಠ_ಠ wrote:
> On 2017/03/30 15:43:41, anthonyvd wrote:
> > On 2017/03/30 at 13:14:09, ಠ_ಠ wrote:
> > > DCHECK(!delegate_);
> > 
> > I don't think that's correct. For example, we could call InvokePaymentApp to
> > show the CVC prompt for a card, the user could hit back, then "Pay" again,
and
> > we'd end up re-entering this function. In that case, we want to overwrite
> > |delegate_|. 
> 
> If the user hits back, then "delegate_ = null" code in
OnFullCardRequestFailed() will be triggered. The DHCECK(!delegate_) is needed to
prevent a bug where you call InvokePaymentApp() twice by accident without
OnFullCardRequestFailed() being called.

Makes sense, done.

[commit-bot: I haz the power, 2017-04-04 13:57:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 13:57:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[anthonyvd, 2017-04-04 18:41:12]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 18:42:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2017-04-04 19:05:48]

Thanks! This is looking very sharp :) Hoping we can make it yet more simple.

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc (right):

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:52:
full_card_request_(base::MakeUnique<autofill::payments::FullCardRequest>(
does this need to be a unique_ptr or can this be a regular member?

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:70:
Profile::FromBrowserContext(web_contents_->GetBrowserContext())
I'm curious what's different when doing GetOriginalProfile vs just 
Profile::FromBrowserContext(web_contents_->GetBrowserContext()) ?

Should we also change in ChromePaymentRequestDelegate::GetPersonalDataManager?

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:76:
login_callback));
does base::Closure() work here?

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:93: NOTREACHED();
or NOTIMPLEMENTED?

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller.h (right):

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:21: class
FullCardRequest;
look like this one is included

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
File components/payments/content/payment_request_dialog.h (right):

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
components/payments/content/payment_request_dialog.h:9: #include
"components/autofill/core/browser/payments/full_card_request.h"
do we need this (vs fwd declare) because of the weakptr?

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
components/payments/content/payment_request_dialog.h:24: virtual void
ShowCvcUnmaskPrompt(
include comment about non trivial function

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
File components/payments/core/autofill_payment_instrument.cc (right):

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
components/payments/core/autofill_payment_instrument.cc:57: std::string
stringified_details;
nit: let's bring this one statement down, where it's used.

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
components/payments/core/autofill_payment_instrument.cc:64: delegate_ = nullptr;
any specific reason for this line and line 41? Since InvokePaymentApp comes with
a |delegate|, I don't think we need them? 

If you disagree, let's put clear comments that we need delegate_ to be nullptr
sometimes because of <blah>

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
File components/payments/core/autofill_payment_instrument.h (right):

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
components/payments/core/autofill_payment_instrument.h:45:
FullCardRequestDelegate* full_card_request_delegate);
Can we move PaymentRequestDelegate to core/ and accept a pointer here? Could
avoid the extra FullCardRequestDelegate

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/payment...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:11: #include
"components/autofill/content/browser/content_autofill_driver.h"
remove?

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:60:
dialog_->ShowCvcUnmaskPrompt(credit_card, result_delegate);
could we also pass the web_contents_ here, in which case we wouldn't need the
GetWebContents interface?

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc (right):

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:48:
payments_client_(base::MakeUnique<autofill::payments::PaymentsClient>(
can this also be a regular member?

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:57:
->GetPersonalDataManager())),
state->GetPersonalDataManager() ?

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller.h (right):

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:86: void
CvcConfirmed();
can we comment what this does

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h (right):

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h:59:
CVC_PROMPT_CONFIRM_BUTTON,
nit: could put this up next to pay button

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.cc
(right):

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:114: 
we could revert this line and thus file

https://codereview.chromium.org/2779283002/diff/180001/components/payments/co...
File components/payments/content/payment_request.h (right):

https://codereview.chromium.org/2779283002/diff/180001/components/payments/co...
components/payments/content/payment_request.h:71: PaymentRequestDelegate*
GetPaymentRequestDelegate() override;
as mentioned let's try to get rid of all the GetPaymentRequestDelegate

[commit-bot: I haz the power, 2017-04-04 21:05:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 21:05:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[anthonyvd, 2017-04-04 23:02:15]

Great suggestions, done. PTAL! :)

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/payment...
File chrome/browser/payments/chrome_payment_request_delegate.cc (right):

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:11: #include
"components/autofill/content/browser/content_autofill_driver.h"
On 2017/04/04 at 19:05:48, Mathieu wrote:
> remove?

Done.

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/payment...
chrome/browser/payments/chrome_payment_request_delegate.cc:60:
dialog_->ShowCvcUnmaskPrompt(credit_card, result_delegate);
On 2017/04/04 at 19:05:48, Mathieu wrote:
> could we also pass the web_contents_ here, in which case we wouldn't need the
GetWebContents interface?

Good point, done.

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc (right):

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:48:
payments_client_(base::MakeUnique<autofill::payments::PaymentsClient>(
On 2017/04/04 at 19:05:48, Mathieu wrote:
> can this also be a regular member?

Done.

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:57:
->GetPersonalDataManager())),
On 2017/04/04 at 19:05:48, Mathieu wrote:
> state->GetPersonalDataManager() ?

Done.

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller.h (right):

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:86: void
CvcConfirmed();
On 2017/04/04 at 19:05:48, Mathieu wrote:
> can we comment what this does

Done.

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h (right):

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h:59:
CVC_PROMPT_CONFIRM_BUTTON,
On 2017/04/04 at 19:05:48, Mathieu wrote:
> nit: could put this up next to pay button

Done.

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/payment_request_sheet_controller.cc
(right):

https://codereview.chromium.org/2779283002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/payment_request_sheet_controller.cc:114: 
On 2017/04/04 at 19:05:48, Mathieu wrote:
> we could revert this line and thus file

Good catch, done.

https://codereview.chromium.org/2779283002/diff/180001/components/payments/co...
File components/payments/content/payment_request.h (right):

https://codereview.chromium.org/2779283002/diff/180001/components/payments/co...
components/payments/content/payment_request.h:71: PaymentRequestDelegate*
GetPaymentRequestDelegate() override;
On 2017/04/04 at 19:05:48, Mathieu wrote:
> as mentioned let's try to get rid of all the GetPaymentRequestDelegate

Done.

[anthonyvd, 2017-04-04 23:02:31]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 23:03:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mathieu, 2017-04-05 00:02:24]

Thanks! There's still an open question + looks like my comments from PS8 were
not addressed (+ the Bump below).

https://codereview.chromium.org/2779283002/diff/120001/components/payments/co...
File components/payments/core/autofill_payment_instrument.cc (right):

https://codereview.chromium.org/2779283002/diff/120001/components/payments/co...
components/payments/core/autofill_payment_instrument.cc:49: return
credit_card_.IsValid();
On 2017/04/03 19:20:53, Mathieu wrote:
> Let's change this to !credit_card_.IsExpired(AutofillClock::Now()), what do
you
> think?

Bump.

https://codereview.chromium.org/2779283002/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2779283002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller_browsertest.cc:2: //
// Use of this source code is governed by a BSD-style license that can be
fix

https://codereview.chromium.org/2779283002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller_browsertest.cc:17:
PersonalDataLoadedObserverMock personal_data_observer_;
remove?

https://codereview.chromium.org/2779283002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller_browsertest.cc:25:
AddCreditCard(card);
AddCreditCard(autofill::test::GetCreditCard());  // Visa.

https://codereview.chromium.org/2779283002/diff/200001/components/payments/co...
File components/payments/core/autofill_payment_instrument.h (right):

https://codereview.chromium.org/2779283002/diff/200001/components/payments/co...
components/payments/core/autofill_payment_instrument.h:29: class
FullCardRequestDelegate {
I think I still see a world where this is not needed? What if
AutofillPaymentInstrument is constructed with a PaymentRequestDelegate* (which
would be moved to /core)? PaymentRequestState would pass it along and would be
itself constructed with it (similar to how you have it now).

[commit-bot: I haz the power, 2017-04-05 00:38:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 00:38:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[anthonyvd, 2017-04-05 02:11:02]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 02:11:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[anthonyvd, 2017-04-05 02:12:37]

Oops, sorry about that. I never saw the comments on PS8.

I imagine the open question you're referring to was the FullCardRequestDelegate
not being required. If so, everything should be addressed now. Let me know if I
missed something again.

PTAL! :)

https://codereview.chromium.org/2779283002/diff/120001/components/payments/co...
File components/payments/core/autofill_payment_instrument.cc (right):

https://codereview.chromium.org/2779283002/diff/120001/components/payments/co...
components/payments/core/autofill_payment_instrument.cc:49: return
credit_card_.IsValid();
On 2017/04/05 at 00:02:23, Mathieu wrote:
> On 2017/04/03 19:20:53, Mathieu wrote:
> > Let's change this to !credit_card_.IsExpired(AutofillClock::Now()), what do
you
> > think?
> 
> Bump.

Done.

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc (right):

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:52:
full_card_request_(base::MakeUnique<autofill::payments::FullCardRequest>(
On 2017/04/04 at 19:05:47, Mathieu wrote:
> does this need to be a unique_ptr or can this be a regular member?

Done.

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:70:
Profile::FromBrowserContext(web_contents_->GetBrowserContext())
On 2017/04/04 at 19:05:47, Mathieu wrote:
> I'm curious what's different when doing GetOriginalProfile vs just 
Profile::FromBrowserContext(web_contents_->GetBrowserContext()) ?
> 
> Should we also change in ChromePaymentRequestDelegate::GetPersonalDataManager?

This is copied from the other implementation of PaymentsClientDelegate
(https://cs.chromium.org/chromium/src/chrome/browser/ui/autofill/chrome_autofi...).
It would have implications in guest and incognito browsing sessions.

TBH, I think our GetPersonalDataManager is correct, and I don't know why
autofill client uses GetOriginalProfile. I'm not familiar enough with the code
to make a call.

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:76:
login_callback));
On 2017/04/04 at 19:05:47, Mathieu wrote:
> does base::Closure() work here?

Done.

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.cc:93: NOTREACHED();
On 2017/04/04 at 19:05:47, Mathieu wrote:
> or NOTIMPLEMENTED?

Done.

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller.h (right):

https://codereview.chromium.org/2779283002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller.h:21: class
FullCardRequest;
On 2017/04/04 at 19:05:47, Mathieu wrote:
> look like this one is included

Done.

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
File components/payments/content/payment_request_dialog.h (right):

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
components/payments/content/payment_request_dialog.h:9: #include
"components/autofill/core/browser/payments/full_card_request.h"
On 2017/04/04 at 19:05:47, Mathieu wrote:
> do we need this (vs fwd declare) because of the weakptr?

Yeah we need this because the fwd declare would be for a nested class, which
isn't fwd-declarable.

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
components/payments/content/payment_request_dialog.h:24: virtual void
ShowCvcUnmaskPrompt(
On 2017/04/04 at 19:05:47, Mathieu wrote:
> include comment about non trivial function

Done.

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
File components/payments/core/autofill_payment_instrument.cc (right):

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
components/payments/core/autofill_payment_instrument.cc:57: std::string
stringified_details;
On 2017/04/04 at 19:05:47, Mathieu wrote:
> nit: let's bring this one statement down, where it's used.

Done.

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
components/payments/core/autofill_payment_instrument.cc:64: delegate_ = nullptr;
On 2017/04/04 at 19:05:47, Mathieu wrote:
> any specific reason for this line and line 41? Since InvokePaymentApp comes
with a |delegate|, I don't think we need them? 
> 
> If you disagree, let's put clear comments that we need delegate_ to be nullptr
sometimes because of <blah>

Yeah so this + the DCHECK on line 41 are coming as changes after one of
Rouslan's comments. The idea is that there can only be one FullCardRequest going
on at a time, so this is one part of the enforcement of that.

Added comment.

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
File components/payments/core/autofill_payment_instrument.h (right):

https://codereview.chromium.org/2779283002/diff/160001/components/payments/co...
components/payments/core/autofill_payment_instrument.h:45:
FullCardRequestDelegate* full_card_request_delegate);
On 2017/04/04 at 19:05:47, Mathieu wrote:
> Can we move PaymentRequestDelegate to core/ and accept a pointer here? Could
avoid the extra FullCardRequestDelegate

Done.

https://codereview.chromium.org/2779283002/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/payments/cvc_unmask_view_controller_browsertest.cc
(right):

https://codereview.chromium.org/2779283002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller_browsertest.cc:2: //
// Use of this source code is governed by a BSD-style license that can be
On 2017/04/05 at 00:02:23, Mathieu wrote:
> fix

Done.

https://codereview.chromium.org/2779283002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller_browsertest.cc:17:
PersonalDataLoadedObserverMock personal_data_observer_;
On 2017/04/05 at 00:02:23, Mathieu wrote:
> remove?

Done.

https://codereview.chromium.org/2779283002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/payments/cvc_unmask_view_controller_browsertest.cc:25:
AddCreditCard(card);
On 2017/04/05 at 00:02:23, Mathieu wrote:
> AddCreditCard(autofill::test::GetCreditCard());  // Visa.

Done.

https://codereview.chromium.org/2779283002/diff/200001/components/payments/co...
File components/payments/core/autofill_payment_instrument.h (right):

https://codereview.chromium.org/2779283002/diff/200001/components/payments/co...
components/payments/core/autofill_payment_instrument.h:29: class
FullCardRequestDelegate {
On 2017/04/05 at 00:02:23, Mathieu wrote:
> I think I still see a world where this is not needed? What if
AutofillPaymentInstrument is constructed with a PaymentRequestDelegate* (which
would be moved to /core)? PaymentRequestState would pass it along and would be
itself constructed with it (similar to how you have it now).

Yep, you're right. Done.

[commit-bot: I haz the power, 2017-04-05 02:16:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 02:16:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[anthonyvd, 2017-04-05 02:20:58]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 02:21:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 02:24:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 02:24:21]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[anthonyvd, 2017-04-05 02:29:51]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 02:30:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 02:35:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 02:35:15]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[anthonyvd, 2017-04-05 02:40:12]

The CQ bit was checked by anthonyvd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 02:40:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 04:50:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-05 04:50:28]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-04-05 14:05:59]

Thanks! lgtm with a teeny nit

https://codereview.chromium.org/2779283002/diff/280001/components/payments/co...
File components/payments/content/BUILD.gn (right):

https://codereview.chromium.org/2779283002/diff/280001/components/payments/co...
components/payments/content/BUILD.gn:107: "//components/payments/core:core",
//components/payments/core is sufficient (an optimization makes the :core
redundant)

[anthonyvd, 2017-04-05 14:17:25]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-04-05 14:17:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2779283002/#ps300001
(title: "BUILD")

[commit-bot: I haz the power, 2017-04-05 14:17:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 15:29:05]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1491401845100850,
"parent_rev": "aa4eb39704c572837d41258bdaf3a46a4c03ed27", "commit_rev":
"d23ed70ac6dd7a254422e929c8175e3db61b0bb5"}

[commit-bot: I haz the power, 2017-04-05 15:29:59]

Description was changed from

==========
[Web Payments] Implement the CVC Unmask dialog.

BUG=706511
==========

to

==========
[Web Payments] Implement the CVC Unmask dialog.

BUG=706511

Review-Url: https://codereview.chromium.org/2779283002
Cr-Commit-Position: refs/heads/master@{#462077}
Committed:
https://chromium.googlesource.com/chromium/src/+/d23ed70ac6dd7a254422e929c817...
==========

[commit-bot: I haz the power, 2017-04-05 15:30:01]

Committed patchset #15 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/d23ed70ac6dd7a254422e929c817...