[Payment Request] Error message screen

ios/chrome/browser/payments/payment_request_manager.mm

Index: ios/chrome/browser/payments/payment_request_manager.mm
diff --git a/ios/chrome/browser/payments/payment_request_manager.mm b/ios/chrome/browser/payments/payment_request_manager.mm
index 5bab9194d65f113d863ccfc166c7dc50593dbf85..4d6d9fb9cb707eda4d1201fe18759f1634a1a18c 100644
--- a/ios/chrome/browser/payments/payment_request_manager.mm
+++ b/ios/chrome/browser/payments/payment_request_manager.mm
@@ -4,6 +4,7 @@
 
 #import "ios/chrome/browser/payments/payment_request_manager.h"
 
+#include "base/ios/block_types.h"
 #include "base/ios/ios_util.h"
 #import "base/mac/bind_objc_block.h"
 #include "base/mac/foundation_util.h"
@@ -88,9 +89,13 @@ const NSTimeInterval kTimeoutInterval = 60.0;
   // Timer used to periodically unblock the webview's JS event queue.
   NSTimer* _unblockEventQueueTimer;
 
-  // Timer used to close the UI if the page does not call
-  // PaymentResponse.complete() in a timely fashion.
+  // Timer used to complete the Payment Request flow and close the UI if the
+  // page does not call PaymentResponse.complete() in a timely fashion.
   NSTimer* _paymentResponseTimeoutTimer;
+
+  // Timer used to cancel the Payment Request flow and close the UI if the
+  // page does not settle the pending update promise in a timely fashion.
+  NSTimer* _updateEventTimeoutTimer;
 }
 
 // Synchronous method executed by -asynchronouslyEnablePaymentRequest:
@@ -107,9 +112,14 @@ const NSTimeInterval kTimeoutInterval = 60.0;
 // invocation was successful.
 - (BOOL)handleRequestShow:(const base::DictionaryValue&)message;
 
+// Called by |_paymentResponseTimeoutTimer|, invokes handleResponseComplete:
+// as if PaymentResponse.complete() was invoked with the default "unknown"
+// argument.
+- (BOOL)handleResponseComplete;
+
 // Handles invocations of PaymentResponse.complete(). Returns YES if the
 // invocation was successful.
-- (BOOL)handleResponseComplete;
+- (BOOL)handleResponseComplete:(const base::DictionaryValue&)message;
 
 // Handles invocations of PaymentRequestUpdateEvent.updateWith(). Returns YES if
 // the invocation was successful.
@@ -126,6 +136,13 @@ const NSTimeInterval kTimeoutInterval = 60.0;
 // called with no arguments.
 - (void)setPaymentResponseTimeoutTimer;
 
+// Establishes a timer that dismisses the Payment Request UI when it times out.
+// Per the spec, implementations may choose to consider a timeout for the
+// promise provided with the PaymentRequestUpdateEvent.updateWith() call. If the
+// promise doesn't get settled in a reasonable amount of time, it is as if it
+// was rejected.
+- (void)setUpdateEventTimeoutTimer;
+
 @end
 
 @implementation PaymentRequestManager
