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

third_party/WebKit/Source/modules/payments/PaymentRequestRespondWithObserver.cpp

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "modules/payments/PaymentRequestRespondWithObserver.h"
+
+#include <v8.h>
+#include "bindings/core/v8/ScriptValue.h"
+#include "bindings/core/v8/V8Binding.h"
+#include "bindings/modules/v8/V8PaymentAppResponse.h"
+#include "core/dom/ExecutionContext.h"
+#include "core/inspector/ConsoleMessage.h"
+#include "modules/payments/PaymentAppResponse.h"
+#include "modules/serviceworkers/ServiceWorkerGlobalScopeClient.h"
+#include "modules/serviceworkers/WaitUntilObserver.h"
+#include "public/platform/modules/payments/WebPaymentAppResponse.h"
+
+namespace blink {
+namespace {
+
+// Returns the error message to let the developer know about the reason of the
+// unusual failures.
+const String getMessageForResponseError(WebServiceWorkerResponseError error) {

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

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

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

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

+  String errorMessage =
+      "The PaymentRequestEvent resulted in a network error response: ";
+  switch (error) {
+    case WebServiceWorkerResponseErrorPromiseRejected:
+      errorMessage = errorMessage + "the promise was rejected.";
+      break;
+    case WebServiceWorkerResponseErrorDefaultPrevented:
+      errorMessage =
+          errorMessage +
+          "preventDefault() was called without calling respondWith().";
+      break;
+    case WebServiceWorkerResponseErrorNoV8Instance:
+      errorMessage = errorMessage +
+                     "an object that was not a PaymentResponse was passed to "
+                     "respondWith().";
+      break;
+    case WebServiceWorkerResponseErrorResponseTypeError:
+      errorMessage = errorMessage +
+                     "the promise was resolved with an error response object.";
+      break;
+    case WebServiceWorkerResponseErrorUnknown:
+      errorMessage = errorMessage + "an unexpected error occurred.";
+      break;
+    case WebServiceWorkerResponseErrorResponseTypeOpaque:
+    case WebServiceWorkerResponseErrorResponseTypeNotBasicOrDefault:
+    case WebServiceWorkerResponseErrorBodyUsed:
+    case WebServiceWorkerResponseErrorResponseTypeOpaqueForClientRequest:
+    case WebServiceWorkerResponseErrorResponseTypeOpaqueRedirect:
+    case WebServiceWorkerResponseErrorBodyLocked:
+    case WebServiceWorkerResponseErrorNoForeignFetchResponse:
+    case WebServiceWorkerResponseErrorForeignFetchHeadersWithoutOrigin:
+    case WebServiceWorkerResponseErrorForeignFetchMismatchedOrigin:
+    case WebServiceWorkerResponseErrorRedirectedResponseForNotFollowRequest:
+      NOTREACHED();
+      errorMessage = errorMessage + "an unexpected error occurred.";
+      break;
+  }
+  return errorMessage;
+}
+
+}  // namespace
+
+PaymentRequestRespondWithObserver::~PaymentRequestRespondWithObserver() {}
+
+PaymentRequestRespondWithObserver* PaymentRequestRespondWithObserver::create(
+    ExecutionContext* context,
+    int eventID,
+    WaitUntilObserver* observer) {
+  return new PaymentRequestRespondWithObserver(context, eventID, observer);
+}
+
+void PaymentRequestRespondWithObserver::onResponseRejected(
+    WebServiceWorkerResponseError error) {
+  getExecutionContext()->addConsoleMessage(ConsoleMessage::create(

[haraken, 2017/03/14 16:15:51]

getExecutionContext() returns nullptr after the context gets detached.

What should we do in that case?

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

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

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

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?

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

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

+      JSMessageSource, WarningMessageLevel, getMessageForResponseError(error)));
+
+  WebPaymentAppResponse webData;
+  ServiceWorkerGlobalScopeClient::from(getExecutionContext())
+      ->respondToPaymentRequestEvent(m_eventID, webData, m_eventDispatchTime);
+}
+
+void PaymentRequestRespondWithObserver::onResponseFulfilled(
+    const ScriptValue& value) {

[haraken, 2017/03/14 16:15:51]

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?

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

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

+  PaymentAppResponse response;
+  ExceptionState exceptionState(value.isolate(), ExceptionState::UnknownContext,
+                                "PaymentRequestEvent", "respondWith");
+  V8PaymentAppResponse::toImpl(toIsolate(getExecutionContext()),
+                               value.v8Value(), response, exceptionState);
+
+  if (exceptionState.hadException()) {
+    exceptionState.clearException();

[haraken, 2017/03/14 16:15:51]

Why is it okay to ignore the exception?

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

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

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

Makes sense.

+    onResponseRejected(WebServiceWorkerResponseErrorNoV8Instance);
+    return;
+  }
+
+  WebPaymentAppResponse webData;
+  webData.methodName = WebString(AtomicString(response.methodName()));

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

webData.methodName = response.methodName(); could work.

[zino, 2017/03/16 14:32:02]

Done.

+
+  v8::Local<v8::String> detailsValue;
+  if (!v8::JSON::Stringify(response.details().context(),
+                           response.details().v8Value().As<v8::Object>())
+           .ToLocal(&detailsValue)) {
+    onResponseRejected(WebServiceWorkerResponseErrorUnknown);
+    return;
+  }
+  webData.stringifiedDetails = WebString(AtomicString(
+      v8StringToWebCoreString<String>(detailsValue, DoNotExternalize)));

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

webData.stringifiedDetails = toCoreString(detailsValue); could work.

[zino, 2017/03/16 14:32:02]

Done.

+  ServiceWorkerGlobalScopeClient::from(getExecutionContext())
+      ->respondToPaymentRequestEvent(m_eventID, webData, m_eventDispatchTime);
+}
+
+void PaymentRequestRespondWithObserver::onNoResponse() {
+  ServiceWorkerGlobalScopeClient::from(getExecutionContext())
+      ->respondToPaymentRequestEvent(m_eventID, WebPaymentAppResponse(),
+                                     m_eventDispatchTime);
+}
+
+PaymentRequestRespondWithObserver::PaymentRequestRespondWithObserver(
+    ExecutionContext* context,
+    int eventID,
+    WaitUntilObserver* observer)
+    : RespondWithObserver(context, eventID, observer) {}
+
+DEFINE_TRACE(PaymentRequestRespondWithObserver) {
+  RespondWithObserver::trace(visitor);
+}
+
+}  // namespace blink