Add data parameter to payment details modifier.

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

 // Copyright 2016 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/PaymentRequest.h"
 
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/JSONValuesForV8.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "bindings/core/v8/ScriptState.h"
@@ skipping 19 matching lines @@
 #include "public/platform/InterfaceProvider.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebTraceLocation.h"
 #include "wtf/HashSet.h"
 #include <utility>
 
 namespace mojo {
 
 using blink::mojom::blink::PaymentCurrencyAmount;
 using blink::mojom::blink::PaymentCurrencyAmountPtr;
-using blink::mojom::blink::PaymentDetails;
-using blink::mojom::blink::PaymentDetailsModifier;
-using blink::mojom::blink::PaymentDetailsModifierPtr;
-using blink::mojom::blink::PaymentDetailsPtr;
-using blink::mojom::blink::PaymentErrorReason;
 using blink::mojom::blink::PaymentItem;
 using blink::mojom::blink::PaymentItemPtr;
-using blink::mojom::blink::PaymentMethodData;
-using blink::mojom::blink::PaymentMethodDataPtr;
 using blink::mojom::blink::PaymentOptions;
 using blink::mojom::blink::PaymentOptionsPtr;
 using blink::mojom::blink::PaymentShippingOption;
 using blink::mojom::blink::PaymentShippingOptionPtr;
 using blink::mojom::blink::PaymentShippingType;
 
 template <>
 struct TypeConverter<PaymentCurrencyAmountPtr, blink::PaymentCurrencyAmount> {
   static PaymentCurrencyAmountPtr Convert(
       const blink::PaymentCurrencyAmount& input) {
@@ skipping 22 matching lines @@
     PaymentShippingOptionPtr output = PaymentShippingOption::New();
     output->id = input.id();
     output->label = input.label();
     output->amount = PaymentCurrencyAmount::From(input.amount());
     output->selected = input.hasSelected() && input.selected();
     return output;
   }
 };
 
 template <>
-struct TypeConverter<PaymentDetailsModifierPtr, blink::PaymentDetailsModifier> {
-  static PaymentDetailsModifierPtr Convert(
-      const blink::PaymentDetailsModifier& input) {
-    PaymentDetailsModifierPtr output = PaymentDetailsModifier::New();
-    output->supported_methods =
-        WTF::Vector<WTF::String>(input.supportedMethods());
-
-    if (input.hasTotal())
-      output->total = PaymentItem::From(input.total());
-
-    if (input.hasAdditionalDisplayItems()) {
-      for (size_t i = 0; i < input.additionalDisplayItems().size(); ++i) {
-        output->additional_display_items.append(
-            PaymentItem::From(input.additionalDisplayItems()[i]));
-      }
-    }
-    return output;
-  }
-};
-
-template <>
-struct TypeConverter<PaymentDetailsPtr, blink::PaymentDetails> {
-  static PaymentDetailsPtr Convert(const blink::PaymentDetails& input) {
-    PaymentDetailsPtr output = PaymentDetails::New();
-    output->total = PaymentItem::From(input.total());
-
-    if (input.hasDisplayItems()) {
-      for (size_t i = 0; i < input.displayItems().size(); ++i) {
-        output->display_items.append(
-            PaymentItem::From(input.displayItems()[i]));
-      }
-    }
-
-    if (input.hasShippingOptions()) {
-      for (size_t i = 0; i < input.shippingOptions().size(); ++i) {
-        output->shipping_options.append(
-            PaymentShippingOption::From(input.shippingOptions()[i]));
-      }
-    }
-
-    if (input.hasModifiers()) {
-      for (size_t i = 0; i < input.modifiers().size(); ++i) {
-        output->modifiers.append(
-            PaymentDetailsModifier::From(input.modifiers()[i]));
-      }
-    }
-
-    if (input.hasError())
-      output->error = input.error();
-    else
-      output->error = WTF::emptyString();
-
-    return output;
-  }
-};
-
-template <>
 struct TypeConverter<PaymentOptionsPtr, blink::PaymentOptions> {
   static PaymentOptionsPtr Convert(const blink::PaymentOptions& input) {
     PaymentOptionsPtr output = PaymentOptions::New();
     output->request_payer_name = input.requestPayerName();
     output->request_payer_email = input.requestPayerEmail();
     output->request_payer_phone = input.requestPayerPhone();
     output->request_shipping = input.requestShipping();
 
     if (input.shippingType() == "delivery")
       output->shipping_type = PaymentShippingType::DELIVERY;
     else if (input.shippingType() == "pickup")
       output->shipping_type = PaymentShippingType::PICKUP;
     else
       output->shipping_type = PaymentShippingType::SHIPPING;
 
     return output;
   }
 };
 
-template <>
-struct TypeConverter<WTFArray<PaymentMethodDataPtr>,
-                     WTF::Vector<blink::PaymentRequest::MethodData>> {
-  static WTFArray<PaymentMethodDataPtr> Convert(
-      const WTF::Vector<blink::PaymentRequest::MethodData>& input) {
-    WTFArray<PaymentMethodDataPtr> output(input.size());
-    for (size_t i = 0; i < input.size(); ++i) {
-      output[i] = PaymentMethodData::New();
-      output[i]->supported_methods =
-          WTF::Vector<WTF::String>(input[i].supportedMethods);
-      output[i]->stringified_data = input[i].stringifiedData;
-    }
-    return output;
-  }
-};
-
 }  // namespace mojo
 
 namespace blink {
 namespace {
 
 // If the website does not call complete() 60 seconds after show() has been
 // resolved, then behave as if the website called complete("fail").
 static const int completeTimeoutSeconds = 60;
 
 // Validates ShippingOption or PaymentItem, which happen to have identical
 // fields, except for "id", which is present only in ShippingOption.
 template <typename T>
 void validateShippingOptionOrPaymentItem(const T& item,

[Mathieu, 2016/11/01 14:39:16]

curious: are you worried that the exception is going to be too generic given
that this function is used to validate many things? Or will the dev tools
provide a clear indication of what's wrong?

[please use gerrit instead, 2016/11/02 16:27:09]

Improving these error messages are a nice to have, but not high priority at the
moment. So far web developer feedback is that the API is easy to use.

                                          ExceptionState& exceptionState) {
   if (!item.hasLabel() || item.label().isEmpty()) {
     exceptionState.throwTypeError("Item label required");
     return;
   }
 
   if (!item.hasAmount()) {
     exceptionState.throwTypeError("Currency amount required");
     return;
   }
@@ skipping 16 matching lines @@
     return;
   }
 
   if (!PaymentsValidators::isValidAmountFormat(item.amount().value(),
                                                &errorMessage)) {
     exceptionState.throwTypeError(errorMessage);
     return;
   }
 }
 
-void validateDisplayItems(const HeapVector<PaymentItem>& items,
-                          ExceptionState& exceptionState) {
-  for (const auto& item : items) {
-    validateShippingOptionOrPaymentItem(item, exceptionState);
-    if (exceptionState.hadException())
-      return;
+String stringifyData(const ScriptValue& data, ExceptionState& exceptionState) {

[Mathieu, 2016/11/01 14:39:16]

Not sure of the convention for documentation in Blink, but this would benefit
from a quick function comment to mention what are valid values and invalid
values.

[please use gerrit instead, 2016/11/02 16:27:09]

Done.

+  if (data.isEmpty())
+    return "";
+
+  std::unique_ptr<JSONValue> value =
+      toJSONValue(data.context(), data.v8Value());
+  if (!value) {
+    exceptionState.throwTypeError(
+        "Unable to parse payment method specific data");
+    return "";
   }
+
+  if (value->isNull())

[Mathieu, 2016/11/01 14:39:16]

combine with previous block?

if (!value || value->isNull())

Unless isNull is a valid state where no exception should be returned?

[please use gerrit instead, 2016/11/02 16:27:09]

That's right.

"!value" catches objects that cannot be parsed into a JSON value. For example:

var foo = {};
foo.bar = foo;

Recursion is valid for objects, but cannot be parsed into JSON. That's not valid
according to the spec.

"value->isNull()" handles null objects. For example.

var foo = {"bar": null};

That's perfectly valid. So we stringify it as "", which means "no data" for us.

+    return "";
+
+  if (value->getType() != JSONValue::TypeObject) {
+    exceptionState.throwTypeError("Data should be a JSON-serializable object");
+    return "";
+  }
+
+  return JSONObject::cast(value.get())->toJSONString();
 }
 
-// Returns false if |options| should be ignored, even if an exception was not
-// thrown. TODO(rouslan): Clear shipping options instead of ignoring them when
-// http://crbug.com/601193 is fixed.
-bool validateShippingOptions(const HeapVector<PaymentShippingOption>& options,
-                             ExceptionState& exceptionState) {
-  HashSet<String> uniqueIds;
-  for (const auto& option : options) {
-    if (!option.hasId() || option.id().isEmpty()) {
-      exceptionState.throwTypeError("ShippingOption id required");
-      return false;
-    }
-
-    if (uniqueIds.contains(option.id()))
-      return false;
-
-    uniqueIds.add(option.id());
-
-    validateShippingOptionOrPaymentItem(option, exceptionState);
-    if (exceptionState.hadException())
-      return false;
-  }
-
-  return true;
-}
-
-void validatePaymentDetailsModifiers(
-    const HeapVector<PaymentDetailsModifier>& modifiers,
-    ExceptionState& exceptionState) {
-  if (modifiers.isEmpty()) {
-    exceptionState.throwTypeError(
-        "Must specify at least one payment details modifier");
+void validateAndConvertPaymentDetails(const PaymentDetails& input,
+                                      bool requestShipping,
+                                      mojom::blink::PaymentDetailsPtr& output,
+                                      ExceptionState& exceptionState) {
+  if (!input.hasTotal()) {
+    exceptionState.throwTypeError("Must specify total");
     return;
   }
 
-  for (const auto& modifier : modifiers) {
-    if (modifier.supportedMethods().isEmpty()) {
-      exceptionState.throwTypeError(
-          "Must specify at least one payment method identifier");
+  validateShippingOptionOrPaymentItem(input.total(), exceptionState);
+  if (exceptionState.hadException())
+    return;
+
+  if (input.total().amount().value()[0] == '-') {
+    exceptionState.throwTypeError("Total amount value should be non-negative");
+    return;
+  }
+
+  output->total = mojom::blink::PaymentItem::From(input.total());
+
+  if (input.hasDisplayItems()) {
+    output->display_items.resize(input.displayItems().size());
+    for (size_t i = 0; i < input.displayItems().size(); ++i) {
+      const PaymentItem& item = input.displayItems()[i];
+      validateShippingOptionOrPaymentItem(item, exceptionState);
+      if (exceptionState.hadException())
+        return;
+
+      output->display_items[i] = mojom::blink::PaymentItem::From(item);
+    }
+  }
+
+  if (input.hasShippingOptions() && requestShipping) {
+    HashSet<String> uniqueIds;
+    output->shipping_options.resize(input.shippingOptions().size());
+    for (size_t i = 0; i < input.shippingOptions().size(); ++i) {
+      const PaymentShippingOption& option = input.shippingOptions()[i];
+      if (!option.hasId() || option.id().isEmpty()) {
+        exceptionState.throwTypeError("ShippingOption id required");
+        return;
+      }
+
+      if (uniqueIds.contains(option.id())) {
+        // Duplicate identifiers cause all shipping options to be ignored.
+        output->shipping_options.resize(0);
+        break;
+      }
+
+      uniqueIds.add(option.id());
+
+      validateShippingOptionOrPaymentItem(option, exceptionState);
+      if (exceptionState.hadException())
+        return;
+
+      output->shipping_options[i] =
+          mojom::blink::PaymentShippingOption::From(option);
+    }
+  }
+
+  if (input.hasModifiers()) {
+    if (input.modifiers().isEmpty()) {
+      exceptionState.throwTypeError("At least one modifier expected");
       return;
     }
 
-    if (modifier.hasTotal()) {
-      validateShippingOptionOrPaymentItem(modifier.total(), exceptionState);
+    output->modifiers.resize(input.modifiers().size());
+    for (size_t i = 0; i < input.modifiers().size(); ++i) {
+      const PaymentDetailsModifier& modifier = input.modifiers()[i];
+      output->modifiers[i] = mojom::blink::PaymentDetailsModifier::New();
+      if (modifier.supportedMethods().isEmpty()) {
+        exceptionState.throwTypeError(
+            "Modifier must specify at least one payment method identifier");
+        return;
+      }
+
+      output->modifiers[i]->supported_methods = modifier.supportedMethods();
+
+      if (modifier.hasTotal()) {
+        validateShippingOptionOrPaymentItem(modifier.total(), exceptionState);
+        if (exceptionState.hadException())
+          return;
+
+        if (modifier.total().amount().value()[0] == '-') {
+          exceptionState.throwTypeError(
+              "Modifier total amount value should be non-negative");
+          return;
+        }
+
+        output->modifiers[i]->total =
+            mojom::blink::PaymentItem::From(modifier.total());
+      }
+
+      output->modifiers[i]->stringified_data =
+          modifier.hasData() ? stringifyData(modifier.data(), exceptionState)
+                             : "";
       if (exceptionState.hadException())
         return;
 
-      if (modifier.total().amount().value()[0] == '-') {
-        exceptionState.throwTypeError(
-            "Total amount value should be non-negative");
-        return;
+      if (modifier.hasAdditionalDisplayItems()) {
+        output->modifiers[i]->additional_display_items.resize(
+            modifier.additionalDisplayItems().size());
+        for (size_t j = 0; j < modifier.additionalDisplayItems().size(); ++j) {
+          const PaymentItem& item = modifier.additionalDisplayItems()[j];
+          validateShippingOptionOrPaymentItem(item, exceptionState);
+          if (exceptionState.hadException())
+            return;
+
+          output->modifiers[i]->additional_display_items[j] =
+              mojom::blink::PaymentItem::From(item);
+        }
       }
     }
-
-    if (modifier.hasAdditionalDisplayItems()) {
-      validateDisplayItems(modifier.additionalDisplayItems(), exceptionState);
-      if (exceptionState.hadException())
-        return;
-    }
-  }
-}
-
-// Returns false if the shipping options should be ignored without throwing an
-// exception.
-bool validatePaymentDetails(const PaymentDetails& details,
-                            ExceptionState& exceptionState) {
-  bool keepShippingOptions = true;
-  if (!details.hasTotal()) {
-    exceptionState.throwTypeError("Must specify total");
-    return keepShippingOptions;
-  }
-
-  validateShippingOptionOrPaymentItem(details.total(), exceptionState);
-  if (exceptionState.hadException())
-    return keepShippingOptions;
-
-  if (details.total().amount().value()[0] == '-') {
-    exceptionState.throwTypeError("Total amount value should be non-negative");
-    return keepShippingOptions;
-  }
-
-  if (details.hasDisplayItems()) {
-    validateDisplayItems(details.displayItems(), exceptionState);
-    if (exceptionState.hadException())
-      return keepShippingOptions;
-  }
-
-  if (details.hasShippingOptions()) {
-    keepShippingOptions =
-        validateShippingOptions(details.shippingOptions(), exceptionState);
-
-    if (exceptionState.hadException())
-      return keepShippingOptions;
-  }
-
-  if (details.hasModifiers()) {
-    validatePaymentDetailsModifiers(details.modifiers(), exceptionState);
-    if (exceptionState.hadException())
-      return keepShippingOptions;
   }
 
   String errorMessage;
-  if (!PaymentsValidators::isValidErrorMsgFormat(details.error(),
+  if (!PaymentsValidators::isValidErrorMsgFormat(input.error(),
                                                  &errorMessage)) {
     exceptionState.throwTypeError(errorMessage);
+    return;
   }
 
-  return keepShippingOptions;
+  output->error =
+      input.hasError() && !input.error().isNull() ? input.error() : "";
 }
 
 void validateAndConvertPaymentMethodData(

[Mathieu, 2016/11/01 14:39:16]

would benefit from a comment such as 

// Takes the PaymentMethodData as specified by the website/javascript and
creates a validated Mojo PaymentMethodDataPtr object to be passed to the
embedder/browser.

[please use gerrit instead, 2016/11/02 16:27:09]

Done. Also added a similar comment to validatedAndConvertPaymentDetails().

-    const HeapVector<PaymentMethodData>& paymentMethodData,
-    Vector<PaymentRequest::MethodData>* methodData,
+    const HeapVector<PaymentMethodData>& input,
+    Vector<mojom::blink::PaymentMethodDataPtr>& output,
     ExceptionState& exceptionState) {
-  if (paymentMethodData.isEmpty()) {
+  if (input.isEmpty()) {
     exceptionState.throwTypeError(
         "Must specify at least one payment method identifier");
     return;
   }
 
-  for (const auto& pmd : paymentMethodData) {
+  output.resize(input.size());
+  for (size_t i = 0; i < input.size(); ++i) {
+    const PaymentMethodData& pmd = input[i];
     if (pmd.supportedMethods().isEmpty()) {
       exceptionState.throwTypeError(
           "Must specify at least one payment method identifier");
       return;
     }
 
-    String stringifiedData = "";
-    if (pmd.hasData() && !pmd.data().isEmpty()) {
-      std::unique_ptr<JSONValue> value =
-          toJSONValue(pmd.data().context(), pmd.data().v8Value());
-      if (!value) {
-        exceptionState.throwTypeError(
-            "Unable to parse payment method specific data");
-        return;
-      }
-      if (!value->isNull()) {
-        if (value->getType() != JSONValue::TypeObject) {
-          exceptionState.throwTypeError(
-              "Data should be a JSON-serializable object");
-          return;
-        }
-        stringifiedData = JSONObject::cast(value.get())->toJSONString();
-      }
-    }
-    methodData->append(
-        PaymentRequest::MethodData(pmd.supportedMethods(), stringifiedData));
+    output[i] = mojom::blink::PaymentMethodData::New();
+    output[i]->supported_methods = pmd.supportedMethods();
+    output[i]->stringified_data =
+        pmd.hasData() ? stringifyData(pmd.data(), exceptionState) : "";
+    if (exceptionState.hadException())
+      return;
   }
 }
 
-String getSelectedShippingOption(const PaymentDetails& details) {
-  String result;
-  if (!details.hasShippingOptions())
-    return result;
+String getSelectedShippingOption(
+    const mojom::blink::PaymentDetailsPtr& details) {
+  if (details->shipping_options.isEmpty())
+    return String();
 
-  for (int i = details.shippingOptions().size() - 1; i >= 0; --i) {
-    if (details.shippingOptions()[i].hasSelected() &&
-        details.shippingOptions()[i].selected()) {
-      return details.shippingOptions()[i].id();
-    }
+  for (int i = details->shipping_options.size() - 1; i >= 0; --i) {
+    if (details->shipping_options[i]->selected)
+      return details->shipping_options[i]->id;
   }
 
-  return result;
+  return String();
 }
 
 String getValidShippingType(const String& shippingType) {
   static const char* const validValues[] = {
       "shipping", "delivery", "pickup",
   };
   for (size_t i = 0; i < WTF_ARRAY_LENGTH(validValues); i++) {
     if (shippingType == validValues[i])
       return shippingType;
   }
   return validValues[0];
 }
 
-mojom::blink::PaymentDetailsPtr maybeKeepShippingOptions(
-    mojom::blink::PaymentDetailsPtr details,
-    bool keep) {
-  if (!keep)
-    details->shipping_options.resize(0);
-
-  return details;
-}
-
 bool allowedToUsePaymentRequest(const Frame* frame) {
   // To determine whether a Document object |document| is allowed to use the
   // feature indicated by attribute name |allowpaymentrequest|, run these steps:
 
   // 1. If |document| has no browsing context, then return false.
   if (!frame)
     return false;
 
   // 2. If |document|'s browsing context is a top-level browsing context, then
   // return true.
   if (frame->isMainFrame())
     return true;
 
   // 3. If |document|'s browsing context has a browsing context container that
   // is an iframe element with an |allowpaymentrequest| attribute specified, and
   // whose node document is allowed to use the feature indicated by
   // |allowpaymentrequest|, then return true.
   HTMLFrameOwnerElement* ownerElement = toHTMLFrameOwnerElement(frame->owner());
   if (ownerElement && isHTMLIFrameElement(ownerElement)) {
     HTMLIFrameElement* iframe = toHTMLIFrameElement(ownerElement);
     if (HTMLIFrameElementPayments::from(*iframe).allowPaymentRequest(*iframe))
       return allowedToUsePaymentRequest(frame->tree().parent());
   }
 
   // 4. Return false.
   return false;
 }
 
-WTF::Vector<mojom::blink::PaymentMethodDataPtr> ConvertPaymentMethodData(
-    const Vector<PaymentRequest::MethodData>& blinkMethods) {
-  WTF::Vector<mojom::blink::PaymentMethodDataPtr> mojoMethods(
-      blinkMethods.size());
-  for (size_t i = 0; i < blinkMethods.size(); ++i) {
-    mojoMethods[i] = mojom::blink::PaymentMethodData::New();
-    mojoMethods[i]->supported_methods =
-        WTF::Vector<WTF::String>(blinkMethods[i].supportedMethods);
-    mojoMethods[i]->stringified_data = blinkMethods[i].stringifiedData;
-  }
-  return mojoMethods;
-}
-
 }  // namespace
 
 PaymentRequest* PaymentRequest::create(
     ScriptState* scriptState,
     const HeapVector<PaymentMethodData>& methodData,
     const PaymentDetails& details,
     ExceptionState& exceptionState) {
   return new PaymentRequest(scriptState, methodData, details, PaymentOptions(),
                             exceptionState);
 }
@@ skipping 97 matching lines @@
   V8PaymentDetails::toImpl(detailsScriptValue.isolate(),
                            detailsScriptValue.v8Value(), details,
                            exceptionState);
   if (exceptionState.hadException()) {
     m_showResolver->reject(
         DOMException::create(SyntaxError, exceptionState.message()));
     clearResolversAndCloseMojoConnection();
     return;
   }
 
-  bool keepShippingOptions = validatePaymentDetails(details, exceptionState);
+  mojom::blink::PaymentDetailsPtr validatedDetails =
+      mojom::blink::PaymentDetails::New();
+  validateAndConvertPaymentDetails(details, m_options.requestShipping(),
+                                   validatedDetails, exceptionState);
   if (exceptionState.hadException()) {
     m_showResolver->reject(
         DOMException::create(SyntaxError, exceptionState.message()));
     clearResolversAndCloseMojoConnection();
     return;
   }
 
-  if (m_options.requestShipping()) {
-    if (keepShippingOptions)
-      m_shippingOption = getSelectedShippingOption(details);
-    else
-      m_shippingOption = String();
-  }
+  if (m_options.requestShipping())
+    m_shippingOption = getSelectedShippingOption(validatedDetails);
 
-  m_paymentProvider->UpdateWith(maybeKeepShippingOptions(
-      mojom::blink::PaymentDetails::From(details), keepShippingOptions));
+  m_paymentProvider->UpdateWith(std::move(validatedDetails));
 }
 
 void PaymentRequest::onUpdatePaymentDetailsFailure(const String& error) {
   if (m_showResolver)
     m_showResolver->reject(DOMException::create(AbortError, error));
   if (m_completeResolver)
     m_completeResolver->reject(DOMException::create(AbortError, error));
   clearResolversAndCloseMojoConnection();
 }
 
@@ skipping 15 matching lines @@
 PaymentRequest::PaymentRequest(ScriptState* scriptState,
                                const HeapVector<PaymentMethodData>& methodData,
                                const PaymentDetails& details,
                                const PaymentOptions& options,
                                ExceptionState& exceptionState)
     : ContextLifecycleObserver(scriptState->getExecutionContext()),
       ActiveScriptWrappable(this),
       m_options(options),
       m_clientBinding(this),
       m_completeTimer(this, &PaymentRequest::onCompleteTimeout) {
-  Vector<MethodData> validatedMethodData;
-  validateAndConvertPaymentMethodData(methodData, &validatedMethodData,
+  Vector<mojom::blink::PaymentMethodDataPtr> validatedMethodData;
+  validateAndConvertPaymentMethodData(methodData, validatedMethodData,
                                       exceptionState);
   if (exceptionState.hadException())
     return;
 
   if (!scriptState->getExecutionContext()->isSecureContext()) {
     exceptionState.throwSecurityError("Must be in a secure context");
     return;
   }
 
   if (!allowedToUsePaymentRequest(scriptState->domWindow()->frame())) {
     exceptionState.throwSecurityError(
         "Must be in a top-level browsing context or an iframe needs to specify "
         "'allowpaymentrequest' explicitly");
     return;
   }
 
-  bool keepShippingOptions = validatePaymentDetails(details, exceptionState);
+  mojom::blink::PaymentDetailsPtr validatedDetails =
+      mojom::blink::PaymentDetails::New();
+  validateAndConvertPaymentDetails(details, m_options.requestShipping(),
+                                   validatedDetails, exceptionState);
   if (exceptionState.hadException())
     return;
 
-  if (details.hasError() && !details.error().isEmpty()) {
+  if (!validatedDetails->error.isEmpty()) {
     exceptionState.throwTypeError("Error value should be empty");
     return;
   }
 
   if (m_options.requestShipping()) {
-    if (keepShippingOptions)
-      m_shippingOption = getSelectedShippingOption(details);
+    m_shippingOption = getSelectedShippingOption(validatedDetails);
     m_shippingType = getValidShippingType(m_options.shippingType());
   }
 
   scriptState->domWindow()->frame()->interfaceProvider()->getInterface(
       mojo::GetProxy(&m_paymentProvider));
   m_paymentProvider.set_connection_error_handler(convertToBaseCallback(
       WTF::bind(&PaymentRequest::OnError, wrapWeakPersistent(this),
                 mojom::blink::PaymentErrorReason::UNKNOWN)));
-  m_paymentProvider->Init(
-      m_clientBinding.CreateInterfacePtrAndBind(),
-      ConvertPaymentMethodData(validatedMethodData),
-      maybeKeepShippingOptions(
-          mojom::blink::PaymentDetails::From(details),
-          keepShippingOptions && m_options.requestShipping()),
-      mojom::blink::PaymentOptions::From(m_options));
+  m_paymentProvider->Init(m_clientBinding.CreateInterfacePtrAndBind(),
+                          std::move(validatedMethodData),
+                          std::move(validatedDetails),
+                          mojom::blink::PaymentOptions::From(m_options));
 }
 
 void PaymentRequest::contextDestroyed() {
   clearResolversAndCloseMojoConnection();
 }
 
 void PaymentRequest::OnShippingAddressChange(
     mojom::blink::PaymentAddressPtr address) {
   DCHECK(m_showResolver);
   DCHECK(!m_completeResolver);
@@ skipping 73 matching lines @@
   m_completeTimer.startOneShot(completeTimeoutSeconds, BLINK_FROM_HERE);
 
   m_showResolver->resolve(new PaymentResponse(std::move(response), this));
 
   // Do not close the mojo connection here. The merchant website should call
   // PaymentResponse::complete(String), which will be forwarded over the mojo
   // connection to display a success or failure message to the user.
   m_showResolver.clear();
 }
 
-void PaymentRequest::OnError(mojo::PaymentErrorReason error) {
+void PaymentRequest::OnError(mojom::blink::PaymentErrorReason error) {
   if (!Platform::current()) {
     // TODO(rockot): Clean this up once renderer shutdown sequence is fixed.
     return;
   }
 
   bool isError = false;
   ExceptionCode ec = UnknownError;
   String message;
 
   switch (error) {
@@ skipping 67 matching lines @@
   m_completeTimer.stop();
   m_completeResolver.clear();
   m_showResolver.clear();
   m_abortResolver.clear();
   if (m_clientBinding.is_bound())
     m_clientBinding.Close();
   m_paymentProvider.reset();
 }
 
 }  // namespace blink