App-modal dialog is removed from queue after response (mac).

App-modal dialog on mac is shown with some delay,
however the dialog must be removed from app-modal dialog queue
immediately after the dialog gets a response,
otherwise some dialogs will receive and handle events addressed
to another dialogs.

Review URL: https://codereview.chromium.org/1240463003

Cr-Commit-Position: refs/heads/master@{#341340}

2 files changed, 207 insertions, 82 deletions

diff --git a/chrome/browser/extensions/alert_apitest.cc b/chrome/browser/extensions/alert_apitest.cc
index 753f149..a4f99f0 100644
--- a/chrome/browser/extensions/alert_apitest.cc
+++ b/chrome/browser/extensions/alert_apitest.cc
@@ -2,17 +2,74 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/app_modal/app_modal_dialog.h"
+#include "components/app_modal/app_modal_dialog_queue.h"
+#include "components/app_modal/javascript_app_modal_dialog.h"
+#include "components/app_modal/native_app_modal_dialog.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/test/test_utils.h"
 #include "extensions/browser/extension_host.h"
 #include "extensions/browser/process_manager.h"
 #include "extensions/common/extension.h"
 
+namespace {
+
+void GetNextDialog(app_modal::NativeAppModalDialog** native_dialog) {
+  DCHECK(native_dialog);
+  *native_dialog = nullptr;
+  app_modal::AppModalDialog* dialog = ui_test_utils::WaitForAppModalDialog();
+  ASSERT_TRUE(dialog->IsJavaScriptModalDialog());
+  app_modal::JavaScriptAppModalDialog* js_dialog =
+      static_cast<app_modal::JavaScriptAppModalDialog*>(dialog);
+  *native_dialog = js_dialog->native_dialog();
+  ASSERT_TRUE(*native_dialog);
+}
+
+void CloseDialog() {
+  app_modal::NativeAppModalDialog* dialog = nullptr;
+  ASSERT_NO_FATAL_FAILURE(GetNextDialog(&dialog));
+  dialog->CloseAppModalDialog();
+}
+
+void AcceptDialog() {
+  app_modal::NativeAppModalDialog* dialog = nullptr;
+  ASSERT_NO_FATAL_FAILURE(GetNextDialog(&dialog));
+  dialog->AcceptAppModalDialog();
+}
+
+void CancelDialog() {
+  app_modal::NativeAppModalDialog* dialog = nullptr;
+  ASSERT_NO_FATAL_FAILURE(GetNextDialog(&dialog));
+  dialog->CancelAppModalDialog();
+}
+
+void CheckAlertResult(const std::string& dialog_name,
+                      size_t* call_count,
+                      const base::Value* value) {
+  ASSERT_TRUE(value) << dialog_name;
+  ASSERT_TRUE(value->IsType(base::Value::TYPE_NULL));
+  ++*call_count;
+}
+
+void CheckConfirmResult(const std::string& dialog_name,
+                        bool expected_value,
+                        size_t* call_count,
+                        const base::Value* value) {
+  ASSERT_TRUE(value) << dialog_name;
+  bool current_value;
+  ASSERT_TRUE(value->GetAsBoolean(&current_value)) << dialog_name;
+  ASSERT_EQ(expected_value, current_value) << dialog_name;
+  ++*call_count;
+}
+
+}  // namespace
+
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, AlertBasic) {
   ASSERT_TRUE(RunExtensionTest("alert")) << message_;
 
@@ -24,7 +81,86 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, AlertBasic) {
   host->host_contents()->GetMainFrame()->ExecuteJavaScriptForTests(
       base::ASCIIToUTF16("alert('This should not crash.');"));
 
-  app_modal::AppModalDialog* alert = ui_test_utils::WaitForAppModalDialog();
-  ASSERT_TRUE(alert);
-  alert->CloseModalDialog();
+  ASSERT_NO_FATAL_FAILURE(CloseDialog());
+}
+
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, AlertQueue) {
+  ASSERT_TRUE(RunExtensionTest("alert")) << message_;
+
+  const extensions::Extension* extension = GetSingleLoadedExtension();
+  extensions::ExtensionHost* host =
+      extensions::ProcessManager::Get(browser()->profile())
+          ->GetBackgroundHostForExtension(extension->id());
+  ASSERT_TRUE(host);
+
+  // Creates several dialogs at the same time.
+  const size_t num_dialogs = 3;
+  size_t call_count = 0;
+  for (size_t i = 0; i != num_dialogs; ++i) {
+    const std::string dialog_name = "Dialog #" + base::SizeTToString(i) + ".";
+    host->host_contents()->GetMainFrame()->ExecuteJavaScriptForTests(
+        base::ASCIIToUTF16("alert('" + dialog_name + "');"),
+        base::Bind(&CheckAlertResult, dialog_name,
+                   base::Unretained(&call_count)));
+  }
+
+  // Closes these dialogs.
+  for (size_t i = 0; i != num_dialogs; ++i) {
+    ASSERT_NO_FATAL_FAILURE(AcceptDialog());
+  }
+
+  // All dialogs must be closed now.
+  app_modal::AppModalDialogQueue* queue =
+      app_modal::AppModalDialogQueue::GetInstance();
+  ASSERT_TRUE(queue);
+  EXPECT_FALSE(queue->HasActiveDialog());
+  EXPECT_EQ(0, queue->end() - queue->begin());
+  while (call_count < num_dialogs)
+    ASSERT_NO_FATAL_FAILURE(content::RunAllPendingInMessageLoop());
+}
+
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, ConfirmQueue) {
+  ASSERT_TRUE(RunExtensionTest("alert")) << message_;
+
+  const extensions::Extension* extension = GetSingleLoadedExtension();
+  extensions::ExtensionHost* host =
+      extensions::ProcessManager::Get(browser()->profile())
+          ->GetBackgroundHostForExtension(extension->id());
+  ASSERT_TRUE(host);
+
+  // Creates several dialogs at the same time.
+  const size_t num_accepted_dialogs = 3;
+  const size_t num_cancelled_dialogs = 3;
+  size_t call_count = 0;
+  for (size_t i = 0; i != num_accepted_dialogs; ++i) {
+    const std::string dialog_name =
+        "Accepted dialog #" + base::SizeTToString(i) + ".";
+    host->host_contents()->GetMainFrame()->ExecuteJavaScriptForTests(
+        base::ASCIIToUTF16("confirm('" + dialog_name + "');"),
+        base::Bind(&CheckConfirmResult, dialog_name, true,
+                   base::Unretained(&call_count)));
+  }
+  for (size_t i = 0; i != num_cancelled_dialogs; ++i) {
+    const std::string dialog_name =
+        "Cancelled dialog #" + base::SizeTToString(i) + ".";
+    host->host_contents()->GetMainFrame()->ExecuteJavaScriptForTests(
+        base::ASCIIToUTF16("confirm('" + dialog_name + "');"),
+        base::Bind(&CheckConfirmResult, dialog_name, false,
+                   base::Unretained(&call_count)));
+  }
+
+  // Closes these dialogs.
+  for (size_t i = 0; i != num_accepted_dialogs; ++i)
+    ASSERT_NO_FATAL_FAILURE(AcceptDialog());
+  for (size_t i = 0; i != num_cancelled_dialogs; ++i)
+    ASSERT_NO_FATAL_FAILURE(CancelDialog());
+
+  // All dialogs must be closed now.
+  app_modal::AppModalDialogQueue* queue =
+      app_modal::AppModalDialogQueue::GetInstance();
+  ASSERT_TRUE(queue);
+  EXPECT_FALSE(queue->HasActiveDialog());
+  EXPECT_EQ(0, queue->end() - queue->begin());
+  while (call_count < num_accepted_dialogs + num_cancelled_dialogs)
+    ASSERT_NO_FATAL_FAILURE(content::RunAllPendingInMessageLoop());
 }
