[Payment Request] Uses PaymentRequest's id property to identify it

Uses the id property of the PaymentDetails provided to PaymentRequest's
constructor or generate a UUID to uniquely identify the PaymentRequest
in order to allow multiple PaymentRequest objects to coexist.

BUG=602666

Change-Id: I420e9fba0314cc9683b274fea71e0118b31e572e
Reviewed-on: https://chromium-review.googlesource.com/565056
Commit-Queue: mahmadi <mahmadi@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486265}

ios/chrome/browser/payments/payment_request.h

@@ -75,6 +75,13 @@ class PaymentRequest : public PaymentOptionsProvider,
                  id<PaymentRequestUIDelegate> payment_request_ui_delegate);
   ~PaymentRequest() override;
 
+  // Functor used as a simplified comparison function for unique pointers to
+  // PaymentRequest. Only compares |web_payment_request_.payment_request_id|.
+  struct Compare {
+    bool operator()(const std::unique_ptr<PaymentRequest>& lhs,
+                    const std::unique_ptr<PaymentRequest>& rhs) const;
+  };
+
   // PaymentRequestBaseDelegate:
   autofill::PersonalDataManager* GetPersonalDataManager() override;
   const std::string& GetApplicationLocale() const override;

ios/chrome/browser/payments/payment_request.mm

