Fix extra call to Cancel() in ProfileSigninConfirmationDialogViews

Bug: 1091232
Change-Id: Idfbecb611c0bde730a375a3bf3059c19c4d115ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2231221Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775155}

chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.cc

@@ -254,10 +254,6 @@ void ProfileSigninConfirmationDialogViews::ViewHierarchyChanged(
                          kPreferredWidth, explanation_label_height);
 }
 
-void ProfileSigninConfirmationDialogViews::WindowClosing() {
-  Cancel();
-}
-
 void ProfileSigninConfirmationDialogViews::StyledLabelLinkClicked(
     views::StyledLabel* label,
     const gfx::Range& range,

chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h

@@ -50,9 +50,6 @@ class ProfileSigninConfirmationDialogViews : public views::DialogDelegateView,
   void ViewHierarchyChanged(
       const views::ViewHierarchyChangedDetails& details) override;
 
-  // views::WidgetDelegate::
-  void WindowClosing() override;
-
   // views::StyledLabelListener:
   void StyledLabelLinkClicked(views::StyledLabel* label,
                               const gfx::Range& range,

chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h"
 #include "chrome/test/views/chrome_views_test_base.h"
+#include "ui/views/controls/button/label_button.h"
 #include "ui/views/test/widget_test.h"
 
 namespace {
@@ -27,29 +28,21 @@ class TestDelegate : public ui::ProfileSigninConfirmationDelegate {
 
 }  // namespace
 
-using ProfileSigninConfirmationDialogTest = ChromeViewsTestBase;
-
-// Regression test for https://crbug.com/1054866
-TEST_F(ProfileSigninConfirmationDialogTest, CloseButtonOnlyCallsDelegateOnce) {
-  int cancels = 0;
-  int continues = 0;
-  int signins = 0;
-
-  auto delegate =
-      std::make_unique<TestDelegate>(&cancels, &continues, &signins);
+class ProfileSigninConfirmationDialogTest : public ChromeViewsTestBase {
+ public:
+  void BuildDialog(
+      std::unique_ptr<ui::ProfileSigninConfirmationDelegate> delegate) {
     auto dialog = std::make_unique<ProfileSigninConfirmationDialogViews>(
         nullptr, "foo@bar.com", std::move(delegate), true);
-  ProfileSigninConfirmationDialogViews* weak_dialog = dialog.get();
-
-  views::Widget* widget = views::DialogDelegate::CreateDialogWidget(
-      dialog.release(), GetContext(), nullptr);
-  views::test::WidgetDestroyedWaiter destroy_waiter(widget);
+    weak_dialog_ = dialog.get();
 
-  widget->Show();
+    widget_ = views::DialogDelegate::CreateDialogWidget(dialog.release(),
+                                                        GetContext(), nullptr);
+    widget_->Show();
+  }
 
-  // Press the "continue signin" button.
-  views::Button* button =
-      static_cast<views::Button*>(weak_dialog->GetExtraView());
+  void PressButton(views::Button* button) {
+    views::test::WidgetDestroyedWaiter destroy_waiter(widget_);
     // Synthesize both press & release - different platforms have different
     // notions about whether buttons activate on press or on release.
     button->OnKeyPressed(
@@ -57,6 +50,26 @@ TEST_F(ProfileSigninConfirmationDialogTest, CloseButtonOnlyCallsDelegateOnce) {
     button->OnKeyReleased(
         ui::KeyEvent(ui::ET_KEY_RELEASED, ui::VKEY_SPACE, ui::EF_NONE));
     destroy_waiter.Wait();
+  }
+
+  ProfileSigninConfirmationDialogViews* weak_dialog_ = nullptr;
+  views::Widget* widget_ = nullptr;
+};
+
+// Regression test for https://crbug.com/1054866
+TEST_F(ProfileSigninConfirmationDialogTest, CloseButtonOnlyCallsDelegateOnce) {
+  int cancels = 0;
+  int continues = 0;
+  int signins = 0;
+  auto delegate =
+      std::make_unique<TestDelegate>(&cancels, &continues, &signins);
+  BuildDialog(std::move(delegate));
+  widget_->Show();
+
+  // Press the "continue signin" button.
+  views::Button* button =
+      static_cast<views::Button*>(weak_dialog_->GetExtraView());
+  PressButton(button);
 
   // The delegate should *not* have gotten a call back to OnCancelSignin. If the
   // fix for https://crbug.com/1054866 regresses, either it will, or we'll have
@@ -65,3 +78,42 @@ TEST_F(ProfileSigninConfirmationDialogTest, CloseButtonOnlyCallsDelegateOnce) {
   EXPECT_EQ(continues, 1);
   EXPECT_EQ(signins, 0);
 }
+
+// Regression test for https://crbug.com/1091232
+TEST_F(ProfileSigninConfirmationDialogTest, CancelButtonOnlyCallsDelegateOnce) {
+  int cancels = 0;
+  int continues = 0;
+  int signins = 0;
+  auto delegate =
+      std::make_unique<TestDelegate>(&cancels, &continues, &signins);
+  BuildDialog(std::move(delegate));
+  widget_->Show();
+
+  // Press the "Cancel" button.
+  views::Button* button = weak_dialog_->GetCancelButton();
+  PressButton(button);
+
+  EXPECT_EQ(cancels, 1);
+  EXPECT_EQ(continues, 0);
+  EXPECT_EQ(signins, 0);
+}
+
+// Regression test for https://crbug.com/1091232
+TEST_F(ProfileSigninConfirmationDialogTest,
+       NewProfileButtonOnlyCallsDelegateOnce) {
+  int cancels = 0;
+  int continues = 0;
+  int signins = 0;
+  auto delegate =
+      std::make_unique<TestDelegate>(&cancels, &continues, &signins);
+  BuildDialog(std::move(delegate));
+  widget_->Show();
+
+  // Press the "Signin with new profile" button.
+  views::Button* button = weak_dialog_->GetOkButton();
+  PressButton(button);
+
+  EXPECT_EQ(cancels, 0);
+  EXPECT_EQ(continues, 0);
+  EXPECT_EQ(signins, 1);
+}