diff --git a/chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm b/chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm
index 73ee1c6..1854d2e 100644
--- a/chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm
+++ b/chrome/browser/ui/cocoa/javascript_app_modal_dialog_cocoa.mm
@@ -29,18 +29,6 @@ namespace {
 const int kSlotsPerLine = 50;
 const int kMessageTextMaxSlots = 2000;
 
-// The presentation of the NSAlert is delayed, due to an AppKit bug. See
-// JavaScriptAppModalDialogCocoa::ShowAppModalDialog for more details.  If the
-// NSAlert has not yet been presented, then actions that affect the NSAlert
-// should be delayed as well. Due to the destructive nature of these actions,
-// at most one action should be queued.
-enum AlertAction {
-  ACTION_NONE,
-  ACTION_CLOSE,
-  ACTION_ACCEPT,
-  ACTION_CANCEL
-};
-
 }  // namespace
 
 // Helper object that receives the notification that the dialog/sheet is
@@ -51,21 +39,12 @@ enum AlertAction {
   JavaScriptAppModalDialogCocoa* nativeDialog_;  // Weak.
   base::scoped_nsobject<NSTextField> textField_;
   BOOL alertShown_;
-  AlertAction queuedAction_;
 }
 
 // Creates an NSAlert if one does not already exist. Otherwise returns the
 // existing NSAlert.
 - (NSAlert*)alert;
 - (void)addTextFieldWithPrompt:(NSString*)prompt;
