[Payments] Improve the closing of the PR dialog.

components/payments/payment_request.cc

Index: components/payments/payment_request.cc
diff --git a/components/payments/payment_request.cc b/components/payments/payment_request.cc
index 282b3fa38afad3d8ef83f6db57f9f05c532ca0ad..912fc258519d0f0023ed983743d339c5546c9a96 100644
--- a/components/payments/payment_request.cc
+++ b/components/payments/payment_request.cc
@@ -23,8 +23,13 @@ PaymentRequest::PaymentRequest(
       delegate_(std::move(delegate)),
       manager_(manager),
       binding_(this, std::move(request)) {
-  binding_.set_connection_error_handler(
-      base::Bind(&PaymentRequest::OnError, base::Unretained(this)));
+  // OnConnectionTerminated will be called when the Mojo pipe is closed. This
+  // will happen as a result of many renderer-side events (both successful and
+  // erroneous in nature).
+  // TODO(crbug.com/683636): Investigate using
+  // set_connection_error_with_reason_handler with Binding::CloseWithReason.
+  binding_.set_connection_error_handler(base::Bind(
+      &PaymentRequest::OnConnectionTerminated, base::Unretained(this)));
 }
 
 PaymentRequest::~PaymentRequest() {}
@@ -38,8 +43,7 @@ void PaymentRequest::Init(
   std::string error;
   if (!payments::validatePaymentDetails(details, &error)) {
     LOG(ERROR) << error;
-    OnError();
-    client_.reset();
+    OnConnectionTerminated();
     return;
   }
   client_ = std::move(client);
@@ -48,18 +52,45 @@ void PaymentRequest::Init(
 
 void PaymentRequest::Show() {
   if (!client_.is_bound() || !binding_.is_bound()) {

[please use gerrit instead, 2017/01/23 21:18:50]

Nit for the future: If the dialog is already showing, it would be nice to
terminate the connection to the renderer and close the dialog.

[Mathieu, 2017/01/23 21:53:57]

Acknowledged.

-    OnError();
+    LOG(ERROR) << "Attempted Show(), but binding(s) missing.";
+    OnConnectionTerminated();
     return;
   }
-  delegate_->ShowPaymentRequestDialog(this);
+  delegate_->ShowDialog(this);
 }
 
-void PaymentRequest::Cancel() {
+void PaymentRequest::Abort() {
+  // The API user has decided to abort. We return a successful abort message to
+  // the renderer, which closes the Mojo message pipe, which triggers
+  // PaymentRequest::OnConnectionTerminated, which destroys this object.
+  if (client_.is_bound())
+    client_->OnAbort(true /* aborted_successfully */);
+}
+
+void PaymentRequest::UserCancelled() {
+  // If |client_| is not bound, then the object is already being destroyed as
+  // a result of a renderer event.
+  if (!client_.is_bound())
+    return;
+
+  // This sends an error to the renderer, which informs the API user.
   client_->OnError(payments::mojom::PaymentErrorReason::USER_CANCEL);
+
+  // We close all bindings and ask to be destroyed.
+  client_.reset();
+  binding_.Close();
+  manager_->DestroyRequest(this);

[please use gerrit instead, 2017/01/23 21:18:50]

This line should be called unconditionally, even if the renderer connection is
gone, right?

[Mathieu, 2017/01/23 21:53:57]

Not really. If we are calling this method while the client is not bound it means
that it was closed as part of OnConnectionTerminated, which means destruction
was already requested on line 94.

 }
 
-void PaymentRequest::OnError() {
+void PaymentRequest::OnConnectionTerminated() {
+  // We are here because of a browser-side error, or likely as a result of the
+  // connection_error_handler on |binding_|, which can mean that the renderer
+  // has decided to close the pipe for various reasons (see all uses of
+  // PaymentRequest::clearResolversAndCloseMojoConnection() in Blink). We close
+  // the binding and the dialog, and ask to be deleted.
+  client_.reset();
   binding_.Close();
+  delegate_->CloseDialog();
   manager_->DestroyRequest(this);
 }
 
@@ -71,7 +102,6 @@ CurrencyFormatter* PaymentRequest::GetOrCreateCurrencyFormatter(
     currency_formatter_.reset(
         new CurrencyFormatter(currency_code, currency_system, locale_name));
   }
-
   return currency_formatter_.get();
 }