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.
Review URL: http://codereview.chromium.org/69043

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

2 files changed, 20 insertions, 28 deletions

diff --git a/chrome/browser/download/download_request_manager.cc b/chrome/browser/download/download_request_manager.cc
index 98efb19..6078fce 100644
--- a/chrome/browser/download/download_request_manager.cc
+++ b/chrome/browser/download/download_request_manager.cc
@@ -177,7 +177,7 @@ DownloadRequestManager::~DownloadRequestManager() {
 
 DownloadRequestManager::DownloadStatus
     DownloadRequestManager::GetDownloadStatus(TabContents* tab) {
-  TabDownloadState* state = GetDownloadState(tab->controller(), NULL, false);
+  TabDownloadState* state = GetDownloadState(&tab->controller(), NULL, false);
   return state ? state->download_status() : ALLOW_ONE_DOWNLOAD;
 }
 
@@ -193,13 +193,7 @@ void DownloadRequestManager::CanDownloadOnIOThread(int render_process_host_id,
 }
 
 void DownloadRequestManager::OnUserGesture(TabContents* tab) {
-  NavigationController* controller = tab->controller();
-  if (!controller) {
-    NOTREACHED();
-    return;
-  }
-
-  TabDownloadState* state = GetDownloadState(controller, NULL, false);
+  TabDownloadState* state = GetDownloadState(&tab->controller(), NULL, false);
   if (!state)
     return;
 
@@ -256,10 +250,8 @@ void DownloadRequestManager::CanDownloadImpl(
         effective_tab->delegate()->GetConstrainingContents(effective_tab);
   }
 
-  NavigationController* controller = effective_tab->controller();
-  DCHECK(controller);
   TabDownloadState* state = GetDownloadState(
-      controller, originating_tab->controller(), true);
+      &effective_tab->controller(), &originating_tab->controller(), true);
   switch (state->download_status()) {
     case ALLOW_ALL_DOWNLOADS:
       ScheduleNotification(callback, true);
diff --git a/chrome/browser/download/download_request_manager_unittest.cc b/chrome/browser/download/download_request_manager_unittest.cc
index 943b439..a8ece7a 100644
--- a/chrome/browser/download/download_request_manager_unittest.cc
+++ b/chrome/browser/download/download_request_manager_unittest.cc
@@ -38,7 +38,7 @@ class DownloadRequestManagerTest
 
   void CanDownload() {
     download_request_manager_->CanDownloadImpl(
-        controller()->tab_contents(), this);
+        controller().tab_contents(), this);
   }
 
   bool ShouldAllowDownload() {
@@ -81,13 +81,13 @@ TEST_F(DownloadRequestManagerTest, Allow) {
   // All tabs should initially start at ALLOW_ONE_DOWNLOAD.
   ASSERT_EQ(DownloadRequestManager::ALLOW_ONE_DOWNLOAD,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
 
   // Ask if the tab can do a download. This moves to PROMPT_BEFORE_DOWNLOAD.
   CanDownload();
   ASSERT_EQ(DownloadRequestManager::PROMPT_BEFORE_DOWNLOAD,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
   // We should have been told we can download.
   ASSERT_EQ(1, continue_count_);
   ASSERT_EQ(0, cancel_count_);
@@ -102,7 +102,7 @@ TEST_F(DownloadRequestManagerTest, Allow) {
   ask_allow_count_ = 0;
   ASSERT_EQ(DownloadRequestManager::ALLOW_ALL_DOWNLOADS,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
   // We should have been told we can download.
   ASSERT_EQ(1, continue_count_);
   ASSERT_EQ(0, cancel_count_);
@@ -114,7 +114,7 @@ TEST_F(DownloadRequestManagerTest, Allow) {
   ASSERT_EQ(0, ask_allow_count_);
   ASSERT_EQ(DownloadRequestManager::ALLOW_ALL_DOWNLOADS,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
   // We should have been told we can download.
   ASSERT_EQ(1, continue_count_);
   ASSERT_EQ(0, cancel_count_);
@@ -131,7 +131,7 @@ TEST_F(DownloadRequestManagerTest, ResetOnNavigation) {
   ask_allow_count_ = continue_count_ = cancel_count_ = 0;
   ASSERT_EQ(DownloadRequestManager::ALLOW_ALL_DOWNLOADS,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
 
   // Navigate to a new URL with the same host, which shouldn't reset the allow
   // all state.
@@ -143,20 +143,20 @@ TEST_F(DownloadRequestManagerTest, ResetOnNavigation) {
   ask_allow_count_ = continue_count_ = cancel_count_ = 0;
   ASSERT_EQ(DownloadRequestManager::ALLOW_ALL_DOWNLOADS,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
 
   // Do a user gesture, because we're at allow all, this shouldn't change the
   // state.
-  download_request_manager_->OnUserGesture(controller()->tab_contents());
+  download_request_manager_->OnUserGesture(controller().tab_contents());
   ASSERT_EQ(DownloadRequestManager::ALLOW_ALL_DOWNLOADS,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
 
   // Navigate to a completely different host, which should reset the state.
   NavigateAndCommit(GURL("http://fooey.com"));
   ASSERT_EQ(DownloadRequestManager::ALLOW_ONE_DOWNLOAD,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
 }
 
 TEST_F(DownloadRequestManagerTest, ResetOnUserGesture) {
@@ -167,13 +167,13 @@ TEST_F(DownloadRequestManagerTest, ResetOnUserGesture) {
   ask_allow_count_ = continue_count_ = cancel_count_ = 0;
   ASSERT_EQ(DownloadRequestManager::PROMPT_BEFORE_DOWNLOAD,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
 
   // Do a user gesture, which should reset back to allow one.
-  download_request_manager_->OnUserGesture(controller()->tab_contents());
+  download_request_manager_->OnUserGesture(controller().tab_contents());
   ASSERT_EQ(DownloadRequestManager::ALLOW_ONE_DOWNLOAD,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
 
   // Ask twice, which triggers calling the delegate. Don't allow the download
   // so that we end up with not allowed.
@@ -182,13 +182,13 @@ TEST_F(DownloadRequestManagerTest, ResetOnUserGesture) {
   CanDownload();
   ASSERT_EQ(DownloadRequestManager::DOWNLOADS_NOT_ALLOWED,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
 
   // A user gesture now should NOT change the state.
-  download_request_manager_->OnUserGesture(controller()->tab_contents());
+  download_request_manager_->OnUserGesture(controller().tab_contents());
   ASSERT_EQ(DownloadRequestManager::DOWNLOADS_NOT_ALLOWED,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
   // And make sure we really can't download.
   ask_allow_count_ = continue_count_ = cancel_count_ = 0;
   CanDownload();
@@ -198,5 +198,5 @@ TEST_F(DownloadRequestManagerTest, ResetOnUserGesture) {
   // And the state shouldn't have changed.
   ASSERT_EQ(DownloadRequestManager::DOWNLOADS_NOT_ALLOWED,
             download_request_manager_->GetDownloadStatus(
-                controller()->tab_contents()));
+                controller().tab_contents()));
 }