-- (void)alertDidEnd:(NSAlert*)alert
-         returnCode:(int)returnCode
-        contextInfo:(void*)contextInfo;
-
-// If the alert has been presented, immediately play the action. Otherwise
-// queue the action for replay immediately after the alert is presented.
-- (void)playOrQueueAction:(AlertAction)action;
-- (void)queueAction:(AlertAction)action;
 
 // Presents an AppKit blocking dialog.
 - (void)showAlert;
@@ -94,10 +73,8 @@ enum AlertAction {
 - (instancetype)initWithNativeDialog:(JavaScriptAppModalDialogCocoa*)dialog {
   DCHECK(dialog);
   self = [super init];
-  if (self) {
+  if (self)
     nativeDialog_ = dialog;
-    queuedAction_ = ACTION_NONE;
-  }
   return self;
 }
 
@@ -121,72 +98,33 @@ enum AlertAction {
 - (void)alertDidEnd:(NSAlert*)alert
          returnCode:(int)returnCode
         contextInfo:(void*)contextInfo {
-  DCHECK(nativeDialog_);
-  base::string16 input;
-  if (textField_)
-    input = base::SysNSStringToUTF16([textField_ stringValue]);
-  bool shouldSuppress = false;
-  if ([alert showsSuppressionButton])
-    shouldSuppress = [[alert suppressionButton] state] == NSOnState;
   switch (returnCode) {
     case NSAlertFirstButtonReturn:  {  // OK
-      nativeDialog_->dialog()->OnAccept(input, shouldSuppress);
+      [self sendAcceptToNativeDialog];
       break;
     }
     case NSAlertSecondButtonReturn:  {  // Cancel
       // If the user wants to stay on this page, stop quitting (if a quit is in
       // progress).
-      if (nativeDialog_->dialog()->is_before_unload_dialog())
-        chrome_browser_application_mac::CancelTerminate();
-      nativeDialog_->dialog()->OnCancel(shouldSuppress);
+      [self sendCancelToNativeDialog];
       break;
     }
     case NSRunStoppedResponse: {  // Window was closed underneath us
-      // Need to call OnCancel() because there is some cleanup that needs
+      // Need to call OnClose() because there is some cleanup that needs
       // to be done.  It won't call back to the javascript since the
       // JavaScriptAppModalDialog knows that the WebContents was destroyed.
-      nativeDialog_->dialog()->OnCancel(shouldSuppress);
+      [self sendCloseToNativeDialog];
       break;
     }
     default:  {
       NOTREACHED();
     }
   }
-
-  delete nativeDialog_;  // Careful, this will delete us.
-}
-
-- (void)playOrQueueAction:(AlertAction)action {
-  if (alertShown_)
-    [self playAlertAction:action];
-  else
-    [self queueAction:action];
-}
-
-- (void)queueAction:(AlertAction)action {
-  DCHECK(!alertShown_);
-  DCHECK(queuedAction_ == ACTION_NONE);
-
-  queuedAction_ = action;
-}
-
-- (void)playAlertAction:(AlertAction)action {
-  switch (action) {
-    case ACTION_NONE:
-      break;
-    case ACTION_CLOSE:
-      [self closeWindow];
-      break;
-    case ACTION_CANCEL:
-      [self cancelAlert];
-      break;
-    case ACTION_ACCEPT:
-      [self acceptAlert];
-      break;
-  }
 }
 
 - (void)showAlert {
+  DCHECK(nativeDialog_);
+  DCHECK(!alertShown_);
   alertShown_ = YES;
   NSAlert* alert = [self alert];
   [alert beginSheetModalForWindow:nil  // nil here makes it app-modal
@@ -196,28 +134,79 @@ enum AlertAction {
 
   if ([alert accessoryView])
     [[alert window] makeFirstResponder:[alert accessoryView]];
-
-  [self playAlertAction:queuedAction_];
 }
 
 - (void)acceptAlert {
+  DCHECK(nativeDialog_);
+  if (!alertShown_) {
+    [self sendAcceptToNativeDialog];
+    return;
+  }
   NSButton* first = [[[self alert] buttons] objectAtIndex:0];
   [first performClick:nil];
 }
 
 - (void)cancelAlert {
-  NSAlert* alert = [self alert];
-  DCHECK([[alert buttons] count] >= 2);
-  NSButton* second = [[alert buttons] objectAtIndex:1];
+  DCHECK(nativeDialog_);
+  if (!alertShown_) {
+    [self sendCancelToNativeDialog];
+    return;
+  }
+  DCHECK_GE([[[self alert] buttons] count], 2U);
+  NSButton* second = [[[self alert] buttons] objectAtIndex:1];
   [second performClick:nil];
 }
 
 - (void)closeWindow {
-  DCHECK([self alert]);
-
+  DCHECK(nativeDialog_);
+  if (!alertShown_) {
+    [self sendCloseToNativeDialog];
+    return;
+  }
   [NSApp endSheet:[[self alert] window]];
 }
 
+- (void)sendAcceptToNativeDialog {
+  DCHECK(nativeDialog_);
+  nativeDialog_->dialog()->OnAccept([self input], [self shouldSuppress]);
+  [self destroyNativeDialog];
+}
+
+- (void)sendCancelToNativeDialog {
+  DCHECK(nativeDialog_);
+  // If the user wants to stay on this page, stop quitting (if a quit is in
+  // progress).
+  if (nativeDialog_->dialog()->is_before_unload_dialog())
+    chrome_browser_application_mac::CancelTerminate();
+  nativeDialog_->dialog()->OnCancel([self shouldSuppress]);
+  [self destroyNativeDialog];
+}
+
+- (void)sendCloseToNativeDialog {
+  DCHECK(nativeDialog_);
+  nativeDialog_->dialog()->OnClose();
+  [self destroyNativeDialog];
+}
+
+- (void)destroyNativeDialog {
+  DCHECK(nativeDialog_);
+  JavaScriptAppModalDialogCocoa* nativeDialog = nativeDialog_;
+  nativeDialog_ = nil;  // Need to fail on DCHECK if something wrong happens.
+  delete nativeDialog;  // Careful, this will delete us.
+}
+
+- (base::string16)input {
+  if (textField_)
+    return base::SysNSStringToUTF16([textField_ stringValue]);
+  return base::string16();
+}
+
+- (bool)shouldSuppress {
+  if ([[self alert] showsSuppressionButton])
+    return [[[self alert] suppressionButton] state] == NSOnState;
+  return false;
+}
+
 @end
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -434,15 +423,15 @@ void JavaScriptAppModalDialogCocoa::ActivateAppModalDialog() {
 }
 
 void JavaScriptAppModalDialogCocoa::CloseAppModalDialog() {
-  [helper_ playOrQueueAction:ACTION_CLOSE];
+  [helper_ closeWindow];
 }
 
 void JavaScriptAppModalDialogCocoa::AcceptAppModalDialog() {
-  [helper_ playOrQueueAction:ACTION_ACCEPT];
+  [helper_ acceptAlert];
 }
 
 void JavaScriptAppModalDialogCocoa::CancelAppModalDialog() {
-  [helper_ playOrQueueAction:ACTION_CANCEL];
+  [helper_ cancelAlert];
 }
 
 bool JavaScriptAppModalDialogCocoa::IsShowing() const {