Re-land my change to clean up TabContents/WebContents ownership. This

is the same except in tab_strip_model_unittest I fixed a leak by making a
WebContents on the stack, I added a factory to the SiteInstance unittest to
prevent another leak, and I re-added a NULL set to the external_tab_container.

Fix the ownership model of TabContents and NavigationController. Previously the
NavigationController owned the TabContents, and there were extra steps required
at creation and destruction to clean everything up properly.

NavigationController is now a member of TabContents, and there is no setup or
tear down necessary other than the constructor and destructor. I could remove
the tab contents creation in the NavigationController, as well as all the
weird destruction code in WebContents which got moved to the destructor.

I made the controller getter return a reference since the ownership is clear
and there is no possibility of NULL. This required changing a lot of tiles, but
many of them were simplified since they no longer have to NULL check.
Previous review URL: http://codereview.chromium.org/69043

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

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

3 files changed, 29 insertions, 33 deletions

diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index 2fc7e51..0470efa 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -94,7 +94,7 @@ void TabStripModel::InsertTabContentsAt(int index,
       ForgetAllOpeners();
     }
     // Anything opened by a link we deem to have an opener.
-    data->SetGroup(selected_contents->controller());
+    data->SetGroup(&selected_contents->controller());
   }
   contents_data_.insert(contents_data_.begin() + index, data);
 
