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 10dc9a605e61d45e4a642548af1f2ae46f80e0c5..331b56045a90f2c983e24ae2de9551eaba036725 100644
--- a/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
+++ b/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
@@ -98,62 +98,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 = 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 = emptyString();
-
-    return output;
-  }
-};
-
-template <>
 struct TypeConverter<PaymentOptionsPtr, blink::PaymentOptions> {
   static PaymentOptionsPtr Convert(const blink::PaymentOptions& input) {
     PaymentOptionsPtr output = PaymentOptions::New();
@@ -240,128 +184,65 @@ void validateShippingOptionOrPaymentItem(const T& item,
   }
 }
 
-void validateDisplayItems(const HeapVector<PaymentItem>& items,
-                          ExceptionState& exceptionState) {
-  for (const auto& item : items) {
+void validateAndConvertDisplayItems(const HeapVector<PaymentItem>& input,
+                                    Vector<PaymentItemPtr>& output,
+                                    ExceptionState& exceptionState) {
+  output.resize(input.size());
+  for (size_t i = 0; i < input.size(); ++i) {
+    const PaymentItem& item = input[i];

[palmer, 2016/11/29 00:24:55]

Nit: If you want, you could declare |i| and |item| with auto.

[please use gerrit instead, 2016/11/29 16:19:52]

I prefer to be explicit, if there's enough space on the line. auto is only
useful when I want to save some space, IMHO. Are there better arguments to use
auto?

[Kevin Bailey, 2016/11/29 17:27:59]

Does HeapVector<> support range-based loops?

[please use gerrit instead, 2016/11/29 19:26:52]

Yes it does. Vector used to not have .append() method. But now that I look at
Vector.h again, I see that .append() and .back() were added. This eliminates the
need for using Vector::resize(n) and index operator.

     validateShippingOptionOrPaymentItem(item, exceptionState);
     if (exceptionState.hadException())
       return;
+    output[i] = payments::mojom::blink::PaymentItem::From(item);
   }
 }
 
-// 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) {
+void validateAndConvertShippingOptions(
+    const HeapVector<PaymentShippingOption>& input,
+    Vector<PaymentShippingOptionPtr>& output,
+    ExceptionState& exceptionState) {
+  output.resize(input.size());
   HashSet<String> uniqueIds;
-  for (const auto& option : options) {
+  for (size_t i = 0; i < input.size(); ++i) {
+    const PaymentShippingOption& option = input[i];

[palmer, 2016/11/29 00:24:55]

Here, too.

[please use gerrit instead, 2016/11/29 16:19:52]

Acknowledged.

     if (!option.hasId() || option.id().isEmpty()) {
       exceptionState.throwTypeError("ShippingOption id required");

[haraken, 2016/11/29 01:58:25]

Don't we need to clear output?

[please use gerrit instead, 2016/11/29 16:19:52]

No need to clear output when throwing an exception. Output is declared on stack
by the caller of this method. The caller aborts own execution when this function
returns an exception. This deletes the output, so there's no need to also clear
it first.

Added comments to that effect.

-      return false;
+      return;
     }
 
-    if (uniqueIds.contains(option.id()))
-      return false;
+    if (uniqueIds.contains(option.id())) {
+      output.clear();
+      return;
+    }
 
     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");
-    return;
-  }
-
-  for (const auto& modifier : modifiers) {
-    if (modifier.supportedMethods().isEmpty()) {
-      exceptionState.throwTypeError(
-          "Must specify at least one payment method identifier");
       return;

[haraken, 2016/11/29 01:58:25]

Ditto.

[please use gerrit instead, 2016/11/29 16:19:52]

Acknowledged.

-    }
-
-    if (modifier.hasTotal()) {
-      validateShippingOptionOrPaymentItem(modifier.total(), exceptionState);
-      if (exceptionState.hadException())
-        return;
 
-      if (modifier.total().amount().value()[0] == '-') {
-        exceptionState.throwTypeError(
-            "Total amount value should be non-negative");
-        return;
-      }
-    }
-
-    if (modifier.hasAdditionalDisplayItems()) {
-      validateDisplayItems(modifier.additionalDisplayItems(), exceptionState);
-      if (exceptionState.hadException())
-        return;
-    }
+    output[i] = payments::mojom::blink::PaymentShippingOption::From(input[i]);
   }
 }
 
-// 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);
+void validateAndConvertTotal(const PaymentItem& input,
+                             PaymentItemPtr& output,
+                             ExceptionState& exceptionState) {
+  validateShippingOptionOrPaymentItem(input, exceptionState);
   if (exceptionState.hadException())
-    return keepShippingOptions;
+    return;
 
-  if (details.total().amount().value()[0] == '-') {
+  if (input.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(),
-                                                 &errorMessage)) {
-    exceptionState.throwTypeError(errorMessage);
+    return;
   }
 
-  return keepShippingOptions;
+  output = payments::mojom::blink::PaymentItem::From(input);
 }
 
-void maybeSetAndroidPayMethodData(
-    const ScriptValue& input,
-    payments::mojom::blink::PaymentMethodDataPtr& output,
-    ExceptionState& exceptionState) {
+void maybeSetAndroidPayMethodData(const ScriptValue& input,
+                                  PaymentMethodDataPtr& output,
+                                  ExceptionState& exceptionState) {
   AndroidPayMethodData androidPay;
   TrackExceptionState trackExceptionState;
   V8AndroidPayMethodData::toImpl(input.isolate(), input.v8Value(), androidPay,
@@ -428,70 +309,171 @@ void maybeSetAndroidPayMethodData(
   }
 }
 
-void validateAndConvertPaymentMethodData(
-    const HeapVector<PaymentMethodData>& input,
-    Vector<payments::mojom::blink::PaymentMethodDataPtr>& output,
+void stringifyAndParseMethodSpecificData(const ScriptValue& input,
+                                         PaymentMethodDataPtr& output,
+                                         ExceptionState& exceptionState) {
+  String stringifiedData = "";

[haraken, 2016/11/29 01:58:25]

'= ""' won't be needed.

[please use gerrit instead, 2016/11/29 16:19:52]

Removed.

+  if (!input.isEmpty()) {
+    if (!input.v8Value()->IsObject() || input.v8Value()->IsArray()) {
+      exceptionState.throwTypeError(
+          "Data should be a JSON-serializable object");
+      return;
+    }
+
+    v8::Local<v8::String> value;
+    if (!v8::JSON::Stringify(input.context(), input.v8Value().As<v8::Object>())
+             .ToLocal(&value)) {
+      exceptionState.throwTypeError(
+          "Unable to parse payment method specific data");
+      return;
+    }
+
+    stringifiedData = v8StringToWebCoreString<String>(value, DoNotExternalize);
+  }
+
+  output->stringified_data = stringifiedData;
+  maybeSetAndroidPayMethodData(input, output, exceptionState);
+}
+
+void validateAndConvertPaymentDetailsModifiers(
+    const HeapVector<PaymentDetailsModifier>& input,
+    Vector<PaymentDetailsModifierPtr>& output,
     ExceptionState& exceptionState) {
   if (input.isEmpty()) {
     exceptionState.throwTypeError(
-        "Must specify at least one payment method identifier");
+        "Must specify at least one payment details modifier");
     return;
   }
 
   output.resize(input.size());
   for (size_t i = 0; i < input.size(); ++i) {
-    const auto& paymentMethodData = input[i];
-    if (paymentMethodData.supportedMethods().isEmpty()) {
+    const PaymentDetailsModifier& modifier = input[i];

[palmer, 2016/11/29 00:24:55]

Here, too.

[please use gerrit instead, 2016/11/29 16:19:52]

Acknowledged.

+    output[i] = payments::mojom::blink::PaymentDetailsModifier::New();
+    if (modifier.hasTotal()) {
+      validateAndConvertTotal(modifier.total(), output[i]->total,
+                              exceptionState);
+      if (exceptionState.hadException())
+        return;

[haraken, 2016/11/29 01:58:25]

Ditto. Do you need to clear output?

[please use gerrit instead, 2016/11/29 16:19:52]

Acknowledged.

+    }
+
+    if (modifier.hasAdditionalDisplayItems()) {
+      validateAndConvertDisplayItems(modifier.additionalDisplayItems(),
+                                     output[i]->additional_display_items,
+                                     exceptionState);
+      if (exceptionState.hadException())
+        return;
+    }
+
+    if (modifier.supportedMethods().isEmpty()) {
       exceptionState.throwTypeError(
           "Must specify at least one payment method identifier");
       return;
     }
 
-    String stringifiedData = "";
-    if (paymentMethodData.hasData() && !paymentMethodData.data().isEmpty()) {
-      if (!paymentMethodData.data().v8Value()->IsObject() ||
-          paymentMethodData.data().v8Value()->IsArray()) {
-        exceptionState.throwTypeError(
-            "Data should be a JSON-serializable object");
-        return;
-      }
+    output[i]->method_data = payments::mojom::blink::PaymentMethodData::New();
+    output[i]->method_data->supported_methods = modifier.supportedMethods();
 
-      v8::Local<v8::String> value;
-      if (!v8::JSON::Stringify(
-               paymentMethodData.data().context(),
-               paymentMethodData.data().v8Value().As<v8::Object>())
-               .ToLocal(&value)) {
-        exceptionState.throwTypeError(
-            "Unable to parse payment method specific data");
+    if (modifier.hasData()) {
+      stringifyAndParseMethodSpecificData(
+          modifier.data(), output[i]->method_data, exceptionState);
+      if (exceptionState.hadException())
         return;
-      }
-      stringifiedData =
-          v8StringToWebCoreString<String>(value, DoNotExternalize);
+    } else {
+      output[i]->method_data->stringified_data = "";
     }
+  }
+}
 
-    output[i] = payments::mojom::blink::PaymentMethodData::New();
-    output[i]->supported_methods = paymentMethodData.supportedMethods();
-    output[i]->stringified_data = stringifiedData;
-    maybeSetAndroidPayMethodData(paymentMethodData.data(), output[i],
-                                 exceptionState);
+String getSelectedShippingOption(
+    const Vector<PaymentShippingOptionPtr>& shippingOptions) {
+  for (int i = shippingOptions.size() - 1; i >= 0; --i) {

[palmer, 2016/11/29 00:24:55]

If this is a wtf::Vector, you should IWYU, and it returns size_t. Hence, a cast
to int is a change in signedness and possibly width. (There is a width
difference in principle, although not in practice, because Vector::m_size is
declared as `unsigned`.) I think this code should produce a compiler warning?
Even if not, it should.

If you declare |i| as auto or size_t, the `i >= 0` condition will always be
true, so you'll need to come up with a different loop construct that doesn't
also loop forever or read OOB memory. If, instead, you stick with int, consider
base::checked_cast or the like to make sure nothing weird happens.

[please use gerrit instead, 2016/11/29 16:19:52]

Switched to looping over all elements in the natural order instead.

As for Vector.h, it is included in PaymentRequest.h:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/paymen...

The style guide says IWYU with exception of headers already included in the .h
file corresponding to this .cpp file.

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

"any includes present in the related header do not need to be included again in
the related cc (i.e., foo.cc can rely on foo.h's includes)."

[Kevin Bailey, 2016/11/29 17:27:59]

I can appreciate the concern in case this size goes over 2 billion :) but in
that case, do we really want to scan through all 2B ? How about a reverse
iterator, or offsetting the index by 1 ?

[please use gerrit instead, 2016/11/29 19:26:52]

The concern was compiler warning, which should be triggered, but has not yet.
Forward loop makes the code more clear and eliminates the possibility of a
warning.

+    if (shippingOptions[i]->selected)
+      return shippingOptions[i]->id;
+  }
+  return String();
+}
+
+void validateAndConvertPaymentDetails(const PaymentDetails& input,
+                                      bool requestShipping,
+                                      PaymentDetailsPtr& output,
+                                      String& shippingOptionOutput,
+                                      ExceptionState& exceptionState) {
+  if (!input.hasTotal()) {
+    exceptionState.throwTypeError("Must specify total");
+    return;
+  }
+
+  validateAndConvertTotal(input.total(), output->total, exceptionState);
+  if (exceptionState.hadException())
+    return;
+
+  if (input.hasDisplayItems()) {
+    validateAndConvertDisplayItems(input.displayItems(), output->display_items,
+                                   exceptionState);
     if (exceptionState.hadException())
       return;
   }
-}
 
-String getSelectedShippingOption(const PaymentDetails& details) {
-  String result;
-  if (!details.hasShippingOptions())
-    return result;
+  if (input.hasShippingOptions() && requestShipping) {
+    validateAndConvertShippingOptions(input.shippingOptions(),
+                                      output->shipping_options, exceptionState);
+    if (exceptionState.hadException())
+      return;
+  }
+
+  shippingOptionOutput = getSelectedShippingOption(output->shipping_options);
+
+  if (input.hasModifiers()) {
+    validateAndConvertPaymentDetailsModifiers(
+        input.modifiers(), output->modifiers, exceptionState);
+    if (exceptionState.hadException())
+      return;
+  }
 
-  for (int i = details.shippingOptions().size() - 1; i >= 0; --i) {
-    if (details.shippingOptions()[i].hasSelected() &&
-        details.shippingOptions()[i].selected()) {
-      return details.shippingOptions()[i].id();
+  if (input.hasError() && !input.error().isNull()) {
+    String errorMessage;
+    if (!PaymentsValidators::isValidErrorMsgFormat(input.error(),
+                                                   &errorMessage)) {
+      exceptionState.throwTypeError(errorMessage);
+      return;
     }
+    output->error = input.error();
+  } else {
+    output->error = "";
+  }
+}
+
+void validateAndConvertPaymentMethodData(
+    const HeapVector<PaymentMethodData>& input,
+    Vector<payments::mojom::blink::PaymentMethodDataPtr>& output,
+    ExceptionState& exceptionState) {
+  if (input.isEmpty()) {
+    exceptionState.throwTypeError(
+        "Must specify at least one payment method identifier");
+    return;
   }
 
-  return result;
+  output.resize(input.size());
+  for (size_t i = 0; i < input.size(); ++i) {
+    const auto& paymentMethodData = input[i];
+    if (paymentMethodData.supportedMethods().isEmpty()) {
+      exceptionState.throwTypeError(
+          "Must specify at least one payment method identifier");
+      return;
+    }
+
+    output[i] = payments::mojom::blink::PaymentMethodData::New();
+    output[i]->supported_methods = paymentMethodData.supportedMethods();
+
+    if (paymentMethodData.hasData()) {
+      stringifyAndParseMethodSpecificData(paymentMethodData.data(), output[i],
+                                          exceptionState);
+      if (exceptionState.hadException())
+        return;
+    } else {
+      output[i]->stringified_data = "";
+    }
+  }
 }
 
 String getValidShippingType(const String& shippingType) {
@@ -505,14 +487,6 @@ String getValidShippingType(const String& shippingType) {
   return validValues[0];
 }
 
-PaymentDetailsPtr maybeKeepShippingOptions(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:
@@ -672,7 +646,11 @@ void PaymentRequest::onUpdatePaymentDetails(
     return;
   }
 
-  bool keepShippingOptions = validatePaymentDetails(details, exceptionState);
+  PaymentDetailsPtr validatedDetails =
+      payments::mojom::blink::PaymentDetails::New();
+  validateAndConvertPaymentDetails(details, m_options.requestShipping(),
+                                   validatedDetails, m_shippingOption,
+                                   exceptionState);
   if (exceptionState.hadException()) {
     m_showResolver->reject(
         DOMException::create(SyntaxError, exceptionState.message()));
@@ -680,16 +658,7 @@ void PaymentRequest::onUpdatePaymentDetails(
     return;
   }
 
-  if (m_options.requestShipping()) {
-    if (keepShippingOptions)
-      m_shippingOption = getSelectedShippingOption(details);
-    else
-      m_shippingOption = String();
-  }
-
-  m_paymentProvider->UpdateWith(maybeKeepShippingOptions(
-      payments::mojom::blink::PaymentDetails::From(details),
-      keepShippingOptions));
+  m_paymentProvider->UpdateWith(std::move(validatedDetails));
 }
 
 void PaymentRequest::onUpdatePaymentDetailsFailure(const String& error) {
@@ -746,20 +715,21 @@ PaymentRequest::PaymentRequest(ScriptState* scriptState,
     return;
   }
 
-  bool keepShippingOptions = validatePaymentDetails(details, exceptionState);
+  PaymentDetailsPtr validatedDetails =
+      payments::mojom::blink::PaymentDetails::New();
+  validateAndConvertPaymentDetails(details, m_options.requestShipping(),
+                                   validatedDetails, m_shippingOption,
+                                   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);
+  if (m_options.requestShipping())
     m_shippingType = getValidShippingType(m_options.shippingType());
-  }
 
   scriptState->domWindow()->frame()->interfaceProvider()->getInterface(
       mojo::GetProxy(&m_paymentProvider));
@@ -768,10 +738,7 @@ PaymentRequest::PaymentRequest(ScriptState* scriptState,
                 PaymentErrorReason::UNKNOWN)));
   m_paymentProvider->Init(
       m_clientBinding.CreateInterfacePtrAndBind(),
-      std::move(validatedMethodData),
-      maybeKeepShippingOptions(
-          payments::mojom::blink::PaymentDetails::From(details),
-          keepShippingOptions && m_options.requestShipping()),
+      std::move(validatedMethodData), std::move(validatedDetails),
       payments::mojom::blink::PaymentOptions::From(m_options));
 }