Don't overwrite PaymentDetails

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 ec17e2590d68417016122e1c7c43ab543c12cda2..b2fddf376d062958cbeed56fef3f246d8327efc7 100644
--- a/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
+++ b/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp
@@ -223,25 +223,27 @@ void validateDisplayItems(const HeapVector<PaymentItem>& items,
   }
 }
 
-void validateAndFixupShippingOptions(HeapVector<PaymentShippingOption>& options,
-                                     ExceptionState& exceptionState) {
+// Returns false if |options| should be ignored without throwing an exception.
+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;
+      return true;
     }
 
-    if (uniqueIds.contains(option.id())) {
-      options = HeapVector<PaymentShippingOption>();
-      return;
-    }
+    if (uniqueIds.contains(option.id()))
+      return false;
+
     uniqueIds.add(option.id());
 
     validateShippingOptionOrPaymentItem(option, exceptionState);
     if (exceptionState.hadException())
-      return;
+      return true;
   }
+
+  return true;
 }
 
 void validatePaymentDetailsModifiers(
@@ -290,35 +292,36 @@ void validatePaymentDetailsModifiers(
   }
 }
 
-void validateAndFixupPaymentDetails(PaymentDetails& details,
-                                    ExceptionState& exceptionState) {
+// Returns false if the shipping options should be ignored without throwing an
+// exception.

[Mathieu, 2016/10/18 14:13:00]

It took me a few passes to understand this pattern. Perhaps you could expand on
the comment here or at the callsite. Also it's a bit jarring to see exceptions
being thrown and "return true;" right after, but I understand the need... 

[please use gerrit instead, 2016/10/19 15:44:34]

Done.

+bool validatePaymentDetails(const PaymentDetails& details,
+                            ExceptionState& exceptionState) {
   if (!details.hasTotal()) {
     exceptionState.throwTypeError("Must specify total");
-    return;
+    return true;
   }
 
   validateShippingOptionOrPaymentItem(details.total(), exceptionState);
   if (exceptionState.hadException())
-    return;
+    return true;
 
   if (details.total().amount().value()[0] == '-') {
     exceptionState.throwTypeError("Total amount value should be non-negative");
-    return;
+    return true;
   }
 
   if (details.hasDisplayItems()) {
     validateDisplayItems(details.displayItems(), exceptionState);
     if (exceptionState.hadException())
-      return;
+      return true;
   }
 
   if (details.hasShippingOptions()) {
-    HeapVector<PaymentShippingOption> fixedShippingOptions =
-        details.shippingOptions();
-    validateAndFixupShippingOptions(fixedShippingOptions, exceptionState);
-    details.setShippingOptions(fixedShippingOptions);
+    if (!validateShippingOptions(details.shippingOptions(), exceptionState))
+      return false;
+
     if (exceptionState.hadException())
-      return;
+      return true;
   }
 
   if (details.hasModifiers()) {
@@ -329,8 +332,9 @@ void validateAndFixupPaymentDetails(PaymentDetails& details,
   if (!PaymentsValidators::isValidErrorMsgFormat(details.error(),
                                                  &errorMessage)) {
     exceptionState.throwTypeError(errorMessage);
-    return;
   }
+
+  return true;
 }
 
 void validateAndConvertPaymentMethodData(
@@ -409,6 +413,15 @@ 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;
+}
+
 }  // namespace
 
 PaymentRequest* PaymentRequest::create(
@@ -526,7 +539,7 @@ void PaymentRequest::onUpdatePaymentDetails(
     return;
   }
 
-  validateAndFixupPaymentDetails(details, exceptionState);
+  bool keepShippingOptions = validatePaymentDetails(details, exceptionState);
   if (exceptionState.hadException()) {
     m_showResolver->reject(
         DOMException::create(SyntaxError, exceptionState.message()));
@@ -534,10 +547,15 @@ void PaymentRequest::onUpdatePaymentDetails(
     return;
   }
 
-  if (m_options.requestShipping())
-    m_shippingOption = getSelectedShippingOption(details);
+  if (m_options.requestShipping()) {
+    if (keepShippingOptions)
+      m_shippingOption = getSelectedShippingOption(details);
+    else
+      m_shippingOption = String();
+  }
 
-  m_paymentProvider->UpdateWith(mojom::blink::PaymentDetails::From(details));
+  m_paymentProvider->UpdateWith(maybeKeepShippingOptions(
+      mojom::blink::PaymentDetails::From(details), keepShippingOptions));
 }
 
 void PaymentRequest::onUpdatePaymentDetailsFailure(const String& error) {
@@ -591,18 +609,18 @@ PaymentRequest::PaymentRequest(ScriptState* scriptState,
     return;
   }
 
-  PaymentDetails fixedDetails(details);
-  validateAndFixupPaymentDetails(fixedDetails, exceptionState);
+  bool keepShippingOptions = validatePaymentDetails(details, exceptionState);
   if (exceptionState.hadException())
     return;
 
-  if (fixedDetails.hasError() && !fixedDetails.error().isEmpty()) {
+  if (details.hasError() && !details.error().isEmpty()) {
     exceptionState.throwTypeError("Error value should be empty");
     return;
   }
 
   if (m_options.requestShipping()) {
-    m_shippingOption = getSelectedShippingOption(fixedDetails);
+    if (keepShippingOptions)
+      m_shippingOption = getSelectedShippingOption(details);
     m_shippingType = getValidShippingType(m_options.shippingType());
   }
 
@@ -615,7 +633,8 @@ PaymentRequest::PaymentRequest(ScriptState* scriptState,
       m_clientBinding.CreateInterfacePtrAndBind(),
       mojo::WTFArray<mojom::blink::PaymentMethodDataPtr>::From(
           validatedMethodData),
-      mojom::blink::PaymentDetails::From(fixedDetails),
+      maybeKeepShippingOptions(mojom::blink::PaymentDetails::From(details),
+                               keepShippingOptions),
       mojom::blink::PaymentOptions::From(m_options));
 }