@@ -198,7 +198,7 @@ int TabStripModel::GetIndexOfController(
   int index = 0;
   TabContentsDataVector::const_iterator iter = contents_data_.begin();
   for (; iter != contents_data_.end(); ++iter, ++index) {
-    if ((*iter)->contents->controller() == controller)
+    if (&(*iter)->contents->controller() == controller)
       return index;
   }
   return kNoTab;
@@ -403,10 +403,8 @@ bool TabStripModel::IsContextMenuCommandEnabled(
     case CommandCloseTabsToRight:
       return context_index < (count() - 1);
     case CommandCloseTabsOpenedBy: {
-      NavigationController* opener =
-          GetTabContentsAt(context_index)->controller();
-      int next_index =
-          GetIndexOfNextTabContentsOpenedBy(opener, context_index, true);
+      int next_index = GetIndexOfNextTabContentsOpenedBy(
+          &GetTabContentsAt(context_index)->controller(), context_index, true);
       return next_index != kNoTab;
     }
     case CommandDuplicate:
@@ -429,7 +427,7 @@ void TabStripModel::ExecuteContextMenuCommand(
       break;
     case CommandReload:
       UserMetrics::RecordAction(L"TabContextMenu_Reload", profile_);
-      GetContentsAt(context_index)->controller()->Reload(true);
+      GetContentsAt(context_index)->controller().Reload(true);
       break;
     case CommandDuplicate:
       UserMetrics::RecordAction(L"TabContextMenu_Duplicate", profile_);
@@ -457,7 +455,7 @@ void TabStripModel::ExecuteContextMenuCommand(
     case CommandCloseTabsOpenedBy: {
       UserMetrics::RecordAction(L"TabContextMenu_CloseTabsOpenedBy", profile_);
       NavigationController* opener =
-          GetTabContentsAt(context_index)->controller();
+          &GetTabContentsAt(context_index)->controller();
 
       for (int i = count() - 1; i >= 0; --i) {
         if (OpenerMatches(contents_data_.at(i), opener, true))
@@ -478,7 +476,7 @@ void TabStripModel::ExecuteContextMenuCommand(
 
 std::vector<int> TabStripModel::GetIndexesOpenedBy(int index) const {
   std::vector<int> indices;
-  NavigationController* opener = GetTabContentsAt(index)->controller();
+  NavigationController* opener = &GetTabContentsAt(index)->controller();
   for (int i = count() - 1; i >= 0; --i) {
     if (OpenerMatches(contents_data_.at(i), opener, true))
       indices.push_back(i);
@@ -511,7 +509,7 @@ bool TabStripModel::IsNewTabAtEndOfTabStrip(TabContents* contents) const {
   return LowerCaseEqualsASCII(contents->GetURL().spec(),
                               chrome::kChromeUINewTabURL) &&
       contents == GetContentsAt(count() - 1) &&
-      contents->controller()->entry_count() == 1;
+      contents->controller().entry_count() == 1;
 }
 
 bool TabStripModel::InternalCloseTabContentsAt(int index,
@@ -534,9 +532,9 @@ bool TabStripModel::InternalCloseTabContentsAt(int index,
     if (create_historical_tab)
       delegate_->CreateHistoricalTab(detached_contents);
 
-    detached_contents->CloseContents();
-    // Closing the TabContents will later call back to us via
-    // NotificationObserver and detach it.
+    // Deleting the TabContents will call back to us via NotificationObserver
+    // and detach it.
+    delete detached_contents;
   }
   return true;
 }
@@ -564,7 +562,7 @@ void TabStripModel::ChangeSelectedContentsFrom(
 void TabStripModel::SetOpenerForContents(TabContents* contents,
                                     TabContents* opener) {
   int index = GetIndexOfTabContents(contents);
-  contents_data_.at(index)->opener = opener->controller();
+  contents_data_.at(index)->opener = &opener->controller();
 }
 
 // static
diff --git a/chrome/browser/tabs/tab_strip_model_order_controller.cc b/chrome/browser/tabs/tab_strip_model_order_controller.cc
index 0c6326a..1db0525 100644
--- a/chrome/browser/tabs/tab_strip_model_order_controller.cc
+++ b/chrome/browser/tabs/tab_strip_model_order_controller.cc
@@ -36,7 +36,7 @@ int TabStripModelOrderController::DetermineInsertionIndex(
       return tabstrip_->selected_index() + 1;
     }
     NavigationController* opener =
-        tabstrip_->GetSelectedTabContents()->controller();
+        &tabstrip_->GetSelectedTabContents()->controller();
     // Get the index of the next item opened by this tab, and insert before
     // it...
     int index = tabstrip_->GetIndexOfLastTabContentsOpenedBy(
@@ -60,7 +60,7 @@ int TabStripModelOrderController::DetermineNewSelectedIndex(
   // want to select the first in that child group, not the next tab in the same
   // group of the removed tab.
   NavigationController* removed_controller =
-      tabstrip_->GetTabContentsAt(removing_index)->controller();
+      &tabstrip_->GetTabContentsAt(removing_index)->controller();
   int index = tabstrip_->GetIndexOfNextTabContentsOpenedBy(removed_controller,
                                                            removing_index,
                                                            false);
@@ -109,8 +109,8 @@ void TabStripModelOrderController::TabSelectedAt(TabContents* old_contents,
   NavigationController* new_opener =
       tabstrip_->GetOpenerOfTabContentsAt(index);
   if (user_gesture && new_opener != old_opener &&
-      new_opener != old_contents->controller() &&
-      old_opener != new_contents->controller()) {
+      new_opener != &old_contents->controller() &&
+      old_opener != &new_contents->controller()) {
     tabstrip_->ForgetAllOpeners();
   }
 }
diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc
index 6b1f3d0..8e052544 100644
--- a/chrome/browser/tabs/tab_strip_model_unittest.cc
+++ b/chrome/browser/tabs/tab_strip_model_unittest.cc
@@ -68,23 +68,21 @@ class TabStripDummyDelegate : public TabStripModelDelegate {
 class TabStripModelTest : public RenderViewHostTestHarness {
  public:
   TabContents* CreateTabContents() {
-    WebContents* con = new WebContents(profile(), NULL, 0, NULL);
-    con->SetupController(profile());
-    return con;
+    return new WebContents(profile(), NULL, 0, NULL);
   }
 
   // Forwards a URL "load" request through to our dummy TabContents
   // implementation.
   void LoadURL(TabContents* con, const std::wstring& url) {
-    controller()->LoadURL(GURL(url), GURL(), PageTransition::LINK);
+    controller().LoadURL(GURL(url), GURL(), PageTransition::LINK);
   }
 
   void GoBack(TabContents* contents) {
-    controller()->GoBack();
+    controller().GoBack();
   }
 
   void GoForward(TabContents* contents) {
-    controller()->GoForward();
+    controller().GoForward();
   }
 
   void SwitchTabTo(TabContents* contents) {
@@ -360,8 +358,8 @@ TEST_F(TabStripModelTest, TestBasicAPI) {
     EXPECT_EQ(contents1, tabstrip.GetTabContentsAt(1));
     EXPECT_EQ(0, tabstrip.GetIndexOfTabContents(contents2));
     EXPECT_EQ(1, tabstrip.GetIndexOfTabContents(contents1));
-    EXPECT_EQ(0, tabstrip.GetIndexOfController(contents2->controller()));
-    EXPECT_EQ(1, tabstrip.GetIndexOfController(contents1->controller()));
+    EXPECT_EQ(0, tabstrip.GetIndexOfController(&contents2->controller()));
+    EXPECT_EQ(1, tabstrip.GetIndexOfController(&contents1->controller()));
   }
 
   // Test UpdateTabContentsStateAt
@@ -414,7 +412,7 @@ TEST_F(TabStripModelTest, TestBasicOpenerAPI) {
   // background with opener_contents set as their opener.
 
   TabContents* opener_contents = CreateTabContents();
-  NavigationController* opener = opener_contents->controller();
+  NavigationController* opener = &opener_contents->controller();
   tabstrip.AppendTabContents(opener_contents, true);
   TabContents* contents1 = CreateTabContents();
   TabContents* contents2 = CreateTabContents();
@@ -447,7 +445,7 @@ TEST_F(TabStripModelTest, TestBasicOpenerAPI) {
 
   // For a tab that has opened no other tabs, the return value should always be
   // -1...
-  NavigationController* o1 = contents1->controller();
+  NavigationController* o1 = &contents1->controller();
   EXPECT_EQ(-1, tabstrip.GetIndexOfNextTabContentsOpenedBy(o1, 3, false));
   EXPECT_EQ(-1, tabstrip.GetIndexOfLastTabContentsOpenedBy(o1, 3));
 
@@ -513,7 +511,7 @@ TEST_F(TabStripModelTest, TestInsertionIndexDetermination) {
   EXPECT_TRUE(tabstrip.empty());
 
   TabContents* opener_contents = CreateTabContents();
-  NavigationController* opener = opener_contents->controller();
+  NavigationController* opener = &opener_contents->controller();
   tabstrip.AppendTabContents(opener_contents, true);
 
   // Open some other random unrelated tab in the background to monkey with our
@@ -592,7 +590,7 @@ TEST_F(TabStripModelTest, TestSelectOnClose) {
   EXPECT_TRUE(tabstrip.empty());
 
   TabContents* opener_contents = CreateTabContents();
-  NavigationController* opener = opener_contents->controller();
+  NavigationController* opener = &opener_contents->controller();
   tabstrip.AppendTabContents(opener_contents, true);
 
   TabContents* contents1 = CreateTabContents();
@@ -670,7 +668,7 @@ TEST_F(TabStripModelTest, TestContextMenuCloseCommands) {
   EXPECT_TRUE(tabstrip.empty());
 
   TabContents* opener_contents = CreateTabContents();
-  NavigationController* opener = opener_contents->controller();
+  NavigationController* opener = &opener_contents->controller();
   tabstrip.AppendTabContents(opener_contents, true);
 
   TabContents* contents1 = CreateTabContents();
@@ -945,8 +943,8 @@ TEST_F(TabStripModelTest, AddTabContents_ForgetOpeners) {
 
 // Added for http://b/issue?id=958960
 TEST_F(TabStripModelTest, AppendContentsReselectionTest) {
-  TabContents* fake_destinations_tab = CreateTabContents();
-  TabStripDummyDelegate delegate(fake_destinations_tab);
+  WebContents fake_destinations_tab(profile(), NULL, 0, NULL);
+  TabStripDummyDelegate delegate(&fake_destinations_tab);
   TabStripModel tabstrip(&delegate, profile());
   EXPECT_TRUE(tabstrip.empty());