Verify behavior of PaymentRequest constructor.

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 <stddef.h>
 #include <utility>
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
@@ skipping 118 matching lines @@
 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 kCompleteTimeoutSeconds = 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,
+                                         const String& item_name,
+                                         ExecutionContext& execution_context,
                                          ExceptionState& exception_state) {
-  if (!item.hasLabel() || item.label().IsEmpty()) {
-    exception_state.ThrowTypeError("Item label required");
+  DCHECK(item.hasLabel());
+  DCHECK(item.hasAmount());
+  DCHECK(item.amount().hasValue());
+  DCHECK(item.amount().hasCurrency());
+
+  String error_message;
+  if (!PaymentsValidators::IsValidAmountFormat(item.amount().value(), item_name,
+                                               &error_message)) {
+    exception_state.ThrowTypeError(error_message);
     return;
   }
 
-  if (!item.hasAmount()) {
-    exception_state.ThrowTypeError("Currency amount required");
+  if (item.label().IsEmpty()) {
+    execution_context.AddConsoleMessage(
+        ConsoleMessage::Create(kJSMessageSource, kErrorMessageLevel,

[Marijn Kruisselbrink, 2017/05/03 23:10:59]

nit: would it make sense for this message to explain why this is discouraged?

[please use gerrit instead, 2017/05/04 15:17:15]

Done.

+                               "Empty " + item_name + " label is discouraged"));
     return;
   }
 
-  if (!item.amount().hasCurrency()) {
-    exception_state.ThrowTypeError("Currency code required");
-    return;
-  }
-
-  if (!item.amount().hasValue()) {
-    exception_state.ThrowTypeError("Currency value required");
-    return;
-  }
-
-  String error_message;
   if (!PaymentsValidators::IsValidCurrencyCodeFormat(
           item.amount().currency(), item.amount().currencySystem(),
           &error_message)) {
     exception_state.ThrowTypeError(error_message);
     return;
   }
-
-  if (!PaymentsValidators::IsValidAmountFormat(item.amount().value(),
-                                               &error_message)) {
-    exception_state.ThrowTypeError(error_message);
-    return;
-  }
 }
 
 void ValidateAndConvertDisplayItems(const HeapVector<PaymentItem>& input,
+                                    const String& item_names,
                                     Vector<PaymentItemPtr>& output,
+                                    ExecutionContext& execution_context,
                                     ExceptionState& exception_state) {
   for (const PaymentItem& item : input) {
-    ValidateShippingOptionOrPaymentItem(item, exception_state);
+    ValidateShippingOptionOrPaymentItem(item, item_names, execution_context,
+                                        exception_state);
     if (exception_state.HadException())
       return;
     output.push_back(payments::mojom::blink::PaymentItem::From(item));
   }
 }
 
 // Validates and converts |input| shipping options into |output|. Throws an
 // exception if the data is not valid, except for duplicate identifiers, which
 // returns an empty |output| instead of throwing an exception. There's no need
 // to clear |output| when an exception is thrown, because the caller takes care
 // of deleting |output|.
 void ValidateAndConvertShippingOptions(
     const HeapVector<PaymentShippingOption>& input,
     Vector<PaymentShippingOptionPtr>& output,
+    String& shipping_option_output,
     ExecutionContext& execution_context,
     ExceptionState& exception_state) {
   HashSet<String> unique_ids;
   for (const PaymentShippingOption& option : input) {
-    if (!option.hasId() || option.id().IsEmpty()) {
-      exception_state.ThrowTypeError("ShippingOption id required");
+    ValidateShippingOptionOrPaymentItem(option, "shippingOptions",
+                                        execution_context, exception_state);
+    if (exception_state.HadException())
+      return;
+
+    DCHECK(option.hasId());
+    if (option.id().IsEmpty()) {
+      execution_context.AddConsoleMessage(
+          ConsoleMessage::Create(kJSMessageSource, kWarningMessageLevel,
+                                 "Empty shipping option ID is discouraged"));

[Marijn Kruisselbrink, 2017/05/03 23:10:59]

nit: same here, why is an empty ID a problem? If I read the spec right, even
though empty IDs are allowed, they still have to be unique, so only one option
can have an empty ID.

[please use gerrit instead, 2017/05/04 15:17:15]

Done.

       return;
     }
 
     if (unique_ids.Contains(option.id())) {
       execution_context.AddConsoleMessage(ConsoleMessage::Create(
           kJSMessageSource, kWarningMessageLevel,
           "Duplicate shipping option identifier '" + option.id() +
               "' is treated as an invalid address indicator."));
       // Clear |output| instead of throwing an exception.
       output.clear();
+      shipping_option_output = String();
       return;
     }
 
+    if (option.selected())
+      shipping_option_output = option.id();
+
     unique_ids.insert(option.id());
 
-    ValidateShippingOptionOrPaymentItem(option, exception_state);
-    if (exception_state.HadException())
-      return;
-
     output.push_back(
         payments::mojom::blink::PaymentShippingOption::From(option));
   }
 }
 
 void ValidateAndConvertTotal(const PaymentItem& input,
+                             const String& item_name,
                              PaymentItemPtr& output,
+                             ExecutionContext& execution_context,
                              ExceptionState& exception_state) {
-  ValidateShippingOptionOrPaymentItem(input, exception_state);
+  ValidateShippingOptionOrPaymentItem(input, item_name, execution_context,
+                                      exception_state);
   if (exception_state.HadException())
     return;
 
   if (input.amount().value()[0] == '-') {
     exception_state.ThrowTypeError("Total amount value should be non-negative");
     return;
   }
 
   output = payments::mojom::blink::PaymentItem::From(input);
 }
@@ skipping 153 matching lines @@
   }
 }
 
 void StringifyAndParseMethodSpecificData(
     const Vector<String>& supported_methods,
     const ScriptValue& input,
     PaymentMethodDataPtr& output,
     ExecutionContext& execution_context,
     ExceptionState& exception_state) {
   DCHECK(!input.IsEmpty());
-  if (!input.V8Value()->IsObject() || input.V8Value()->IsArray()) {
-    exception_state.ThrowTypeError("Data should be a JSON-serializable object");
+  v8::Local<v8::String> value;
+  if (!input.V8Value()->IsObject() || input.V8Value()->IsArray() ||

[haraken, 2017/05/04 09:47:51]

Is it worth checking IsArray? To check whether the object is serializable, I
guess we can just call v8::JSON::Stringify and see the returned value.

[please use gerrit instead, 2017/05/04 15:17:15]

Good point, the spec does prohibit arrays. Added a layout test for that as well.

+      !v8::JSON::Stringify(input.GetContext(), input.V8Value().As<v8::Object>())
+           .ToLocal(&value)) {
+    exception_state.ThrowTypeError(
+        "Payment method data should be a JSON-serializable object");
     return;
   }
 
-  v8::Local<v8::String> value;
-  if (!v8::JSON::Stringify(input.GetContext(), input.V8Value().As<v8::Object>())
-           .ToLocal(&value)) {
-    exception_state.ThrowTypeError(
-        "Unable to parse payment method specific data");
-    return;
-  }
-
   output->stringified_data =
       V8StringToWebCoreString<String>(value, kDoNotExternalize);
 
   // Serialize payment method specific data to be sent to the payment apps. The
   // payment apps are responsible for validating and processing their method
   // data asynchronously. Do not throw exceptions here.
   if (supported_methods.Contains("https://android.com/pay")) {
     SetAndroidPayMethodData(input, output, exception_state);
     if (exception_state.HadException())
       exception_state.ClearException();
   }
   if (RuntimeEnabledFeatures::paymentRequestBasicCardEnabled() &&
       supported_methods.Contains("basic-card")) {
     SetBasicCardMethodData(input, output, execution_context, exception_state);
     if (exception_state.HadException())
       exception_state.ClearException();
   }
 }
 
 void ValidateAndConvertPaymentDetailsModifiers(
     const HeapVector<PaymentDetailsModifier>& input,
     Vector<PaymentDetailsModifierPtr>& output,
     ExecutionContext& execution_context,
     ExceptionState& exception_state) {
-  if (input.IsEmpty()) {
-    exception_state.ThrowTypeError(
-        "Must specify at least one payment details modifier");
-    return;
-  }
-
   for (const PaymentDetailsModifier& modifier : input) {
     output.push_back(payments::mojom::blink::PaymentDetailsModifier::New());
     if (modifier.hasTotal()) {
-      ValidateAndConvertTotal(modifier.total(), output.back()->total,
+      ValidateAndConvertTotal(modifier.total(), "modifier total",
+                              output.back()->total, execution_context,
                               exception_state);
       if (exception_state.HadException())
         return;
     }
 
     if (modifier.hasAdditionalDisplayItems()) {
       ValidateAndConvertDisplayItems(modifier.additionalDisplayItems(),
+                                     "additional display items in modifier",
                                      output.back()->additional_display_items,
-                                     exception_state);
+                                     execution_context, exception_state);
       if (exception_state.HadException())
         return;
     }
 
     if (modifier.supportedMethods().IsEmpty()) {
       exception_state.ThrowTypeError(
           "Must specify at least one payment method identifier");
       return;
     }
 
     output.back()->method_data =
         payments::mojom::blink::PaymentMethodData::New();
     output.back()->method_data->supported_methods = modifier.supportedMethods();
 
     if (modifier.hasData() && !modifier.data().IsEmpty()) {
       StringifyAndParseMethodSpecificData(
           modifier.supportedMethods(), modifier.data(),
           output.back()->method_data, execution_context, exception_state);
     } else {
       output.back()->method_data->stringified_data = "";
     }
   }
 }
 
-String GetSelectedShippingOption(
-    const Vector<PaymentShippingOptionPtr>& shipping_options) {
-  String result;
-  for (const PaymentShippingOptionPtr& shipping_option : shipping_options) {
-    if (shipping_option->selected)
-      result = shipping_option->id;
-  }
-  return result;
-}
-
 void ValidateAndConvertPaymentDetailsBase(const PaymentDetailsBase& input,
-                                          bool request_shipping,
                                           PaymentDetailsPtr& output,
                                           String& shipping_option_output,
                                           ExecutionContext& execution_context,
                                           ExceptionState& exception_state) {
   if (input.hasDisplayItems()) {
-    ValidateAndConvertDisplayItems(input.displayItems(), output->display_items,
+    ValidateAndConvertDisplayItems(input.displayItems(), "display items",
+                                   output->display_items, execution_context,
                                    exception_state);
     if (exception_state.HadException())
       return;
   }
 
-  if (input.hasShippingOptions() && request_shipping) {
-    ValidateAndConvertShippingOptions(input.shippingOptions(),
-                                      output->shipping_options,
-                                      execution_context, exception_state);
+  if (input.hasShippingOptions()) {
+    ValidateAndConvertShippingOptions(
+        input.shippingOptions(), output->shipping_options,
+        shipping_option_output, execution_context, exception_state);
     if (exception_state.HadException())
       return;
+  } else {
+    shipping_option_output = String();
   }
 
-  shipping_option_output = GetSelectedShippingOption(output->shipping_options);
-
   if (input.hasModifiers()) {
     ValidateAndConvertPaymentDetailsModifiers(
         input.modifiers(), output->modifiers, execution_context,
         exception_state);
     if (exception_state.HadException())
       return;
   }
 }
 
 void ValidateAndConvertPaymentDetailsInit(const PaymentDetailsInit& input,
-                                          bool request_shipping,
                                           PaymentDetailsPtr& output,
                                           String& shipping_option_output,
                                           ExecutionContext& execution_context,
                                           ExceptionState& exception_state) {
-  ValidateAndConvertPaymentDetailsBase(input, request_shipping, output,
-                                       shipping_option_output,
+  DCHECK(input.hasTotal());
+  ValidateAndConvertTotal(input.total(), "total", output->total,
+                          execution_context, exception_state);
+  if (exception_state.HadException())
+    return;
+
+  ValidateAndConvertPaymentDetailsBase(input, output, shipping_option_output,
                                        execution_context, exception_state);
   if (exception_state.HadException())
     return;
-
-  if (!input.hasTotal()) {
-    exception_state.ThrowTypeError("Must specify total");
-    return;
-  }
-
-  if (input.hasId())
-    output->id = input.id();
-  else
-    output->id = CreateCanonicalUUIDString();
-
-  ValidateAndConvertTotal(input.total(), output->total, exception_state);
 }
 
 void ValidateAndConvertPaymentDetailsUpdate(const PaymentDetailsUpdate& input,
-                                            bool request_shipping,
                                             PaymentDetailsPtr& output,
                                             String& shipping_option_output,
                                             ExecutionContext& execution_context,
                                             ExceptionState& exception_state) {
-  ValidateAndConvertPaymentDetailsBase(input, request_shipping, output,
-                                       shipping_option_output,
+  ValidateAndConvertPaymentDetailsBase(input, output, shipping_option_output,
                                        execution_context, exception_state);
   if (exception_state.HadException())
     return;
 
   if (input.hasTotal()) {
-    ValidateAndConvertTotal(input.total(), output->total, exception_state);
+    ValidateAndConvertTotal(input.total(), "total", output->total,
+                            execution_context, exception_state);
     if (exception_state.HadException())
       return;
   }
 
   if (input.hasError() && !input.error().IsNull()) {
     String error_message;
     if (!PaymentsValidators::IsValidErrorMsgFormat(input.error(),
                                                    &error_message)) {
       exception_state.ThrowTypeError(error_message);
       return;
     }
     output->error = input.error();
   } else {
     output->error = "";
   }
 }
 
 void ValidateAndConvertPaymentMethodData(
     const HeapVector<PaymentMethodData>& input,
     Vector<payments::mojom::blink::PaymentMethodDataPtr>& output,
     ExecutionContext& execution_context,
     ExceptionState& exception_state) {
   if (input.IsEmpty()) {
-    exception_state.ThrowTypeError(
-        "Must specify at least one payment method identifier");
+    exception_state.ThrowTypeError("At least one payment method is required");
     return;
   }
 
   for (const PaymentMethodData payment_method_data : input) {
     if (payment_method_data.supportedMethods().IsEmpty()) {
       exception_state.ThrowTypeError(
-          "Must specify at least one payment method identifier");
+          "Each payment method needs to include at least one payment method "
+          "identifier");
       return;
     }
 
     output.push_back(payments::mojom::blink::PaymentMethodData::New());
     output.back()->supported_methods = payment_method_data.supportedMethods();
 
     if (payment_method_data.hasData() &&
         !payment_method_data.data().IsEmpty()) {
       StringifyAndParseMethodSpecificData(
           payment_method_data.supportedMethods(), payment_method_data.data(),
           output.back(), execution_context, exception_state);
     } else {
       output.back()->stringified_data = "";
     }
   }
 }
 
-String GetValidShippingType(const String& shipping_type) {
-  static const char* const kValidValues[] = {
-      "shipping", "delivery", "pickup",
-  };
-  for (size_t i = 0; i < arraysize(kValidValues); i++) {
-    if (shipping_type == kValidValues[i])
-      return shipping_type;
-  }
-  return kValidValues[0];
-}
-
 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;
 
   if (!RuntimeEnabledFeatures::featurePolicyEnabled()) {
     // 2. If |document|'s browsing context is a top-level browsing context, then
@@ skipping 173 matching lines @@
   if (exception_state.HadException()) {
     show_resolver_->Reject(
         DOMException::Create(kSyntaxError, exception_state.Message()));
     ClearResolversAndCloseMojoConnection();
     return;
   }
 
   PaymentDetailsPtr validated_details =
       payments::mojom::blink::PaymentDetails::New();
   ValidateAndConvertPaymentDetailsUpdate(
-      details, options_.requestShipping(), validated_details, shipping_option_,
-      *GetExecutionContext(), exception_state);
+      details, validated_details, shipping_option_, *GetExecutionContext(),
+      exception_state);
   if (exception_state.HadException()) {
     show_resolver_->Reject(
         DOMException::Create(kSyntaxError, exception_state.Message()));
     ClearResolversAndCloseMojoConnection();
     return;
   }
 
+  if (!options_.requestShipping())
+    validated_details->shipping_options.clear();
+
   payment_provider_->UpdateWith(std::move(validated_details));
 }
 
 void PaymentRequest::OnUpdatePaymentDetailsFailure(const String& error) {
   if (show_resolver_)
     show_resolver_->Reject(DOMException::Create(kAbortError, error));
   if (complete_resolver_)
     complete_resolver_->Reject(DOMException::Create(kAbortError, error));
   ClearResolversAndCloseMojoConnection();
 }
@@ skipping 19 matching lines @@
                                const PaymentDetailsInit& details,
                                const PaymentOptions& options,
                                ExceptionState& exception_state)
     : ContextLifecycleObserver(execution_context),
       options_(options),
       client_binding_(this),
       complete_timer_(
           TaskRunnerHelper::Get(TaskType::kMiscPlatformAPI, GetFrame()),
           this,
           &PaymentRequest::OnCompleteTimeout) {
-  Vector<payments::mojom::blink::PaymentMethodDataPtr> validated_method_data;
-  ValidateAndConvertPaymentMethodData(method_data, validated_method_data,
-                                      *GetExecutionContext(), exception_state);
-  if (exception_state.HadException())
-    return;
-
   if (!GetExecutionContext()->IsSecureContext()) {
     exception_state.ThrowSecurityError("Must be in a secure context");
     return;
   }
 
   if (!AllowedToUsePaymentRequest(GetFrame())) {
     exception_state.ThrowSecurityError(
         "Must be in a top-level browsing context or an iframe needs to specify "
         "'allowpaymentrequest' explicitly");
     return;
   }
 
   PaymentDetailsPtr validated_details =
       payments::mojom::blink::PaymentDetails::New();
-  ValidateAndConvertPaymentDetailsInit(details, options_.requestShipping(),
-                                       validated_details, shipping_option_,
-                                       *GetExecutionContext(), exception_state);
+  validated_details->id = id_ =
+      details.hasId() ? details.id() : CreateCanonicalUUIDString();
+
+  Vector<payments::mojom::blink::PaymentMethodDataPtr> validated_method_data;
+  ValidateAndConvertPaymentMethodData(method_data, validated_method_data,
+                                      *GetExecutionContext(), exception_state);
   if (exception_state.HadException())
     return;
 
-  id_ = validated_details->id;
+  ValidateAndConvertPaymentDetailsInit(details, validated_details,
+                                       shipping_option_, *GetExecutionContext(),
+                                       exception_state);
+  if (exception_state.HadException())
+    return;
 
   if (options_.requestShipping())
-    shipping_type_ = GetValidShippingType(options_.shippingType());
+    shipping_type_ = options_.shippingType();
+  else
+    validated_details->shipping_options.clear();
+
+  DCHECK(shipping_type_.IsNull() || shipping_type_ == "shipping" ||
+         shipping_type_ == "delivery" || shipping_type_ == "pickup");
 
   GetFrame()->GetInterfaceProvider()->GetInterface(
       mojo::MakeRequest(&payment_provider_));
   payment_provider_.set_connection_error_handler(ConvertToBaseCallback(
       WTF::Bind(&PaymentRequest::OnError, WrapWeakPersistent(this),
                 PaymentErrorReason::UNKNOWN)));
   payment_provider_->Init(
       client_binding_.CreateInterfacePtrAndBind(),
       std::move(validated_method_data), std::move(validated_details),
       payments::mojom::blink::PaymentOptions::From(options_));
@@ skipping 185 matching lines @@
   complete_resolver_.Clear();
   show_resolver_.Clear();
   abort_resolver_.Clear();
   can_make_payment_resolver_.Clear();
   if (client_binding_.is_bound())
     client_binding_.Close();
   payment_provider_.reset();
 }
 
 }  // namespace blink