Add test coverage for SecurePaymentConfirmationController

* Create a new test observer interface PaymentUIObserver
  intended for cross-platform UI event waiters for testing.
* Expand the existing secure_payment_confirmation_browsertests
  to wait until the UI is displayed rather than just waiting for the
  PaymentRequest kAppListReady event. This ensures the controller
  successfully creates the DialogView and calls ShowDialog.

Bug: 1124929
Change-Id: Iacec5998e1b15c4e6def109f2c9a465b0f6abffb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2631425
Commit-Queue: Nick Burris <nburris@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844216}

chrome/browser/payments/chrome_payment_request_delegate.cc

@@ -301,4 +301,9 @@ content::BrowserContext* ChromePaymentRequestDelegate::GetBrowserContextOrNull()
   return rfh ? rfh->GetBrowserContext() : nullptr;
 }
 
+const PaymentUIObserver* ChromePaymentRequestDelegate::GetPaymentUIObserver()
+    const {
+  return nullptr;
+}
+
 }  // namespace payments

chrome/browser/payments/chrome_payment_request_delegate.h

@@ -21,6 +21,7 @@ class BrowserContext;
 namespace payments {
 
 class PaymentRequestDialog;
+class PaymentUIObserver;
 
 class ChromePaymentRequestDelegate : public ContentPaymentRequestDelegate {
  public:
@@ -63,6 +64,7 @@ class ChromePaymentRequestDelegate : public ContentPaymentRequestDelegate {
   bool SkipUiForBasicCard() const override;
   std::string GetTwaPackageName() const override;
   PaymentRequestDialog* GetDialogForTesting() override;
+  const PaymentUIObserver* GetPaymentUIObserver() const override;
 
  protected:
   // Reference to the dialog so that we can satisfy calls to CloseDialog(). This

chrome/browser/payments/secure_payment_confirmation_browsertest.cc

@@ -157,7 +157,7 @@ IN_PROC_BROWSER_TEST_F(SecurePaymentConfirmationTest,
               std::move(credential_id), "relying-party.example",
               base::ASCIIToUTF16("Stub label"), std::move(icon)),
           /*consumer=*/this);
-  ResetEventWaiterForSingleEvent(TestEvent::kAppListReady);
+  ResetEventWaiterForSingleEvent(TestEvent::kUIDisplayed);
 
   // ExecJs starts executing JavaScript and immediately returns, not waiting for
   // any promise to return.
@@ -346,7 +346,7 @@ IN_PROC_BROWSER_TEST_F(SecurePaymentConfirmationCreationTest,
   // Cross the origin boundary.
   NavigateTo("b.com", "/secure_payment_confirmation.html");
   test_controller()->SetHasAuthenticator(true);
-  ResetEventWaiterForSingleEvent(TestEvent::kAppListReady);
+  ResetEventWaiterForSingleEvent(TestEvent::kUIDisplayed);
 
   // ExecJs starts executing JavaScript and immediately returns, not waiting for
   // any promise to return.

chrome/browser/ui/views/payments/secure_payment_confirmation_dialog_view.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/ui/views/chrome_typography.h"
 #include "chrome/browser/ui/views/payments/payment_request_views_util.h"
 #include "components/constrained_window/constrained_window_views.h"
+#include "components/payments/content/payment_ui_observer.h"
 #include "components/payments/content/secure_payment_confirmation_model.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/controls/image_view.h"
@@ -49,8 +50,10 @@ constexpr int kRowHeight = 48;
 }  // namespace
 
 SecurePaymentConfirmationDialogView::SecurePaymentConfirmationDialogView(
-    ObserverForTest* observer_for_test)
-    : observer_for_test_(observer_for_test) {}
+    ObserverForTest* observer_for_test,
+    const PaymentUIObserver* ui_observer_for_test)
+    : observer_for_test_(observer_for_test),
+      ui_observer_for_test_(ui_observer_for_test) {}
 SecurePaymentConfirmationDialogView::~SecurePaymentConfirmationDialogView() =
     default;
 
@@ -81,8 +84,13 @@ void SecurePaymentConfirmationDialogView::ShowDialog(
 
   constrained_window::ShowWebModalDialogViews(this, web_contents);
 
+  // observer_for_test_ is used in views browsertests.
   if (observer_for_test_)
     observer_for_test_->OnDialogOpened();
+
+  // ui_observer_for_test_ is used in platform browsertests.
+  if (ui_observer_for_test_)
+    ui_observer_for_test_->OnUIDisplayed();
 }
 
 void SecurePaymentConfirmationDialogView::OnDialogAccepted() {

chrome/browser/ui/views/payments/secure_payment_confirmation_dialog_view.h

@@ -20,6 +20,8 @@ class ProgressBar;
 
 namespace payments {
 
+class PaymentUIObserver;
+
 // Draws the user interface in the secure payment confirmation flow. Owned by
 // the SecurePaymentConfirmationController.
 class SecurePaymentConfirmationDialogView
@@ -51,7 +53,8 @@ class SecurePaymentConfirmationDialogView
   };
 
   explicit SecurePaymentConfirmationDialogView(
-      ObserverForTest* observer_for_test);
+      ObserverForTest* observer_for_test,
+      const PaymentUIObserver* ui_observer_for_test);
   ~SecurePaymentConfirmationDialogView() override;
 
   // SecurePaymentConfirmationView:
@@ -95,6 +98,7 @@ class SecurePaymentConfirmationDialogView
 
   // May be null.
   ObserverForTest* observer_for_test_ = nullptr;
+  const PaymentUIObserver* ui_observer_for_test_ = nullptr;
 
   VerifyCallback verify_callback_;
   CancelCallback cancel_callback_;

chrome/browser/ui/views/payments/secure_payment_confirmation_view.cc

@@ -10,9 +10,10 @@ namespace payments {
 
 // static
 base::WeakPtr<SecurePaymentConfirmationView>
-SecurePaymentConfirmationView::Create() {
+SecurePaymentConfirmationView::Create(
+    const PaymentUIObserver* payment_ui_observer) {
   return (new SecurePaymentConfirmationDialogView(
-              /*observer_for_test=*/nullptr))
+              /*observer_for_test=*/nullptr, payment_ui_observer))
       ->GetWeakPtr();
 }
 

chrome/browser/ui/views/payments/test_secure_payment_confirmation_payment_request_delegate.cc

@@ -20,8 +20,10 @@ TestSecurePaymentConfirmationPaymentRequestDelegate::
           render_frame_host->GetProcess()->GetID(),
           render_frame_host->GetRoutingID())),
       model_(model),
