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

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

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

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] This patch
  [3/3] https://codereview.chromium.org/2718013004/ (content side and test)

BUG=661608
TEST=payment_app_browsertest.cc

Review-Url: https://codereview.chromium.org/2705293010
Cr-Commit-Position: refs/heads/master@{#457841}
Committed: https://chromium.googlesource.com/chromium/src/+/af737fca2a12ce091d13c99cfec05d159af0748c

[zino, 2017-03-07 15:15:47]

Description was changed from

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

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

to

==========
PaymentApp: Implement respondWith() in PaymentRequestEvent. (blink 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:18:55]

Description was changed from

==========
PaymentApp: Implement respondWith() in PaymentRequestEvent. (blink 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. (blink side)

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

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] This patch
  [3/3] https://codereview.chromium.org/2718013004/ (content side and test)

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

[zino, 2017-03-07 15:30:45]

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

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

PTAL

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

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

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

drive-by comments:

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.h
(right):

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.h:20:
class MODULES_EXPORT PaymentRequestRespondWithObserver
I feel this class could be placed under modules/payments/ rather than
serviceworkers/ because it's used only for PaymentRequestEvent.

[nhiroki, 2017-03-08 02:28:16]

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.cpp:45:
default:
I'd recommend not to use 'default' for WebServiceWorkerResponseError because
it's used from some APIs and this code could silently get broken if one of them
adds a very common error type.

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.cpp:100:
DummyExceptionStateForTesting exceptionState;
Using a dummy exception state looks strange. I don't know the proper way, but
maybe you can get/create it from ExecutionContext/ScriptState...?

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.h
(right):

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.h:18:
// This class observes the service worker's handling of a PaymentReqestEvent and
s/PaymentReqestEvent/PaymentRequestEvent/

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.h:20:
class MODULES_EXPORT PaymentRequestRespondWithObserver
final?

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.h:35:
protected:
private?

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:344:
ScriptState::Scope scope(
You enter the scope because toPaymentAppRequestData() internally uses
ScriptState, right? Can you add a comment about it here so that future readers
can easily understand it?

[haraken, 2017-03-08 09:28:06]

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:344:
ScriptState::Scope scope(
On 2017/03/08 02:28:16, nhiroki (slow) wrote:
> You enter the scope because toPaymentAppRequestData() internally uses
> ScriptState, right? Can you add a comment about it here so that future readers
> can easily understand it?

Alternately, how about making toPaymentAppRequest enter the ScriptState?

[zino, 2017-03-10 17:57:52]

Thank you for input and reviews!

I addressed your comments. PTAL.

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.cpp:45:
default:
On 2017/03/08 02:28:16, nhiroki (ooo until Mar 14) wrote:
> I'd recommend not to use 'default' for WebServiceWorkerResponseError because
> it's used from some APIs and this code could silently get broken if one of
them
> adds a very common error type.

Done.

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.cpp:100:
DummyExceptionStateForTesting exceptionState;
On 2017/03/08 02:28:16, nhiroki (ooo until Mar 13) wrote:
> Using a dummy exception state looks strange. I don't know the proper way, but
> maybe you can get/create it from ExecutionContext/ScriptState...?

Done.

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.h
(right):

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.h:18:
// This class observes the service worker's handling of a PaymentReqestEvent and
On 2017/03/08 02:28:16, nhiroki (ooo until Mar 14) wrote:
> s/PaymentReqestEvent/PaymentRequestEvent/

Done.

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.h:20:
class MODULES_EXPORT PaymentRequestRespondWithObserver
On 2017/03/08 01:39:25, shimazu (OOO till Mar 20) wrote:
> I feel this class could be placed under modules/payments/ rather than
> serviceworkers/ because it's used only for PaymentRequestEvent.

Done.

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/PaymentRequestRespondWithObserver.h:35:
protected:
On 2017/03/08 02:28:16, nhiroki (ooo until Mar 14) wrote:
> private?

Done.

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2705293010/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:344:
ScriptState::Scope scope(
On 2017/03/08 09:28:06, haraken wrote:
> On 2017/03/08 02:28:16, nhiroki (slow) wrote:
> > You enter the scope because toPaymentAppRequestData() internally uses
> > ScriptState, right? Can you add a comment about it here so that future
readers
> > can easily understand it?
> 
> Alternately, how about making toPaymentAppRequest enter the ScriptState?

Done.

[zino, 2017-03-14 14:11:23]

Ping nhiroki@ and haraken@

[haraken, 2017-03-14 16:15:52]

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentAppRequestConversion.cpp
(right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppRequestConversion.cpp:90:
if (!scriptState || !scriptState->contextIsValid())

scriptState should not be null.

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:77:
getExecutionContext()->addConsoleMessage(ConsoleMessage::create(

getExecutionContext() returns nullptr after the context gets detached.

What should we do in that case?

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:86:
const ScriptValue& value) {

Who calls onResponseFulfilled?

onResponseFulfilled executes some V8 stuff so we'll need to enter a ScriptState.
Or have we already entered the ScriptState on the caller side?

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:94:
exceptionState.clearException();

Why is it okay to ignore the exception?

[nhiroki, 2017-03-16 13:24:47]

Sorry for my delayed review. LGTM from ServiceWorker's point of view.

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:35: if
(m_observer) {
Is it possible that |m_observer| is null here?

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:100:
webData.methodName = WebString(AtomicString(response.methodName()));
webData.methodName = response.methodName(); could work.

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:110:
v8StringToWebCoreString<String>(detailsValue, DoNotExternalize)));
webData.stringifiedDetails = toCoreString(detailsValue); could work.

[zino, 2017-03-16 13:47:31]

Thank you for review.

I left some comments. haraken@

Thanks :)

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentAppRequestConversion.cpp
(right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppRequestConversion.cpp:90:
if (!scriptState || !scriptState->contextIsValid())
Sorry, I don't understand exactly.
Did you mean that I have to use DCHECK instead of null check?
Or is it unncessary to check whether it is null?

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:77:
getExecutionContext()->addConsoleMessage(ConsoleMessage::create(
On 2017/03/14 16:15:51, haraken wrote:
> 
> getExecutionContext() returns nullptr after the context gets detached.
> 
> What should we do in that case?

The PaymentRequestRespondWithObserver is derived from RespondWithObserver
abstract class[1].
So, if the context is gone, this function will not be called.
In other words, the getExecutionContext() will be always non-null in this
function.
Therefore, I'll add a DCHECK in the next patch set.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:86:
const ScriptValue& value) {
On 2017/03/14 16:15:51, haraken wrote:
> 
> Who calls onResponseFulfilled?
> 
> onResponseFulfilled executes some V8 stuff so we'll need to enter a
ScriptState.
> Or have we already entered the ScriptState on the caller side?
> 

The scriptState is coming from PaymentRequestEvent.idl in this patch [1].
Therefore, I didn't think we need to enter a ScriptState. [2]

[1] https://codereview.chromium.org/2705293010/patch/40001/50007
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/....

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:94:
exceptionState.clearException();
On 2017/03/14 16:15:51, haraken wrote:
> 
> Why is it okay to ignore the exception?

I hoped to use the same pattern with existing code style.
The onResponseRejected() takes WebServiceWorkerResponseError type instead of
string or other values.
IMHO, we don't need to make a new function to take other values or string.

The pattern is already used in ForeignFetchRespondWithObserver.cpp [1].

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...

[zino, 2017-03-16 14:24:22]

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

[commit-bot: I haz the power, 2017-03-16 14:24:32]

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

[zino, 2017-03-16 14:32:03]

Thank you for review nhiroki@.

PTAL

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:35: if
(m_observer) {
On 2017/03/16 13:24:47, nhiroki (slow) wrote:
> Is it possible that |m_observer| is null here?

In the extream case, nullptr might be coming when creating PaymentRequestEvent.
The ExtendableEvent[1] and the FetchEvent[2] are already checked whether it is
null.
What do you think? (If you don't want it, I'll remove this)

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:100:
webData.methodName = WebString(AtomicString(response.methodName()));
On 2017/03/16 13:24:47, nhiroki (slow) wrote:
> webData.methodName = response.methodName(); could work.

Done.

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:110:
v8StringToWebCoreString<String>(detailsValue, DoNotExternalize)));
On 2017/03/16 13:24:47, nhiroki (slow) wrote:
> webData.stringifiedDetails = toCoreString(detailsValue); could work.

Done.

[haraken, 2017-03-16 15:16:52]

nhiroki: Would there be any way to reduce code duplication with
FetchRespondWithObserver?

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentAppRequestConversion.cpp
(right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppRequestConversion.cpp:90:
if (!scriptState || !scriptState->contextIsValid())
On 2017/03/16 13:47:31, zino wrote:
> Sorry, I don't understand exactly.
> Did you mean that I have to use DCHECK instead of null check?
> Or is it unncessary to check whether it is null?

Yeah, you can use DCHECK.

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:23:
const String getMessageForResponseError(WebServiceWorkerResponseError error) {

Would there be any way to avoid code duplication with FetchRespondWithObserver?

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:77:
getExecutionContext()->addConsoleMessage(ConsoleMessage::create(
On 2017/03/16 13:47:31, zino wrote:
> On 2017/03/14 16:15:51, haraken wrote:
> > 
> > getExecutionContext() returns nullptr after the context gets detached.
> > 
> > What should we do in that case?
> 
> The PaymentRequestRespondWithObserver is derived from RespondWithObserver
> abstract class[1].
> So, if the context is gone, this function will not be called.
> In other words, the getExecutionContext() will be always non-null in this
> function.
> Therefore, I'll add a DCHECK in the next patch set.
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...

Makes sense. You're adding a bunch of getExecutionContext()'s to many methods in
this file, but are all the methods guaranteed not to be called after the context
detachment?

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:94:
exceptionState.clearException();
On 2017/03/16 13:47:31, zino wrote:
> On 2017/03/14 16:15:51, haraken wrote:
> > 
> > Why is it okay to ignore the exception?
> 
> I hoped to use the same pattern with existing code style.
> The onResponseRejected() takes WebServiceWorkerResponseError type instead of
> string or other values.
> IMHO, we don't need to make a new function to take other values or string.
> 
> The pattern is already used in ForeignFetchRespondWithObserver.cpp [1].
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...

Makes sense.

[zino, 2017-03-16 16:34:19]

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentAppRequestConversion.cpp
(right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentAppRequestConversion.cpp:90:
if (!scriptState || !scriptState->contextIsValid())
On 2017/03/16 15:16:52, haraken wrote:
> On 2017/03/16 13:47:31, zino wrote:
> > Sorry, I don't understand exactly.
> > Did you mean that I have to use DCHECK instead of null check?
> > Or is it unncessary to check whether it is null?
> 
> Yeah, you can use DCHECK.

Done.

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:23:
const String getMessageForResponseError(WebServiceWorkerResponseError error) {
On 2017/03/16 15:16:52, haraken wrote:
> 
> Would there be any way to avoid code duplication with
FetchRespondWithObserver?
> 

It might be possible but the |request_url| will not display in the error message
in FetchEvent.
In the FetchRespondWithObserver, the getMessageForResponseError() function takes
|request_url| as well as |error|.
On the other hands, this getMessageForResponseError() only takes |error|.

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:77:
getExecutionContext()->addConsoleMessage(ConsoleMessage::create(
On 2017/03/16 15:16:52, haraken wrote:
> On 2017/03/16 13:47:31, zino wrote:
> > On 2017/03/14 16:15:51, haraken wrote:
> > > 
> > > getExecutionContext() returns nullptr after the context gets detached.
> > > 
> > > What should we do in that case?
> > 
> > The PaymentRequestRespondWithObserver is derived from RespondWithObserver
> > abstract class[1].
> > So, if the context is gone, this function will not be called.
> > In other words, the getExecutionContext() will be always non-null in this
> > function.
> > Therefore, I'll add a DCHECK in the next patch set.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...
> 
> Makes sense. You're adding a bunch of getExecutionContext()'s to many methods
in
> this file, but are all the methods guaranteed not to be called after the
context
> detachment?

Yeah, before my patch, the RespondWithObserver::responseWasReject() uses
ASSERT(getExecutionContext()); (other places as well)
Please see this comment: https://codereview.chromium.org/571843003#msg16

[nhiroki, 2017-03-17 07:44:52]

On 2017/03/16 15:16:52, haraken wrote:
> nhiroki: Would there be any way to reduce code duplication with
> FetchRespondWithObserver?

I think this is already deduped enough because the patch [1/3] factored out the
common code into RespondWithObserver and almost all code in
PaymentRequestRespondWithObserver is now PaymentRequestAPI specific.

[nhiroki, 2017-03-17 07:45:12]

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp (right):

https://codereview.chromium.org/2705293010/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/payments/PaymentRequestEvent.cpp:35: if
(m_observer) {
On 2017/03/16 14:32:02, zino wrote:
> On 2017/03/16 13:24:47, nhiroki (slow) wrote:
> > Is it possible that |m_observer| is null here?
> 
> In the extream case, nullptr might be coming when creating
PaymentRequestEvent.
> The ExtendableEvent[1] and the FetchEvent[2] are already checked whether it is
> null.
> What do you think? (If you don't want it, I'll remove this)
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...
> [2]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/servic...

Thank you for the clarification. I'm ok with keeping this check for consistency.
If necessary, we can remove them all later.

[haraken, 2017-03-17 11:55:57]

LGTM

https://codereview.chromium.org/2705293010/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2705293010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:92:
V8PaymentAppResponse::toImpl(toIsolate(getExecutionContext()),

Can we use ScriptValue<PaymentAppResponse>::to(...) just like what
ForeignFetchRespondWithObserver.cpp is doing?

[haraken, 2017-03-17 11:56:14]

Sorry about the review delay.

[please use gerrit instead, 2017-03-17 12:43:57]

payments lgtm

[zino, 2017-03-17 15:22:02]

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

[commit-bot: I haz the power, 2017-03-17 15:22: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-17 16:29:30]

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

[commit-bot: I haz the power, 2017-03-17 16:29:31]

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

[zino, 2017-03-17 17:03:09]

Thank you for review!

https://codereview.chromium.org/2705293010/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp
(right):

https://codereview.chromium.org/2705293010/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp:92:
V8PaymentAppResponse::toImpl(toIsolate(getExecutionContext()),
On 2017/03/17 11:55:57, haraken wrote:
> 
> Can we use ScriptValue<PaymentAppResponse>::to(...) just like what
> ForeignFetchRespondWithObserver.cpp is doing?

Done.

[zino, 2017-03-17 17:03:19]

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

[zino, 2017-03-17 17:03:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, haraken@chromium.org, rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2705293010/#ps120001
(title: "PaymentApp: Implement respondWith() in PaymentRequestEvent. (blink
side)")

[commit-bot: I haz the power, 2017-03-17 17:03:50]

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-17 19:11:15]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489770199604450,
"parent_rev": "b448f058eaaad3baa7ad402d45885fb867e65c94", "commit_rev":
"af737fca2a12ce091d13c99cfec05d159af0748c"}

[commit-bot: I haz the power, 2017-03-17 19:12:04]

Description was changed from

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

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

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] This patch
  [3/3] https://codereview.chromium.org/2718013004/ (content side and test)

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

to

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

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

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] This patch
  [3/3] https://codereview.chromium.org/2718013004/ (content side and test)

BUG=661608
TEST=payment_app_browsertest.cc

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

[commit-bot: I haz the power, 2017-03-17 19:12:07]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/af737fca2a12ce091d13c99cfec0...