Don't show FileManagerDialog if another FileManagerDialog is shown in the same tab.

chrome/browser/ui/views/file_manager_dialog_browsertest.cc

 // Copyright (c) 2011 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 "chrome/browser/ui/views/file_manager_dialog.h"
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
@@ skipping 72 matching lines @@
 
   // Creates a file system mount point for a directory.
   void AddMountPoint(const FilePath& path) {
     fileapi::FileSystemPathManager* path_manager =
         browser()->profile()->GetFileSystemContext()->path_manager();
     fileapi::ExternalFileSystemMountPointProvider* provider =
         path_manager->external_provider();
     provider->AddMountPoint(path);
   }
 
+  void OpenDialog(SelectFileDialog::Type dialog_type,

[James Cook, 2011/11/10 21:04:54]

Nice that you extracted this shared code!

+                  const FilePath& file_path,
+                  const gfx::NativeWindow& owning_window,
+                  const std::string& additional_message) {
+    // Spawn a dialog to open a file.  The dialog will signal that it is ready
+    // via chrome.test.sendMessage() in the extension JavaScript.
+    ExtensionTestMessageListener init_listener("worker-initialized",
+                                               false /* will_reply */);
+
+    scoped_ptr<ExtensionTestMessageListener> additional_listener;
+    if (!additional_message.empty()) {
+      additional_listener.reset(
+          new ExtensionTestMessageListener(additional_message, false));
+    }
+
+    dialog_->SelectFile(dialog_type,
+                        string16() /* title */,
+                        file_path,
+                        NULL /* file_types */,
+                         0 /* file_type_index */,
+                        FILE_PATH_LITERAL("") /* default_extension */,
+                        NULL /* source_contents */,
+                        owning_window,
+                        this /* params */);
+
+    LOG(INFO) << "Waiting for JavaScript ready message.";
+    ASSERT_TRUE(init_listener.WaitUntilSatisfied());
+
+    if (additional_listener.get()) {
+      LOG(INFO) << "Waiting for JavaScript " << additional_message
+                << " message.";
+      ASSERT_TRUE(additional_listener->WaitUntilSatisfied());
+    }
+
+    // Dialog should be running now.
+    ASSERT_TRUE(dialog_->IsRunning(owning_window));
+  }
+
+  void CloseDialog(bool press_ok, const gfx::NativeWindow& owning_window) {

[James Cook, 2011/11/10 21:04:54]

Maybe use an enum or a string for the button to press?  This would make the
calling code self-documenting, like: CloseDialog(window, "ok") or
CloseDialog(window, BTN_OK).

[tbarzic, 2011/11/10 22:00:48]

Done.

+    // Inject JavaScript to click the cancel button and wait for notification
+    // that the window has closed.
+    ui_test_utils::WindowedNotificationObserver host_destroyed(
+        content::NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED,
+        content::NotificationService::AllSources());
+    RenderViewHost* host = dialog_->GetRenderViewHost();
+    string16 main_frame;
+    std::string button_class = press_ok ? ".ok" : ".cancel";
+    string16 script = ASCIIToUTF16(
+        "console.log(\'Test JavaScript injected.\');"
+        "document.querySelector(\'" + button_class + "\').click();");
+    // The file selection handler closes the dialog and does not return control
+    // to JavaScript, so do not wait for return values.
+    host->ExecuteJavascriptInWebFrame(main_frame, script);
+    LOG(INFO) << "Waiting for window close notification.";
+    host_destroyed.Wait();
+
+    // Dialog no longer believes it is running.
+    ASSERT_FALSE(dialog_->IsRunning(owning_window));
+  }
+
   scoped_ptr<MockSelectFileDialogListener> listener_;
   scoped_refptr<FileManagerDialog> dialog_;
 };
 
 IN_PROC_BROWSER_TEST_F(FileManagerDialogBrowserTest, CreateAndDestroy) {
   // Browser window must be up for us to test dialog window parent.
   gfx::NativeWindow native_window = browser()->window()->GetNativeHandle();
   ASSERT_TRUE(native_window != NULL);
 
   // Before we call SelectFile, dialog is not running/visible.
@@ skipping 10 matching lines @@
 // TODO(jamescook): Add a test for selecting a file for an <input type='file'/>
 // page element, as that uses different memory management pathways.
 // crbug.com/98791
 IN_PROC_BROWSER_TEST_F(FileManagerDialogBrowserTest, SelectFileAndCancel) {
   // Add tmp mount point even though this test won't use it directly.
   // We need this to make sure that at least one top-level directory exists
   // in the file browser.
   FilePath tmp_dir("/tmp");
   AddMountPoint(tmp_dir);
 
-  // Spawn a dialog to open a file.  The dialog will signal that it is ready
-  // via chrome.test.sendMessage() in the extension JavaScript.
-  ExtensionTestMessageListener init_listener("worker-initialized",
-                                             false /* will_reply */);
   gfx::NativeWindow owning_window = browser()->window()->GetNativeHandle();
-  dialog_->SelectFile(SelectFileDialog::SELECT_OPEN_FILE,
-                      string16() /* title */,
-                      FilePath() /* default_path */,
-                      NULL /* file_types */,
-                       0 /* file_type_index */,
-                      FILE_PATH_LITERAL("") /* default_extension */,
-                      NULL /* source_contents */,
-                      owning_window,
-                      this /* params */);
-  LOG(INFO) << "Waiting for JavaScript ready message.";
-  ASSERT_TRUE(init_listener.WaitUntilSatisfied());
 
-  // Dialog should be running now.
-  ASSERT_TRUE(dialog_->IsRunning(owning_window));
+  // FilePath() for default path.
+  OpenDialog(SelectFileDialog::SELECT_OPEN_FILE, FilePath(), owning_window, "");
 
-  // Inject JavaScript to click the cancel button and wait for notification
-  // that the window has closed.
-  ui_test_utils::WindowedNotificationObserver host_destroyed(
-      content::NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED,
-      content::NotificationService::AllSources());
-  RenderViewHost* host = dialog_->GetRenderViewHost();
-  string16 main_frame;
-  string16 script = ASCIIToUTF16(
-      "console.log(\'Test JavaScript injected.\');"
-      "document.querySelector(\'.cancel\').click();");
-  // The file selection handler closes the dialog and does not return control
-  // to JavaScript, so do not wait for return values.
-  host->ExecuteJavascriptInWebFrame(main_frame, script);
-  LOG(INFO) << "Waiting for window close notification.";
-  host_destroyed.Wait();
-
-  // Dialog no longer believes it is running.
-  ASSERT_FALSE(dialog_->IsRunning(owning_window));
+  // Press cancel button.
+  CloseDialog(false, owning_window);
 
   // Listener should have been informed of the cancellation.
   ASSERT_FALSE(listener_->file_selected());
   ASSERT_TRUE(listener_->canceled());
   ASSERT_EQ(this, listener_->params());
 }
 
 IN_PROC_BROWSER_TEST_F(FileManagerDialogBrowserTest, SelectFileAndOpen) {
   // Allow the tmp directory to be mounted.  We explicitly use /tmp because
   // it it whitelisted for file system access on Chrome OS.
   FilePath tmp_dir("/tmp");
   AddMountPoint(tmp_dir);
 
   // Create a directory with a single file in it.  ScopedTempDir will delete
   // itself and our temp file when it goes out of scope.
   ScopedTempDir scoped_temp_dir;
   ASSERT_TRUE(scoped_temp_dir.CreateUniqueTempDirUnderPath(tmp_dir));
   FilePath temp_dir = scoped_temp_dir.path();
   FilePath test_file = temp_dir.AppendASCII("file_manager_test.html");
 
   // Create an empty file to give us something to select.
   FILE* fp = file_util::OpenFile(test_file, "w");
   ASSERT_TRUE(fp != NULL);
   ASSERT_TRUE(file_util::CloseFile(fp));
 
+  gfx::NativeWindow owning_window = browser()->window()->GetNativeHandle();
+
   // Spawn a dialog to open a file.  Provide the path to the file so the dialog
   // will automatically select it.  Ensure that the OK button is enabled by
   // waiting for chrome.test.sendMessage('selection-change-complete').
   // The extension starts a Web Worker to read file metadata, so it may send
   // 'selection-change-complete' before 'worker-initialized'.  This is OK.
-  ExtensionTestMessageListener init_listener("worker-initialized",
-                                             false /* will_reply */);
-  ExtensionTestMessageListener selection_listener("selection-change-complete",
-                                                  false /* will_reply */);
-  gfx::NativeWindow owning_window = browser()->window()->GetNativeHandle();
-  dialog_->SelectFile(SelectFileDialog::SELECT_OPEN_FILE,
-                      string16() /* title */,
-                      test_file,
-                      NULL /* file_types */,
-                      0 /* file_type_index */,
-                      FILE_PATH_LITERAL("") /* default_extension */,
-                      NULL /* source_contents */,
-                      owning_window,
-                      this /* params */);
-  LOG(INFO) << "Waiting for JavaScript initialized message.";
-  ASSERT_TRUE(init_listener.WaitUntilSatisfied());
-  LOG(INFO) << "Waiting for JavaScript selection-change-complete message.";
-  ASSERT_TRUE(selection_listener.WaitUntilSatisfied());
+  OpenDialog(SelectFileDialog::SELECT_OPEN_FILE, test_file, owning_window,
+             "selection-change-complete");
 
-  // Dialog should be running now.
-  ASSERT_TRUE(dialog_->IsRunning(owning_window));
-
-  // Inject JavaScript to click the open button and wait for notification
-  // that the window has closed.
-  ui_test_utils::WindowedNotificationObserver host_destroyed(
-      content::NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED,
-      content::NotificationService::AllSources());
-  RenderViewHost* host = dialog_->GetRenderViewHost();
-  string16 main_frame;
-  string16 script = ASCIIToUTF16(
-      "console.log(\'Test JavaScript injected.\');"
-      "document.querySelector('.ok').click();");
-  // The file selection handler closes the dialog and does not return control
-  // to JavaScript, so do not wait for return values.
-  host->ExecuteJavascriptInWebFrame(main_frame, script);
-  LOG(INFO) << "Waiting for window close notification.";
-  host_destroyed.Wait();
-
-  // Dialog no longer believes it is running.
-  ASSERT_FALSE(dialog_->IsRunning(owning_window));
+  // Click open button.
+  CloseDialog(true, owning_window);
 
   // Listener should have been informed that the file was opened.
   ASSERT_TRUE(listener_->file_selected());
   ASSERT_FALSE(listener_->canceled());
   ASSERT_EQ(test_file, listener_->path());
   ASSERT_EQ(this, listener_->params());
 }
 
 IN_PROC_BROWSER_TEST_F(FileManagerDialogBrowserTest, SelectFileAndSave) {
   // Allow the tmp directory to be mounted.  We explicitly use /tmp because
   // it it whitelisted for file system access on Chrome OS.
   FilePath tmp_dir("/tmp");
   AddMountPoint(tmp_dir);
 
   // Create a directory with a single file in it.  ScopedTempDir will delete
   // itself and our temp file when it goes out of scope.
   ScopedTempDir scoped_temp_dir;
   ASSERT_TRUE(scoped_temp_dir.CreateUniqueTempDirUnderPath(tmp_dir));
   FilePath temp_dir = scoped_temp_dir.path();
   FilePath test_file = temp_dir.AppendASCII("file_manager_test.html");
 
+  gfx::NativeWindow owning_window = browser()->window()->GetNativeHandle();
+
   // Spawn a dialog to save a file, providing a suggested path.
   // Ensure "Save" button is enabled by waiting for notification from
   // chrome.test.sendMessage().
   // The extension starts a Web Worker to read file metadata, so it may send
   // 'directory-change-complete' before 'worker-initialized'.  This is OK.
-  ExtensionTestMessageListener init_listener("worker-initialized",
-                                             false /* will_reply */);
-  ExtensionTestMessageListener dir_change_listener("directory-change-complete",
-                                                   false /* will_reply */);
-  gfx::NativeWindow owning_window = browser()->window()->GetNativeHandle();
-  dialog_->SelectFile(SelectFileDialog::SELECT_SAVEAS_FILE,
-                      string16() /* title */,
-                      test_file,
-                      NULL /* file_types */,
-                      0 /* file_type_index */,
-                      FILE_PATH_LITERAL("") /* default_extension */,
-                      NULL /* source_contents */,
-                      owning_window,
-                      this /* params */);
-  LOG(INFO) << "Waiting for JavaScript initialized message.";
-  ASSERT_TRUE(init_listener.WaitUntilSatisfied());
-  LOG(INFO) << "Waiting for JavaScript directory-change-complete message.";
-  ASSERT_TRUE(dir_change_listener.WaitUntilSatisfied());
+  OpenDialog(SelectFileDialog::SELECT_SAVEAS_FILE, test_file, owning_window,
+             "directory-change-complete");
 
-  // Dialog should be running now.
-  ASSERT_TRUE(dialog_->IsRunning(owning_window));
-
-  // Inject JavaScript to click the save button and wait for notification
-  // that the window has closed.
-  ui_test_utils::WindowedNotificationObserver host_destroyed(
-      content::NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED,
-      content::NotificationService::AllSources());
-  RenderViewHost* host = dialog_->GetRenderViewHost();
-  string16 main_frame;
-  string16 script = ASCIIToUTF16(
-      "console.log(\'Test JavaScript injected.\');"
-      "document.querySelector('.ok').click();");
-  // The file selection handler closes the dialog and does not return control
-  // to JavaScript, so do not wait for return values.
-  host->ExecuteJavascriptInWebFrame(main_frame, script);
-  LOG(INFO) << "Waiting for window close notification.";
-  host_destroyed.Wait();
-
-  // Dialog no longer believes it is running.
-  ASSERT_FALSE(dialog_->IsRunning(owning_window));
+  // Click save button.
+  CloseDialog(true, owning_window);
 
   // Listener should have been informed that the file was selected.
   ASSERT_TRUE(listener_->file_selected());
   ASSERT_FALSE(listener_->canceled());
   ASSERT_EQ(test_file, listener_->path());
   ASSERT_EQ(this, listener_->params());
 }
 
 IN_PROC_BROWSER_TEST_F(FileManagerDialogBrowserTest,
                        OpenSingletonTabAndCancel) {
   // Add tmp mount point even though this test won't use it directly.
   // We need this to make sure that at least one top-level directory exists
   // in the file browser.
   FilePath tmp_dir("/tmp");
   AddMountPoint(tmp_dir);
 
-  // Spawn a dialog to open a file.  The dialog will signal that it is ready
-  // via chrome.test.sendMessage() in the extension JavaScript.
-  ExtensionTestMessageListener init_listener("worker-initialized",
-                                             false /* will_reply */);
   gfx::NativeWindow owning_window = browser()->window()->GetNativeHandle();
-  dialog_->SelectFile(SelectFileDialog::SELECT_OPEN_FILE,
-                      string16() /* title */,
-                      FilePath() /* default_path */,
-                      NULL /* file_types */,
-                       0 /* file_type_index */,
-                      FILE_PATH_LITERAL("") /* default_extension */,
-                      NULL /* source_contents */,
-                      owning_window,
-                      this /* params */);
-  LOG(INFO) << "Waiting for JavaScript ready message.";
-  ASSERT_TRUE(init_listener.WaitUntilSatisfied());
 
-  // Dialog should be running now.
-  ASSERT_TRUE(dialog_->IsRunning(owning_window));
+  OpenDialog(SelectFileDialog::SELECT_OPEN_FILE, FilePath(), owning_window, "");

[James Cook, 2011/11/10 21:04:54]

Yeah, these are much easier to read with the common code extracted.  Good work.

[tbarzic, 2011/11/10 22:00:48]

:)

 
   // Open a singleton tab in background.
   browser::NavigateParams p(browser(), GURL("www.google.com"),
                             content::PAGE_TRANSITION_LINK);
   p.window_action = browser::NavigateParams::SHOW_WINDOW;
   p.disposition = SINGLETON_TAB;
   browser::Navigate(&p);
 
-  // Inject JavaScript to click the cancel button and wait for notification
-  // that the window has closed.
-  ui_test_utils::WindowedNotificationObserver host_destroyed(
-      content::NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED,
-      content::NotificationService::AllSources());
-  RenderViewHost* host = dialog_->GetRenderViewHost();
-  string16 main_frame;
-  string16 script = ASCIIToUTF16(
-      "console.log(\'Test JavaScript injected.\');"
-      "document.querySelector(\'.cancel\').click();");
-  // The file selection handler closes the dialog and does not return control
-  // to JavaScript, so do not wait for return values.
-  host->ExecuteJavascriptInWebFrame(main_frame, script);
-  LOG(INFO) << "Waiting for window close notification.";
-  host_destroyed.Wait();
-
-  // Dialog no longer believes it is running.
-  ASSERT_FALSE(dialog_->IsRunning(owning_window));
+  // Press cancel button.
+  CloseDialog(false, owning_window);
 
   // Listener should have been informed of the cancellation.
   ASSERT_FALSE(listener_->file_selected());
   ASSERT_TRUE(listener_->canceled());
   ASSERT_EQ(this, listener_->params());
 }
