[Payments] Return a basic card response

[Payments] Return a basic card response in the PaymentResponse

* Creates AutofillPaymentInstrument (and the abstract type PaymentInstrument), 
which returns a basic card response.
* Brings PaymentAddress and BasicCardResponse from iOS code, because they
can be reused when serializing the stringified basic card details.
* Brings the utility functions used for PaymentAddress and BasicCardResponse.
* Hooks up Complete, OnComplete and CanMakePayment in PaymentRequest.
* Adds many TODOs!

BUG=696733
TEST=components_unittests, browser_tests (PaymentRequest*)

Review-Url: https://codereview.chromium.org/2733953003
Cr-Commit-Position: refs/heads/master@{#455540}
Committed: https://chromium.googlesource.com/chromium/src/+/4b85b58e36ed9f14bfda0ca84c82f5bdfb509ab7

[Mathieu, 2017-03-07 03:36:34]

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

[commit-bot: I haz the power, 2017-03-07 03:37:31]

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

[Mathieu, 2017-03-07 03:44:28]

Description was changed from

==========
[Payments] Return a basic card response

Creates AutofillPaymentInstrument, which returns a basic card response.

BUG=696733
==========

to

==========
[Payments] Return a basic card response in the PaymentResponse

* Creates AutofillPaymentInstrument (and the abstract type PaymentInstrument), 
which returns a basic card response.
* Brings PaymentAddress and BasicCardResponse from iOS code, because they
can be reused when serializing the stringified basic card details.
* Brings the utility functions used for PaymentAddress and BasicCardResponse.
* Hooks up Complete and OnComplete in PaymentRequest.

BUG=696733
TEST=components_unittests,
==========

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

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

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

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Mathieu, 2017-03-07 15:09:05]

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

[commit-bot: I haz the power, 2017-03-07 15:09:32]

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-07 15:19:59]

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

[commit-bot: I haz the power, 2017-03-07 15:20:00]

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

[Mathieu, 2017-03-07 15:34:28]

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

[commit-bot: I haz the power, 2017-03-07 15:35:12]

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

[Mathieu, 2017-03-07 15:35:28]

Patchset #1 (id:1) has been deleted

[Mathieu, 2017-03-07 15:35:36]

Patchset #1 (id:20001) has been deleted

[Mathieu, 2017-03-07 15:35:43]

Patchset #1 (id:40001) has been deleted

[Mathieu, 2017-03-07 15:35:51]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-03-07 15:45:46]

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

[commit-bot: I haz the power, 2017-03-07 15:45:46]

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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Mathieu, 2017-03-08 03:03:22]

Description was changed from

==========
[Payments] Return a basic card response in the PaymentResponse

* Creates AutofillPaymentInstrument (and the abstract type PaymentInstrument), 
which returns a basic card response.
* Brings PaymentAddress and BasicCardResponse from iOS code, because they
can be reused when serializing the stringified basic card details.
* Brings the utility functions used for PaymentAddress and BasicCardResponse.
* Hooks up Complete and OnComplete in PaymentRequest.

BUG=696733
TEST=components_unittests,
==========

to

==========
[Payments] Return a basic card response in the PaymentResponse

* Creates AutofillPaymentInstrument (and the abstract type PaymentInstrument), 
which returns a basic card response.
* Brings PaymentAddress and BasicCardResponse from iOS code, because they
can be reused when serializing the stringified basic card details.
* Brings the utility functions used for PaymentAddress and BasicCardResponse.
* Hooks up Complete, OnComplete and CanMakePayment in PaymentRequest.
* Adds many TODOs!

BUG=696733
TEST=components_unittests,
==========

[Mathieu, 2017-03-08 03:03:22]

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

[Mathieu, 2017-03-08 03:10:32]

Hi, would like both your set of eyes on this. 

Still having an iOS compilation issue, will look at it later before sending for
the iOS review.

Thanks!

[Mathieu, 2017-03-08 03:10:54]

Description was changed from

==========
[Payments] Return a basic card response in the PaymentResponse

* Creates AutofillPaymentInstrument (and the abstract type PaymentInstrument), 
which returns a basic card response.
* Brings PaymentAddress and BasicCardResponse from iOS code, because they
can be reused when serializing the stringified basic card details.
* Brings the utility functions used for PaymentAddress and BasicCardResponse.
* Hooks up Complete, OnComplete and CanMakePayment in PaymentRequest.
* Adds many TODOs!

