[Payments] Improve the closing of the PR dialog.

components/payments/payment_request.cc

 // 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 "components/payments/payment_request.h"
 
 #include "base/memory/ptr_util.h"
 #include "components/autofill/core/browser/personal_data_manager.h"
 #include "components/payments/payment_details_validation.h"
 #include "components/payments/payment_request_delegate.h"
 #include "components/payments/payment_request_web_contents_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
 
 namespace payments {
 
 PaymentRequest::PaymentRequest(
     content::WebContents* web_contents,
     std::unique_ptr<PaymentRequestDelegate> delegate,
     PaymentRequestWebContentsManager* manager,
     mojo::InterfaceRequest<payments::mojom::PaymentRequest> request)
     : 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() {}
 
 void PaymentRequest::Init(
     payments::mojom::PaymentRequestClientPtr client,
     std::vector<payments::mojom::PaymentMethodDataPtr> methodData,
     payments::mojom::PaymentDetailsPtr details,
     payments::mojom::PaymentOptionsPtr options) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   std::string error;
   if (!payments::validatePaymentDetails(details, &error)) {
     LOG(ERROR) << error;
-    OnError();
+    OnConnectionTerminated();
     client_.reset();
     return;
   }
   client_ = std::move(client);
   details_ = std::move(details);
 }
 
 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);
 }
 
 CurrencyFormatter* PaymentRequest::GetOrCreateCurrencyFormatter(
     const std::string& currency_code,
     const base::Optional<std::string> currency_system,
     const std::string& locale_name) {
   if (!currency_formatter_) {
     currency_formatter_.reset(
         new CurrencyFormatter(currency_code, currency_system, locale_name));
   }
-
   return currency_formatter_.get();
 }
 
 autofill::AutofillProfile* PaymentRequest::GetCurrentlySelectedProfile() {
   // TODO(tmartino): Implement more sophisticated algorithm for populating
   // this when it starts empty.
   if (!profile_) {
     autofill::PersonalDataManager* data_manager =
         delegate_->GetPersonalDataManager();
     auto profiles = data_manager->GetProfiles();
@@ skipping 16 matching lines @@
       cards.begin(),
       cards.end(),
       [] (autofill::CreditCard* card) {
         return card->IsValid();
   });
 
   return first_complete_card == cards.end() ? nullptr : *first_complete_card;
 }
 
 }  // namespace payments