CrOS - Fix file picker to route callbacks using TabContents

Previously, when the user clicked open/save/cancel in the CrOS file picker, it would route that callback data to the frontmost tab.  Sometimes other tabs can switch to frontmost, however.  Use set_associated_tab_contents() to inform the extension function implementions of the tab to which to route the callbacks.  This also adds a test from tbarzic for the referenced bug.

BUG=chromium-os:18956
TEST=browser_tests FileManagerDialogTest.*, also verify you can open the file picker and open/save/cancel under normal conditions


Review URL: http://codereview.chromium.org/7830024

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@99327 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 86 insertions, 21 deletions

diff --git a/chrome/browser/extensions/extension_file_browser_private_api.cc b/chrome/browser/extensions/extension_file_browser_private_api.cc
index 2cd65da..206d5bf 100644
--- a/chrome/browser/extensions/extension_file_browser_private_api.cc
+++ b/chrome/browser/extensions/extension_file_browser_private_api.cc
@@ -907,23 +907,21 @@ FileBrowserFunction::~FileBrowserFunction() {
 }
 
 int32 FileBrowserFunction::GetTabId() const {
-  int32 tab_id = 0;
   if (!dispatcher()) {
-    NOTREACHED();
-    return tab_id;
+    LOG(WARNING) << "No dispatcher";
+    return 0;
   }
-
-  // TODO(jamescook):  This is going to fail when we switch to tab-modal
-  // dialogs.  Figure out a way to find which SelectFileDialog::Listener
-  // to call from inside these extension FileBrowserFunctions.
-  Browser* browser =
-      const_cast<FileBrowserFunction*>(this)->GetCurrentBrowser();
-  if (browser) {
-    TabContents* contents = browser->GetSelectedTabContents();
-    if (contents)
-      tab_id = ExtensionTabUtil::GetTabId(contents);
+  if (!dispatcher()->delegate()) {
+    LOG(WARNING) << "No delegate";
+    return 0;
+  }
+  TabContents* tab_contents =
+      dispatcher()->delegate()->GetAssociatedTabContents();
+  if (!tab_contents) {
+    LOG(WARNING) << "No associated tab contents";
+    return 0;
   }
-  return tab_id;
+  return ExtensionTabUtil::GetTabId(tab_contents);
 }
 
 // GetFileSystemRootPathOnFileThread can only be called from the file thread,
diff --git a/chrome/browser/ui/views/extensions/extension_dialog.cc b/chrome/browser/ui/views/extensions/extension_dialog.cc
index aadb4c5..d263eb2 100644
--- a/chrome/browser/ui/views/extensions/extension_dialog.cc
+++ b/chrome/browser/ui/views/extensions/extension_dialog.cc
@@ -54,6 +54,7 @@ ExtensionDialog::~ExtensionDialog() {
 ExtensionDialog* ExtensionDialog::Show(
     const GURL& url,
     Browser* browser,
+    TabContents* tab_contents,
     int width,
     int height,
     ExtensionDialogObserver* observer) {
@@ -64,7 +65,9 @@ ExtensionDialog* ExtensionDialog::Show(
   if (!manager)
     return NULL;
   ExtensionHost* host = manager->CreateDialogHost(url, browser);
-  DCHECK(host);
+  if (!host)
+    return NULL;
+  host->set_associated_tab_contents(tab_contents);
   return new ExtensionDialog(browser, host, width, height, observer);
 }
 
diff --git a/chrome/browser/ui/views/extensions/extension_dialog.h b/chrome/browser/ui/views/extensions/extension_dialog.h
index 741b2c4..654aad1 100644
--- a/chrome/browser/ui/views/extensions/extension_dialog.h
+++ b/chrome/browser/ui/views/extensions/extension_dialog.h
@@ -17,6 +17,7 @@ class ExtensionDialogObserver;
 class ExtensionHost;
 class GURL;
 class Profile;
+class TabContents;
 
 namespace views {
 class View;
@@ -35,8 +36,10 @@ class ExtensionDialog : public views::WidgetDelegate,
 
   // Create and show a dialog with |url| centered over the browser window.
   // |browser| is the browser to which the pop-up will be attached.
+  // |tab_contents| is the tab that spawned the dialog.
   // |width| and |height| are the size of the dialog in pixels.
   static ExtensionDialog* Show(const GURL& url, Browser* browser,
+                               TabContents* tab_contents,
                                int width,
                                int height,
                                ExtensionDialogObserver* observer);
diff --git a/chrome/browser/ui/views/file_manager_dialog.cc b/chrome/browser/ui/views/file_manager_dialog.cc
index 322ee87..e7f1564 100644
--- a/chrome/browser/ui/views/file_manager_dialog.cc
+++ b/chrome/browser/ui/views/file_manager_dialog.cc
@@ -182,18 +182,19 @@ void FileManagerDialog::SelectFileImpl(
     virtual_path = FilePath();
   }
 
+  // Connect our listener to FileDialogFunction's per-tab callbacks.
+  TabContentsWrapper* tab = owner_browser->GetSelectedTabContentsWrapper();
+  int32 tab_id = (tab ? tab->restore_tab_helper()->session_id().id() : 0);
+  PendingDialog::Add(tab_id, this);
+
   GURL file_browser_url = FileManagerUtil::GetFileBrowserUrlWithParams(
       type, title, virtual_path, file_types, file_type_index,
       default_extension);
   extension_dialog_ = ExtensionDialog::Show(file_browser_url,
-      owner_browser, kFileManagerWidth, kFileManagerHeight,
+      owner_browser, tab->tab_contents(),
+      kFileManagerWidth, kFileManagerHeight,
       this /* ExtensionDialog::Observer */);
 
-  // Connect our listener to FileDialogFunction's per-tab callbacks.
-  TabContentsWrapper* tab = owner_browser->GetSelectedTabContentsWrapper();
-  int32 tab_id = (tab ? tab->restore_tab_helper()->session_id().id() : 0);
-  PendingDialog::Add(tab_id, this);
-
   params_ = params;
   tab_id_ = tab_id;
   owner_window_ = owner_window;
diff --git a/chrome/browser/ui/views/file_manager_dialog_browsertest.cc b/chrome/browser/ui/views/file_manager_dialog_browsertest.cc
index 882fc19..0a75797 100644
--- a/chrome/browser/ui/views/file_manager_dialog_browsertest.cc
+++ b/chrome/browser/ui/views/file_manager_dialog_browsertest.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/extensions/extension_test_message_listener.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/shell_dialogs.h"  // SelectFileDialog
 #include "chrome/common/chrome_paths.h"
@@ -296,3 +297,62 @@ IN_PROC_BROWSER_TEST_F(FileManagerDialogTest, SelectFileAndSave) {
   ASSERT_EQ(test_file, listener_->path());
   ASSERT_EQ(this, listener_->params());
 }
+
+IN_PROC_BROWSER_TEST_F(FileManagerDialogTest, 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));
+
+  // Open a singleton tab in background.
+  browser::NavigateParams p(browser(), GURL("www.google.com"),
+                            PageTransition::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,
+      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));
+
+  // Listener should have been informed of the cancellation.
+  ASSERT_FALSE(listener_->file_selected());
+  ASSERT_TRUE(listener_->canceled());
+  ASSERT_EQ(this, listener_->params());
+}