PaymentApp: Implement invokePaymentApp() in renderer side. (issue 2646313002 by jinho.bang@samsung.com)

[jinho...@samsung.com]

Reviewers: nhiroki, Tom Sepez, rouslan, tommyt
CL: https://codereview.chromium.org/2646313002/

Message:
PTAL

nhiroki@ for service worker.
Tom Sepez@ for type converter. (Currently we can not use typemap until move SW
into Blink)
rouslan@ and tommyt@ for payments.

Description:
PaymentApp: Implement invokePaymentApp() in renderer side.

This is the final patch of a series of three that implements invokePaymentApp().
- 1/3: https://codereview.chromium.org/2610163002/ (browser side)
- 2/3: https://codereview.chromium.org/2647243002/ (layout test helper)
- 3/3: This patch (renderer side)

After this patch, it is possible to invoke a selected payment app if calling
the invokePaymentApp() method in PaymentRequest UI implementation. This CL also
includes a test to verify such behavior.

BUG=661608
TEST=payment-app-invocation.html

Affected files (+282, -2 lines):
M content/common/DEPS
M content/common/service_worker/service_worker_type_converters.h
M content/common/service_worker/service_worker_type_converters.cc
M content/renderer/service_worker/service_worker_context_client.h
M content/renderer/service_worker/service_worker_context_client.cc
A third_party/WebKit/LayoutTests/http/tests/payments/chromium/payment-app-invocation.html
A third_party/WebKit/LayoutTests/http/tests/payments/chromium/resources/payment-app.js
A third_party/WebKit/LayoutTests/http/tests/payments/chromium/resources/payment-app-window.html
M third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp

[tom...@opera.com]

Looking good! Thanks Jinho :)


https://codereview.chromium.org/2646313002/diff/1/content/common/service_worker/service_worker_type_converters.cc
File content/common/service_worker/service_worker_type_converters.cc
(right):

https://codereview.chromium.org/2646313002/diff/1/content/common/service_worker/service_worker_type_converters.cc#newcode97
content/common/service_worker/service_worker_type_converters.cc:97:
output.currencySystem = blink::WebString::fromUTF8(input->value);
input->currency_system is an optional value, so this line should
probably be replaced with something like this:

if (input->currency_system) {
output.currencySystem =
blink::WebString::fromUTF8(*input->currency_system);
}

https://codereview.chromium.org/2646313002/

[nhi...@chromium.org]

lgtm




https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp
File
third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp
(right):

https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp#newcode89
third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:89:
// When handling a notificationclick event, we want to allow one window
to
Can you update this comment?

https://codereview.chromium.org/2646313002/

[tom...@opera.com]

https://codereview.chromium.org/2646313002/diff/1/content/renderer/service_worker/service_worker_context_client.cc
File content/renderer/service_worker/service_worker_context_client.cc
(right):

https://codereview.chromium.org/2646313002/diff/1/content/renderer/service_worker/service_worker_context_client.cc#newcode890
content/renderer/service_worker/service_worker_context_client.cc:890:
int request_id = context_->payment_request_event_callbacks.Add(
I think it might be better to do the following here:

context_->payment_request_event_callbacks.AddWithID(
base::MakeUnique<PaymentRequestEventCallback>(callback),
event_id);

(and replace "request_id" with "event_id" below).

The reason is that later on we are going to want to receive a response
back from the service worker's event handler (when the script resolves
the promise from respondWith). This depends, of course, on how you want
to implement the response handling, but in my own experiment, I
implemented it as a call to active_version->RegisterRequestCallback()
from payment_app_provider_impl.cc:DispatchPaymentRequestEvent(). If you
want to do it this way, too, then you'll want to pass the same
request_id to ServiceWorkerVersion::RegisterRequestCallback() as you
pass to ServiceWorkerGlobalScopeProxy::dispatchPaymentRequestEvent().

https://codereview.chromium.org/2646313002/

[tse...@chromium.org]

Type converters LGTM after fixing tommyt's comment.

https://codereview.chromium.org/2646313002/

[rou...@chromium.org]

lgtm

https://codereview.chromium.org/2646313002/

[fal...@chromium.org]

Some questions. It's a bit difficult to follow the flow of the test.


