Add data parameter to payment details modifier.

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

Index: third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
diff --git a/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp b/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
index 54bac4920032ba1be47d41d67e947a92412d5df8..464a893d248c770d0c3925ccf1af9ab35c710122 100644
--- a/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
+++ b/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
@@ -37,15 +37,8 @@ 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;
@@ -88,63 +81,6 @@ struct TypeConverter<PaymentShippingOptionPtr, blink::PaymentShippingOption> {
 };
 
 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();
@@ -164,22 +100,6 @@ struct TypeConverter<PaymentOptionsPtr, blink::PaymentOptions> {
   }
 };
 
-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 {
@@ -229,177 +149,193 @@ void validateShippingOptionOrPaymentItem(const T& item,
   }
 }
 
-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 "";
   }
-}
 
-// 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 (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 (uniqueIds.contains(option.id()))
-      return false;
+  if (value->getType() != JSONValue::TypeObject) {
+    exceptionState.throwTypeError("Data should be a JSON-serializable object");
+    return "";
+  }
 
-    uniqueIds.add(option.id());
+  return JSONObject::cast(value.get())->toJSONString();
+}
 
-    validateShippingOptionOrPaymentItem(option, exceptionState);
-    if (exceptionState.hadException())
-      return false;
+void validateAndConvertPaymentDetails(const PaymentDetails& input,
+                                      bool requestShipping,
+                                      mojom::blink::PaymentDetailsPtr& output,
+                                      ExceptionState& exceptionState) {
+  if (!input.hasTotal()) {
+    exceptionState.throwTypeError("Must specify total");
+    return;
   }
 
-  return true;
-}
+  validateShippingOptionOrPaymentItem(input.total(), exceptionState);
+  if (exceptionState.hadException())
+    return;
 
-void validatePaymentDetailsModifiers(
-    const HeapVector<PaymentDetailsModifier>& modifiers,
-    ExceptionState& exceptionState) {
-  if (modifiers.isEmpty()) {
-    exceptionState.throwTypeError(
-        "Must specify at least one payment details modifier");
+  if (input.total().amount().value()[0] == '-') {
+    exceptionState.throwTypeError("Total amount value should be non-negative");
     return;
   }
 
-  for (const auto& modifier : modifiers) {
-    if (modifier.supportedMethods().isEmpty()) {
-      exceptionState.throwTypeError(
-          "Must specify at least one payment method identifier");
-      return;
-    }
+  output->total = mojom::blink::PaymentItem::From(input.total());
 
-    if (modifier.hasTotal()) {
-      validateShippingOptionOrPaymentItem(modifier.total(), exceptionState);
+  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;
 
-      if (modifier.total().amount().value()[0] == '-') {
-        exceptionState.throwTypeError(
-            "Total amount value should be non-negative");
+      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 (modifier.hasAdditionalDisplayItems()) {
-      validateDisplayItems(modifier.additionalDisplayItems(), exceptionState);
+      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);
     }
   }
-}
 
-// 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;
-  }
+  if (input.hasModifiers()) {
+    if (input.modifiers().isEmpty()) {
+      exceptionState.throwTypeError("At least one modifier expected");
+      return;
+    }
 
-  validateShippingOptionOrPaymentItem(details.total(), exceptionState);
-  if (exceptionState.hadException())
-    return keepShippingOptions;
+    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;
+      }
 
-  if (details.total().amount().value()[0] == '-') {
-    exceptionState.throwTypeError("Total amount value should be non-negative");
-    return keepShippingOptions;
-  }
+      output->modifiers[i]->supported_methods = modifier.supportedMethods();
 
-  if (details.hasDisplayItems()) {
-    validateDisplayItems(details.displayItems(), exceptionState);
-    if (exceptionState.hadException())
-      return keepShippingOptions;
-  }
+      if (modifier.hasTotal()) {
+        validateShippingOptionOrPaymentItem(modifier.total(), exceptionState);
+        if (exceptionState.hadException())
+          return;
 
-  if (details.hasShippingOptions()) {
-    keepShippingOptions =
-        validateShippingOptions(details.shippingOptions(), exceptionState);
+        if (modifier.total().amount().value()[0] == '-') {
+          exceptionState.throwTypeError(
+              "Modifier total amount value should be non-negative");
+          return;
+        }
 
-    if (exceptionState.hadException())
-      return keepShippingOptions;
-  }
+        output->modifiers[i]->total =
+            mojom::blink::PaymentItem::From(modifier.total());
+      }
 
-  if (details.hasModifiers()) {
-    validatePaymentDetailsModifiers(details.modifiers(), exceptionState);
-    if (exceptionState.hadException())
-      return keepShippingOptions;
+      output->modifiers[i]->stringified_data =
+          modifier.hasData() ? stringifyData(modifier.data(), exceptionState)
+                             : "";
+      if (exceptionState.hadException())
+        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);
+        }
+      }
+    }
   }
 
   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) {
@@ -413,15 +349,6 @@ String getValidShippingType(const String& 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:
@@ -450,19 +377,6 @@ bool allowedToUsePaymentRequest(const Frame* frame) {
   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(
@@ -580,7 +494,10 @@ void PaymentRequest::onUpdatePaymentDetails(
     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()));
@@ -588,15 +505,10 @@ void PaymentRequest::onUpdatePaymentDetails(
     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) {
@@ -632,8 +544,8 @@ PaymentRequest::PaymentRequest(ScriptState* scriptState,
       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;
@@ -650,18 +562,20 @@ PaymentRequest::PaymentRequest(ScriptState* scriptState,
     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());
   }
 
@@ -670,13 +584,10 @@ PaymentRequest::PaymentRequest(ScriptState* scriptState,
   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() {
@@ -770,7 +681,7 @@ void PaymentRequest::OnPaymentResponse(
   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;