[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..2daba0185e20c5d38c28a27ba7cf89e9f476f8fc 100644
--- a/components/payments/payment_request.cc
+++ b/components/payments/payment_request.cc
@@ -22,9 +22,15 @@ PaymentRequest::PaymentRequest(
     : web_contents_(web_contents),
       delegate_(std::move(delegate)),
       manager_(manager),
-      binding_(this, std::move(request)) {
-  binding_.set_connection_error_handler(
-      base::Bind(&PaymentRequest::OnError, base::Unretained(this)));
+      binding_(this, std::move(request)),
+      user_cancelled_dialog_(false) {
+  // 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,7 +44,7 @@ void PaymentRequest::Init(
   std::string error;
   if (!payments::validatePaymentDetails(details, &error)) {
     LOG(ERROR) << error;
-    OnError();
+    OnConnectionTerminated();
     client_.reset();
     return;
   }
@@ -48,18 +54,37 @@ void PaymentRequest::Init(
 
 void PaymentRequest::Show() {
   if (!client_.is_bound() || !binding_.is_bound()) {
-    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.
+  client_->OnAbort(true /* aborted_successfully */);
+}
+
+void PaymentRequest::UserCancelled() {
+  // This sends an error to the renderer, which closes the Mojo message pipe,
+  // which triggers PaymentRequest::OnConnectionTerminated, which destroys this
+  // object.
+  user_cancelled_dialog_ = true;
   client_->OnError(payments::mojom::PaymentErrorReason::USER_CANCEL);
 }
 
-void PaymentRequest::OnError() {
+void PaymentRequest::OnConnectionTerminated() {

[please use gerrit instead, 2017/01/23 15:02:08]

Also need to close "client_" connection everywhere you close "binding_"
connection. PaymentRequest has two pipes to the renderer, unfortunately.

[Mathieu, 2017/01/23 20:58:36]

Done.

+  // 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.
   binding_.Close();

[please use gerrit instead, 2017/01/23 15:02:08]

I have a hunch that "binding_.is_bound()" is equiavlent to
"!user_cancelled_dialog_". Can you double-check? If so, then we don't need the
user_cancelled_dialog_ variable.

We can also withstand a call to UserCancelled() if we check for
"client_.is_bound()" and "binding_.is_bound()" in there. Then we also don't need
the "propagate_to_renderer" flag. What do you think?

As a general rule of thumb #1, every method that's going to talk to UI and finds
UI gone should abort communications with renderer, because it's misbehaving.

General rule of thumb #2, every method that's going to the renderer first should
check that both pipes to the renderer are open first. We can't tell anything to
a renderer, if we're not connected to it.

[Mathieu, 2017/01/23 20:58:36]

I rearranged things so that when UserCancelled is called, the renderer is
informed and bindings are closed afterwards (+PaymentRequest destroyed). It
avoids waiting for the OnConnectionTerminated message to achieve the same. By
the way if the renderer closes the binding, |binding_.is_bound()| stays true on
the browser side.

Applied rule #2 to check for client_.is_bound() before calling methods on
|client_|.

Removed the propagate_to_renderer flag because UserCancelled aborts when the
client is not bound.

+  if (!user_cancelled_dialog_)
+    delegate_->CloseDialog();
   manager_->DestroyRequest(this);
 }
 
@@ -71,7 +96,6 @@ CurrencyFormatter* PaymentRequest::GetOrCreateCurrencyFormatter(
     currency_formatter_.reset(
         new CurrencyFormatter(currency_code, currency_system, locale_name));
   }
-
   return currency_formatter_.get();
 }