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

chrome/browser/browser_navigator.cc

@@ -101,41 +101,59 @@ Browser* GetBrowserForDisposition(browser::NavigateParams* params) {
   // If no source TabContents was specified, we use the selected one from the
   // target browser. This must happen first, before GetBrowserForDisposition()
   // has a chance to replace |params->browser| with another one.
-  if (!params->source_contents)
+  if (!params->source_contents && params->browser)
     params->source_contents = params->browser->GetSelectedTabContents();
 
+  Profile* profile =
+      params->browser ? params->browser->profile() : params->profile;
+
   switch (params->disposition) {
     case CURRENT_TAB:
+      if (!params->browser && profile) {
+        // We specified a profile instead of a browser; find or create one.
+        params->browser = Browser::GetOrCreateTabbedBrowser(profile);
+      }
       return params->browser;
     case SINGLETON_TAB:
     case NEW_FOREGROUND_TAB:
     case NEW_BACKGROUND_TAB:
       // See if we can open the tab in the window this navigator is bound to.
-      if (WindowCanOpenTabs(params->browser))
+      if (params->browser && WindowCanOpenTabs(params->browser))
         return params->browser;
       // Find a compatible window and re-execute this command in it. Otherwise
       // re-run with NEW_WINDOW.
-      return GetOrCreateBrowser(params->browser->profile());
+      if (profile)
+        return GetOrCreateBrowser(profile);
+      return NULL;
     case NEW_POPUP: {
       // Make a new popup window. Coerce app-style if |params->browser| or the
       // |source| represents an app.
       Browser::Type type = Browser::TYPE_POPUP;
-      if (params->browser->type() == Browser::TYPE_APP ||
+      if ((params->browser && params->browser->type() == Browser::TYPE_APP) ||
           (params->source_contents && params->source_contents->is_app())) {
         type = Browser::TYPE_APP_POPUP;
       }
-      Browser* browser = new Browser(type, params->browser->profile());
-      browser->set_override_bounds(params->window_bounds);
-      browser->CreateBrowserWindow();
-      return browser;
+      if (profile) {
+        Browser* browser = new Browser(type, profile);
+        browser->set_override_bounds(params->window_bounds);
+        browser->CreateBrowserWindow();
+        return browser;
+      }
+      return NULL;
     }
     case NEW_WINDOW:
       // Make a new normal browser window.
-      return Browser::Create(params->browser->profile());
+      if (profile) {
+        Browser* browser = new Browser(Browser::TYPE_NORMAL, profile);
+        browser->CreateBrowserWindow();
+        return browser;
+      }
+      return NULL;
     case OFF_THE_RECORD:
       // Make or find an incognito window.
-      return GetOrCreateBrowser(
-          params->browser->profile()->GetOffTheRecordProfile());
+      if (profile)
+        return GetOrCreateBrowser(profile->GetOffTheRecordProfile());
+      return NULL;
     // The following types all result in no navigation.
     case SUPPRESS_OPEN:
     case SAVE_TO_DISK:
@@ -153,7 +171,8 @@ void NormalizeDisposition(browser::NavigateParams* params) {
   // Calculate the WindowOpenDisposition if necessary.
   if (params->browser->tabstrip_model()->empty() &&
       (params->disposition == NEW_BACKGROUND_TAB ||
-       params->disposition == CURRENT_TAB)) {
+       params->disposition == CURRENT_TAB ||
+       params->disposition == SINGLETON_TAB)) {
     params->disposition = NEW_FOREGROUND_TAB;
   }
   if (params->browser->profile()->IsOffTheRecord() &&
@@ -241,8 +260,8 @@ NavigateParams::NavigateParams(
       tabstrip_index(-1),
       tabstrip_add_types(TabStripModel::ADD_SELECTED),
       show_window(false),
-      browser(a_browser) {
-  DCHECK(browser);
+      browser(a_browser),
+      profile(NULL) {
 }
 
 NavigateParams::NavigateParams(Browser* a_browser,
@@ -254,8 +273,8 @@ NavigateParams::NavigateParams(Browser* a_browser,
       tabstrip_index(-1),
       tabstrip_add_types(TabStripModel::ADD_SELECTED),
       show_window(false),
-      browser(a_browser) {
-  DCHECK(browser);
+      browser(a_browser),
+      profile(NULL) {
 }
 
 NavigateParams::~NavigateParams() {
@@ -310,6 +329,7 @@ void Navigate(NavigateParams* params) {
       // ... otherwise if we're loading in the current tab, the target is the
       // same as the source.
       params->target_contents = params->source_contents;
+      DCHECK(params->target_contents);
     }
 
     if (user_initiated) {
@@ -360,4 +380,3 @@ void Navigate(NavigateParams* params) {
 }
 
 }  // namespace browser
-

chrome/browser/browser_navigator.h

@@ -118,8 +118,8 @@ struct NavigateParams {
 
   // [in]  Specifies a Browser object where the navigation could occur or the
   //       tab could be added. Navigate() is not obliged to use this Browser if
-  //       it is not compatible with the operation being performed. Cannot be
-  //       NULL since Navigate() uses this Browser's Profile.
+  //       it is not compatible with the operation being performed. If NULL,
+  //       |profile| should be specified to find or create a matching Browser.
   // [out] Specifies the Browser object where the navigation occurred or the
   //       tab was added. Guaranteed non-NULL unless the disposition did not
   //       require a navigation, in which case this is set to NULL
@@ -130,6 +130,10 @@ struct NavigateParams {
   //       objects are deleted when the user closes a visible browser window).
   Browser* browser;
 
+  // If |browser| == NULL, specifies a Profile to use when finding or
+  // creating a Browser.
+  Profile* profile;
+
  private:
   NavigateParams();
 };
@@ -140,4 +144,3 @@ void Navigate(NavigateParams* params);
 }  // namespace browser
 
 #endif  // CHROME_BROWSER_BROWSER_NAVIGATOR_H_
-

chrome/browser/browser_navigator_browsertest.cc

@@ -407,4 +407,61 @@ IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, Tabstrip_InsertAtIndex) {
   EXPECT_EQ(2, browser()->tab_count());
 }
 
+// This test verifies that constructing params with a NULL browser has
+// the same result as navigating to a new foreground tab in the (only)
+// active browser. Tests are the same as for Disposition_NewForegroundTab.
+IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, NullBrowser_NewForegroundTab) {
+  TabContents* old_contents = browser()->GetSelectedTabContents();
+  // Navigate with a NULL browser.
+  browser::NavigateParams p(MakeNavigateParams(NULL));
+  p.disposition = NEW_FOREGROUND_TAB;
+  p.profile = browser()->profile();
+  browser::Navigate(&p);
+
+  // Navigate() should have found browser() and create a new tab.
+  EXPECT_EQ(browser(), p.browser);
+  EXPECT_NE(old_contents, browser()->GetSelectedTabContents());
+  EXPECT_EQ(browser()->GetSelectedTabContents(), p.target_contents);
+  EXPECT_EQ(2, browser()->tab_count());
+}
+
+// This test verifies that constructing params with a NULL browser and
+// a specific profile matches the specified profile.
+IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, NullBrowser_MatchProfile) {
+  // Create a new browser with using the incognito profile.
+  Browser* incognito =
+      Browser::Create(browser()->profile()->GetOffTheRecordProfile());
+
+  // Navigate with a NULL browser and the incognito profile.
+  browser::NavigateParams p(MakeNavigateParams(NULL));
+  p.disposition = NEW_FOREGROUND_TAB;
+  p.profile = incognito->profile();
+  browser::Navigate(&p);
+
+  // Navigate() should have found incognito, not browser().
+  EXPECT_EQ(incognito, p.browser);
+  EXPECT_EQ(incognito->GetSelectedTabContents(), p.target_contents);
+  EXPECT_EQ(1, incognito->tab_count());
+}
+
+// This test verifies that constructing params with a NULL browser and
+// disposition = NEW_WINDOW always opens exactly one new window.
+IN_PROC_BROWSER_TEST_F(BrowserNavigatorTest, NullBrowser_NewWindow) {
+  browser::NavigateParams p(MakeNavigateParams(NULL));
+  p.disposition = NEW_WINDOW;
+  p.profile = browser()->profile();
+  browser::Navigate(&p);
+
+  // Navigate() should have created a new browser.
+  EXPECT_NE(browser(), p.browser);
+  EXPECT_EQ(Browser::TYPE_NORMAL, p.browser->type());
+
+  // We should now have two windows, the browser() provided by the framework and
+  // the new normal window.
+  EXPECT_EQ(2u, BrowserList::size());
+  EXPECT_EQ(1, browser()->tab_count());
+  EXPECT_EQ(1, p.browser->tab_count());
+}
+
+
 }  // namespace

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;
 }
-

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_
-

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
-

chrome/browser/views/bookmark_bar_view.cc

@@ -447,11 +447,13 @@ void BookmarkBarView::SetProfile(Profile* profile) {
     delete GetChildViewAt(0);
 
   model_ = profile_->GetBookmarkModel();
-  model_->AddObserver(this);
-  if (model_->IsLoaded())
-    Loaded(model_);
-  // else case: we'll receive notification back from the BookmarkModel when done
-  // loading, then we'll populate the bar.
+  if (model_) {
+    model_->AddObserver(this);
+    if (model_->IsLoaded())
+      Loaded(model_);
+    // else case: we'll receive notification back from the BookmarkModel when
+    // done loading, then we'll populate the bar.
+  }
 }
 
 void BookmarkBarView::SetPageNavigator(PageNavigator* navigator) {