BUG=696733
TEST=components_unittests,
==========

to

==========
[Payments] Return a basic card response in the PaymentResponse

* Creates AutofillPaymentInstrument (and the abstract type PaymentInstrument), 
which returns a basic card response.
* Brings PaymentAddress and BasicCardResponse from iOS code, because they
can be reused when serializing the stringified basic card details.
* Brings the utility functions used for PaymentAddress and BasicCardResponse.
* Hooks up Complete, OnComplete and CanMakePayment in PaymentRequest.
* Adds many TODOs!

BUG=696733
TEST=components_unittests, browser_tests (PaymentRequest*)
==========

[please use gerrit instead, 2017-03-08 14:46:10]

lgtm % nits

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
File components/payments/content/payment_request.cc (right):

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/content/payment_request.cc:157: }
Maybe hide the dialog, so it does not show up behind the CVC unlock.

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

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/content/payment_request.h:69: void
OnInstrumentDetailsError() override {}
If you hide the dialog in Pay(), then OnInstrumentDetailsError() should show the
dialog again. OnInstrumentDetailsReady() should show a spinner while the website
processes the transaction.

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

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/autofill_payment_instrument.cc:32: // placeholder
string.
FYI, this can be accomplished via FullCardRequest class.

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

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/autofill_payment_instrument.h:26:
AutofillPaymentInstrument(
Mention that |billing_profiles| are owned by the caller and must outlive this
object.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
File components/payments/core/basic_card_response.cc (right):

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/basic_card_response.cc:15: //
https://w3c.github.io/browser-payment-api/.
https://w3c.github.io/webpayments-methods-card/#basiccardresponse

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/basic_card_response.cc:44:
std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue());
base::MakeUnique<base::DitionaryValue>();

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
File components/payments/core/basic_card_response.h (right):

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/basic_card_response.h:21: class BasicCardResponse {
IMHO, this should be a struct, because it has no private section and follows
struct field naming convention (no trailing underscore).

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
File components/payments/core/payment_address.cc (right):

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_address.cc:15: //
https://w3c.github.io/browser-payment-api/.
https://w3c.github.io/browser-payment-api/#paymentaddress-interface

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_address.cc:59: std::unique_ptr<base::ListValue>
address_line(new base::ListValue);
base::MakeUnique<base::ListValue>();

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_address.cc:60: for (base::string16
address_line_string : this->address_line) {
Use "const base::string16&" instead of "base::string16" to avoid copies.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
File components/payments/core/payment_address.h (right):

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_address.h:15: //
https://w3c.github.io/browser-payment-api/#paymentaddress-interface.
Please remove the period at the end of the link for easier copy-pasting.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_address.h:21: namespace web {
Can we keep "payments" namespace for everything in components/payments?

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_address.h:74: base::string16 care_of;
No longer part of the spec. Should be removed from here.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
File components/payments/core/payment_instrument.h (right):

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_instrument.h:19: public:
Need an empty virtual destrutor.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_instrument.h:20: // Should be called with
method name (e.g. "visa"), and json-serialized
Remove comma before "and". Add comma after "e.g.".

https://codereview.chromium.org/2733953003/diff/100001/ios/chrome/browser/pay...
File ios/chrome/browser/payments/payment_request_coordinator.mm (right):

https://codereview.chromium.org/2733953003/diff/100001/ios/chrome/browser/pay...
ios/chrome/browser/payments/payment_request_coordinator.mm:37: using
payments::data_util::GetPaymentAddressFromAutofillProfile;
It's recommended to prepend top-level namespaces with "::" in "using"
statements.

https://codereview.chromium.org/2733953003/diff/100001/ios/web/DEPS
File ios/web/DEPS (right):

https://codereview.chromium.org/2733953003/diff/100001/ios/web/DEPS#newcode2
ios/web/DEPS:2: "+components/payments/core",
Can you put this in ios/web/payments/DEPS?

[Mathieu, 2017-03-08 15:11:47]

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

[commit-bot: I haz the power, 2017-03-08 15:12:18]

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-08 15:23:37]

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

[commit-bot: I haz the power, 2017-03-08 15:23:38]

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

[Mathieu, 2017-03-08 15:29:40]

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

[commit-bot: I haz the power, 2017-03-08 15:29:55]

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

[Mathieu, 2017-03-08 15:55:46]

Patchset #3 (id:120001) has been deleted

[Mathieu, 2017-03-08 16:30:58]

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

[commit-bot: I haz the power, 2017-03-08 16:31:14]

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-08 16:39:57]

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

[commit-bot: I haz the power, 2017-03-08 16:39:58]

Dry run: Try jobs failed on following builders:
  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-03-08 16:41:48]

