Fix window.open() from panels in ChromeOS.

This is a regression caused by http://codereview.chromium.org/6881073/

BUG=chromium-os:15598
TEST=Opening a large window using JS window.open from a panel should open a new tab in the parent browser. See issue for repro.

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

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

15 files changed, 192 insertions, 35 deletions

diff --git a/chrome/browser/chromeos/frame/browser_view.cc b/chrome/browser/chromeos/frame/browser_view.cc
index b6d7e65..1677ab1 100644
--- a/chrome/browser/chromeos/frame/browser_view.cc
+++ b/chrome/browser/chromeos/frame/browser_view.cc
@@ -376,7 +376,9 @@ void BrowserView::Paste() {
   gtk_util::DoPaste(this);
 }
 
-WindowOpenDisposition BrowserView::GetDispositionForPopupBounds(
+// This is a static function so that the logic can be shared by
+// chromeos::PanelBrowserView.
+WindowOpenDisposition BrowserView::DispositionForPopupBounds(
     const gfx::Rect& bounds) {
   // If a popup is larger than a given fraction of the screen, turn it into
   // a foreground tab. Also check for width or height == 0, which would
@@ -393,6 +395,11 @@ WindowOpenDisposition BrowserView::GetDispositionForPopupBounds(
   return NEW_POPUP;
 }
 
+WindowOpenDisposition BrowserView::GetDispositionForPopupBounds(
+    const gfx::Rect& bounds) {
+  return DispositionForPopupBounds(bounds);
+}
+
 // views::ContextMenuController implementation.
 void BrowserView::ShowContextMenuForView(views::View* source,
                                          const gfx::Point& p,
diff --git a/chrome/browser/chromeos/frame/browser_view.h b/chrome/browser/chromeos/frame/browser_view.h
index 957d07e..792e20f 100644
--- a/chrome/browser/chromeos/frame/browser_view.h
+++ b/chrome/browser/chromeos/frame/browser_view.h
@@ -83,6 +83,10 @@ class BrowserView : public ::BrowserView,
     return saved_focused_widget_;
   }
 
+  // static implementation for chromeos::PanelBrowserView.
+  static WindowOpenDisposition DispositionForPopupBounds(
+      const gfx::Rect& bounds);
+
  protected:
   virtual void GetAccessiblePanes(
       std::vector<AccessiblePaneView*>* panes);
diff --git a/chrome/browser/chromeos/frame/panel_browser_view.cc b/chrome/browser/chromeos/frame/panel_browser_view.cc
index bce3233..3a49089 100644
--- a/chrome/browser/chromeos/frame/panel_browser_view.cc
+++ b/chrome/browser/chromeos/frame/panel_browser_view.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/frame/panel_browser_view.h"
 
+#include "chrome/browser/chromeos/frame/browser_view.h"
 #include "chrome/browser/chromeos/frame/panel_controller.h"
 #include "third_party/cros/chromeos_wm_ipc_enums.h"
 #include "views/widget/widget.h"
@@ -100,6 +101,11 @@ void PanelBrowserView::SetCreatorView(PanelBrowserView* creator) {
   creator_xid_ = ui::GetX11WindowFromGtkWidget(GTK_WIDGET(window));
 }
 
+WindowOpenDisposition PanelBrowserView::GetDispositionForPopupBounds(
+    const gfx::Rect& bounds) {
+  return chromeos::BrowserView::DispositionForPopupBounds(bounds);
+}
+
 bool PanelBrowserView::GetSavedWindowBounds(gfx::Rect* bounds) const {
   bool res = ::BrowserView::GetSavedWindowBounds(bounds);
   if (res)
@@ -150,4 +156,7 @@ void PanelBrowserView::ActivatePanel() {
   Activate();
 }
 
+void PanelBrowserView::OnPanelStateChanged(PanelController::State state) {
+}
+
 }  // namespace chromeos
diff --git a/chrome/browser/chromeos/frame/panel_browser_view.h b/chrome/browser/chromeos/frame/panel_browser_view.h
index b31d022..487914f 100644
--- a/chrome/browser/chromeos/frame/panel_browser_view.h
+++ b/chrome/browser/chromeos/frame/panel_browser_view.h
@@ -27,27 +27,31 @@ class PanelBrowserView : public ::BrowserView,
   virtual ~PanelBrowserView();
 
   // BrowserView overrides.
-  virtual void Show();
-  virtual void ShowInactive();
-  virtual void SetBounds(const gfx::Rect& bounds);
-  virtual void Close();
-  virtual void UpdateTitleBar();
-  virtual void SetCreatorView(PanelBrowserView* creator);
-  virtual bool GetSavedWindowBounds(gfx::Rect* bounds) const;
-  virtual void OnWindowActivationChanged(bool active);
+  virtual void Show() OVERRIDE;
+  virtual void ShowInactive() OVERRIDE;
+  virtual void SetBounds(const gfx::Rect& bounds) OVERRIDE;
+  virtual void Close() OVERRIDE;
+  virtual void UpdateTitleBar() OVERRIDE;
+  virtual WindowOpenDisposition GetDispositionForPopupBounds(
+      const gfx::Rect& bounds) OVERRIDE OVERRIDE;
+  virtual bool GetSavedWindowBounds(gfx::Rect* bounds) const OVERRIDE;
+  virtual void OnWindowActivationChanged(bool active) OVERRIDE;
 
   // BrowserView : TabStripModelObserver overrides.
   virtual void TabChangedAt(TabContentsWrapper* contents,
                             int index,
-                            TabChangeType change_type);
-
-  // PanelController::Delegate overrides
-  virtual string16 GetPanelTitle();
-  virtual SkBitmap GetPanelIcon();
-  virtual bool CanClosePanel();
-  virtual void ClosePanel();
-  virtual void ActivatePanel();
-  virtual void OnPanelStateChanged(PanelController::State state) {}
+                            TabChangeType change_type) OVERRIDE;
+
+  // PanelController::Delegate overrides.
+  virtual string16 GetPanelTitle() OVERRIDE;
+  virtual SkBitmap GetPanelIcon() OVERRIDE;
+  virtual bool CanClosePanel() OVERRIDE;
+  virtual void ClosePanel() OVERRIDE;
+  virtual void ActivatePanel() OVERRIDE;
+  virtual void OnPanelStateChanged(PanelController::State state) OVERRIDE;
+
+  // Specific to PanelBrowserView.
+  void SetCreatorView(PanelBrowserView* creator);
 
  private:
   // Enforces the min, max, and default bounds.
diff --git a/chrome/browser/extensions/window_open_apitest.cc b/chrome/browser/extensions/window_open_apitest.cc
index ed877c2..92e08b3 100644
--- a/chrome/browser/extensions/window_open_apitest.cc
+++ b/chrome/browser/extensions/window_open_apitest.cc
@@ -26,26 +26,26 @@ void WaitForTabsAndPopups(Browser* browser, int num_tabs, int num_popups) {
   ++num_tabs;
   size_t num_browsers = static_cast<size_t>(num_popups) + 1;
 
-  while (true) {
-    if (BrowserList::GetBrowserCount(browser->profile()) < num_browsers ||
-        browser->tab_count() < num_tabs) {
-      MessageLoopForUI::current()->RunAllPending();
-      continue;
-    }
-
-    ASSERT_EQ(num_browsers, BrowserList::GetBrowserCount(browser->profile()));
-    ASSERT_EQ(num_tabs, browser->tab_count());
+  const base::TimeDelta kWaitTime = base::TimeDelta::FromSeconds(15);
+  base::TimeTicks end_time = base::TimeTicks::Now() + kWaitTime;
+  while (base::TimeTicks::Now() < end_time) {
+    if (BrowserList::GetBrowserCount(browser->profile()) >= num_browsers &&
+        browser->tab_count() >= num_tabs)
+      break;
+
+    MessageLoopForUI::current()->RunAllPending();
+  }
 
-    for (BrowserList::const_iterator iter = BrowserList::begin();
-         iter != BrowserList::end(); ++iter) {
-      if (*iter == browser)
-        continue;
+  EXPECT_EQ(num_browsers, BrowserList::GetBrowserCount(browser->profile()));
+  EXPECT_EQ(num_tabs, browser->tab_count());
 
-      // Check for TYPE_POPUP or TYPE_PANEL.
-      ASSERT_TRUE((*iter)->is_type_popup() || (*iter)->is_type_panel());
-    }
+  for (BrowserList::const_iterator iter = BrowserList::begin();
+       iter != BrowserList::end(); ++iter) {
+    if (*iter == browser)
+      continue;
 
-    break;
+    // Check for TYPE_POPUP or TYPE_PANEL.
+    EXPECT_TRUE((*iter)->is_type_popup() || (*iter)->is_type_panel());
   }
 }
 
@@ -66,6 +66,45 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, BrowserIsApp) {
   }
 }
 
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupDefault) {
+  ASSERT_TRUE(StartTestServer());
+  ASSERT_TRUE(LoadExtension(
+      test_data_dir_.AppendASCII("window_open").AppendASCII("popup")));
+
+  const int num_tabs = 1;
+  const int num_popups = 0;
+  WaitForTabsAndPopups(browser(), num_tabs, num_popups);
+}
+
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupLarge) {
+  ASSERT_TRUE(StartTestServer());
+  ASSERT_TRUE(LoadExtension(
+      test_data_dir_.AppendASCII("window_open").AppendASCII("popup_large")));
+
+#if defined(OS_CHROMEOS)
+  // On ChromeOS this should open a new tab.
+  const int num_tabs = 1;
+  const int num_popups = 0;
+#else
+  // On other systems this should open a new popup window.
+  const int num_tabs = 0;
+  const int num_popups = 1;
+#endif
+  WaitForTabsAndPopups(browser(), num_tabs, num_popups);
+}
+
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupSmall) {
+  ASSERT_TRUE(StartTestServer());
+  ASSERT_TRUE(LoadExtension(
+      test_data_dir_.AppendASCII("window_open").AppendASCII("popup_small")));
+
+  // On ChromeOS this should open a new panel (acts like a new popup window).
+  // On other systems this should open a new popup window.
+  const int num_tabs = 0;
+  const int num_popups = 1;
+  WaitForTabsAndPopups(browser(), num_tabs, num_popups);
+}
+
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PopupBlockingExtension) {
   host_resolver()->AddRule("*", "127.0.0.1");
   ASSERT_TRUE(StartTestServer());
diff --git a/chrome/browser/ui/browser_navigator_browsertest_chromeos.cc b/chrome/browser/ui/browser_navigator_browsertest_chromeos.cc
index d70599f..22d99c6 100644
--- a/chrome/browser/ui/browser_navigator_browsertest_chromeos.cc
+++ b/chrome/browser/ui/browser_navigator_browsertest_chromeos.cc
@@ -10,6 +10,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_navigator.h"
+#include "chrome/test/ui_test_utils.h"
 #include "content/browser/tab_contents/tab_contents.h"
 
 namespace {
@@ -52,4 +53,52 @@ IN_PROC_BROWSER_TEST_F(BrowserGuestSessionNavigatorTest,
             incognito_browser->GetSelectedTabContents()->GetURL());
 }
 
+// This test verifies that navigating to a large window with
+// WindowOpenDisposition = NEW_POPUP from a normal Browser results in a new tab.
+IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, Disposition_LargePopup) {
+  browser::NavigateParams p(MakeNavigateParams());
+  p.disposition = NEW_POPUP;
+  p.window_bounds = gfx::Rect(0, 0, 10000, 10000);
+  browser::Navigate(&p);
+  // Wait for page to load.
+  ui_test_utils::WaitForNavigationInCurrentTab(p.browser);
+
+  // Navigate() should have opened a new tab.
+  EXPECT_EQ(browser(), p.browser);
+
+  // We should have one window with two tabs.
+  EXPECT_EQ(1u, BrowserList::size());
+  EXPECT_EQ(2, browser()->tab_count());
+}
+
+// This test verifies that navigating to a large window with
+// WindowOpenDisposition = NEW_POPUP from a popup window results in a new tab
+// in the parent browser.
+IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, Disposition_LargePopupFromPopup) {
+  // Open a popup.
+  browser::NavigateParams p1(MakeNavigateParams());
+  p1.disposition = NEW_POPUP;
+  p1.window_bounds = gfx::Rect(0, 0, 200, 200);
+  browser::Navigate(&p1);
+
+  // Wait for page to load.
+  ui_test_utils::WaitForNavigationInCurrentTab(p1.browser);
+
+  // Open a large popup from the popup.
+  browser::NavigateParams p2(MakeNavigateParams(p1.browser));
+  p2.disposition = NEW_POPUP;
+  p2.window_bounds = gfx::Rect(0, 0, 10000, 10000);
+  browser::Navigate(&p2);
+
+  // Wait for page to load.
+  ui_test_utils::WaitForNavigationInCurrentTab(p2.browser);
+
+  // Navigate() should have opened a new tab in the primary browser.
+  EXPECT_EQ(browser(), p2.browser);
+
+  // We should have two windows. browser() should have two tabs.
+  EXPECT_EQ(2u, BrowserList::size());
+  EXPECT_EQ(2, browser()->tab_count());
+}
+
 }  // namespace
