Re-land popup routing fix for browser.

Just the bugfix + test now. Test is disabled pending a working framework.

http://crbug.com/8472

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

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

15 files changed, 162 insertions, 75 deletions

diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index 2d73a9c..471b4bc 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -993,8 +993,9 @@ void AutomationProvider::AppendTab(int handle, const GURL& url,
     Browser* browser = browser_tracker_->GetResource(handle);
     observer = AddTabStripObserver(browser, reply_message->routing_id(),
                                    reply_message);
-    TabContents* tab_contents =
-        browser->AddTabWithURL(url, GURL(), PageTransition::TYPED, true, NULL);
+    TabContents* tab_contents = browser->AddTabWithURL(url, GURL(),
+                                                       PageTransition::TYPED,
+                                                       true, -1, NULL);
     if (tab_contents) {
       append_tab_response =
           GetIndexForNavigationController(tab_contents->controller(), browser);
diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index 374ca34..9a8648f 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -308,7 +308,7 @@ void Browser::OpenURLOffTheRecord(Profile* profile, const GURL& url) {
   if (!browser)
     browser = Browser::Create(off_the_record_profile);
   // TODO(eroman): should we have referrer here?
-  browser->AddTabWithURL(url, GURL(), PageTransition::LINK, true, NULL);
+  browser->AddTabWithURL(url, GURL(), PageTransition::LINK, true, -1, NULL);
   browser->window()->Show();
 }
 
@@ -318,7 +318,8 @@ void Browser::OpenApplicationWindow(Profile* profile, const GURL& url) {
   RegisterAppPrefs(app_name);
 
   Browser* browser = Browser::CreateForApp(app_name, profile, false);
-  browser->AddTabWithURL(url, GURL(), PageTransition::START_PAGE, true, NULL);
+  browser->AddTabWithURL(url, GURL(), PageTransition::START_PAGE, true, -1,
+                         NULL);
   browser->window()->Show();
   // TODO(jcampan): http://crbug.com/8123 we should not need to set the initial
   //                focus explicitly.
@@ -479,23 +480,27 @@ void Browser::OnWindowClosing() {
 
 TabContents* Browser::AddTabWithURL(
     const GURL& url, const GURL& referrer, PageTransition::Type transition,
-    bool foreground, SiteInstance* instance) {
-  if ((type_ & TYPE_APP) != 0 && tabstrip_model_.count() == 1) {
-    NOTREACHED() << "Cannot add a tab in a mono tab application.";
-    return NULL;
-  }
-
-  GURL url_to_load = url;
-  if (url_to_load.is_empty())
-    url_to_load = GetHomePage();
-  TabContents* contents =
-      CreateTabContentsForURL(url_to_load, referrer, profile_, transition,
-                              false, instance);
-  tabstrip_model_.AddTabContents(contents, -1, transition, foreground);
-  // By default, content believes it is not hidden.  When adding contents
-  // in the background, tell it that it's hidden.
-  if (!foreground)
-    contents->WasHidden();
+    bool foreground, int index, SiteInstance* instance) {
+  TabContents* contents = NULL;
+  if (type_ == TYPE_NORMAL || tabstrip_model()->empty()) {
+    GURL url_to_load = url;
+    if (url_to_load.is_empty())
+      url_to_load = GetHomePage();
+    contents = CreateTabContentsForURL(url_to_load, referrer, profile_,
+                                       transition, false, instance);
+    tabstrip_model_.AddTabContents(contents, index, transition, foreground);
+    // By default, content believes it is not hidden.  When adding contents
+    // in the background, tell it that it's hidden.
+    if (!foreground)
+      contents->WasHidden();
+  } else {
+    // We're in an app window or a popup window. Find an existing browser to
+    // open this URL in, creating one if none exists.
+    Browser* b = GetOrCreateTabbedBrowser();
+    contents = b->AddTabWithURL(url, referrer, transition, foreground, index,
+                                instance);
+    b->window()->Show();
+  }
   return contents;
 }
 
@@ -548,7 +553,7 @@ void Browser::ShowSingleDOMUITab(const GURL& url) {
   }
 
   // Otherwise, just create a new tab.
-  AddTabWithURL(url, GURL(), PageTransition::AUTO_BOOKMARK, true, NULL);
+  AddTabWithURL(url, GURL(), PageTransition::AUTO_BOOKMARK, true, -1, NULL);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -1076,7 +1081,7 @@ void Browser::OpenAboutChromeDialog() {
 
 void Browser::OpenHelpTab() {
   GURL help_url(WideToASCII(l10n_util::GetString(IDS_HELP_CONTENT_URL)));
-  AddTabWithURL(help_url, GURL(), PageTransition::AUTO_BOOKMARK, true,
+  AddTabWithURL(help_url, GURL(), PageTransition::AUTO_BOOKMARK, true, -1,
                 NULL);
 }
 
@@ -1306,8 +1311,13 @@ void Browser::ExecuteCommand(int id) {
 ///////////////////////////////////////////////////////////////////////////////
 // Browser, TabStripModelDelegate implementation:
 
-GURL Browser::GetBlankTabURL() const {
-  return GURL(chrome::kChromeUINewTabURL);
+TabContents* Browser::AddBlankTab(bool foreground) {
+  return AddBlankTabAt(-1, foreground);
+}
+
+TabContents* Browser::AddBlankTabAt(int index, bool foreground) {
+  return AddTabWithURL(GURL(chrome::kChromeUINewTabURL), GURL(),
+                       PageTransition::TYPED, foreground, index, NULL);
 }
 
 Browser* Browser::CreateNewStripWithContents(TabContents* detached_contents,
@@ -1655,7 +1665,7 @@ void Browser::OpenURLFromTab(TabContents* source,
     return;
   } else if (disposition == NEW_WINDOW) {
     Browser* browser = Browser::Create(profile_);
-    new_contents = browser->AddTabWithURL(url, referrer, transition, true,
+    new_contents = browser->AddTabWithURL(url, referrer, transition, true, -1,
                                           instance);
     browser->window()->Show();
   } else if ((disposition == CURRENT_TAB) && current_tab) {
@@ -1682,7 +1692,8 @@ void Browser::OpenURLFromTab(TabContents* source,
     return;
   } else if (disposition != SUPPRESS_OPEN) {
     new_contents = AddTabWithURL(url, referrer, transition,
-                                 disposition != NEW_BACKGROUND_TAB, instance);
+                                 disposition != NEW_BACKGROUND_TAB, -1,
+                                 instance);
   }
 
   if (disposition != NEW_BACKGROUND_TAB && source_tab_was_frontmost) {
diff --git a/chrome/browser/browser.h b/chrome/browser/browser.h
index e16a510..3ac5b47 100644
--- a/chrome/browser/browser.h
+++ b/chrome/browser/browser.h
@@ -202,9 +202,6 @@ class Browser : public TabStripModelDelegate,
   void SelectTabContentsAt(int index, bool user_gesture) {
     tabstrip_model_.SelectTabContentsAt(index, user_gesture);
   }
-  TabContents* AddBlankTab(bool foreground) {
-    return tabstrip_model_.AddBlankTab(foreground);
-  }
   void CloseAllTabs() {
     tabstrip_model_.CloseAllTabs();
   }
@@ -215,7 +212,7 @@ class Browser : public TabStripModelDelegate,
   // will be used to render the tab.
   TabContents* AddTabWithURL(
       const GURL& url, const GURL& referrer,
-      PageTransition::Type transition, bool foreground,
+      PageTransition::Type transition, bool foreground, int index,
       SiteInstance* instance);
 
   // Add a new tab, given a NavigationController. A TabContents appropriate to
@@ -371,7 +368,8 @@ class Browser : public TabStripModelDelegate,
   virtual void ExecuteCommand(int id);
 
   // Overridden from TabStripModelDelegate:
-  virtual GURL GetBlankTabURL() const;
+  virtual TabContents* AddBlankTab(bool foreground);
+  virtual TabContents* AddBlankTabAt(int index, bool foreground);
   virtual Browser* CreateNewStripWithContents(TabContents* detached_contents,
                                               const gfx::Rect& window_bounds,
                                               const DockInfo& dock_info);
@@ -572,6 +570,8 @@ class Browser : public TabStripModelDelegate,
   // as a key to store window location per application URLs.
   static std::wstring ComputeApplicationNameFromURL(const GURL& url);
 
+  FRIEND_TEST(BrowserTest, NoTabsInPopups);
+
   // Create a preference dictionary for the provided application name. This is
   // done only once per application name / per session.
   static void RegisterAppPrefs(const std::wstring& app_name);
diff --git a/chrome/browser/browser_focus_uitest.cc b/chrome/browser/browser_focus_uitest.cc
index 70b9a2a..cadb548 100644
--- a/chrome/browser/browser_focus_uitest.cc
+++ b/chrome/browser/browser_focus_uitest.cc
@@ -177,8 +177,10 @@ IN_PROC_BROWSER_TEST_F(BrowserFocusTest, TabsRememberFocus) {
   ASSERT_TRUE(focus_manager);
 
   // Create several tabs.
-  for (int i = 0; i < 4; ++i)
-    browser()->AddTabWithURL(url, GURL(), PageTransition::TYPED, true, NULL);
+  for (int i = 0; i < 4; ++i) {
+    browser()->AddTabWithURL(url, GURL(), PageTransition::TYPED, true, -1,
+                             NULL);
+  }
 
   // Alternate focus for the tab.
   const bool kFocusPage[3][5] = {
diff --git a/chrome/browser/browser_init.cc b/chrome/browser/browser_init.cc
index 0e8ac37..183bc9e 100644
--- a/chrome/browser/browser_init.cc
+++ b/chrome/browser/browser_init.cc
@@ -336,7 +336,7 @@ Browser* BrowserInit::LaunchWithProfile::OpenURLsInBrowser(
 
   for (size_t i = 0; i < urls.size(); ++i) {
     TabContents* tab = browser->AddTabWithURL(
-        urls[i], GURL(), PageTransition::START_PAGE, (i == 0), NULL);
+        urls[i], GURL(), PageTransition::START_PAGE, (i == 0), -1, NULL);
     if (i == 0 && process_startup)
       AddCrashedInfoBarIfNecessary(tab);
   }
diff --git a/chrome/browser/browser_unittest.cc b/chrome/browser/browser_unittest.cc
new file mode 100644
index 0000000..1ef86da
--- /dev/null
+++ b/chrome/browser/browser_unittest.cc
@@ -0,0 +1,85 @@
+// Copyright (c) 2009 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/browser.h"
+#include "chrome/test/in_process_browser_test.h"
+#include "net/base/host_resolver_unittest.h"
+
+class BrowserTest : public InProcessBrowserTest {
+ public:
+  BrowserTest() {
+    host_mapper_ = new net::RuleBasedHostMapper();
+    // Avoid making external DNS lookups. In this test we don't need this
+    // to succeed.
+    host_mapper_->AddSimulatedFailure("*.google.com");
+    scoped_host_mapper_.Init(host_mapper_.get());
+  }
+
+ private:
+  scoped_refptr<net::RuleBasedHostMapper> host_mapper_;
+  net::ScopedHostMapper scoped_host_mapper_;
+};
+
+/*
+// This tests that windows without tabstrips can't have new tabs opened in
+// them.
+IN_PROC_BROWSER_TEST_F(BrowserTest, NoTabsInPopups) {
+  Browser::RegisterAppPrefs(L"Test");
+
+  // We start with a normal browser with one tab.
+  EXPECT_EQ(1, browser()->tab_count());
+
+  // Open a popup browser with a single blank foreground tab.
+  Browser* popup_browser = browser()->CreateForPopup(browser()->profile());
+  popup_browser->AddBlankTab(true);
+  EXPECT_EQ(1, popup_browser->tab_count());
+
+  // Now try opening another tab in the popup browser.
+  popup_browser->AddTabWithURL(GURL("about:blank"), GURL(),
+                               PageTransition::TYPED, true, -1, NULL);
+  
+  // The popup should still only have one tab.
+  EXPECT_EQ(1, popup_browser->tab_count());
+
+  // The normal browser should now have two.
+  EXPECT_EQ(2, browser()->tab_count());
+
+  // Open an app frame browser with a single blank foreground tab.
+  Browser* app_browser =
+      browser()->CreateForApp(L"Test", browser()->profile(), false);
+  app_browser->AddBlankTab(true);
+  EXPECT_EQ(1, app_browser->tab_count());
+
+  // Now try opening another tab in the app browser.
+  app_browser->AddTabWithURL(GURL("about:blank"), GURL(),
+                             PageTransition::TYPED, true, -1, NULL);
+  
+  // The popup should still only have one tab.
+  EXPECT_EQ(1, app_browser->tab_count());
+
+  // The normal browser should now have three.
+  EXPECT_EQ(3, browser()->tab_count());
+
+  // Open an app frame popup browser with a single blank foreground tab.
+  Browser* app_popup_browser =
+      browser()->CreateForApp(L"Test", browser()->profile(), false);
+  app_popup_browser->AddBlankTab(true);
+  EXPECT_EQ(1, app_popup_browser->tab_count());
+
+  // Now try opening another tab in the app popup browser.
+  app_popup_browser->AddTabWithURL(GURL("about:blank"), GURL(),
+                                   PageTransition::TYPED, true, -1, NULL);
+  
+  // The popup should still only have one tab.
+  EXPECT_EQ(1, app_popup_browser->tab_count());
+
+  // The normal browser should now have four.
+  EXPECT_EQ(4, browser()->tab_count());
+
+  // Close the additional browsers.
+  popup_browser->CloseAllTabs();
+  app_browser->CloseAllTabs();
+  app_popup_browser->CloseAllTabs();
+}
+*/
\ No newline at end of file
diff --git a/chrome/browser/extensions/extension_tabs_module.cc b/chrome/browser/extensions/extension_tabs_module.cc
index 8568b43..5d0f6f2 100644
--- a/chrome/browser/extensions/extension_tabs_module.cc
+++ b/chrome/browser/extensions/extension_tabs_module.cc
@@ -44,7 +44,8 @@ bool CreateTabFunction::RunImpl() {
   // TODO(aa): Handle all the other properties of the new tab.
   std::string url;
   static_cast<const DictionaryValue*>(args_)->GetString(L"url", &url);
-  browser->AddTabWithURL(GURL(url), GURL(), PageTransition::TYPED, true, NULL);
+  browser->AddTabWithURL(GURL(url), GURL(), PageTransition::TYPED, true, -1,
+                         NULL);
 
   // Return data about the newly created tab.
   if (has_callback())
diff --git a/chrome/browser/profile_manager.cc b/chrome/browser/profile_manager.cc
index 407c39b..82d7192 100644
--- a/chrome/browser/profile_manager.cc
+++ b/chrome/browser/profile_manager.cc
@@ -139,7 +139,7 @@ Profile* ProfileManager::AddProfileByPath(const FilePath& path) {
 void ProfileManager::NewWindowWithProfile(Profile* profile) {
   DCHECK(profile);
   Browser* browser = Browser::Create(profile);
-  browser->AddTabWithURL(GURL(), GURL(), PageTransition::TYPED, true, NULL);
+  browser->AddTabWithURL(GURL(), GURL(), PageTransition::TYPED, true, -1, NULL);
   browser->window()->Show();
 }
 
diff --git a/chrome/browser/sessions/session_restore.cc b/chrome/browser/sessions/session_restore.cc
index 9563204..d7fb091 100644
--- a/chrome/browser/sessions/session_restore.cc
+++ b/chrome/browser/sessions/session_restore.cc
@@ -374,7 +374,7 @@ class SessionRestoreImpl : public NotificationObserver {
   void AppendURLsToBrowser(Browser* browser, const std::vector<GURL>& urls) {
     for (size_t i = 0; i < urls.size(); ++i) {
       browser->AddTabWithURL(urls[i], GURL(), PageTransition::START_PAGE,
-                            (i == 0), NULL);
+                            (i == 0), -1, NULL);
     }
   }
 
diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index e5ee652..43ec063 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -335,22 +335,6 @@ bool TabStripModel::ShouldResetGroupOnSelect(TabContents* contents) const {
   return contents_data_.at(index)->reset_group_on_select;
 }
 
-TabContents* TabStripModel::AddBlankTab(bool foreground) {
-  TabContents* contents = delegate_->CreateTabContentsForURL(
-      delegate_->GetBlankTabURL(), GURL(), profile_, PageTransition::TYPED,
-      false, NULL);
-  AddTabContents(contents, -1, PageTransition::TYPED, foreground);
-  return contents;
-}
-
-TabContents* TabStripModel::AddBlankTabAt(int index, bool foreground) {
-  TabContents* contents = delegate_->CreateTabContentsForURL(
-      delegate_->GetBlankTabURL(), GURL(), profile_, PageTransition::LINK,
-      false, NULL);
-  AddTabContents(contents, index, PageTransition::LINK, foreground);
-  return contents;
-}
-
 void TabStripModel::AddTabContents(TabContents* contents,
                                    int index,
                                    PageTransition::Type transition,
@@ -450,7 +434,7 @@ void TabStripModel::ExecuteContextMenuCommand(
   switch (command_id) {
     case CommandNewTab:
       UserMetrics::RecordAction(L"TabContextMenu_NewTab", profile_);
-      AddBlankTabAt(context_index + 1, true);
+      delegate()->AddBlankTabAt(context_index + 1, true);
       break;
     case CommandReload:
       UserMetrics::RecordAction(L"TabContextMenu_Reload", profile_);
diff --git a/chrome/browser/tabs/tab_strip_model.h b/chrome/browser/tabs/tab_strip_model.h
index 8a17b7a..3386954 100644
--- a/chrome/browser/tabs/tab_strip_model.h
+++ b/chrome/browser/tabs/tab_strip_model.h
@@ -105,8 +105,9 @@ class TabStripModelObserver {
 ///////////////////////////////////////////////////////////////////////////////
 class TabStripModelDelegate {
  public:
-  // Retrieve the URL that should be used to construct blank tabs.
-  virtual GURL GetBlankTabURL() const = 0;
+  // Adds what the delegate considers to be a blank tab to the model.
+  virtual TabContents* AddBlankTab(bool foreground) = 0;
+  virtual TabContents* AddBlankTabAt(int index, bool foreground) = 0;
 
   // Ask for a new TabStripModel to be created and the given tab contents to
   // be added to it. Its size and position are reflected in |window_bounds|.
@@ -360,10 +361,6 @@ class TabStripModel : public NotificationObserver {
 
   // Command level API /////////////////////////////////////////////////////////
 
-  // Adds a blank tab to the TabStripModel.
-  TabContents* AddBlankTab(bool foreground);
-  TabContents* AddBlankTabAt(int index, bool foreground);
-
   // Adds a TabContents at the best position in the TabStripModel given the
   // specified insertion index, transition, etc. Ultimately, the insertion
   // index of the TabContents is left up to the Order Controller associated
diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc
index 9c5e646..2ff6db6 100644
--- a/chrome/browser/tabs/tab_strip_model_unittest.cc
+++ b/chrome/browser/tabs/tab_strip_model_unittest.cc
@@ -26,8 +26,9 @@ class TabStripDummyDelegate : public TabStripModelDelegate {
   virtual ~TabStripDummyDelegate() {}
 
   // Overridden from TabStripModelDelegate:
-  virtual GURL GetBlankTabURL() const {
-    return GURL(chrome::kChromeUINewTabURL);
+  virtual TabContents* AddBlankTab(bool foreground) { return NULL; }
+  virtual TabContents* AddBlankTabAt(int index, bool foreground) {
+    return NULL;
   }
   virtual Browser* CreateNewStripWithContents(TabContents* contents,
                                               const gfx::Rect& window_bounds,
@@ -987,14 +988,6 @@ TEST_F(TabStripModelTest, AppendContentsReselectionTest) {
   tabstrip.CloseTabContentsAt(2);
   EXPECT_EQ(0, tabstrip.selected_index());
 
-  // Now open a blank tab...
-  // (see also AddTabContents_NewTabAtEndOfStripInheritsGroup for an
-  // explanation of this behavior)
-  tabstrip.AddBlankTab(true);
-  EXPECT_EQ(2, tabstrip.selected_index());
-  tabstrip.CloseTabContentsAt(2);
-  EXPECT_EQ(0, tabstrip.selected_index());
-
   // clean up after ourselves
   tabstrip.CloseAllTabs();
 }
diff --git a/chrome/browser/views/tabs/tab_strip.cc b/chrome/browser/views/tabs/tab_strip.cc
index d377e0b..51baf95 100644
--- a/chrome/browser/views/tabs/tab_strip.cc
+++ b/chrome/browser/views/tabs/tab_strip.cc
@@ -1011,7 +1011,7 @@ bool TabStrip::HasAvailableDragActions() const {
 void TabStrip::ButtonPressed(views::Button* sender) {
   if (sender == newtab_button_) {
     UserMetrics::RecordAction(L"NewTab_Button", model_->profile());
-    model_->AddBlankTab(true);
+    model_->delegate()->AddBlankTab(true);
   }
 }
 
diff --git a/chrome/test/in_process_browser_test.cc b/chrome/test/in_process_browser_test.cc
index a9fa5c4..39114ee 100644
--- a/chrome/test/in_process_browser_test.cc
+++ b/chrome/test/in_process_browser_test.cc
@@ -43,6 +43,15 @@ bool DieFileDie(const std::wstring& file, bool recurse) {
   return false;
 }
 
+class ShadowingAtExitManager : public base::AtExitManager {
+ public:
+  ShadowingAtExitManager() : base::AtExitManager(true) {}
+  virtual ~ShadowingAtExitManager() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ShadowingAtExitManager);
+};
+
 }  // namespace
 
 InProcessBrowserTest::InProcessBrowserTest()
@@ -54,6 +63,8 @@ InProcessBrowserTest::InProcessBrowserTest()
 }
 
 void InProcessBrowserTest::SetUp() {
+  ShadowingAtExitManager at_exit_manager;
+
   // Cleanup the user data dir.
   std::wstring user_data_dir;
   PathService::Get(chrome::DIR_USER_DATA, &user_data_dir);
@@ -163,7 +174,7 @@ Browser* InProcessBrowserTest::CreateBrowser(Profile* profile) {
   Browser* browser = Browser::Create(profile);
 
   browser->AddTabWithURL(
-      GURL("about:blank"), GURL(), PageTransition::START_PAGE, true, NULL);
+      GURL("about:blank"), GURL(), PageTransition::START_PAGE, true, -1, NULL);
 
   // Wait for the page to finish loading.
   ui_test_utils::WaitForNavigation(
@@ -190,8 +201,6 @@ void InProcessBrowserTest::RunTestOnMainThreadLoop() {
     return;
   }
 
-  profile->InitExtensions();
-
   browser_ = CreateBrowser(profile);
 
   registrar_.Add(this,
diff --git a/chrome/test/unit/unittests.vcproj b/chrome/test/unit/unittests.vcproj
index d4e3864..947f797 100644
--- a/chrome/test/unit/unittests.vcproj
+++ b/chrome/test/unit/unittests.vcproj
@@ -428,6 +428,10 @@
 				>
 			</File>
 			<File
+				RelativePath="..\..\browser\browser_unittest.cc"
+				>
+			</File>
+			<File
 				RelativePath="..\..\browser\chrome_thread_unittest.cc"
 				>
 			</File>