lgtm % a few nits and the start of a bigger discussion.

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

https://codereview.chromium.org/2733953003/diff/140001/components/payments/co...
components/payments/content/payment_request.h:65: // PaymentInstrument::Delegate
nit: "PaymentInstrument::Delegate:"

https://codereview.chromium.org/2733953003/diff/140001/components/payments/co...
components/payments/content/payment_request.h:195:
std::unique_ptr<PaymentInstrument> payment_instrument_;
Can this be renamed so that it's clearer that it represents the currently
selected payment instrument?

https://codereview.chromium.org/2733953003/diff/140001/components/payments/co...
components/payments/content/payment_request.h:196: mojom::PaymentResponsePtr
payment_response_;
We can discuss it offline a bit and defer to a different CL anyways but I'd
probably encapsulate those 2 members in a different object. That object would be
the one implementing PaymentInstrument::Delegate, creating the PaymentResponse
object, and passing it back to PaymentRequest.

There are 3 benefits to this approach:

1- It'll be easier to unit test this smaller object in isolation (right now,
testing PaymentRequest is pretty involved, with the mojo and the webcontents and
everything else).
2- It'll be less bug-prone. The current solution makes the lifetime of
payment_instrument_ and payment_response unclear and partially modifies their
data at arbitrary points (for example, you could prepare half of the response,
get an InstrumentDetailsError, have the user try again. As the code grows, this
might get confusing and introduce bugs).
3- As we get different Payment Methods that prepare different response objects,
we'll get to avoid polluting PaymentRequest itself too much.

In any case, I think this CL is big enough to defer that to a refactor if we
decide to go that way.

https://codereview.chromium.org/2733953003/diff/140001/components/payments/co...
File components/payments/core/basic_card_response.cc (right):

https://codereview.chromium.org/2733953003/diff/140001/components/payments/co...
components/payments/core/basic_card_response.cc:44:
std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue());
nit: base::MakeUnique

[Mathieu, 2017-03-08 16:48:26]

Patchset #4 (id:160001) has been deleted

[Mathieu, 2017-03-08 16:48:31]

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

[commit-bot: I haz the power, 2017-03-08 16:48:48]

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-08 16:57:40]

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

[commit-bot: I haz the power, 2017-03-08 16:57:41]

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...)

[Mathieu, 2017-03-08 17:10:02]

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

[commit-bot: I haz the power, 2017-03-08 17:10:19]

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

[Mathieu, 2017-03-08 17:51:32]

Patchset #4 (id:180001) has been deleted

[Mathieu, 2017-03-08 18:02:35]

mathp@chromium.org changed reviewers:
+ eugenebut@chromium.org, mahmadi@chromium.org

[Mathieu, 2017-03-08 18:04:26]

Thanks for the review.

+Moe for ios/chrome/browser/payments
+Eugene for ios/web

For context: we are implementing this spec cross-platform, so reusing code where
possible is essential to avoid divergence. PTAL

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
File components/payments/content/payment_request.cc (right):

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/content/payment_request.cc:157: }
On 2017/03/08 14:46:08, ಠ_ಠ wrote:
> Maybe hide the dialog, so it does not show up behind the CVC unlock.

Acknowledged.

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

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/content/payment_request.h:69: void
OnInstrumentDetailsError() override {}
On 2017/03/08 14:46:09, ಠ_ಠ wrote:
> If you hide the dialog in Pay(), then OnInstrumentDetailsError() should show
the
> dialog again. OnInstrumentDetailsReady() should show a spinner while the
website
> processes the transaction.

