Change window opening behavior for HtmlDialogTabContentsDelegate.

Also changed code to use existing Browser APIs and preserved the desired disposition.
See issue for more info.

BUG=http://code.google.com/p/chromium-os/issues/detail?id=8013
TEST=See issue

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

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

3 files changed, 20 insertions, 81 deletions

diff --git a/chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc b/chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc
index 303d071..cc8ae07 100644
--- a/chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc
+++ b/chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc
@@ -27,22 +27,17 @@ void HtmlDialogTabContentsDelegate::Detach() {
   profile_ = NULL;
 }
 
-Browser* HtmlDialogTabContentsDelegate::CreateBrowser() {
-  DCHECK(profile_);
-  return Browser::Create(profile_);
-}
-
 void HtmlDialogTabContentsDelegate::OpenURLFromTab(
     TabContents* source, const GURL& url, const GURL& referrer,
     WindowOpenDisposition disposition, PageTransition::Type transition) {
   if (profile_) {
-    // Force all links to open in a new window, ignoring the incoming
-    // disposition. This is a tabless, modal dialog so we can't just
-    // open it in the current frame.  Code adapted from
-    // Browser::OpenURLFromTab() with disposition == NEW_WINDOW.
-    browser::NavigateParams params(CreateBrowser(), url, transition);
+    // Specify a NULL browser for navigation. This will cause Navigate()
+    // to find a browser matching params.profile or create a new one.
+    Browser* browser = NULL;
+    browser::NavigateParams params(browser, url, transition);
+    params.profile = profile_;
     params.referrer = referrer;
-    params.disposition = NEW_FOREGROUND_TAB;
+    params.disposition = disposition;
     params.show_window = true;
     browser::Navigate(&params);
   }
@@ -59,11 +54,13 @@ void HtmlDialogTabContentsDelegate::AddNewContents(
     WindowOpenDisposition disposition, const gfx::Rect& initial_pos,
     bool user_gesture) {
   if (profile_) {
-    // Force this to open in a new window so that it appears at the top
-    // of the z-index.
-    browser::NavigateParams params(CreateBrowser(), new_contents);
+    // Specify a NULL browser for navigation. This will cause Navigate()
+    // to find a browser matching params.profile or create a new one.
+    Browser* browser = NULL;
+    browser::NavigateParams params(browser, new_contents);
+    params.profile = profile_;
     params.source_contents = source;
-    params.disposition = NEW_FOREGROUND_TAB;
+    params.disposition = disposition;
     params.window_bounds = initial_pos;
     params.show_window = true;
     browser::Navigate(&params);
@@ -111,4 +108,3 @@ bool HtmlDialogTabContentsDelegate::ShouldAddNavigationToHistory(
     NavigationType::Type navigation_type) {
   return false;
 }
-
diff --git a/chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h b/chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h
index b0ab266..c6ec732 100644
--- a/chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h
+++ b/chrome/browser/dom_ui/html_dialog_tab_contents_delegate.h
@@ -58,13 +58,8 @@ class HtmlDialogTabContentsDelegate : public TabContentsDelegate {
       const history::HistoryAddPageArgs& add_page_args,
       NavigationType::Type navigation_type);
 
- protected:
-  // Overridden only for testing.
-  virtual Browser* CreateBrowser();
-
  private:
   Profile* profile_;  // Weak pointer.  Always an original profile.
 };
 
 #endif  // CHROME_BROWSER_DOM_UI_HTML_DIALOG_TAB_CONTENTS_DELEGATE_H_
-
diff --git a/chrome/browser/dom_ui/html_dialog_tab_contents_delegate_unittest.cc b/chrome/browser/dom_ui/html_dialog_tab_contents_delegate_unittest.cc
index ac3ccaa..d49bfdd 100644
--- a/chrome/browser/dom_ui/html_dialog_tab_contents_delegate_unittest.cc
+++ b/chrome/browser/dom_ui/html_dialog_tab_contents_delegate_unittest.cc
@@ -23,63 +23,18 @@
 
 namespace {
 
-class MockTestBrowserWindow : public TestBrowserWindow {
- public:
-  // TestBrowserWindow() doesn't actually use its browser argument so we just
-  // pass NULL.
-  MockTestBrowserWindow() : TestBrowserWindow(NULL) {}
-
-  virtual ~MockTestBrowserWindow() {}
-
-  MOCK_METHOD0(Show, void());
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockTestBrowserWindow);
-};
-
 class TestTabContentsDelegate : public HtmlDialogTabContentsDelegate {
  public:
   explicit TestTabContentsDelegate(Profile* profile)
-    : HtmlDialogTabContentsDelegate(profile),
-      window_for_next_created_browser_(NULL) {}
+    : HtmlDialogTabContentsDelegate(profile) {}
 
   virtual ~TestTabContentsDelegate() {
-    CHECK(!window_for_next_created_browser_);
-    for (std::vector<Browser*>::iterator i = created_browsers_.begin();
-         i != created_browsers_.end(); ++i) {
-      (*i)->CloseAllTabs();
-      // We need to explicitly cast this since BrowserWindow does *not*
-      // have a virtual destructor.
-      delete static_cast<MockTestBrowserWindow*>((*i)->window());
-      delete *i;
-    }
   }
 
   virtual void MoveContents(TabContents* source, const gfx::Rect& pos) {}
   virtual void ToolbarSizeChanged(TabContents* source, bool is_animating) {}
 
-  // Takes ownership of window.
-  void SetWindowForNextCreatedBrowser(BrowserWindow* window) {
-    CHECK(window);
-    window_for_next_created_browser_ = window;
-  }
-
- protected:
-  // CreateBrowser() is called by OpenURLFromTab() and AddNewContents().
-  virtual Browser* CreateBrowser() {
-    DCHECK(profile());
-    DCHECK(window_for_next_created_browser_);
-    Browser* browser = new Browser(Browser::TYPE_NORMAL, profile());
-    browser->set_window(window_for_next_created_browser_);
-    window_for_next_created_browser_ = NULL;
-    created_browsers_.push_back(browser);
-    return browser;
-  }
-
  private:
-  BrowserWindow* window_for_next_created_browser_;
-  std::vector<Browser*> created_browsers_;
-
   DISALLOW_COPY_AND_ASSIGN(TestTabContentsDelegate);
 };
 
@@ -124,28 +79,22 @@ TEST_F(HtmlDialogTabContentsDelegateTest, DoNothingMethodsTest) {
 }
 
 TEST_F(HtmlDialogTabContentsDelegateTest, OpenURLFromTabTest) {
-  MockTestBrowserWindow* window = new MockTestBrowserWindow();
-  EXPECT_CALL(*window, Show()).Times(1);
-  test_tab_contents_delegate_->SetWindowForNextCreatedBrowser(window);
-
   test_tab_contents_delegate_->OpenURLFromTab(
     NULL, GURL(chrome::kAboutBlankURL), GURL(),
     NEW_FOREGROUND_TAB, PageTransition::LINK);
-  EXPECT_EQ(0, browser()->tab_count());
-  EXPECT_EQ(2U, BrowserList::size());
+  // This should create a new foreground tab in the existing browser.
+  EXPECT_EQ(1, browser()->tab_count());
+  EXPECT_EQ(1U, BrowserList::size());
 }
 
-TEST_F(HtmlDialogTabContentsDelegateTest, AddNewContentsTest) {
-  MockTestBrowserWindow* window = new MockTestBrowserWindow();
-  EXPECT_CALL(*window, Show()).Times(1);
-  test_tab_contents_delegate_->SetWindowForNextCreatedBrowser(window);
-
+TEST_F(HtmlDialogTabContentsDelegateTest, AddNewContentsForegroundTabTest) {
   TabContents* contents =
       new TabContents(profile(), NULL, MSG_ROUTING_NONE, NULL, NULL);
   test_tab_contents_delegate_->AddNewContents(
       NULL, contents, NEW_FOREGROUND_TAB, gfx::Rect(), false);
-  EXPECT_EQ(0, browser()->tab_count());
-  EXPECT_EQ(2U, BrowserList::size());
+  // This should create a new foreground tab in the existing browser.
+  EXPECT_EQ(1, browser()->tab_count());
+  EXPECT_EQ(1U, BrowserList::size());
 }
 
 TEST_F(HtmlDialogTabContentsDelegateTest, DetachTest) {
@@ -163,4 +112,3 @@ TEST_F(HtmlDialogTabContentsDelegateTest, DetachTest) {
 }
 
 }  // namespace
-