-      dialog_view_(
-          (new SecurePaymentConfirmationDialogView(observer))->GetWeakPtr()) {}
+      dialog_view_((new SecurePaymentConfirmationDialogView(
+                        observer,
+                        /*ui_observer_for_test=*/nullptr))
+                       ->GetWeakPtr()) {}
 
 TestSecurePaymentConfirmationPaymentRequestDelegate::
     ~TestSecurePaymentConfirmationPaymentRequestDelegate() = default;

chrome/test/payments/payment_request_platform_browsertest_base.cc

@@ -148,6 +148,11 @@ void PaymentRequestPlatformBrowserTestBase::OnMinimalUIReady() {
     event_waiter_->OnEvent(TestEvent::kMinimalUIReady);
 }
 
+void PaymentRequestPlatformBrowserTestBase::OnUIDisplayed() {
+  if (event_waiter_)
+    event_waiter_->OnEvent(TestEvent::kUIDisplayed);
+}
+
 void PaymentRequestPlatformBrowserTestBase::ResetEventWaiterForSingleEvent(
     TestEvent event) {
   event_waiter_ = std::make_unique<EventWaiter>(

chrome/test/payments/payment_request_platform_browsertest_base.h

@@ -85,6 +85,7 @@ class PaymentRequestPlatformBrowserTestBase
   void OnAppListReady() override;
   void OnCompleteCalled() override;
   void OnMinimalUIReady() override;
+  void OnUIDisplayed() override;
 
   // Resets the event waiter for a given |event| or |event_sequence|.
   void ResetEventWaiterForSingleEvent(TestEvent event);

chrome/test/payments/payment_request_test_controller.h

@@ -46,6 +46,7 @@ class PaymentRequestTestObserver {
   virtual void OnAbortCalled() {}
   virtual void OnCompleteCalled() {}
   virtual void OnMinimalUIReady() {}
+  virtual void OnUIDisplayed() {}
 
  protected:
   virtual ~PaymentRequestTestObserver() = default;
@@ -127,6 +128,7 @@ class PaymentRequestTestController {
   void OnAbortCalled();
   void OnCompleteCalled();
   void OnMinimalUIReady();
+  void OnUIDisplayed();
 
   PaymentRequestTestObserver* observer_ = nullptr;
 

chrome/test/payments/payment_request_test_controller_desktop.cc

@@ -15,6 +15,7 @@
 #include "components/payments/content/android_app_communication.h"
 #include "components/payments/content/payment_request.h"
 #include "components/payments/content/payment_request_web_contents_manager.h"
+#include "components/payments/content/payment_ui_observer.h"
 #include "components/payments/core/payment_prefs.h"
 #include "components/payments/core/payment_request_delegate.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
@@ -56,7 +57,8 @@ class ChromePaymentRequestTestDelegate : public ChromePaymentRequestDelegate {
                                    bool valid_ssl,
                                    PrefService* prefs,
                                    const std::string& twa_package_name,
-                                   bool has_authenticator)
+                                   bool has_authenticator,
+                                   PaymentUIObserver* ui_observer_for_test)
       : ChromePaymentRequestDelegate(render_frame_host),
         frame_routing_id_(content::GlobalFrameRoutingId(
             render_frame_host->GetProcess()->GetID(),
@@ -65,7 +67,8 @@ class ChromePaymentRequestTestDelegate : public ChromePaymentRequestDelegate {
         valid_ssl_(valid_ssl),
         prefs_(prefs),
         twa_package_name_(twa_package_name),
-        has_authenticator_(has_authenticator) {}
+        has_authenticator_(has_authenticator),
+        ui_observer_for_test_(ui_observer_for_test) {}
 
   bool IsOffTheRecord() const override { return is_off_the_record_; }
   std::string GetInvalidSslCertificateErrorMessage() override {
@@ -81,6 +84,9 @@ class ChromePaymentRequestTestDelegate : public ChromePaymentRequestDelegate {
                                                      has_authenticator_)
                : nullptr;
   }
+  const PaymentUIObserver* GetPaymentUIObserver() const override {
+    return ui_observer_for_test_;
+  }
 
  private:
   content::GlobalFrameRoutingId frame_routing_id_;
@@ -89,16 +95,19 @@ class ChromePaymentRequestTestDelegate : public ChromePaymentRequestDelegate {
   PrefService* const prefs_;
   const std::string twa_package_name_;
   const bool has_authenticator_;
+  const PaymentUIObserver* const ui_observer_for_test_;
 };
 
 }  // namespace
 
 class PaymentRequestTestController::ObserverConverter
-    : public PaymentRequest::ObserverForTest {
+    : public PaymentRequest::ObserverForTest,
+      public PaymentUIObserver {
  public:
   explicit ObserverConverter(PaymentRequestTestController* controller)
       : controller_(controller) {}
 
+  // PaymentRequest::ObserverForTest:
   void OnCanMakePaymentCalled() override {
     controller_->OnCanMakePaymentCalled();
   }
@@ -135,6 +144,9 @@ class PaymentRequestTestController::ObserverConverter
   void OnAbortCalled() override { controller_->OnAbortCalled(); }
   void OnCompleteCalled() override { controller_->OnCompleteCalled(); }
 
+  // PaymentUIObserver:
+  void OnUIDisplayed() const override { controller_->OnUIDisplayed(); }
+
  private:
   PaymentRequestTestController* const controller_;
 };
@@ -240,8 +252,8 @@ void PaymentRequestTestController::SetTwaPaymentApp(
 
 void PaymentRequestTestController::UpdateDelegateFactory() {
   SetPaymentRequestFactoryForTesting(base::BindRepeating(
-      [](PaymentRequest::ObserverForTest* observer_for_test,
-         bool is_off_the_record, bool valid_ssl, PrefService* prefs,
+      [](ObserverConverter* observer_for_test, bool is_off_the_record,
+         bool valid_ssl, PrefService* prefs,
          const std::string& twa_package_name, bool has_authenticator,
          const std::string& twa_payment_app_method_name,
          const std::string& twa_payment_app_response,
@@ -252,7 +264,7 @@ void PaymentRequestTestController::UpdateDelegateFactory() {
         DCHECK(render_frame_host->IsCurrent());
         auto delegate = std::make_unique<ChromePaymentRequestTestDelegate>(
             render_frame_host, is_off_the_record, valid_ssl, prefs,
-            twa_package_name, has_authenticator);
+            twa_package_name, has_authenticator, observer_for_test);
         *delegate_weakptr = delegate->GetContentWeakPtr();
         PaymentRequestWebContentsManager* manager =
             PaymentRequestWebContentsManager::GetOrCreateForWebContents(
@@ -306,6 +318,12 @@ void PaymentRequestTestController::OnMinimalUIReady() {
   NOTREACHED();
 }
 
+void PaymentRequestTestController::OnUIDisplayed() {
+  if (observer_) {
+    observer_->OnUIDisplayed();
+  }
+}
+
 void PaymentRequestTestController::OnNotSupportedError() {
   if (observer_)
     observer_->OnNotSupportedError();

chrome/test/payments/test_event_waiter.cc

@@ -40,6 +40,9 @@ std::ostream& operator<<(std::ostream& out, TestEvent event) {
     case TestEvent::kMinimalUIReady:
       out << "MinimalUIReady";
       break;
+    case TestEvent::kUIDisplayed:
+      out << "UIDisplayed";
+      break;
   }
   return out;
 }

chrome/test/payments/test_event_waiter.h

@@ -28,6 +28,7 @@ enum class TestEvent : int32_t {
   kAppListReady,
   kPaymentCompleted,
   kMinimalUIReady,
+  kUIDisplayed,
 };
 
 std::ostream& operator<<(std::ostream& out, TestEvent event);

components/payments/content/BUILD.gn

@@ -40,6 +40,7 @@ static_library("content") {
     "payment_request_converter.h",
     "payment_request_spec.cc",
     "payment_request_spec.h",
+    "payment_ui_observer.h",
     "payments_userdata_key.cc",
     "payments_userdata_key.h",
     "secure_payment_confirmation_app.cc",

components/payments/content/content_payment_request_delegate.h

@@ -24,6 +24,7 @@ namespace payments {
 class PaymentManifestWebDataService;
 class PaymentRequestDialog;
 class PaymentRequestDisplayManager;
+class PaymentUIObserver;
 
 // The delegate for PaymentRequest that can use content.
 class ContentPaymentRequestDelegate : public PaymentRequestDelegate {
@@ -74,6 +75,8 @@ class ContentPaymentRequestDelegate : public PaymentRequestDelegate {
 
   virtual PaymentRequestDialog* GetDialogForTesting() = 0;
 
+  virtual const PaymentUIObserver* GetPaymentUIObserver() const = 0;
+
   // Returns a weak pointer to this delegate.
   base::WeakPtr<ContentPaymentRequestDelegate> GetContentWeakPtr();
 

components/payments/content/payment_ui_observer.h

+// Copyright 2021 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.
+
+#ifndef COMPONENTS_PAYMENTS_CONTENT_PAYMENT_UI_OBSERVER_H_
+#define COMPONENTS_PAYMENTS_CONTENT_PAYMENT_UI_OBSERVER_H_
+
+namespace payments {
+
+// Interface for cross-platform tests to observe UI events.
+class PaymentUIObserver {
+ public:
+  virtual void OnUIDisplayed() const = 0;
+
+ protected:
+  virtual ~PaymentUIObserver() = default;
+};
+
+}  // namespace payments
+
+#endif  // COMPONENTS_PAYMENTS_CONTENT_PAYMENT_UI_OBSERVER_H_

components/payments/content/secure_payment_confirmation_controller.cc

@@ -11,6 +11,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
+#include "components/payments/content/content_payment_request_delegate.h"
 #include "components/payments/content/payment_request.h"
 #include "components/payments/core/currency_formatter.h"
 #include "components/payments/core/method_strings.h"
@@ -107,7 +108,8 @@ void SecurePaymentConfirmationController::
                          request_->state()->GetApplicationLocale())
            .Format(total->amount->value)}));
 
-  view_ = SecurePaymentConfirmationView::Create();
+  view_ = SecurePaymentConfirmationView::Create(
+      request_->state()->GetPaymentRequestDelegate()->GetPaymentUIObserver());
   view_->ShowDialog(
       request_->web_contents(), model_.GetWeakPtr(),
       base::BindOnce(&SecurePaymentConfirmationController::OnConfirm,

components/payments/content/secure_payment_confirmation_view.h

@@ -14,6 +14,7 @@ class WebContents;
 
 namespace payments {
 
+class PaymentUIObserver;
 class SecurePaymentConfirmationModel;
 
 // Draws the user interface in the secure payment confirmation flow. Owned by
@@ -23,7 +24,8 @@ class SecurePaymentConfirmationView {
   using VerifyCallback = base::OnceCallback<void()>;
   using CancelCallback = base::OnceCallback<void()>;
 
-  static base::WeakPtr<SecurePaymentConfirmationView> Create();
+  static base::WeakPtr<SecurePaymentConfirmationView> Create(
+      const PaymentUIObserver* payment_ui_observer);
 
   virtual ~SecurePaymentConfirmationView() = 0;
 

components/payments/content/secure_payment_confirmation_view_stub.cc

@@ -8,7 +8,8 @@ namespace payments {
 
 // static
 base::WeakPtr<SecurePaymentConfirmationView>
-SecurePaymentConfirmationView::Create() {
+SecurePaymentConfirmationView::Create(
+    const PaymentUIObserver* payment_ui_observer) {
   return nullptr;
 }
 

components/payments/content/test_content_payment_request_delegate.cc

@@ -143,4 +143,9 @@ void TestContentPaymentRequestDelegate::CompleteFullCardRequest() {
   core_delegate_.CompleteFullCardRequest();
 }
 
+const PaymentUIObserver*
+TestContentPaymentRequestDelegate::GetPaymentUIObserver() const {
+  return nullptr;
+}
+
 }  // namespace payments

components/payments/content/test_content_payment_request_delegate.h

@@ -21,6 +21,8 @@ class SingleThreadTaskExecutor;
 
 namespace payments {
 
+class PaymentUIObserver;
+
 class TestContentPaymentRequestDelegate : public ContentPaymentRequestDelegate {
  public:
   TestContentPaymentRequestDelegate(
@@ -65,6 +67,7 @@ class TestContentPaymentRequestDelegate : public ContentPaymentRequestDelegate {
   autofill::TestAddressNormalizer* test_address_normalizer();
   void DelayFullCardRequestCompletion();
   void CompleteFullCardRequest();
+  const PaymentUIObserver* GetPaymentUIObserver() const override;
 
  private:
   TestPaymentRequestDelegate core_delegate_;