PaymentRequest: Use ScriptValue for details in PaymentResponse

This patch is initiated from this comment[1].
After this patch, the following small bugs are also fixed.
  - The PaymentResponse.details should always return same object.
  - If no details, should return empty object instead of undefined.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1226774/6/third_party/blink/renderer/modules/payments/payment_method_change_event.h#42

Bug: none
Change-Id: Ib784013fbdedd42d30eb820a82d51ec4d08550f4
Reviewed-on: https://chromium-review.googlesource.com/c/1253862
Commit-Queue: Jinho Bang <jinho.bang@samsung.com>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596143}

third_party/blink/renderer/modules/payments/payment_request.cc

@@ -1222,7 +1222,8 @@ void PaymentRequest::OnPaymentResponse(PaymentResponsePtr response) {
 
   if (retry_resolver_) {
     DCHECK(payment_response_);
-    payment_response_->Update(std::move(response), shipping_address_.Get());
+    payment_response_->Update(retry_resolver_->GetScriptState(),
+                              std::move(response), shipping_address_.Get());
     retry_resolver_->Resolve();
 
     // Do not close the mojo connection here. The merchant website should call
@@ -1230,9 +1231,9 @@ void PaymentRequest::OnPaymentResponse(PaymentResponsePtr response) {
     // connection to display a success or failure message to the user.
     retry_resolver_.Clear();
   } else if (accept_resolver_) {
-    payment_response_ =
-        new PaymentResponse(GetExecutionContext(), std::move(response),
-                            shipping_address_.Get(), this, id_);
+    payment_response_ = new PaymentResponse(accept_resolver_->GetScriptState(),
+                                            std::move(response),
+                                            shipping_address_.Get(), this, id_);
     accept_resolver_->Resolve(payment_response_);
 
     // Do not close the mojo connection here. The merchant website should call

third_party/blink/renderer/modules/payments/payment_response.cc

@@ -10,20 +10,20 @@
 #include "third_party/blink/renderer/modules/payments/payment_state_resolver.h"
 #include "third_party/blink/renderer/modules/payments/payment_validation_errors.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
+#include "third_party/blink/renderer/platform/bindings/script_state.h"
 #include "third_party/blink/renderer/platform/wtf/assertions.h"
 
 namespace blink {
 
 PaymentResponse::PaymentResponse(
-    ExecutionContext* execution_context,
+    ScriptState* script_state,
     payments::mojom::blink::PaymentResponsePtr response,
     PaymentAddress* shipping_address,
     PaymentStateResolver* payment_state_resolver,
-    const String& requestId)
-    : ContextLifecycleObserver(execution_context),
-      requestId_(requestId),
+    const String& request_id)
+    : ContextLifecycleObserver(ExecutionContext::From(script_state)),
+      request_id_(request_id),
       method_name_(response->method_name),
-      stringified_details_(response->stringified_details),
       shipping_address_(shipping_address),
       shipping_option_(response->shipping_option),
       payer_name_(response->payer->name),
@@ -31,22 +31,24 @@ PaymentResponse::PaymentResponse(
       payer_phone_(response->payer->phone),
       payment_state_resolver_(payment_state_resolver) {
   DCHECK(payment_state_resolver_);
+  UpdateDetailsFromJSON(script_state, response->stringified_details);
 }
 
 PaymentResponse::~PaymentResponse() = default;
 
 void PaymentResponse::Update(
+    ScriptState* script_state,
     payments::mojom::blink::PaymentResponsePtr response,
     PaymentAddress* shipping_address) {
   DCHECK(response);
   DCHECK(response->payer);
   method_name_ = response->method_name;
-  stringified_details_ = response->stringified_details;
   shipping_address_ = shipping_address;
   shipping_option_ = response->shipping_option;
   payer_name_ = response->payer->name;
   payer_email_ = response->payer->email;
   payer_phone_ = response->payer->phone;
+  UpdateDetailsFromJSON(script_state, response->stringified_details);
 }
 
 void PaymentResponse::UpdatePayerDetail(
@@ -57,11 +59,33 @@ void PaymentResponse::UpdatePayerDetail(
   payer_phone_ = detail->phone;
 }
 
+void PaymentResponse::UpdateDetailsFromJSON(ScriptState* script_state,
+                                            const String& json) {
+  ScriptState::Scope scope(script_state);
+  if (json.IsEmpty()) {
+    details_ = V8ObjectBuilder(script_state).GetScriptValue();
+    return;
+  }
+
+  ExceptionState exception_state(script_state->GetIsolate(),
+                                 ExceptionState::kConstructionContext,
+                                 "PaymentResponse");
+  v8::Local<v8::Value> parsed_value =
+      FromJSONString(script_state->GetIsolate(), script_state->GetContext(),
+                     json, exception_state);
+  if (exception_state.HadException()) {
+    exception_state.ClearException();
+    details_ = V8ObjectBuilder(script_state).GetScriptValue();
+    return;
+  }
+  details_ = ScriptValue(script_state, parsed_value);
+}
+
 ScriptValue PaymentResponse::toJSONForBinding(ScriptState* script_state) const {
   V8ObjectBuilder result(script_state);
   result.AddString("requestId", requestId());
   result.AddString("methodName", methodName());
-  result.Add("details", details(script_state, ASSERT_NO_EXCEPTION));
+  result.Add("details", details(script_state));
 
   if (shippingAddress())
     result.Add("shippingAddress",
@@ -77,12 +101,8 @@ ScriptValue PaymentResponse::toJSONForBinding(ScriptState* script_state) const {
   return result.GetScriptValue();
 }
 
-ScriptValue PaymentResponse::details(ScriptState* script_state,
-                                     ExceptionState& exception_state) const {
-  return ScriptValue(
-      script_state,
-      FromJSONString(script_state->GetIsolate(), script_state->GetContext(),
-                     stringified_details_, exception_state));
+ScriptValue PaymentResponse::details(ScriptState* script_state) const {
+  return ScriptValue(script_state, details_.V8ValueFor(script_state));
 }
 
 ScriptPromise PaymentResponse::complete(ScriptState* script_state,

third_party/blink/renderer/modules/payments/payment_response.h

@@ -20,7 +20,6 @@
 
 namespace blink {
 
-class ExceptionState;
 class PaymentAddress;
 class PaymentStateResolver;
 class PaymentValidationErrors;
@@ -35,21 +34,24 @@ class MODULES_EXPORT PaymentResponse final
   WTF_MAKE_NONCOPYABLE(PaymentResponse);
 
  public:
-  PaymentResponse(ExecutionContext*,
-                  payments::mojom::blink::PaymentResponsePtr,
-                  PaymentAddress* shipping_address_,
-                  PaymentStateResolver*,
-                  const String& requestId);
+  PaymentResponse(ScriptState* script_state,
+                  payments::mojom::blink::PaymentResponsePtr response,
+                  PaymentAddress* shipping_address,
+                  PaymentStateResolver* payment_state_resolver,
+                  const String& request_id);
   ~PaymentResponse() override;
 
-  void Update(payments::mojom::blink::PaymentResponsePtr, PaymentAddress*);
+  void Update(ScriptState* script_state,
+              payments::mojom::blink::PaymentResponsePtr response,
+              PaymentAddress* shipping_address);
   void UpdatePayerDetail(payments::mojom::blink::PayerDetailPtr);
+  void UpdateDetailsFromJSON(ScriptState* script_state, const String& json);
 
   ScriptValue toJSONForBinding(ScriptState*) const;
 
-  const String& requestId() const { return requestId_; }
+  const String& requestId() const { return request_id_; }
   const String& methodName() const { return method_name_; }
-  ScriptValue details(ScriptState*, ExceptionState&) const;
+  ScriptValue details(ScriptState* script_state) const;
   PaymentAddress* shippingAddress() const { return shipping_address_.Get(); }
   const String& shippingOption() const { return shipping_option_; }
   const String& payerName() const { return payer_name_; }
@@ -69,9 +71,9 @@ class MODULES_EXPORT PaymentResponse final
   void Trace(blink::Visitor*) override;
 
  private:
-  String requestId_;
+  String request_id_;
   String method_name_;
-  String stringified_details_;
+  ScriptValue details_;
   Member<PaymentAddress> shipping_address_;
   String shipping_option_;
   String payer_name_;

third_party/blink/renderer/modules/payments/payment_response.idl

@@ -22,7 +22,7 @@ enum PaymentComplete {
 
     readonly attribute DOMString requestId;
     readonly attribute DOMString methodName;
-    [CallWith=ScriptState, RaisesException] readonly attribute object details;
+    [CallWith=ScriptState] readonly attribute object details;
     readonly attribute PaymentAddress? shippingAddress;
     readonly attribute DOMString? shippingOption;
     readonly attribute DOMString? payerName;

third_party/blink/renderer/modules/payments/payment_response_test.cc

@@ -11,6 +11,7 @@
 #include "third_party/blink/renderer/bindings/core/v8/script_value.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_core.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_testing.h"
+#include "third_party/blink/renderer/bindings/core/v8/v8_object_builder.h"
 #include "third_party/blink/renderer/modules/payments/payment_address.h"
 #include "third_party/blink/renderer/modules/payments/payment_state_resolver.h"
 #include "third_party/blink/renderer/modules/payments/payment_test_helper.h"
@@ -59,8 +60,8 @@ TEST(PaymentResponseTest, DataCopiedOver) {
   MockPaymentStateResolver* complete_callback = new MockPaymentStateResolver;
 
   PaymentResponse* output =
-      new PaymentResponse(scope.GetExecutionContext(), std::move(input),
-                          nullptr, complete_callback, "id");
+      new PaymentResponse(scope.GetScriptState(), std::move(input), nullptr,
+                          complete_callback, "id");
 
   EXPECT_EQ("foo", output->methodName());
   EXPECT_EQ("standardShippingOption", output->shippingOption());
@@ -69,8 +70,7 @@ TEST(PaymentResponseTest, DataCopiedOver) {
   EXPECT_EQ("0123", output->payerPhone());
   EXPECT_EQ("id", output->requestId());
 
-  ScriptValue details =
-      output->details(scope.GetScriptState(), scope.GetExceptionState());
+  ScriptValue details = output->details(scope.GetScriptState());
 
   ASSERT_FALSE(scope.GetExceptionState().HadException());
   ASSERT_TRUE(details.V8Value()->IsObject());
@@ -84,20 +84,41 @@ TEST(PaymentResponseTest, DataCopiedOver) {
   EXPECT_EQ(123, transaction_id.V8Value().As<v8::Number>()->Value());
 }
 
-TEST(PaymentResponseTest, PaymentResponseDetailsJSONObject) {
+TEST(PaymentResponseTest,
+     PaymentResponseDetailsWithUnexpectedJSONFormatString) {
   V8TestingScope scope;
   payments::mojom::blink::PaymentResponsePtr input =
       BuildPaymentResponseForTest();
   input->stringified_details = "transactionId";
   MockPaymentStateResolver* complete_callback = new MockPaymentStateResolver;
   PaymentResponse* output =
-      new PaymentResponse(scope.GetExecutionContext(), std::move(input),
-                          nullptr, complete_callback, "id");
+      new PaymentResponse(scope.GetScriptState(), std::move(input), nullptr,
+                          complete_callback, "id");
 
-  ScriptValue details =
-      output->details(scope.GetScriptState(), scope.GetExceptionState());
+  ScriptValue details = output->details(scope.GetScriptState());
+  ASSERT_TRUE(details.V8Value()->IsObject());
+
+  String stringified_details = ToBlinkString<String>(
+      v8::JSON::Stringify(scope.GetContext(),
+                          details.V8Value().As<v8::Object>())
+          .ToLocalChecked(),
+      kDoNotExternalize);
+
+  EXPECT_EQ("{}", stringified_details);
+}
 
-  ASSERT_TRUE(scope.GetExceptionState().HadException());
+TEST(PaymentResponseTest, PaymentResponseDetailsRetrunsTheSameObject) {
+  V8TestingScope scope;
+  payments::mojom::blink::PaymentResponsePtr input =
+      BuildPaymentResponseForTest();
+  input->method_name = "foo";
+  input->stringified_details = "{\"transactionId\": 123}";
+  MockPaymentStateResolver* complete_callback = new MockPaymentStateResolver;
+  PaymentResponse* output =
+      new PaymentResponse(scope.GetScriptState(), std::move(input), nullptr,
+                          complete_callback, "id");
+  EXPECT_EQ(output->details(scope.GetScriptState()),
+            output->details(scope.GetScriptState()));
 }
 
 TEST(PaymentResponseTest, CompleteCalledWithSuccess) {
@@ -108,8 +129,8 @@ TEST(PaymentResponseTest, CompleteCalledWithSuccess) {
   input->stringified_details = "{\"transactionId\": 123}";
   MockPaymentStateResolver* complete_callback = new MockPaymentStateResolver;
   PaymentResponse* output =
-      new PaymentResponse(scope.GetExecutionContext(), std::move(input),
-                          nullptr, complete_callback, "id");
+      new PaymentResponse(scope.GetScriptState(), std::move(input), nullptr,
+                          complete_callback, "id");
 
   EXPECT_CALL(*complete_callback,
               Complete(scope.GetScriptState(), PaymentStateResolver::kSuccess));
@@ -125,8 +146,8 @@ TEST(PaymentResponseTest, CompleteCalledWithFailure) {
   input->stringified_details = "{\"transactionId\": 123}";
   MockPaymentStateResolver* complete_callback = new MockPaymentStateResolver;
   PaymentResponse* output =
-      new PaymentResponse(scope.GetExecutionContext(), std::move(input),
-                          nullptr, complete_callback, "id");
+      new PaymentResponse(scope.GetScriptState(), std::move(input), nullptr,
+                          complete_callback, "id");
 
   EXPECT_CALL(*complete_callback,
               Complete(scope.GetScriptState(), PaymentStateResolver::kFail));
@@ -155,8 +176,8 @@ TEST(PaymentResponseTest, JSONSerializerTest) {
       new PaymentAddress(std::move(input->shipping_address));
 
   PaymentResponse* output =
-      new PaymentResponse(scope.GetExecutionContext(), std::move(input),
-                          address, new MockPaymentStateResolver, "id");
+      new PaymentResponse(scope.GetScriptState(), std::move(input), address,
+                          new MockPaymentStateResolver, "id");
   ScriptValue json_object = output->toJSONForBinding(scope.GetScriptState());
   EXPECT_TRUE(json_object.IsObject());