@@ -275,7 +292,7 @@ const NSTimeInterval kTimeoutInterval = 60.0;
     return [self handleRequestCancel];
   }
   if (command == "paymentRequest.responseComplete") {
-    return [self handleResponseComplete];
+    return [self handleResponseComplete:JSONCommand];
   }
   if (command == "paymentRequest.updatePaymentDetails") {
     return [self handleUpdatePaymentDetails:JSONCommand];
@@ -334,22 +351,51 @@ const NSTimeInterval kTimeoutInterval = 60.0;
 
   [_unblockEventQueueTimer invalidate];
   [_paymentResponseTimeoutTimer invalidate];
+  [_updateEventTimeoutTimer invalidate];
 
-  [self cancelRequestWithErrorMessage:@"Request canceled by the page."];
+  __weak PaymentRequestManager* weakSelf = self;
+  ProceduralBlock callback = ^{
+    PaymentRequestManager* strongSelf = weakSelf;
+    [strongSelf cancelRequestWithErrorMessage:@"Request canceled."];

[lpromero, 2017/02/23 09:17:41]

Add early return if strongSelf == nil. The behavior will technically be the say
(nil-messaging will no-op here), but it's more maintainable. For example if
tomorrow this code is changed in:

PaymentRequestManager* strongSelf = weakSelf;
[strongSelf cancelRequestWithErrorMessage:@"Request canceled."];
metrics::Record("cancel");

Then you over-record the metric.

[Moe, 2017/02/23 14:44:01]

That makes sense. The base ACRify CL removes the early return in a couple of
places:
https://codereview.chromium.org/2710493006/patch/60001/70017
I'll add them back once it lands.

+  };
+
+  [_paymentRequestCoordinator displayErrorWithCallback:callback];
 
   return YES;
 }
 
 - (BOOL)handleResponseComplete {
+  base::DictionaryValue command;
+  command.SetString("result", "unknown");
+  return [self handleResponseComplete:command];
+}
+
+- (BOOL)handleResponseComplete:(const base::DictionaryValue&)message {
   // TODO(crbug.com/602666): Check that there *is* a pending response here.
-  // TODO(crbug.com/602666): Indicate success or failure in the UI.
 
   [_unblockEventQueueTimer invalidate];
   [_paymentResponseTimeoutTimer invalidate];
+  [_updateEventTimeoutTimer invalidate];
 
-  [self dismissUI];
+  std::string result;
+  if (!message.GetString("result", &result)) {
+    DLOG(ERROR) << "JS message parameter 'result' is missing";
+    return NO;
+  }
 
-  [_paymentRequestJsManager resolveResponsePromiseWithCompletionHandler:nil];
+  __weak PaymentRequestManager* weakSelf = self;
+  ProceduralBlock callback = ^{
+    PaymentRequestManager* strongSelf = weakSelf;

[lpromero, 2017/02/23 09:17:41]

Add early return if strongSelf == nil.

[Moe, 2017/02/23 14:44:01]

Done.

+    [strongSelf dismissUI];
+    [_paymentRequestJsManager resolveResponsePromiseWithCompletionHandler:nil];

[lpromero, 2017/02/23 09:17:41]

strongSelf->_paymentRequestJsManager, otherwise you retain self.

[Moe, 2017/02/23 14:44:01]

Done!

+  };
+
+  // Display UI indicating failure if the value of |result| is "fail".
+  if (result == "fail") {
+    [_paymentRequestCoordinator displayErrorWithCallback:callback];
+  } else {
+    callback();
+  }
 
   return YES;
 }
@@ -358,6 +404,7 @@ const NSTimeInterval kTimeoutInterval = 60.0;
   // TODO(crbug.com/602666): Check that there is already a pending request.
 
   [_unblockEventQueueTimer invalidate];
+  [_updateEventTimeoutTimer invalidate];
 
   const base::DictionaryValue* paymentDetailsData = nullptr;
   web::PaymentDetails paymentDetails;
@@ -393,6 +440,15 @@ const NSTimeInterval kTimeoutInterval = 60.0;
                                       repeats:NO];
 }
 
+- (void)setUpdateEventTimeoutTimer {
+  _updateEventTimeoutTimer =
+      [NSTimer scheduledTimerWithTimeInterval:kTimeoutInterval
+                                       target:self
+                                     selector:@selector(handleRequestCancel)
+                                     userInfo:nil
+                                      repeats:NO];
+}
+
 - (void)dismissUI {
   [_paymentRequestCoordinator stop];
   _paymentRequestCoordinator = nil;
@@ -419,7 +475,7 @@ const NSTimeInterval kTimeoutInterval = 60.0;
 
 - (void)paymentRequestCoordinatorDidCancel:
     (PaymentRequestCoordinator*)coordinator {
-  [self cancelRequestWithErrorMessage:@"Request canceled by user."];
+  [self cancelRequestWithErrorMessage:@"Request canceled."];
 }
 
 - (void)paymentRequestCoordinator:(PaymentRequestCoordinator*)coordinator
@@ -436,6 +492,7 @@ const NSTimeInterval kTimeoutInterval = 60.0;
   [_paymentRequestJsManager updateShippingAddress:shippingAddress
                                 completionHandler:nil];
   [self setUnblockEventQueueTimer];
+  [self setUpdateEventTimeoutTimer];
 }
 
 - (void)paymentRequestCoordinator:(PaymentRequestCoordinator*)coordinator
@@ -443,6 +500,7 @@ const NSTimeInterval kTimeoutInterval = 60.0;
   [_paymentRequestJsManager updateShippingOption:shippingOption
                                completionHandler:nil];
   [self setUnblockEventQueueTimer];
+  [self setUpdateEventTimeoutTimer];
 }
 
 #pragma mark - CRWWebStateObserver methods