@@ -127,6 +127,13 @@ PaymentRequest::PaymentRequest(
 
 PaymentRequest::~PaymentRequest() {}
 
+bool PaymentRequest::Compare::operator()(
+    const std::unique_ptr<PaymentRequest>& lhs,
+    const std::unique_ptr<PaymentRequest>& rhs) const {
+  return lhs->web_payment_request().payment_request_id !=
+         rhs->web_payment_request().payment_request_id;
+}
+
 autofill::PersonalDataManager* PaymentRequest::GetPersonalDataManager() {
   return personal_data_manager_;
 }

ios/chrome/browser/ui/payments/payment_request_manager.mm

@@ -5,9 +5,9 @@
 #import "ios/chrome/browser/ui/payments/payment_request_manager.h"
 
 #include <memory>
+#include <set>
 #include <string>
 #include <unordered_map>
-#include <vector>
 
 #include "base/ios/block_types.h"
 #include "base/ios/ios_util.h"
@@ -97,7 +97,8 @@ struct PendingPaymentResponse {
   // The of map WebState to the list of payments::PaymentRequest instances
   // maintained for that WebState.
   std::unordered_map<web::WebState*,
-                     std::vector<std::unique_ptr<payments::PaymentRequest>>>
+                     std::set<std::unique_ptr<payments::PaymentRequest>,
+                              payments::PaymentRequest::Compare>>
       _paymentRequests;
 
   // The observer for |_activeWebState|.
@@ -150,6 +151,11 @@ struct PendingPaymentResponse {
 // Handler for injected JavaScript callbacks.
 - (BOOL)handleScriptCommand:(const base::DictionaryValue&)JSONCommand;
 
+// Handles creation of a PaymentRequest instance. The value of the JavaScript
+// PaymentRequest object should be provided in |message|. Returns YES if the
+// invocation was successful.
+- (BOOL)handleCreatePaymentRequest:(const base::DictionaryValue&)message;
+
 // Handles invocations of PaymentRequest.show(). The value of the JavaScript
 // PaymentRequest object should be provided in |message|. Returns YES if the
 // invocation was successful.
@@ -244,7 +250,8 @@ struct PendingPaymentResponse {
     _activeWebState = webState;
     if (_paymentRequests.find(webState) == _paymentRequests.end()) {
       _paymentRequests[webState] =
-          std::vector<std::unique_ptr<payments::PaymentRequest>>();
+          std::set<std::unique_ptr<payments::PaymentRequest>,
+                   payments::PaymentRequest::Compare>();
     }
     _activeWebStateObserver =
         base::MakeUnique<web::WebStateObserverBridge>(webState, self);
@@ -351,6 +358,9 @@ struct PendingPaymentResponse {
     return NO;
   }
 
+  if (command == "paymentRequest.createPaymentRequest") {
+    return [self handleCreatePaymentRequest:JSONCommand];
+  }
   if (command == "paymentRequest.requestShow") {
     return [self handleRequestShow:JSONCommand];
   }
@@ -369,12 +379,40 @@ struct PendingPaymentResponse {
   return NO;
 }
 
-// Extracts a web::PaymentRequest from |message|. Returns the instance of
+// Extracts a web::PaymentRequest from |message|. Creates and returns an
+// instance of payments::PaymentRequest which is initialized with the
+// web::PaymentRequest object. Returns nullptr if it cannot extract a
+// web::PaymentRequest from |message|.
+- (payments::PaymentRequest*)newPaymentRequestFromMessage:
+    (const base::DictionaryValue&)message {
+  const base::DictionaryValue* paymentRequestData;
+  web::PaymentRequest webPaymentRequest;
+  if (!message.GetDictionary("payment_request", &paymentRequestData)) {
+    DLOG(ERROR) << "JS message parameter 'payment_request' is missing";
+    return nullptr;
+  }
+  if (!webPaymentRequest.FromDictionaryValue(*paymentRequestData)) {
+    DLOG(ERROR) << "JS message parameter 'payment_request' is invalid";
+    return nullptr;
+  }
+
+  const auto iterator = _paymentRequests.find(_activeWebState);
+  DCHECK(iterator != _paymentRequests.end());
+  const auto result =
+      iterator->second.insert(base::MakeUnique<payments::PaymentRequest>(
+          webPaymentRequest, _browserState, _activeWebState,
+          _personalDataManager, self));
+  DCHECK(result.first != iterator->second.end());
+  return result.first->get();
+}
+
+// Extracts a web::PaymentRequest from |message|. Returns the cached instance of
 // payments::PaymentRequest that corresponds to the extracted
 // web::PaymentRequest object, if one exists. Otherwise, creates and returns a
 // new one which is initialized with the web::PaymentRequest object. Returns
-// nullptr if it cannot extract a web::PaymentRequest from |message|.
-- (payments::PaymentRequest*)getOrCreatePaymentRequestFromMessage:
+// nullptr if it cannot extract a web::PaymentRequest from |message| or cannot
+// find the payments::PaymentRequest instance.
+- (payments::PaymentRequest*)paymentRequestFromMessage:
     (const base::DictionaryValue&)message {
   const base::DictionaryValue* paymentRequestData;
   web::PaymentRequest webPaymentRequest;
@@ -387,22 +425,27 @@ struct PendingPaymentResponse {
     return nullptr;
   }
 
+  std::unique_ptr<payments::PaymentRequest> temporaryPaymentRequest =
+      base::MakeUnique<payments::PaymentRequest>(webPaymentRequest,
+                                                 _browserState, _activeWebState,
+                                                 _personalDataManager, self);
   const auto iterator = _paymentRequests.find(_activeWebState);
   DCHECK(iterator != _paymentRequests.end());
-  const auto found = std::find_if(
-      iterator->second.begin(), iterator->second.end(),
-      [webPaymentRequest](
-          const std::unique_ptr<payments::PaymentRequest>& paymentRequest) {
-        return paymentRequest->web_payment_request() == webPaymentRequest;
-      });
-  if (found != iterator->second.end()) {
-    return (*found).get();
-  }
+  const auto found = iterator->second.find(temporaryPaymentRequest);
+  return found != iterator->second.end() ? found->get() : nullptr;
+}
 
-  iterator->second.push_back(base::MakeUnique<payments::PaymentRequest>(
-      webPaymentRequest, _browserState, _activeWebState, _personalDataManager,
-      self));
-  return iterator->second.back().get();
+- (BOOL)handleCreatePaymentRequest:(const base::DictionaryValue&)message {
+  payments::PaymentRequest* paymentRequest =
+      [self newPaymentRequestFromMessage:message];
+  if (!paymentRequest) {
+    // TODO(crbug.com/602666): Reject the promise with an error of
+    // "InvalidStateError" type.
+    [_paymentRequestJsManager
+        rejectCanMakePaymentPromiseWithErrorMessage:@"Invalid state error"
+                                  completionHandler:nil];
+  }
+  return YES;
 }
 
 - (BOOL)handleRequestShow:(const base::DictionaryValue&)message {
@@ -412,9 +455,14 @@ struct PendingPaymentResponse {
   //   if the intersection is empty.
 
   payments::PaymentRequest* paymentRequest =
-      [self getOrCreatePaymentRequestFromMessage:message];
+      [self paymentRequestFromMessage:message];
   if (!paymentRequest) {
-    return NO;
+    // TODO(crbug.com/602666): Reject the promise with an error of
+    // "InvalidStateError" type.
+    [_paymentRequestJsManager
+        rejectCanMakePaymentPromiseWithErrorMessage:@"Invalid state error"
+                                  completionHandler:nil];
+    return YES;
   }
 
   UIImage* pageFavicon = nil;
@@ -472,7 +520,7 @@ struct PendingPaymentResponse {
 
 - (BOOL)handleCanMakePayment:(const base::DictionaryValue&)message {
   payments::PaymentRequest* paymentRequest =
-      [self getOrCreatePaymentRequestFromMessage:message];
+      [self paymentRequestFromMessage:message];
   if (!paymentRequest) {
     // TODO(crbug.com/602666): Reject the promise with an error of
     // "InvalidStateError" type.
@@ -723,6 +771,9 @@ requestFullCreditCard:(const autofill::CreditCard&)creditCard
     (payments::PaymentRequest*)paymentRequest {
   web::PaymentResponse paymentResponse;
 
+  paymentResponse.payment_request_id =
+      paymentRequest->web_payment_request().payment_request_id;
+
   paymentResponse.method_name =
       base::ASCIIToUTF16(_pendingPaymentResponse.methodName);
 

ios/chrome/browser/web/resources/payment_request.js

@@ -161,6 +161,24 @@ var SerializedPaymentResponse;
   // been defined.
   __gCrWeb.paymentRequestManager = {};
 
+  /**
+   * Generates a random string identfier resembling a GUID.
+   * @return {string}
+   */
+  __gCrWeb['paymentRequestManager'].guid = function() {
+    /**
+     * Generates a stringified random 4 digit hexadecimal number.
+     * @return {string}
+     */
+    var s4 = function() {
+      return Math.floor((1 + Math.random()) * 0x10000)
+          .toString(16)
+          .substring(1);
+    };
+    return s4() + s4() + '-' + s4() + '-' + s4() + '-' + s4() + '-' + s4() +
+        s4() + s4();
+  };
+
   // Store paymentRequestManager namespace object in a global __gCrWeb object
   // referenced by a string, so it does not get renamed by closure compiler
   // during
@@ -203,8 +221,8 @@ var SerializedPaymentResponse;
   __gCrWeb['paymentRequestManager'].parsePaymentResponseData = function(
       paymentResponseData) {
     var response = new window.PaymentResponse(
-        paymentResponseData['paymentRequestID'],
-        paymentResponseData['methodName'], paymentResponseData['details']);
+        paymentResponseData['requestId'], paymentResponseData['methodName'],
+        paymentResponseData['details']);
     if (paymentResponseData['shippingAddress'])
       response.shippingAddress = paymentResponseData['shippingAddress'];
     if (paymentResponseData['shippingOption'])
@@ -377,6 +395,7 @@ var SerializedPaymentResponse;
   __gCrWeb['paymentRequestManager'].serializePaymentRequest = function(
       paymentRequest) {
     var serialized = {
+      'id': paymentRequest.id,
       'methodData': paymentRequest.methodData,
       'details': paymentRequest.details,
     };
@@ -551,10 +570,9 @@ window.PaymentRequest = function(methodData, details, opt_options) {
 
   /**
    * A provided or generated ID for the this Payment Request instance.
-   * TODO(crbug.com/602666): Generate an ID if one is not provided.
-   * @type {?string}
+   * @type {string}
    */
-  this.paymentRequestID = null;
+  this.id = details.id ? details.id : __gCrWeb['paymentRequestManager'].guid();
 
   /**
    * Shipping address selected by the user.
@@ -582,6 +600,13 @@ window.PaymentRequest = function(methodData, details, opt_options) {
       this.shippingType = PaymentShippingType.SHIPPING;
     }
   }
+
+  var message = {
+    'command': 'paymentRequest.createPaymentRequest',
+    'payment_request':
+        __gCrWeb['paymentRequestManager'].serializePaymentRequest(this),
+  };
+  __gCrWeb.message.invokeOnHost(message);
 };
 
 window.PaymentRequest.prototype = {
@@ -675,6 +700,7 @@ window.PaymentCurrencyAmount;
 
 /**
  * @typedef {{
+ *   id: (string|undefined),
  *   total: (window.PaymentItem|undefined),
  *   displayItems: (!Array<!window.PaymentItem>|undefined),
  *   shippingOptions: (!Array<!window.PaymentShippingOption>|undefined),
@@ -761,12 +787,12 @@ window.PaymentShippingOption;
  * @constructor
  * @private
  */
-window.PaymentResponse = function(paymentRequestID, methodName, details) {
+window.PaymentResponse = function(requestId, methodName, details) {
   /**
-   * The same paymentRequestID present in the original window.PaymentRequest.
+   * The same identifier present in the original window.PaymentRequest.
    * @type {string}
    */
-  this.paymentRequestID = paymentRequestID;
+  this.requestId = requestId;
 
   /**
    * The payment method identifier for the payment method that the user selected
@@ -789,7 +815,7 @@ window.PaymentResponse = function(paymentRequestID, methodName, details) {
    * final shipping address chosen by the user.
    * @type {?window.PaymentAddress}
    */
-   this.shippingAddress = null;
+  this.shippingAddress = null;
 
   /**
    * If the requestShipping flag was set to true in the window.PaymentOptions
@@ -797,7 +823,7 @@ window.PaymentResponse = function(paymentRequestID, methodName, details) {
    * attribute of the selected shipping option.
    * @type {?string}
    */
-   this.shippingOption = null;
+  this.shippingOption = null;
 
   /**
    * If the requestPayerName flag was set to true in the window.PaymentOptions
@@ -805,7 +831,7 @@ window.PaymentResponse = function(paymentRequestID, methodName, details) {
    * provided by the user.
    * @type {?string}
    */
-   this.payerName = null;
+  this.payerName = null;
 
   /**
    * If the requestPayerEmail flag was set to true in the window.PaymentOptions
@@ -813,7 +839,7 @@ window.PaymentResponse = function(paymentRequestID, methodName, details) {
    * address chosen by the user.
    * @type {?string}
    */
-   this.payerEmail = null;
+  this.payerEmail = null;
 
   /**
    * If the requestPayerPhone flag was set to true in the window.PaymentOptions
@@ -821,7 +847,7 @@ window.PaymentResponse = function(paymentRequestID, methodName, details) {
    * number chosen by the user.
    * @type {?string}
    */
-   this.payerPhone = null;
+  this.payerPhone = null;
 };
 
 /**

ios/web/payments/payment_request.cc

@@ -18,6 +18,7 @@ static const char kPaymentCurrencyAmountCurrencySystemISO4217[] =
 static const char kPaymentCurrencyAmountCurrencySystem[] = "currencySystem";
 static const char kPaymentCurrencyAmountCurrency[] = "currency";
 static const char kPaymentCurrencyAmountValue[] = "value";
+static const char kPaymentDetailsId[] = "id";
 static const char kPaymentDetailsDisplayItems[] = "displayItems";
 static const char kPaymentDetailsError[] = "error";
 static const char kPaymentDetailsShippingOptions[] = "shippingOptions";
@@ -33,9 +34,10 @@ static const char kPaymentOptionsShippingTypeDelivery[] = "delivery";
 static const char kPaymentOptionsShippingTypePickup[] = "pickup";
 static const char kPaymentOptionsShippingType[] = "shippingType";
 static const char kPaymentRequestDetails[] = "details";
-static const char kPaymentRequestId[] = "paymentRequestID";
+static const char kPaymentRequestId[] = "id";
 static const char kPaymentRequestMethodData[] = "methodData";
 static const char kPaymentRequestOptions[] = "options";
+static const char kPaymentResponseId[] = "requestId";
 static const char kPaymentResponseDetails[] = "details";
 static const char kPaymentResponseMethodName[] = "methodName";
 static const char kPaymentResponsePayerEmail[] = "payerEmail";
@@ -179,7 +181,7 @@ PaymentDetails::PaymentDetails(const PaymentDetails& other) = default;
 PaymentDetails::~PaymentDetails() = default;
 
 bool PaymentDetails::operator==(const PaymentDetails& other) const {
-  return this->total == other.total &&
+  return this->id == other.id && this->total == other.total &&
          this->display_items == other.display_items &&
          this->shipping_options == other.shipping_options &&
          this->modifiers == other.modifiers && this->error == other.error;
@@ -195,6 +197,9 @@ bool PaymentDetails::FromDictionaryValue(const base::DictionaryValue& value,
   this->shipping_options.clear();
   this->modifiers.clear();
 
+  // ID is optional.
+  value.GetString(kPaymentDetailsId, &this->id);
+
   const base::DictionaryValue* total_dict = nullptr;
   if (!value.GetDictionary(kPaymentDetailsTotal, &total_dict) &&
       requires_total) {
@@ -305,6 +310,10 @@ bool PaymentRequest::operator!=(const PaymentRequest& other) const {
 bool PaymentRequest::FromDictionaryValue(const base::DictionaryValue& value) {
   this->method_data.clear();
 
+  if (!value.GetString(kPaymentRequestId, &this->payment_request_id)) {
+    return false;
+  }
+
   // Parse the payment method data.
   const base::ListValue* method_data_list = nullptr;
   // At least one method is required.
@@ -364,7 +373,7 @@ std::unique_ptr<base::DictionaryValue> PaymentResponse::ToDictionaryValue()
     const {
   std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue());
 
-  result->SetString(kPaymentRequestId, this->payment_request_id);
+  result->SetString(kPaymentResponseId, this->payment_request_id);
   result->SetString(kPaymentResponseMethodName, this->method_name);
   // |this.details| is a json-serialized string. Parse it to a base::Value so
   // that when |this| is converted to a JSON string, |this.details| won't get

ios/web/payments/payment_request_unittest.cc

@@ -252,6 +252,8 @@ TEST(PaymentRequestTest, ParsingFullyPopulatedRequestDictionarySucceeds) {
   base::DictionaryValue request_dict;
 
   // Add the expected values to expected_request.
+  expected_request.payment_request_id = "123456789";
+  expected_request.details.id = "12345";
   expected_request.details.total.label = base::ASCIIToUTF16("TOTAL");
   expected_request.details.total.amount.currency = base::ASCIIToUTF16("GBP");
   expected_request.details.total.amount.value = base::ASCIIToUTF16("6.66");
@@ -273,6 +275,7 @@ TEST(PaymentRequestTest, ParsingFullyPopulatedRequestDictionarySucceeds) {
   amount_dict->SetString("value", "6.66");
   total_dict->Set("amount", std::move(amount_dict));
   details_dict->Set("total", std::move(total_dict));
+  details_dict->SetString("id", "12345");
   details_dict->SetString("error", "Error in details");
   request_dict.Set("details", std::move(details_dict));
 
@@ -284,6 +287,7 @@ TEST(PaymentRequestTest, ParsingFullyPopulatedRequestDictionarySucceeds) {
   method_data_dict->Set("supportedMethods", std::move(supported_methods_list));
   method_data_list->Append(std::move(method_data_dict));
   request_dict.Set("methodData", std::move(method_data_list));
+  request_dict.SetString("id", "123456789");
 
   // With the required values present, parsing should succeed.
   EXPECT_TRUE(output_request.FromDictionaryValue(request_dict));
@@ -313,7 +317,7 @@ TEST(PaymentRequestTest, ParsingFullyPopulatedRequestDictionarySucceeds) {
 TEST(PaymentRequestTest, EmptyResponseDictionary) {
   base::DictionaryValue expected_value;
 
-  expected_value.SetString("paymentRequestID", "");
+  expected_value.SetString("requestId", "");
   expected_value.SetString("methodName", "");
 
   PaymentResponse payment_response;
@@ -337,7 +341,7 @@ TEST(PaymentRequestTest, PopulatedResponseDictionary) {
   billing_address->SetString("postalCode", "90210");
   details->Set("billingAddress", std::move(billing_address));
   expected_value.Set("details", std::move(details));
-  expected_value.SetString("paymentRequestID", "12345");
+  expected_value.SetString("requestId", "12345");
   expected_value.SetString("methodName", "American Express");
   std::unique_ptr<base::DictionaryValue> shipping_address(
       new base::DictionaryValue);
@@ -349,7 +353,7 @@ TEST(PaymentRequestTest, PopulatedResponseDictionary) {
   expected_value.SetString("payerPhone", "1234-567-890");
 
   PaymentResponse payment_response;
-  payment_response.payment_request_id = base::ASCIIToUTF16("12345");
+  payment_response.payment_request_id = "12345";
   payment_response.method_name = base::ASCIIToUTF16("American Express");
 
   payments::BasicCardResponse payment_response_details;
@@ -518,6 +522,13 @@ TEST(PaymentRequestTest, PaymentDetailsEquality) {
   PaymentDetails details2;
   EXPECT_EQ(details1, details2);
 
+  details1.id = "12345";
+  EXPECT_NE(details1, details2);
+  details2.id = "54321";
+  EXPECT_NE(details1, details2);
+  details2.id = details1.id;
+  EXPECT_EQ(details1, details2);
+
   details1.total.label = base::ASCIIToUTF16("Total");
   EXPECT_NE(details1, details2);
   details2.total.label = base::ASCIIToUTF16("Shipping");
@@ -619,6 +630,13 @@ TEST(PaymentRequestTest, PaymentRequestEquality) {
   PaymentRequest request2;
   EXPECT_EQ(request1, request2);
 
+  request1.payment_request_id = "12345";
+  EXPECT_NE(request1, request2);
+  request2.payment_request_id = "54321";
+  EXPECT_NE(request1, request2);
+  request2.payment_request_id = request1.payment_request_id;
+  EXPECT_EQ(request1, request2);
+
   payments::PaymentAddress address1;
   address1.recipient = base::ASCIIToUTF16("Jessica Jones");
   request1.shipping_address = address1;

ios/web/public/payments/payment_request.h

@@ -6,6 +6,7 @@
 #define IOS_WEB_PUBLIC_PAYMENTS_PAYMENT_REQUEST_H_
 
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "base/strings/string16.h"
@@ -152,6 +153,9 @@ class PaymentDetails {
   bool FromDictionaryValue(const base::DictionaryValue& value,
                            bool requires_total);
 
+  // The unique free-form identifier for this payment request.
+  std::string id;
+
   // The total amount of the payment request.
   PaymentItem total;
 
@@ -230,7 +234,7 @@ class PaymentRequest {
 
   // The unique ID for this PaymentRequest. If it is not provided during
   // construction, one is generated.
-  base::string16 payment_request_id;
+  std::string payment_request_id;
 
   // Properties set in order to communicate user choices back to the page.
   payments::PaymentAddress shipping_address;
@@ -257,8 +261,8 @@ class PaymentResponse {
   // Populates |value| with the properties of this PaymentResponse.
   std::unique_ptr<base::DictionaryValue> ToDictionaryValue() const;
 
-  // The same paymentRequestID present in the original PaymentRequest.
-  base::string16 payment_request_id;
+  // The same ID present in the original PaymentRequest.
+  std::string payment_request_id;
 
   // The payment method identifier for the payment method that the user selected
   // to fulfil the transaction.