Acknowledged.

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

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/autofill_payment_instrument.h:26:
AutofillPaymentInstrument(
On 2017/03/08 14:46:09, ಠ_ಠ wrote:
> Mention that |billing_profiles| are owned by the caller and must outlive this
> object.

Done.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
File components/payments/core/basic_card_response.cc (right):

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/basic_card_response.cc:15: //
https://w3c.github.io/browser-payment-api/.
On 2017/03/08 14:46:09, ಠ_ಠ wrote:
> https://w3c.github.io/webpayments-methods-card/#basiccardresponse

Done.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/basic_card_response.cc:44:
std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue());
On 2017/03/08 14:46:09, ಠ_ಠ wrote:
> base::MakeUnique<base::DitionaryValue>();

Done.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
File components/payments/core/basic_card_response.h (right):

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/basic_card_response.h:21: class BasicCardResponse {
On 2017/03/08 14:46:09, ಠ_ಠ wrote:
> IMHO, this should be a struct, because it has no private section and follows
> struct field naming convention (no trailing underscore).

Done.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
File components/payments/core/payment_address.cc (right):

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_address.cc:15: //
https://w3c.github.io/browser-payment-api/.
On 2017/03/08 14:46:09, ಠ_ಠ wrote:
> https://w3c.github.io/browser-payment-api/#paymentaddress-interface

Done.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_address.cc:59: std::unique_ptr<base::ListValue>
address_line(new base::ListValue);
On 2017/03/08 14:46:09, ಠ_ಠ wrote:
> base::MakeUnique<base::ListValue>();

Done.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_address.cc:60: for (base::string16
address_line_string : this->address_line) {
On 2017/03/08 14:46:09, ಠ_ಠ wrote:
> Use "const base::string16&" instead of "base::string16" to avoid copies.

Done.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
File components/payments/core/payment_address.h (right):

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_address.h:15: //
https://w3c.github.io/browser-payment-api/#paymentaddress-interface.
On 2017/03/08 14:46:10, ಠ_ಠ wrote:
> Please remove the period at the end of the link for easier copy-pasting.

Done.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_address.h:21: namespace web {
On 2017/03/08 14:46:09, ಠ_ಠ wrote:
> Can we keep "payments" namespace for everything in components/payments?

Certainly.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_address.h:74: base::string16 care_of;
On 2017/03/08 14:46:09, ಠ_ಠ wrote:
> No longer part of the spec. Should be removed from here.

Done.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
File components/payments/core/payment_instrument.h (right):

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_instrument.h:19: public:
On 2017/03/08 14:46:10, ಠ_ಠ wrote:
> Need an empty virtual destrutor.

Done.

https://codereview.chromium.org/2733953003/diff/100001/components/payments/co...
components/payments/core/payment_instrument.h:20: // Should be called with
method name (e.g. "visa"), and json-serialized
On 2017/03/08 14:46:10, ಠ_ಠ wrote:
> Remove comma before "and". Add comma after "e.g.".

Done.

https://codereview.chromium.org/2733953003/diff/100001/ios/chrome/browser/pay...
File ios/chrome/browser/payments/payment_request_coordinator.mm (right):

https://codereview.chromium.org/2733953003/diff/100001/ios/chrome/browser/pay...
ios/chrome/browser/payments/payment_request_coordinator.mm:37: using
payments::data_util::GetPaymentAddressFromAutofillProfile;
On 2017/03/08 14:46:10, ಠ_ಠ wrote:
> It's recommended to prepend top-level namespaces with "::" in "using"
> statements.

Done.

https://codereview.chromium.org/2733953003/diff/100001/ios/web/DEPS
File ios/web/DEPS (right):

https://codereview.chromium.org/2733953003/diff/100001/ios/web/DEPS#newcode2
ios/web/DEPS:2: "+components/payments/core",
On 2017/03/08 14:46:10, ಠ_ಠ wrote:
> Can you put this in ios/web/payments/DEPS?

Done.

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

https://codereview.chromium.org/2733953003/diff/140001/components/payments/co...
components/payments/content/payment_request.h:65: // PaymentInstrument::Delegate
On 2017/03/08 16:41:48, anthonyvd wrote:
> nit: "PaymentInstrument::Delegate:"

Done.

https://codereview.chromium.org/2733953003/diff/140001/components/payments/co...
components/payments/content/payment_request.h:195:
std::unique_ptr<PaymentInstrument> payment_instrument_;
On 2017/03/08 16:41:48, anthonyvd wrote:
> Can this be renamed so that it's clearer that it represents the currently
> selected payment instrument?

Done.

https://codereview.chromium.org/2733953003/diff/140001/components/payments/co...
components/payments/content/payment_request.h:196: mojom::PaymentResponsePtr
payment_response_;
On 2017/03/08 16:41:48, anthonyvd wrote:
> We can discuss it offline a bit and defer to a different CL anyways but I'd
> probably encapsulate those 2 members in a different object. That object would
be
> the one implementing PaymentInstrument::Delegate, creating the PaymentResponse
> object, and passing it back to PaymentRequest.
> 
> There are 3 benefits to this approach:
> 
> 1- It'll be easier to unit test this smaller object in isolation (right now,
> testing PaymentRequest is pretty involved, with the mojo and the webcontents
and
> everything else).
> 2- It'll be less bug-prone. The current solution makes the lifetime of
> payment_instrument_ and payment_response unclear and partially modifies their
> data at arbitrary points (for example, you could prepare half of the response,
> get an InstrumentDetailsError, have the user try again. As the code grows,
this
> might get confusing and introduce bugs).
> 3- As we get different Payment Methods that prepare different response
objects,
> we'll get to avoid polluting PaymentRequest itself too much.
> 
> In any case, I think this CL is big enough to defer that to a refactor if we
> decide to go that way.

Discussed offline, sounds good!

https://codereview.chromium.org/2733953003/diff/140001/components/payments/co...
File components/payments/core/basic_card_response.cc (right):

https://codereview.chromium.org/2733953003/diff/140001/components/payments/co...
components/payments/core/basic_card_response.cc:44:
std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue());
On 2017/03/08 16:41:48, anthonyvd wrote:
> nit: base::MakeUnique

Done.

[Eugene But (OOO till 7-30), 2017-03-08 19:23:35]

ios/web lgtm

[Moe, 2017-03-08 19:29:51]

On 2017/03/08 19:23:35, Eugene But wrote:
> ios/web lgtm

ios/chrome/browser/payments lgtm

[Mathieu, 2017-03-08 19:33:10]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-03-08 19:33:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2733953003/#ps220001
(title: "addressed comments from anthony")

[commit-bot: I haz the power, 2017-03-08 19:34:18]

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-08 21:07:20]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1489001590858970,
"parent_rev": "0df5baacc99135ecb3402eaf046d960b5d65fbfd", "commit_rev":
"4b85b58e36ed9f14bfda0ca84c82f5bdfb509ab7"}