https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/LayoutTests/http/tests/payments/chromium/payment-app-invocation.html
File
third_party/WebKit/LayoutTests/http/tests/payments/chromium/payment-app-invocation.html
(right):

https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/LayoutTests/http/tests/payments/chromium/payment-app-invocation.html#newcode1
third_party/WebKit/LayoutTests/http/tests/payments/chromium/payment-app-invocation.html:1:
<!doctype html>
Can you put a comment here about why is this test in chromium/, which
means it's not intended to be upstreamed to WPT? I guess it's for the
testRunner stuff.

It's unfortunate if this test needs testRunner. Is there any way to
write a browser-independent WPT test for this feature?

https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/LayoutTests/http/tests/payments/chromium/payment-app-invocation.html#newcode26
third_party/WebKit/LayoutTests/http/tests/payments/chromium/payment-app-invocation.html:26:
assert_equals(state, 'activated');
This assert just tests that the helper function is working. It's not
really an assert of the browser implementation. A wait_for_state that
does "return Promise.resolve('activated')" would still pass this assert.

To test that at this point service worker is activated, you can do
assert_equals(service_worker.state, 'activated').

https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/LayoutTests/http/tests/payments/chromium/payment-app-invocation.html#newcode43
third_party/WebKit/LayoutTests/http/tests/payments/chromium/payment-app-invocation.html:43:
return new Promise(resolve => { port.onmessage = resolve; });
If you can, avoid MessageChannel. Just do service_worker.postMessage().
Then the service worker does event.source.postMessage().

The early tests used MessageChannel() since this wasn't yet implemented.

https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/LayoutTests/http/tests/payments/chromium/payment-app-invocation.html#newcode58
third_party/WebKit/LayoutTests/http/tests/payments/chromium/payment-app-invocation.html:58:
.catch(unreached_rejection(test));
This catch isn't needed. Just return the entire promise chain, and the
test harness will error if a rejection/exception happens.

https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/LayoutTests/http/tests/payments/chromium/resources/payment-app.js
File
third_party/WebKit/LayoutTests/http/tests/payments/chromium/resources/payment-app.js
(right):

https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/LayoutTests/http/tests/payments/chromium/resources/payment-app.js#newcode9
third_party/WebKit/LayoutTests/http/tests/payments/chromium/resources/payment-app.js:9:
// If the message event is initiated from tester, ignores this event.
What is "tester", and why is it sending a message which is ignored?

https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/LayoutTests/http/tests/payments/chromium/resources/payment-app.js#newcode24
third_party/WebKit/LayoutTests/http/tests/payments/chromium/resources/payment-app.js:24:
self.removeEventListener(listener);
nit: Having multiple message handlers seems more complicated than
needed. Would it be clearer if there is just one message handler that
does the intended thing based on the data in the message?

https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/LayoutTests/http/tests/payments/chromium/resources/payment-app.js#newcode38
third_party/WebKit/LayoutTests/http/tests/payments/chromium/resources/payment-app.js:38:
if (message == 'payment_app_window_ready')
What other message could arrive here? Could this be an assert instead?

