PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side)

PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side)

The respondWith() method is used by the payment app to provide a
PaymentAppResponse when the payment successfully completes.

This CL includes a content-side implementation and browser test. Especially, add
a callback to InvokePaymentApp() in PaymentAppProvider. The callback is used to
pass a response data from payment app using respondWith().

Related Spec Link:
  https://w3c.github.io/webpayments-payment-apps-api/#idl-def-paymentrequestevent

See the other CLs in this series:
  [1/3] https://codereview.chromium.org/2715663002/ (RespondWithObserver)
  [2/3] https://codereview.chromium.org/2705293010/ (blink side)
  [3/3] This patch

BUG=661608
TEST=payment_app_browsertest.cc

Review-Url: https://codereview.chromium.org/2718013004
Cr-Commit-Position: refs/heads/master@{#460634}
Committed: https://chromium.googlesource.com/chromium/src/+/865ce0550e0ba864c2a830bde1f7d74004edca9b

[zino, 2017-03-07 15:13:29]

Description was changed from

==========
invoke

BUG=
==========

to

==========
PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side)

The respondWith() method is used by the payment app to provide a
PaymentAppResponse when the payment successfully completes.

This CL includes a content-side implementation and browser test. Especially, add
a callback to InvokePaymentApp() in PaymentAppProvider. The callback is used to
pass a response data from payment app using respondWith().

Related Spec Link:
 
https://w3c.github.io/webpayments-payment-apps-api/#idl-def-paymentrequestevent

See the other CLs in this series:
  [1/3] https://codereview.chromium.org/2715663002/ (RespondWithObserver)
  [2/3] https://codereview.chromium.org/2705293010/ (blink side patch)
  [3/3] This patch

BUG=661608
TEST=payment_app_browsertest.cc
==========

[zino, 2017-03-07 15:25:55]

Description was changed from

==========
PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side)

The respondWith() method is used by the payment app to provide a
PaymentAppResponse when the payment successfully completes.

This CL includes a content-side implementation and browser test. Especially, add
a callback to InvokePaymentApp() in PaymentAppProvider. The callback is used to
pass a response data from payment app using respondWith().

Related Spec Link:
 
https://w3c.github.io/webpayments-payment-apps-api/#idl-def-paymentrequestevent

See the other CLs in this series:
  [1/3] https://codereview.chromium.org/2715663002/ (RespondWithObserver)
  [2/3] https://codereview.chromium.org/2705293010/ (blink side patch)
  [3/3] This patch

BUG=661608
TEST=payment_app_browsertest.cc
==========

to

==========
PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side)

The respondWith() method is used by the payment app to provide a
PaymentAppResponse when the payment successfully completes.

This CL includes a content-side implementation and browser test. Especially, add
a callback to InvokePaymentApp() in PaymentAppProvider. The callback is used to
pass a response data from payment app using respondWith().

Related Spec Link:
 
https://w3c.github.io/webpayments-payment-apps-api/#idl-def-paymentrequestevent

See the other CLs in this series:
  [1/3] https://codereview.chromium.org/2715663002/ (RespondWithObserver)
  [2/3] https://codereview.chromium.org/2705293010/ (blink side)
  [3/3] This patch

BUG=661608
TEST=payment_app_browsertest.cc
==========

[zino, 2017-03-07 15:31:48]

jinho.bang@samsung.com changed reviewers:
+ haraken@chromium.org, nhiroki@chromium.org, rouslan@chromium.org,
tsepez@chromium.org

[zino, 2017-03-07 15:51:13]

PTAL

[Tom Sepez, 2017-03-07 17:22:39]

https://codereview.chromium.org/2718013004/diff/60001/components/payments/con...
File components/payments/content/payment_app.mojom (right):

https://codereview.chromium.org/2718013004/diff/60001/components/payments/con...
components/payments/content/payment_app.mojom:47: string method_name;
What are the restrictions on the values of these two fields?
These are only processed in a renderer?

[shimazu, 2017-03-08 01:39:38]

shimazu@chromium.org changed reviewers:
+ shimazu@chromium.org

[shimazu, 2017-03-08 01:39:40]

drive-by comments:

https://codereview.chromium.org/2718013004/diff/60001/content/browser/payment...
File content/browser/payments/payment_app_provider_impl.cc (right):

https://codereview.chromium.org/2718013004/diff/60001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.cc:97: int event_id =
active_version->StartRequest(
|event_id| vs |event_finish_id| seems a bit ambiguous. How about
|payment_response_id| instead?

https://codereview.chromium.org/2718013004/diff/60001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.cc:109:
base::Bind(&DidDispatchPaymentRequestEvent, active_version,
Could you use active_version->CreateSimpleEventCallback(event_finish_id)? I
think you don't have to implement DidDispatchPaymentRequestEvent by your own if
you don't have any other caller of DidDIspatchPaymetnRequestEvent.

https://codereview.chromium.org/2718013004/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2718013004/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:207: int event_id,
|payment_response_id|

https://codereview.chromium.org/2718013004/diff/60001/content/common/service_...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2718013004/diff/60001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:40:
OnPaymentAppResponse(payments.mojom.PaymentAppResponse response,
Could you move the callback to the directory for payment requests?

https://codereview.chromium.org/2718013004/diff/60001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:75:
DispatchPaymentRequestEvent(int32 event_id,
|payment_response_id|

https://codereview.chromium.org/2718013004/diff/60001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2718013004/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:201:
IDMap<std::unique_ptr<mojom::PaymentAppResponseCallbackPtr>>;
IDMap was originally used here, but now we can use std::map<somecallback>
instead. I'm now planning to convert IDMaps to std::map. 
Could you also use std::map here if possible?

https://codereview.chromium.org/2718013004/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:792: int
event_id,
|payment_request_id|

https://codereview.chromium.org/2718013004/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:927: int
event_id,
|payment_request_id|

https://codereview.chromium.org/2718013004/diff/60001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2718013004/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:242: int
event_id,
|payment_request_id|

[zino, 2017-03-19 04:47:23]

Thank you for review.

PTAL

https://codereview.chromium.org/2718013004/diff/60001/content/browser/payment...
File content/browser/payments/payment_app_provider_impl.cc (right):

https://codereview.chromium.org/2718013004/diff/60001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.cc:97: int event_id =
active_version->StartRequest(
On 2017/03/08 01:39:39, shimazu (OOO till Mar 20) wrote:
> |event_id| vs |event_finish_id| seems a bit ambiguous. How about
> |payment_response_id| instead?

Done.

https://codereview.chromium.org/2718013004/diff/60001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.cc:109:
base::Bind(&DidDispatchPaymentRequestEvent, active_version,
On 2017/03/08 01:39:39, shimazu (OOO till Mar 20) wrote:
> Could you use active_version->CreateSimpleEventCallback(event_finish_id)? I
> think you don't have to implement DidDispatchPaymentRequestEvent by your own
if
> you don't have any other caller of DidDIspatchPaymetnRequestEvent.

Done.

https://codereview.chromium.org/2718013004/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2718013004/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:207: int event_id,
On 2017/03/08 01:39:39, shimazu (OOO till Mar 20) wrote:
> |payment_response_id|

Done.

https://codereview.chromium.org/2718013004/diff/60001/content/common/service_...
File content/common/service_worker/service_worker_event_dispatcher.mojom
(right):

https://codereview.chromium.org/2718013004/diff/60001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:40:
OnPaymentAppResponse(payments.mojom.PaymentAppResponse response,
On 2017/03/08 01:39:39, shimazu (OOO till Mar 20) wrote:
> Could you move the callback to the directory for payment requests? 

Done.

https://codereview.chromium.org/2718013004/diff/60001/content/common/service_...
content/common/service_worker/service_worker_event_dispatcher.mojom:75:
DispatchPaymentRequestEvent(int32 event_id,
On 2017/03/08 01:39:39, shimazu (OOO till Mar 20) wrote:
> |payment_response_id|

Done.

https://codereview.chromium.org/2718013004/diff/60001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2718013004/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:201:
IDMap<std::unique_ptr<mojom::PaymentAppResponseCallbackPtr>>;
On 2017/03/08 01:39:40, shimazu (OOO till Mar 20) wrote:
> IDMap was originally used here, but now we can use std::map<somecallback>
> instead. I'm now planning to convert IDMaps to std::map. 
> Could you also use std::map here if possible?

Done.

https://codereview.chromium.org/2718013004/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:792: int
event_id,
On 2017/03/08 01:39:40, shimazu (OOO till Mar 20) wrote:
> |payment_request_id|

Done.

https://codereview.chromium.org/2718013004/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:927: int
event_id,
On 2017/03/08 01:39:39, shimazu (OOO till Mar 20) wrote:
> |payment_request_id|

Done.

https://codereview.chromium.org/2718013004/diff/60001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.h (right):

https://codereview.chromium.org/2718013004/diff/60001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.h:242: int
event_id,
On 2017/03/08 01:39:40, shimazu (OOO till Mar 20) wrote:
> |payment_request_id|

Done.

[zino, 2017-03-21 13:05:36]

Owners, PTAL

[please use gerrit instead, 2017-03-21 16:09:42]

Great stuff!

https://codereview.chromium.org/2718013004/diff/80001/components/payments/con...
File components/payments/content/payment_app.mojom (right):

https://codereview.chromium.org/2718013004/diff/80001/components/payments/con...
components/payments/content/payment_app.mojom:47: struct PaymentAppResponse {
If mojom language supports inheritance, then please move PaymentAppResponse to
payment_request.mojom, then make PaymentResponse inherit from
PayemntAppResponse.

https://codereview.chromium.org/2718013004/diff/80001/components/payments/con...
components/payments/content/payment_app.mojom:52: interface
PaymentAppResponseCallback {
Nit: would be more clear to have an interface like this, don't you think?

interface PaymentApp {
  InvokePaymentApp(PaymentAppRequest request)
      => (PaymentAppResponse response,
          mojo.common.mojom.Time dispatch_even_type);
};

https://codereview.chromium.org/2718013004/diff/80001/components/payments/con...
components/payments/content/payment_app.mojom:53:
OnPaymentAppResponse(payments.mojom.PaymentAppResponse response,
No need for "payments.mojom." module name here, because that's the module of
this file.

https://codereview.chromium.org/2718013004/diff/80001/content/browser/payment...
File content/browser/payments/payment_app_provider_impl.h (right):

https://codereview.chromium.org/2718013004/diff/80001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.h:8: #include <string>
Not used.

https://codereview.chromium.org/2718013004/diff/80001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.h:12: #include
"components/payments/content/payment_app.mojom.h"
Used only in overriding PaymentAppProvider interface, so no need to include it
here, as a general rule of thumb.

https://codereview.chromium.org/2718013004/diff/80001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.h:18: class BrowserContext;
Used only in overriding PaymentAppProvider interface, so no need to forward
declare it here, as a general rule of thumb.

https://codereview.chromium.org/2718013004/diff/80001/content/public/browser/...
File content/public/browser/payment_app_provider.h (right):

https://codereview.chromium.org/2718013004/diff/80001/content/public/browser/...
content/public/browser/payment_app_provider.h:31: std::pair<int64_t,
payments::mojom::PaymentAppManifestPtr>;
#include "components/payments/content/payment_app.mojom.h"

https://codereview.chromium.org/2718013004/diff/80001/content/public/browser/...
content/public/browser/payment_app_provider.h:33: using GetAllManifestsCallback
= base::Callback<void(Manifests)>;
Is it possible to use the move-only base::OnceCallback? See
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md for
more info.

https://codereview.chromium.org/2718013004/diff/80001/content/public/browser/...
content/public/browser/payment_app_provider.h:35:
base::Callback<void(payments::mojom::PaymentAppResponsePtr)>;
Is base::OnceCallback usable here?

https://codereview.chromium.org/2718013004/diff/80001/content/public/browser/...
content/public/browser/payment_app_provider.h:39: const GetAllManifestsCallback&
callback) = 0;
If you use base::OnceCallback to define GetAllManifestsCallback, then remove
const-ref from the callback parameter, because it's move-only. That is:

virtual void GetAllManifests(BrowserContext* browser_context,
GetAllManifestsCallback callback) = 0;

https://codereview.chromium.org/2718013004/diff/80001/content/public/browser/...
content/public/browser/payment_app_provider.h:42: int64_t registration_id,
#include <stdint.h>

https://codereview.chromium.org/2718013004/diff/80001/content/public/browser/...
content/public/browser/payment_app_provider.h:44: const
InvokePaymentAppCallback& callback) = 0;
If you use base::OnceCallback, then there's no need for const-ref of the
callback paramater.

https://codereview.chromium.org/2718013004/diff/80001/content/test/data/payme...
File content/test/data/payments/payment_app_window.html (right):

https://codereview.chromium.org/2718013004/diff/80001/content/test/data/payme...
content/test/data/payments/payment_app_window.html:11: </body>
Strange the close the body and html tags without opening them. Can you be
consistent, please?

[nhiroki, 2017-03-22 02:19:16]

service worker lgtm

https://codereview.chromium.org/2718013004/diff/80001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2718013004/diff/80001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:917:
std::make_pair(payment_request_id, std::move(callback)));
It looks strange that std::move() is used for const-ref object.

[nhiroki, 2017-03-22 02:24:48]

https://codereview.chromium.org/2718013004/diff/80001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2718013004/diff/80001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:209:
payments::mojom::PaymentAppResponseCallbackPtr response_callback,
|response_callback| is not used. Is this OK? Do you plan to use this in a
follow-up CL?

[haraken, 2017-03-22 04:29:30]

(I'll defer this review to nhiroki@.)

[zino, 2017-03-22 18:04:45]

Thank you for review.

rouslan@, I addressed your comments except base::OnceCallback.
I'll work on it in follow-up CL. Instead I filed a bug and left a comment.

Thanks!

https://codereview.chromium.org/2718013004/diff/80001/components/payments/con...
File components/payments/content/payment_app.mojom (right):

https://codereview.chromium.org/2718013004/diff/80001/components/payments/con...
components/payments/content/payment_app.mojom:47: struct PaymentAppResponse {
On 2017/03/21 16:09:40, ಠ_ಠ wrote:
> If mojom language supports inheritance, then please move PaymentAppResponse to
> payment_request.mojom, then make PaymentResponse inherit from
> PayemntAppResponse.

As far as I know, the mojom struct doesn't support inheritance :(

I try to find codes in cs.chromium.org but I didn't find anything:
|struct\ [A-Z|a-z|1-9|_]*\ [^\{]  file:.*mojom$ package:^chromium$|

Also, I found the following link:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/6_Kw33AiH0A

https://codereview.chromium.org/2718013004/diff/80001/components/payments/con...
components/payments/content/payment_app.mojom:52: interface
PaymentAppResponseCallback {
On 2017/03/21 16:09:40, ಠ_ಠ wrote:
> Nit: would be more clear to have an interface like this, don't you think?
> 
> interface PaymentApp {
>   InvokePaymentApp(PaymentAppRequest request)
>       => (PaymentAppResponse response,
>           mojo.common.mojom.Time dispatch_even_type);
> };

To dispatch PaymentRequestEvent in registered service worker, we use the
DispatchPaymentRequestEvent() function in service_worker_event_dispatcher.mojom.
This interface is to provide a callback for respondWith() in blink side.
I left comments.

https://codereview.chromium.org/2718013004/diff/80001/components/payments/con...
components/payments/content/payment_app.mojom:53:
OnPaymentAppResponse(payments.mojom.PaymentAppResponse response,
On 2017/03/21 16:09:40, ಠ_ಠ wrote:
> No need for "payments.mojom." module name here, because that's the module of
> this file.

Done.

https://codereview.chromium.org/2718013004/diff/80001/content/browser/payment...
File content/browser/payments/payment_app_provider_impl.h (right):

https://codereview.chromium.org/2718013004/diff/80001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.h:8: #include <string>
On 2017/03/21 16:09:41, ಠ_ಠ wrote:
> Not used.

Done.

https://codereview.chromium.org/2718013004/diff/80001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.h:12: #include
"components/payments/content/payment_app.mojom.h"
On 2017/03/21 16:09:41, ಠ_ಠ wrote:
> Used only in overriding PaymentAppProvider interface, so no need to include it
> here, as a general rule of thumb.

Done.

https://codereview.chromium.org/2718013004/diff/80001/content/browser/payment...
content/browser/payments/payment_app_provider_impl.h:18: class BrowserContext;
On 2017/03/21 16:09:41, ಠ_ಠ wrote:
> Used only in overriding PaymentAppProvider interface, so no need to forward
> declare it here, as a general rule of thumb.

Done.

https://codereview.chromium.org/2718013004/diff/80001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2718013004/diff/80001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:209:
payments::mojom::PaymentAppResponseCallbackPtr response_callback,
On 2017/03/22 02:24:48, nhiroki (busy) wrote:
> |response_callback| is not used. Is this OK? Do you plan to use this in a
> follow-up CL?

Ah, I forgot it.

I implement it in this patch.

Thanks!

https://codereview.chromium.org/2718013004/diff/80001/content/public/browser/...
File content/public/browser/payment_app_provider.h (right):

https://codereview.chromium.org/2718013004/diff/80001/content/public/browser/...
content/public/browser/payment_app_provider.h:31: std::pair<int64_t,
payments::mojom::PaymentAppManifestPtr>;
On 2017/03/21 16:09:41, ಠ_ಠ wrote:
> #include "components/payments/content/payment_app.mojom.h"

Done.

https://codereview.chromium.org/2718013004/diff/80001/content/public/browser/...
content/public/browser/payment_app_provider.h:42: int64_t registration_id,
On 2017/03/21 16:09:41, ಠ_ಠ wrote:
> #include <stdint.h>

Done.

https://codereview.chromium.org/2718013004/diff/80001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2718013004/diff/80001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:917:
std::make_pair(payment_request_id, std::move(callback)));
On 2017/03/22 02:19:15, nhiroki (busy) wrote:
> It looks strange that std::move() is used for const-ref object.

Done.

https://codereview.chromium.org/2718013004/diff/80001/content/test/data/payme...
File content/test/data/payments/payment_app_window.html (right):

https://codereview.chromium.org/2718013004/diff/80001/content/test/data/payme...
content/test/data/payments/payment_app_window.html:11: </body>
On 2017/03/21 16:09:41, ಠ_ಠ wrote:
> Strange the close the body and html tags without opening them. Can you be
> consistent, please?

Done.

[please use gerrit instead, 2017-03-22 18:19:09]

On 2017/03/22 18:04:45, zino wrote:
> rouslan@, I addressed your comments

Looks like the fixes have not been uploaded yet?

[zino, 2017-03-22 18:29:12]

jinho.bang@samsung.com changed reviewers:
+ avi@chromium.org

[zino, 2017-03-22 18:30:09]

On 2017/03/22 18:19:09, ಠ_ಠ wrote:
> On 2017/03/22 18:04:45, zino wrote:
> > rouslan@, I addressed your comments
> 
> Looks like the fixes have not been uploaded yet?

Ah, sorry.. my mistake.

PTAL patch set 6.

Thanks!

[zino, 2017-03-22 18:31:14]

+Avi@ for non-payment part in content/

[zino, 2017-03-22 18:31:34]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 18:32:17]

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-22 18:47:00]

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

[commit-bot: I haz the power, 2017-03-22 18:47:00]

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

[zino, 2017-03-22 19:22:52]

Patchset #7 (id:120001) has been deleted

[zino, 2017-03-22 19:23:19]

The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 19:24:21]

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-22 19:55:08]

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

[commit-bot: I haz the power, 2017-03-22 19:55:09]

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

[please use gerrit instead, 2017-03-24 17:01:30]

lgtm

[Avi (use Gerrit), 2017-03-24 17:13:41]

lgtm

[zino, 2017-03-27 15:27:05]

Tom Sepez@ PTAL

I left a comment.

If we need to add length limits, please let me know how much length limits are
appropriate.

Thank you.

https://codereview.chromium.org/2718013004/diff/60001/components/payments/con...
File components/payments/content/payment_app.mojom (right):

https://codereview.chromium.org/2718013004/diff/60001/components/payments/con...
components/payments/content/payment_app.mojom:47: string method_name;
On 2017/03/07 17:22:39, Tom Sepez wrote:
> What are the restrictions on the values of these two fields?
> These are only processed in a renderer?

As I know, there are no restrictions in spec side.
The spec says that "Payment method security is outside the scope of this
specification and is addressed by payment handlers that support those payment
methods.".

The spec concept is providing a web based payment app using JS with service
worker.
So, we can not sure that the payment app provider what they want. So, it is
difficult to restrict the values of the method and details explicitly.

I think we can add length limits for them. Do we need them in mojo?

[zino, 2017-03-28 13:51:12]

Tom Sepez@, gentle ping

[zino, 2017-03-29 15:59:26]

On 2017/03/28 13:51:12, zino wrote:
> Tom Sepez@, gentle ping

Tom Sepez@, Could you please review this?

[Tom Sepez, 2017-03-29 20:33:16]

lgtm

[zino, 2017-03-29 23:56:09]

jinho.bang@samsung.com changed reviewers:
+ tedchoc@chromium.org

[zino, 2017-03-30 00:00:04]

+Ted C@ for chrome directory.

PTAL

(Other reviwers, I'm sorry for spam..)

[Ted C, 2017-03-30 00:17:47]

On 2017/03/30 00:00:04, zino wrote:
> +Ted C@ for chrome directory.
> 
> PTAL
> 
> (Other reviwers, I'm sorry for spam..)

chrome lgtm

[zino, 2017-03-30 00:18:57]

The CQ bit was checked by jinho.bang@samsung.com

[zino, 2017-03-30 00:18:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, avi@chromium.org, rouslan@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2718013004/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-30 00:20:39]

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-30 02:22:58]

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

[commit-bot: I haz the power, 2017-03-30 02:23:00]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't
started yet, builder probably lacks capacity)

[zino, 2017-03-30 02:57:57]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2017-03-30 02:58:26]

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-30 03:04:40]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490842677323640,
"parent_rev": "000981803bc1d1d56f6455b231f30c72724ffa00", "commit_rev":
"865ce0550e0ba864c2a830bde1f7d74004edca9b"}

[commit-bot: I haz the power, 2017-03-30 03:06:38]

Description was changed from

==========
PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side)

The respondWith() method is used by the payment app to provide a
PaymentAppResponse when the payment successfully completes.

This CL includes a content-side implementation and browser test. Especially, add
a callback to InvokePaymentApp() in PaymentAppProvider. The callback is used to
pass a response data from payment app using respondWith().

Related Spec Link:
 
https://w3c.github.io/webpayments-payment-apps-api/#idl-def-paymentrequestevent

See the other CLs in this series:
  [1/3] https://codereview.chromium.org/2715663002/ (RespondWithObserver)
  [2/3] https://codereview.chromium.org/2705293010/ (blink side)
  [3/3] This patch

BUG=661608
TEST=payment_app_browsertest.cc
==========

to

==========
PaymentApp: Implement respondWith() in PaymentRequestEvent. (content side)

The respondWith() method is used by the payment app to provide a
PaymentAppResponse when the payment successfully completes.

This CL includes a content-side implementation and browser test. Especially, add
a callback to InvokePaymentApp() in PaymentAppProvider. The callback is used to
pass a response data from payment app using respondWith().

Related Spec Link:
 
https://w3c.github.io/webpayments-payment-apps-api/#idl-def-paymentrequestevent

See the other CLs in this series:
  [1/3] https://codereview.chromium.org/2715663002/ (RespondWithObserver)
  [2/3] https://codereview.chromium.org/2705293010/ (blink side)
  [3/3] This patch

BUG=661608
TEST=payment_app_browsertest.cc

Review-Url: https://codereview.chromium.org/2718013004
Cr-Commit-Position: refs/heads/master@{#460634}
Committed:
https://chromium.googlesource.com/chromium/src/+/865ce0550e0ba864c2a830bde1f7...
==========

[commit-bot: I haz the power, 2017-03-30 03:06:46]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/865ce0550e0ba864c2a830bde1f7...