[commit-bot: I haz the power, 2017-03-08 21:08:44]

Description was changed from

==========
[Payments] Return a basic card response in the PaymentResponse

* Creates AutofillPaymentInstrument (and the abstract type PaymentInstrument), 
which returns a basic card response.
* Brings PaymentAddress and BasicCardResponse from iOS code, because they
can be reused when serializing the stringified basic card details.
* Brings the utility functions used for PaymentAddress and BasicCardResponse.
* Hooks up Complete, OnComplete and CanMakePayment in PaymentRequest.
* Adds many TODOs!

BUG=696733
TEST=components_unittests, browser_tests (PaymentRequest*)
==========

to

==========
[Payments] Return a basic card response in the PaymentResponse

* Creates AutofillPaymentInstrument (and the abstract type PaymentInstrument), 
which returns a basic card response.
* Brings PaymentAddress and BasicCardResponse from iOS code, because they
can be reused when serializing the stringified basic card details.
* Brings the utility functions used for PaymentAddress and BasicCardResponse.
* Hooks up Complete, OnComplete and CanMakePayment in PaymentRequest.
* Adds many TODOs!

BUG=696733
TEST=components_unittests, browser_tests (PaymentRequest*)

Review-Url: https://codereview.chromium.org/2733953003
Cr-Commit-Position: refs/heads/master@{#455540}
Committed:
https://chromium.googlesource.com/chromium/src/+/4b85b58e36ed9f14bfda0ca84c82...
==========

[commit-bot: I haz the power, 2017-03-08 21:08:46]

Committed patchset #5 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/4b85b58e36ed9f14bfda0ca84c82...