https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/LayoutTests/http/tests/payments/chromium/resources/payment-app.js#newcode43
third_party/WebKit/LayoutTests/http/tests/payments/chromium/resources/payment-app.js:43:
if (message == 'payment_app_response') {
Same.

https://codereview.chromium.org/2646313002/

[jinho...@samsung.com]

This issue is pending due to https://codereview.chromium.org/2647243002/.
If the issue is resolved, I'll restart this work again :)

Thank you for review.

https://codereview.chromium.org/2646313002/

[jinho...@samsung.com]

PTAL owners,

I don't want to defer this CL anymore.
So, I wrote a new browser test that can cover JS side instead of layout test.

falken@,
The payment app API requires user interaction like push notification example.
So, we had to use testRunner but the testRunner CL(in my previous comment) is
still pending. So, I wrote a new test for this CL by using browser test.

https://codereview.chromium.org/2646313002/diff/1/content/common/service_worker/service_worker_type_converters.cc
File content/common/service_worker/service_worker_type_converters.cc
(right):

https://codereview.chromium.org/2646313002/diff/1/content/common/service_worker/service_worker_type_converters.cc#newcode97
content/common/service_worker/service_worker_type_converters.cc:97:
output.currencySystem = blink::WebString::fromUTF8(input->value);

As you know, this is already resolved in the following CL:
https://codereview.chromium.org/2649143003/

Thanks!

https://codereview.chromium.org/2646313002/diff/1/content/renderer/service_worker/service_worker_context_client.cc
File content/renderer/service_worker/service_worker_context_client.cc
(right):

https://codereview.chromium.org/2646313002/diff/1/content/renderer/service_worker/service_worker_context_client.cc#newcode890
content/renderer/service_worker/service_worker_context_client.cc:890:
int request_id = context_->payment_request_event_callbacks.Add(

On 2017/01/23 12:54:30, tommyt wrote:
> I think it might be better to do the following here:
>
> context_->payment_request_event_callbacks.AddWithID(
> base::MakeUnique<PaymentRequestEventCallback>(callback),
event_id);
>
> (and replace "request_id" with "event_id" below).
>
> The reason is that later on we are going to want to receive a response
back from
> the service worker's event handler (when the script resolves the
promise from
> respondWith). This depends, of course, on how you want to implement
the response
> handling, but in my own experiment, I implemented it as a call to
> active_version->RegisterRequestCallback() from
> payment_app_provider_impl.cc:DispatchPaymentRequestEvent(). If you
want to do it
> this way, too, then you'll want to pass the same request_id to
> ServiceWorkerVersion::RegisterRequestCallback() as you pass to
> ServiceWorkerGlobalScopeProxy::dispatchPaymentRequestEvent().

Good point. Yes, we might have to do that later.
But I don't want to add it in this patch because the respondWith
function is not implemented yet.

IMHO, the addWithID() is used to explicitly specify the ID generated
from browser side with the same value.
Because the RegisterRequestCallback() will use the request event id to
call its callback.
The RegisterRequestCallback() is using legacy IPC. So, it's not a right
way.
Moreover, we should call the startRequest() function two times in
browser side in order to use RegisterRequestCallback().
because we have to consider waitUntil as well as respondWith. It's not
goal in this CL.

When implementing respondWith() function (in follow-up CL), then we can
use the addWithID if we really need it.

https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp
File
third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp
(right):

https://codereview.chromium.org/2646313002/diff/1/third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp#newcode89
third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.cpp:89:
// When handling a notificationclick event, we want to allow one window
to

On 2017/01/23 10:10:16, nhiroki (slow) wrote:
> Can you update this comment?

Done.

https://codereview.chromium.org/2646313002/

[tom...@opera.com]

https://codereview.chromium.org/2646313002/diff/1/content/common/service_worker/service_worker_type_converters.cc
File content/common/service_worker/service_worker_type_converters.cc
(right):

https://codereview.chromium.org/2646313002/diff/1/content/common/service_worker/service_worker_type_converters.cc#newcode97
content/common/service_worker/service_worker_type_converters.cc:97:
output.currencySystem = blink::WebString::fromUTF8(input->value);

On 2017/02/01 09:24:48, zino wrote:
> As you know, this is already resolved in the following CL:
> https://codereview.chromium.org/2649143003/
>
> Thanks!

Good, but you still need to change "input->value" to
"input->currency_system" on this line

https://codereview.chromium.org/2646313002/diff/1/content/renderer/service_worker/service_worker_context_client.cc
File content/renderer/service_worker/service_worker_context_client.cc
(right):

https://codereview.chromium.org/2646313002/diff/1/content/renderer/service_worker/service_worker_context_client.cc#newcode890
content/renderer/service_worker/service_worker_context_client.cc:890:
int request_id = context_->payment_request_event_callbacks.Add(

I agree. Let's leave this for later.

https://codereview.chromium.org/2646313002/

[jinho...@samsung.com]

tommyt@, Thank you for review.

Tom Sepez@,
I modified OWNERS file additionally because the presubmit error happens as
follows.

***************
content/common/service_worker/OWNERS is missing the following lines:

per-file *_type_converter*.*=set noparent
per-file *_type_converter*.*=file://ipc/SECURITY_OWNERS

for changed files:
content/common/service_worker/service_worker_event_dispatcher.mojom
content/common/service_worker/service_worker_type_converters.cc
content/common/service_worker/service_worker_type_converters.h
***************

https://codereview.chromium.org/2646313002/diff/1/content/common/service_worker/service_worker_type_converters.cc
File content/common/service_worker/service_worker_type_converters.cc
(right):

https://codereview.chromium.org/2646313002/diff/1/content/common/service_worker/service_worker_type_converters.cc#newcode97
content/common/service_worker/service_worker_type_converters.cc:97:
output.currencySystem = blink::WebString::fromUTF8(input->value);

On 2017/02/01 09:42:00, tommyt wrote:
> On 2017/02/01 09:24:48, zino wrote:
> > As you know, this is already resolved in the following CL:
> > https://codereview.chromium.org/2649143003/
> >
> > Thanks!
>
> Good, but you still need to change "input->value" to
"input->currency_system" on
> this line

Done.

Great review!

https://codereview.chromium.org/2646313002/

[tom...@opera.com]

lgtm

https://codereview.chromium.org/2646313002/

[jinho...@samsung.com]

falken@, Gentle Ping.

https://codereview.chromium.org/2646313002/

[fal...@chromium.org]

service_worker lgtm
the comments below are non OWNER comments


https://codereview.chromium.org/2646313002/diff/80001/content/browser/payments/payment_app_browsertest.cc
File content/browser/payments/payment_app_browsertest.cc (right):

https://codereview.chromium.org/2646313002/diff/80001/content/browser/payments/payment_app_browsertest.cc#newcode104
content/browser/payments/payment_app_browsertest.cc:104:
IN_PROC_BROWSER_TEST_F(PaymentAppBrowserTest, SW) {
nit: Does "SW" just mean "ServiceWorker"? Can you make the test name
more descriptive? If the Payment API is already based on service worker,
some name other than "ServiceWorker" could be better.

https://codereview.chromium.org/2646313002/diff/80001/content/test/data/result_queue.js
File content/test/data/result_queue.js (right):

https://codereview.chromium.org/2646313002/diff/80001/content/test/data/result_queue.js#newcode1
content/test/data/result_queue.js:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
nit: 2017?

https://codereview.chromium.org/2646313002/diff/80001/content/test/data/result_queue.js#newcode1
content/test/data/result_queue.js:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
Did you intend for this file to be in content/test/data rather than
content/test/data/payments like the other test files?

https://codereview.chromium.org/2646313002/

[jinho...@samsung.com]

Thank you for review.

https://codereview.chromium.org/2646313002/diff/80001/content/browser/payments/payment_app_browsertest.cc
File content/browser/payments/payment_app_browsertest.cc (right):

https://codereview.chromium.org/2646313002/diff/80001/content/browser/payments/payment_app_browsertest.cc#newcode104
content/browser/payments/payment_app_browsertest.cc:104:
IN_PROC_BROWSER_TEST_F(PaymentAppBrowserTest, SW) {

On 2017/02/04 22:02:10, falken wrote:
> nit: Does "SW" just mean "ServiceWorker"? Can you make the test name
more
> descriptive? If the Payment API is already based on service worker,
some name
> other than "ServiceWorker" could be better.

Done.

https://codereview.chromium.org/2646313002/diff/80001/content/test/data/result_queue.js
File content/test/data/result_queue.js (right):

https://codereview.chromium.org/2646313002/diff/80001/content/test/data/result_queue.js#newcode1
content/test/data/result_queue.js:1: // Copyright 2016 The Chromium
Authors. All rights reserved.

On 2017/02/04 22:02:10, falken wrote:
> Did you intend for this file to be in content/test/data rather than
> content/test/data/payments like the other test files?

Done.

https://codereview.chromium.org/2646313002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2646313002/100001


https://codereview.chromium.org/2646313002/

[commit-bot@chromium.org via codereview.chromium.org]

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_presubmit/builds/358578)

https://codereview.chromium.org/2646313002/

[jinho...@samsung.com]

+Avi@ for content/common/DEPS

PTAL

https://codereview.chromium.org/2646313002/

[a...@chromium.org]

On 2017/02/07 18:10:16, zino wrote:
> +Avi@ for content/common/DEPS
>
> PTAL

DEPS LGTM.

https://codereview.chromium.org/2646313002/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c4545cd9e65fd3a2276e1676ed57c85d1bb7631a

https://codereview.chromium.org/2646313002/