diff --git a/chrome/test/data/extensions/api_test/window_open/popup/background.html b/chrome/test/data/extensions/api_test/window_open/popup/background.html
new file mode 100644
index 0000000..4da3b44
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/window_open/popup/background.html
@@ -0,0 +1,3 @@
+<script>
+  window.open("content.html", "content", "");
+</script>
diff --git a/chrome/test/data/extensions/api_test/window_open/popup/content.html b/chrome/test/data/extensions/api_test/window_open/popup/content.html
new file mode 100644
index 0000000..be80a9c
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/window_open/popup/content.html
@@ -0,0 +1,5 @@
+<html>
+  <body>
+    window.open extension test for ChromeOS.
+  </body>
+</html>
diff --git a/chrome/test/data/extensions/api_test/window_open/popup/manifest.json b/chrome/test/data/extensions/api_test/window_open/popup/manifest.json
new file mode 100644
index 0000000..8da17dd
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/window_open/popup/manifest.json
@@ -0,0 +1,7 @@
+{
+  "name": "ChromeOS window.open Test",
+  "version": "1",
+  "description": "Test for window.open() from an extension (Unspecified size)",
+  "background_page": "background.html",
+  "permissions": ["tabs"]
+}
diff --git a/chrome/test/data/extensions/api_test/window_open/popup_large/background.html b/chrome/test/data/extensions/api_test/window_open/popup_large/background.html
new file mode 100644
index 0000000..0984b89
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/window_open/popup_large/background.html
@@ -0,0 +1,3 @@
+<script>
+  window.open("content.html", "content", "height=10000,width=10000");
+</script>
diff --git a/chrome/test/data/extensions/api_test/window_open/popup_large/content.html b/chrome/test/data/extensions/api_test/window_open/popup_large/content.html
new file mode 100644
index 0000000..be80a9c
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/window_open/popup_large/content.html
@@ -0,0 +1,5 @@
+<html>
+  <body>
+    window.open extension test for ChromeOS.
+  </body>
+</html>
diff --git a/chrome/test/data/extensions/api_test/window_open/popup_large/manifest.json b/chrome/test/data/extensions/api_test/window_open/popup_large/manifest.json
new file mode 100644
index 0000000..65f714a1
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/window_open/popup_large/manifest.json
@@ -0,0 +1,7 @@
+{
+  "name": "ChromeOS window.open Test",
+  "version": "1",
+  "description": "Test for window.open() from an extension (Large window)",
+  "background_page": "background.html",
+  "permissions": ["tabs"]
+}
diff --git a/chrome/test/data/extensions/api_test/window_open/popup_small/background.html b/chrome/test/data/extensions/api_test/window_open/popup_small/background.html
new file mode 100644
index 0000000..7913d9a
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/window_open/popup_small/background.html
@@ -0,0 +1,3 @@
+<script>
+  window.open("content.html", "content", "height=200,width=200");
+</script>
diff --git a/chrome/test/data/extensions/api_test/window_open/popup_small/content.html b/chrome/test/data/extensions/api_test/window_open/popup_small/content.html
new file mode 100644
index 0000000..be80a9c
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/window_open/popup_small/content.html
@@ -0,0 +1,5 @@
+<html>
+  <body>
+    window.open extension test for ChromeOS.
+  </body>
+</html>
diff --git a/chrome/test/data/extensions/api_test/window_open/popup_small/manifest.json b/chrome/test/data/extensions/api_test/window_open/popup_small/manifest.json
new file mode 100644
index 0000000..98808c9
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/window_open/popup_small/manifest.json
@@ -0,0 +1,7 @@
+{
+  "name": "ChromeOS window.open Test",
+  "version": "1",
+  "description": "Test for window.open() from an extension (Small)",
+  "background_page": "background.html",
+  "permissions": ["tabs"]
+}