+
+IN_PROC_BROWSER_TEST_F(FileManagerDialogBrowserTest, OpenTwoDialogs) {
+  // Add tmp mount point even though this test won't use it directly.
+  // We need this to make sure that at least one top-level directory exists
+  // in the file browser.
+  FilePath tmp_dir("/tmp");
+  AddMountPoint(tmp_dir);
+
+  gfx::NativeWindow owning_window = browser()->window()->GetNativeHandle();
+
+  OpenDialog(SelectFileDialog::SELECT_OPEN_FILE, FilePath(), owning_window, "");
+
+  scoped_ptr<MockSelectFileDialogListener>
+      listener2(new MockSelectFileDialogListener());
+  scoped_refptr<FileManagerDialog>  dialog2 =

[James Cook, 2011/11/10 21:04:54]

Optional: dialog2 could become a member variable of this test. 
FileManagerDialog objects are cheap to construct, so I think it would be OK to
make two of them for every test run.  That would eliminate the extra friend
declaration.  Up to you; I don't feel strongly about adding friends for testing.

[tbarzic, 2011/11/10 22:00:48]

Done.

+      new FileManagerDialog(listener2.get());
+  dialog2->SelectFile(SelectFileDialog::SELECT_OPEN_FILE,
+                      string16() /* title */,
+                      FilePath() /* default_path */,
+                      NULL /* file_types */,
+                      0 /* file_type_index */,
+                      FILE_PATH_LITERAL("") /* default_extension */,
+                      NULL /* source_contents */,
+                      owning_window,
+                      this /* params */);
+
+  // Dialog should not be running now.
+  ASSERT_FALSE(dialog2->IsRunning(owning_window));
+
+  // Click cancel button.
+  CloseDialog(false, owning_window);
+
+  // Listener should have been informed of the cancellation.
+  ASSERT_FALSE(listener_->file_selected());
+  ASSERT_TRUE(listener_->canceled());
+  ASSERT_EQ(this, listener_->params());
+
+  dialog2 = NULL;
+  listener2.reset();
+}