diff options
74 files changed, 1342 insertions, 997 deletions
diff --git a/chrome/browser/app_modal_dialog.cc b/chrome/browser/app_modal_dialog.cc index dac60e4..91cffdd 100644 --- a/chrome/browser/app_modal_dialog.cc +++ b/chrome/browser/app_modal_dialog.cc @@ -36,8 +36,7 @@ void AppModalDialog::Observe(NotificationType type, return; if (type == NotificationType::NAV_ENTRY_COMMITTED && - Source<NavigationController>(source).ptr() == - &web_contents_->controller()) + Source<NavigationController>(source).ptr() == web_contents_->controller()) web_contents_ = NULL; if (type == NotificationType::TAB_CONTENTS_DESTROYED && diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc index 8db09bf..bac531e5 100644 --- a/chrome/browser/automation/automation_provider.cc +++ b/chrome/browser/automation/automation_provider.cc @@ -1069,7 +1069,7 @@ void AutomationProvider::AppendTab(int handle, const GURL& url, true, -1, NULL); if (tab_contents) { append_tab_response = - GetIndexForNavigationController(&tab_contents->controller(), browser); + GetIndexForNavigationController(tab_contents->controller(), browser); } } @@ -1730,7 +1730,7 @@ void AutomationProvider::GetTab(int win_handle, int tab_index, if (tab_index < browser->tab_count()) { TabContents* tab_contents = browser->GetTabContentsAt(tab_index); - *tab_handle = tab_tracker_->Add(&tab_contents->controller()); + *tab_handle = tab_tracker_->Add(tab_contents->controller()); } } } @@ -2292,7 +2292,7 @@ void AutomationProvider::CreateExternalTab(HWND parent, external_tab_container->Init(profile_, parent, dimensions, style); TabContents* tab_contents = external_tab_container->tab_contents(); if (tab_contents) { - *tab_handle = tab_tracker_->Add(&tab_contents->controller()); + *tab_handle = tab_tracker_->Add(tab_contents->controller()); *tab_container_window = *external_tab_container; } else { delete external_tab_container; diff --git a/chrome/browser/back_forward_menu_model.cc b/chrome/browser/back_forward_menu_model.cc index 83b8e53..380f70b 100644 --- a/chrome/browser/back_forward_menu_model.cc +++ b/chrome/browser/back_forward_menu_model.cc @@ -21,14 +21,16 @@ const int BackForwardMenuModel::kMaxChapterStops = 5; int BackForwardMenuModel::GetHistoryItemCount() const { TabContents* contents = GetTabContents(); + NavigationController* controller = contents->controller(); + int items = 0; if (model_type_ == FORWARD_MENU_DELEGATE) { // Only count items from n+1 to end (if n is current entry) - items = contents->controller().entry_count() - - contents->controller().GetCurrentEntryIndex() - 1; + items = controller->entry_count() - + controller->GetCurrentEntryIndex() - 1; } else { - items = contents->controller().GetCurrentEntryIndex(); + items = controller->GetCurrentEntryIndex(); } if (items > kMaxHistoryItems) @@ -41,9 +43,10 @@ int BackForwardMenuModel::GetHistoryItemCount() const { int BackForwardMenuModel::GetChapterStopCount(int history_items) const { TabContents* contents = GetTabContents(); + NavigationController* controller = contents->controller(); int chapter_stops = 0; - int current_entry = contents->controller().GetCurrentEntryIndex(); + int current_entry = controller->GetCurrentEntryIndex(); if (history_items == kMaxHistoryItems) { int chapter_id = current_entry; @@ -88,9 +91,9 @@ int BackForwardMenuModel::GetTotalItemCount() const { int BackForwardMenuModel::GetIndexOfNextChapterStop(int start_from, bool forward) const { TabContents* contents = GetTabContents(); - NavigationController& controller = contents->controller(); + NavigationController* controller = contents->controller(); - int max_count = controller.entry_count(); + int max_count = controller->entry_count(); if (start_from < 0 || start_from >= max_count) return -1; // Out of bounds. @@ -104,7 +107,7 @@ int BackForwardMenuModel::GetIndexOfNextChapterStop(int start_from, } } - NavigationEntry* start_entry = controller.GetEntryAtIndex(start_from); + NavigationEntry* start_entry = controller->GetEntryAtIndex(start_from); const GURL& url = start_entry->url(); if (!forward) { @@ -112,7 +115,7 @@ int BackForwardMenuModel::GetIndexOfNextChapterStop(int start_from, // different domain. for (int i = start_from - 1; i >= 0; --i) { if (!net::RegistryControlledDomainService::SameDomainOrHost(url, - controller.GetEntryAtIndex(i)->url())) + controller->GetEntryAtIndex(i)->url())) return i; } // We have reached the beginning without finding a chapter stop. @@ -122,7 +125,7 @@ int BackForwardMenuModel::GetIndexOfNextChapterStop(int start_from, // different domain. for (int i = start_from + 1; i < max_count; ++i) { if (!net::RegistryControlledDomainService::SameDomainOrHost(url, - controller.GetEntryAtIndex(i)->url())) + controller->GetEntryAtIndex(i)->url())) return i - 1; } // Last entry is always considered a chapter stop. @@ -140,7 +143,10 @@ int BackForwardMenuModel::FindChapterStop(int offset, offset *= -1; TabContents* contents = GetTabContents(); - int entry = contents->controller().GetCurrentEntryIndex() + offset; + NavigationController* controller = contents->controller(); + + int entry = controller->GetCurrentEntryIndex() + offset; + for (int i = 0; i < skip + 1; i++) entry = GetIndexOfNextChapterStop(entry, forward); @@ -149,14 +155,14 @@ int BackForwardMenuModel::FindChapterStop(int offset, void BackForwardMenuModel::ExecuteCommandById(int menu_id) { TabContents* contents = GetTabContents(); - NavigationController& controller = contents->controller(); + NavigationController* controller = contents->controller(); DCHECK(!IsSeparator(menu_id)); // Execute the command for the last item: "Show Full History". if (menu_id == GetTotalItemCount()) { UserMetrics::RecordComputedAction(BuildActionName(L"ShowFullHistory", -1), - controller.profile()); + controller->profile()); #if defined(OS_WIN) browser_->ShowSingleDOMUITab(GURL(chrome::kChromeUIHistoryURL)); #else @@ -168,16 +174,16 @@ void BackForwardMenuModel::ExecuteCommandById(int menu_id) { // Log whether it was a history or chapter click. if (menu_id <= GetHistoryItemCount()) { UserMetrics::RecordComputedAction( - BuildActionName(L"HistoryClick", menu_id), controller.profile()); + BuildActionName(L"HistoryClick", menu_id), controller->profile()); } else { UserMetrics::RecordComputedAction( BuildActionName(L"ChapterClick", menu_id - GetHistoryItemCount() - 1), - controller.profile()); + controller->profile()); } int index = MenuIdToNavEntryIndex(menu_id); - if (index >= 0 && index < controller.entry_count()) - controller.GoToIndex(index); + if (index >= 0 && index < controller->entry_count()) + controller->GoToIndex(index); } bool BackForwardMenuModel::IsSeparator(int menu_id) const { @@ -211,7 +217,7 @@ std::wstring BackForwardMenuModel::GetItemLabel(int menu_id) const { NavigationEntry* entry = GetNavigationEntry(menu_id); return UTF16ToWideHack(entry->GetTitleForDisplay( - &GetTabContents()->controller())); + GetTabContents()->controller())); } const SkBitmap& BackForwardMenuModel::GetItemIcon(int menu_id) const { @@ -243,6 +249,8 @@ TabContents* BackForwardMenuModel::GetTabContents() const { int BackForwardMenuModel::MenuIdToNavEntryIndex(int menu_id) const { TabContents* contents = GetTabContents(); + NavigationController* controller = contents->controller(); + int history_items = GetHistoryItemCount(); DCHECK(menu_id > 0); @@ -251,10 +259,10 @@ int BackForwardMenuModel::MenuIdToNavEntryIndex(int menu_id) const { if (menu_id <= history_items) { if (model_type_ == FORWARD_MENU_DELEGATE) { // The |menu_id| is relative to our current position, so we need to add. - menu_id += contents->controller().GetCurrentEntryIndex(); + menu_id += controller->GetCurrentEntryIndex(); } else { // Back menu is reverse. - menu_id = contents->controller().GetCurrentEntryIndex() - menu_id; + menu_id = controller->GetCurrentEntryIndex() - menu_id; } return menu_id; } @@ -273,8 +281,11 @@ int BackForwardMenuModel::MenuIdToNavEntryIndex(int menu_id) const { } NavigationEntry* BackForwardMenuModel::GetNavigationEntry(int menu_id) const { + TabContents* contents = GetTabContents(); + NavigationController* controller = contents->controller(); + int index = MenuIdToNavEntryIndex(menu_id); - return GetTabContents()->controller().GetEntryAtIndex(index); + return controller->GetEntryAtIndex(index); } std::wstring BackForwardMenuModel::BuildActionName( diff --git a/chrome/browser/back_forward_menu_model_unittest.cc b/chrome/browser/back_forward_menu_model_unittest.cc index 708847d..a171302 100644 --- a/chrome/browser/back_forward_menu_model_unittest.cc +++ b/chrome/browser/back_forward_menu_model_unittest.cc @@ -33,33 +33,33 @@ class BackFwdMenuModelTest : public RenderViewHostTestHarness { void LoadURLAndUpdateState(const char* url, const char* title) { NavigateAndCommit(GURL(url)); - controller().GetLastCommittedEntry()->set_title(UTF8ToUTF16(title)); + controller()->GetLastCommittedEntry()->set_title(UTF8ToUTF16(title)); } // Navigate back or forward the given amount and commits the entry (which // will be pending after we ask to navigate there). void NavigateToOffset(int offset) { - controller().GoToOffset(offset); - const NavigationEntry* entry = controller().pending_entry(); + controller()->GoToOffset(offset); + const NavigationEntry* entry = controller()->pending_entry(); rvh()->SendNavigate(entry->page_id(), entry->url()); } // Same as NavigateToOffset but goes to an absolute index. void NavigateToIndex(int index) { - controller().GoToIndex(index); - const NavigationEntry* entry = controller().pending_entry(); + controller()->GoToIndex(index); + const NavigationEntry* entry = controller()->pending_entry(); rvh()->SendNavigate(entry->page_id(), entry->url()); } // Goes back/forward and commits the load. void GoBack() { - controller().GoBack(); - const NavigationEntry* entry = controller().pending_entry(); + controller()->GoBack(); + const NavigationEntry* entry = controller()->pending_entry(); rvh()->SendNavigate(entry->page_id(), entry->url()); } void GoForward() { - controller().GoForward(); - const NavigationEntry* entry = controller().pending_entry(); + controller()->GoForward(); + const NavigationEntry* entry = controller()->pending_entry(); rvh()->SendNavigate(entry->page_id(), entry->url()); } }; diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc index a8501d9..0a2ebad 100644 --- a/chrome/browser/browser.cc +++ b/chrome/browser/browser.cc @@ -536,7 +536,6 @@ TabContents* Browser::AddTabWithURL( return contents; } -// TODO(brettw) this should be just AddTab and it should take a TabContents. TabContents* Browser::AddTabWithNavigationController( NavigationController* ctrl, PageTransition::Type type) { TabContents* tc = ctrl->tab_contents(); @@ -544,34 +543,35 @@ TabContents* Browser::AddTabWithNavigationController( return tc; } -TabContents* Browser::AddRestoredTab( +NavigationController* Browser::AddRestoredTab( const std::vector<TabNavigation>& navigations, int tab_index, int selected_navigation, bool select) { - TabContents* new_tab = new WebContents(profile(), NULL, - MSG_ROUTING_NONE, NULL); - new_tab->controller().RestoreFromState(navigations, selected_navigation); + NavigationController* restored_controller = + BuildRestoredNavigationController(navigations, selected_navigation); - tabstrip_model_.InsertTabContentsAt(tab_index, new_tab, select, false); + tabstrip_model_.InsertTabContentsAt( + tab_index, + restored_controller->tab_contents(), + select, false); if (profile_->HasSessionService()) { SessionService* session_service = profile_->GetSessionService(); if (session_service) - session_service->TabRestored(&new_tab->controller()); + session_service->TabRestored(restored_controller); } - return new_tab; + return restored_controller; } void Browser::ReplaceRestoredTab( const std::vector<TabNavigation>& navigations, int selected_navigation) { - TabContents* replacement = new WebContents(profile(), NULL, - MSG_ROUTING_NONE, NULL); - replacement->controller().RestoreFromState(navigations, selected_navigation); + NavigationController* restored_controller = + BuildRestoredNavigationController(navigations, selected_navigation); tabstrip_model_.ReplaceNavigationControllerAt( tabstrip_model_.selected_index(), - &replacement->controller()); + restored_controller); } bool Browser::CanRestoreTab() { @@ -611,34 +611,33 @@ void Browser::GoBack(WindowOpenDisposition disposition) { return; } - if (current_tab->controller().CanGoBack()) { - NavigationController* controller = NULL; - if (disposition == NEW_FOREGROUND_TAB || - disposition == NEW_BACKGROUND_TAB) { + if (current_tab->controller()->CanGoBack()) { + NavigationController* controller = 0; + if (disposition == NEW_FOREGROUND_TAB || disposition == NEW_BACKGROUND_TAB){ + controller = GetSelectedTabContents()->controller()->Clone(); tabstrip_model_.AddTabContents( - GetSelectedTabContents()->Clone(), -1, + controller->tab_contents(), -1, PageTransition::LINK, disposition == NEW_FOREGROUND_TAB); } else { // Default disposition is CURRENT_TAB. - controller = ¤t_tab->controller(); + controller = current_tab->controller(); } controller->GoBack(); } } void Browser::GoForward(WindowOpenDisposition disp) { - // TODO(brettw) this is mostly duplicated from GoBack, these should have a - // common backend or something. UserMetrics::RecordAction(L"Forward", profile_); - if (GetSelectedTabContents()->controller().CanGoForward()) { + if (GetSelectedTabContents()->controller()->CanGoForward()) { NavigationController* controller = 0; if (disp == NEW_FOREGROUND_TAB || disp == NEW_BACKGROUND_TAB) { + controller = GetSelectedTabContents()->controller()->Clone(); tabstrip_model_.AddTabContents( - GetSelectedTabContents()->Clone(), -1, + controller->tab_contents(), -1, PageTransition::LINK, disp == NEW_FOREGROUND_TAB); } else { // Default disposition is CURRENT_TAB. - controller = &GetSelectedTabContents()->controller(); + controller = GetSelectedTabContents()->controller(); } controller->GoForward(); } @@ -652,7 +651,7 @@ void Browser::Reload() { if (current_tab) { WebContents* web_contents = current_tab->AsWebContents(); if (web_contents && web_contents->showing_interstitial_page()) { - NavigationEntry* entry = current_tab->controller().GetActiveEntry(); + NavigationEntry* entry = current_tab->controller()->GetActiveEntry(); DCHECK(entry); // Should exist if interstitial is showing. OpenURL(entry->url(), GURL(), CURRENT_TAB, PageTransition::RELOAD); return; @@ -662,7 +661,7 @@ void Browser::Reload() { if (current_tab) { // As this is caused by a user action, give the focus to the page. current_tab->Focus(); - current_tab->controller().Reload(true); + current_tab->controller()->Reload(true); } } @@ -798,7 +797,7 @@ void Browser::BookmarkCurrentPage() { if (!model || !model->IsLoaded()) return; // Ignore requests until bookmarks are loaded. - NavigationEntry* entry = contents->controller().GetActiveEntry(); + NavigationEntry* entry = contents->controller()->GetActiveEntry(); if (!entry) return; // Can't star if there is no URL. const GURL& url = entry->display_url(); @@ -823,7 +822,7 @@ void Browser::ViewSource() { UserMetrics::RecordAction(L"ViewSource", profile_); TabContents* current_tab = GetSelectedTabContents(); - NavigationEntry* entry = current_tab->controller().GetLastCommittedEntry(); + NavigationEntry* entry = current_tab->controller()->GetLastCommittedEntry(); if (entry) { GURL url("view-source:" + entry->url().spec()); OpenURL(url, GURL(), NEW_FOREGROUND_TAB, PageTransition::LINK); @@ -1391,19 +1390,23 @@ TabContents* Browser::CreateTabContentsForURL( SiteInstance* instance) const { WebContents* contents = new WebContents(profile, instance, MSG_ROUTING_NONE, NULL); + contents->SetupController(profile); if (!defer_load) { // Load the initial URL before adding the new tab contents to the tab strip // so that the tab contents has navigation state. - contents->controller().LoadURL(url, referrer, transition); + contents->controller()->LoadURL(url, referrer, transition); } return contents; } bool Browser::CanDuplicateContentsAt(int index) { - NavigationController& nc = GetTabContentsAt(index)->controller(); - return nc.tab_contents() && nc.GetLastCommittedEntry(); + TabContents* contents = GetTabContentsAt(index); + DCHECK(contents); + + NavigationController* nc = contents->controller(); + return nc ? (nc->tab_contents() && nc->GetLastCommittedEntry()) : false; } void Browser::DuplicateContentsAt(int index) { @@ -1414,7 +1417,7 @@ void Browser::DuplicateContentsAt(int index) { if (type_ == TYPE_NORMAL) { // If this is a tabbed browser, just create a duplicate tab inside the same // window next to the tab being duplicated. - new_contents = contents->Clone(); + new_contents = contents->controller()->Clone()->tab_contents(); // If you duplicate a tab that is not selected, we need to make sure to // select the tab being duplicated so that DetermineInsertionIndex returns // the right index (if tab 5 is selected and we right-click tab 1 we want @@ -1443,14 +1446,14 @@ void Browser::DuplicateContentsAt(int index) { // The page transition below is only for the purpose of inserting the tab. new_contents = browser->AddTabWithNavigationController( - &contents->Clone()->controller(), + contents->controller()->Clone(), PageTransition::LINK); } if (profile_->HasSessionService()) { SessionService* session_service = profile_->GetSessionService(); if (session_service) - session_service->TabRestored(&new_contents->controller()); + session_service->TabRestored(new_contents->controller()); } } @@ -1476,7 +1479,7 @@ void Browser::CreateHistoricalTab(TabContents* contents) { // We only create historical tab entries for normal tabbed browser windows. if (type() == TYPE_NORMAL) { profile()->GetTabRestoreService()->CreateHistoricalTab( - &contents->controller()); + contents->controller()); } } @@ -1507,7 +1510,7 @@ void Browser::TabInsertedAt(TabContents* contents, int index, bool foreground) { contents->set_delegate(this); - contents->controller().SetWindowID(session_id()); + contents->controller()->SetWindowID(session_id()); SyncHistoryWithTabs(tabstrip_model_.GetIndexOfTabContents(contents)); @@ -1524,9 +1527,11 @@ void Browser::TabInsertedAt(TabContents* contents, } void Browser::TabClosingAt(TabContents* contents, int index) { + NavigationController* controller = contents->controller(); + DCHECK(controller); NotificationService::current()->Notify( NotificationType::TAB_CLOSING, - Source<NavigationController>(&contents->controller()), + Source<NavigationController>(controller), NotificationService::NoDetails()); // Sever the TabContents' connection back to us. @@ -1699,8 +1704,15 @@ void Browser::OpenURLFromTab(TabContents* source, } else if ((disposition == CURRENT_TAB) && current_tab) { tabstrip_model_.TabNavigating(current_tab, transition); - current_tab->controller().LoadURL(url, referrer, transition); - new_contents = current_tab; + // TODO(beng): remove all this once there are no TabContents types. + // It seems like under some circumstances current_tab can be dust after the + // call to LoadURL (perhaps related to TabContents type switching), so we + // save the NavigationController here. + NavigationController* controller = current_tab->controller(); + controller->LoadURL(url, referrer, transition); + // If the TabContents type has been swapped, we need to point to the current + // active type otherwise there will be weirdness. + new_contents = controller->tab_contents(); if (GetStatusBubble()) GetStatusBubble()->Hide(); @@ -1874,7 +1886,7 @@ void Browser::ConvertContentsToApplication(TabContents* contents) { if (index < 0) return; - const GURL& url = contents->controller().GetActiveEntry()->url(); + const GURL& url = contents->controller()->GetActiveEntry()->url(); std::wstring app_name = ComputeApplicationNameFromURL(url); RegisterAppPrefs(app_name); @@ -1982,7 +1994,7 @@ void Browser::Observe(NotificationType type, // actually be for a different window while we're doing asynchronous // closing of this one. if (GetSelectedTabContents() && - &GetSelectedTabContents()->controller() == + GetSelectedTabContents()->controller() == Source<NavigationController>(source).ptr()) UpdateToolbar(false); break; @@ -2125,9 +2137,9 @@ void Browser::UpdateCommandsForTabState() { return; // Navigation commands - NavigationController& nc = current_tab->controller(); - command_updater_.UpdateCommandEnabled(IDC_BACK, nc.CanGoBack()); - command_updater_.UpdateCommandEnabled(IDC_FORWARD, nc.CanGoForward()); + NavigationController* nc = current_tab->controller(); + command_updater_.UpdateCommandEnabled(IDC_BACK, nc->CanGoBack()); + command_updater_.UpdateCommandEnabled(IDC_FORWARD, nc->CanGoForward()); // Window management commands command_updater_.UpdateCommandEnabled(IDC_DUPLICATE_TAB, @@ -2139,7 +2151,7 @@ void Browser::UpdateCommandsForTabState() { bool is_web_contents = web_contents != NULL; // Current navigation entry, may be NULL. - NavigationEntry* active_entry = current_tab->controller().GetActiveEntry(); + NavigationEntry* active_entry = current_tab->controller()->GetActiveEntry(); // Page-related commands // Only allow bookmarking for web content in normal windows. @@ -2233,7 +2245,7 @@ void Browser::ScheduleUIUpdate(const TabContents* source, // this for any tab so they start & stop quickly, but the source can be // NULL, so we have to check for that. tabstrip_model_.UpdateTabContentsStateAt( - tabstrip_model_.GetIndexOfController(&source->controller()), true); + tabstrip_model_.GetIndexOfController(source->controller()), true); } // If the only updates were synchronously handled above, we're done. @@ -2261,8 +2273,8 @@ void Browser::ProcessPendingUIUpdates() { for (size_t i = 0; i < scheduled_updates_.size(); i++) { bool found = false; for (int tab = 0; tab < tab_count(); tab++) { - if (&GetTabContentsAt(tab)->controller() == - &scheduled_updates_[i].source->controller()) { + if (GetTabContentsAt(tab)->controller() == + scheduled_updates_[i].source->controller()) { found = true; break; } @@ -2319,7 +2331,7 @@ void Browser::ProcessPendingUIUpdates() { if (invalidate_tab) { // INVALIDATE_TITLE or INVALIDATE_FAVICON. tabstrip_model_.UpdateTabContentsStateAt( - tabstrip_model_.GetIndexOfController(&contents->controller()), false); + tabstrip_model_.GetIndexOfController(contents->controller()), false); window_->UpdateTitleBar(); if (contents == GetSelectedTabContents()) { @@ -2370,13 +2382,13 @@ void Browser::SyncHistoryWithTabs(int index) { TabContents* contents = GetTabContentsAt(i); if (contents) { session_service->SetTabIndexInWindow( - session_id(), contents->controller().session_id(), i); + session_id(), contents->controller()->session_id(), i); } } } } -TabContents* Browser::BuildRestoredTab( +NavigationController* Browser::BuildRestoredNavigationController( const std::vector<TabNavigation>& navigations, int selected_navigation) { if (!navigations.empty()) { @@ -2384,14 +2396,13 @@ TabContents* Browser::BuildRestoredTab( selected_navigation < static_cast<int>(navigations.size())); // Create a NavigationController. This constructor creates the appropriate // set of TabContents. - TabContents* new_tab = new WebContents(profile_, NULL, - MSG_ROUTING_NONE, NULL); - new_tab->controller().RestoreFromState(navigations, selected_navigation); - return new_tab; + return new NavigationController(profile_, navigations, selected_navigation); } else { // No navigations. Create a tab with about:blank. - return CreateTabContentsForURL(GURL("about:blank"), GURL(), profile_, - PageTransition::START_PAGE, false, NULL); + TabContents* contents = + CreateTabContentsForURL(GURL("about:blank"), GURL(), profile_, + PageTransition::START_PAGE, false, NULL); + return new NavigationController(contents, profile_); } } diff --git a/chrome/browser/browser.h b/chrome/browser/browser.h index d0f2d6f..af0fb52 100644 --- a/chrome/browser/browser.h +++ b/chrome/browser/browser.h @@ -227,13 +227,15 @@ class Browser : public TabStripModelDelegate, PageTransition::Type type); // Add a tab with its session history restored from the SessionRestore - // system. If select is true, the tab is selected. |tab_index| gives the index - // to insert the tab at. |selected_navigation| is the index of the - // TabNavigation in |navigations| to select. - TabContents* AddRestoredTab(const std::vector<TabNavigation>& navigations, - int tab_index, - int selected_navigation, - bool select); + // system. If select is true, the tab is selected. Returns the created + // NavigationController. |tab_index| gives the index to insert the tab at. + // |selected_navigation| is the index of the TabNavigation in |navigations| + // to select. + NavigationController* AddRestoredTab( + const std::vector<TabNavigation>& navigations, + int tab_index, + int selected_navigation, + bool select); // Replaces the state of the currently selected tab with the session // history restored from the SessionRestore system. @@ -523,10 +525,11 @@ class Browser : public TabStripModelDelegate, void SyncHistoryWithTabs(int index); // Called from AddRestoredTab and ReplaceRestoredTab to build a - // TabContents from an incoming vector of TabNavigations. - // Caller takes ownership of the returned TabContents. - TabContents* BuildRestoredTab(const std::vector<TabNavigation>& navigations, - int selected_navigation); + // NavigationController from an incoming vector of TabNavigations. + // Caller takes ownership of the returned NavigationController. + NavigationController* BuildRestoredNavigationController( + const std::vector<TabNavigation>& navigations, + int selected_navigation); // OnBeforeUnload handling ////////////////////////////////////////////////// @@ -636,7 +639,7 @@ class Browser : public TabStripModelDelegate, // toolbar during window creation (i.e. before any tabs have been added // to the window). TabContents* current_tab = browser_->GetSelectedTabContents(); - return current_tab ? ¤t_tab->controller() : NULL; + return current_tab ? current_tab->controller() : NULL; } private: diff --git a/chrome/browser/browser_commands_unittest.cc b/chrome/browser/browser_commands_unittest.cc index 5cecf0a..f55cb8e 100644 --- a/chrome/browser/browser_commands_unittest.cc +++ b/chrome/browser/browser_commands_unittest.cc @@ -68,13 +68,13 @@ TEST_F(BrowserCommandsTest, DuplicateTab) { ASSERT_EQ(2, browser()->tab_count()); // Verify the stack of urls. - NavigationController& controller = + NavigationController* controller = browser()->GetTabContentsAt(1)->controller(); - ASSERT_EQ(3, controller.entry_count()); - ASSERT_EQ(2, controller.GetCurrentEntryIndex()); - ASSERT_TRUE(url1 == controller.GetEntryAtIndex(0)->url()); - ASSERT_TRUE(url2 == controller.GetEntryAtIndex(1)->url()); - ASSERT_TRUE(url3 == controller.GetEntryAtIndex(2)->url()); + ASSERT_EQ(3, controller->entry_count()); + ASSERT_EQ(2, controller->GetCurrentEntryIndex()); + ASSERT_TRUE(url1 == controller->GetEntryAtIndex(0)->url()); + ASSERT_TRUE(url2 == controller->GetEntryAtIndex(1)->url()); + ASSERT_TRUE(url3 == controller->GetEntryAtIndex(2)->url()); } TEST_F(BrowserCommandsTest, BookmarkCurrentPage) { diff --git a/chrome/browser/debugger/debugger_host_impl.cpp b/chrome/browser/debugger/debugger_host_impl.cpp index 1f98f4b8..e22a391 100644 --- a/chrome/browser/debugger/debugger_host_impl.cpp +++ b/chrome/browser/debugger/debugger_host_impl.cpp @@ -16,7 +16,7 @@ class TabContentsReference : public NotificationObserver { public: TabContentsReference(TabContents *c) : navigation_controller_(NULL) { - navigation_controller_ = &c->controller(); + navigation_controller_ = c->controller(); NotificationService* service = NotificationService::current(); DCHECK(service); diff --git a/chrome/browser/debugger/debugger_node.cc b/chrome/browser/debugger/debugger_node.cc index 0beedce..c22bd7f 100644 --- a/chrome/browser/debugger/debugger_node.cc +++ b/chrome/browser/debugger/debugger_node.cc @@ -291,12 +291,12 @@ v8::Handle<v8::Value> TabListNode::IndexGetter(uint32_t index, ///////////////////////////////////////////// TabNode::TabNode(TabContents *c) { - data_ = &c->controller(); + data_ = c->controller(); NotificationService* service = NotificationService::current(); DCHECK(service); service->AddObserver(this, NotificationType::TAB_CLOSING, - Source<NavigationController>(&c->controller())); + Source<NavigationController>(c->controller())); observing_ = true; } diff --git a/chrome/browser/debugger/debugger_view.cc b/chrome/browser/debugger/debugger_view.cc index 0c9f6f8..3a72171 100644 --- a/chrome/browser/debugger/debugger_view.cc +++ b/chrome/browser/debugger/debugger_view.cc @@ -99,13 +99,14 @@ void DebuggerView::OnInit() { Profile* profile = BrowserList::GetLastActive()->profile(); web_contents_ = new WebContents(profile, NULL, MSG_ROUTING_NONE, NULL); + web_contents_->SetupController(profile); web_contents_->set_delegate(this); web_container_->SetTabContents(web_contents_); web_contents_->render_view_host()->AllowDOMUIBindings(); GURL contents("chrome-ui://inspector/debugger.html"); - web_contents_->controller().LoadURL(contents, GURL(), - PageTransition::START_PAGE); + web_contents_->controller()->LoadURL(contents, GURL(), + PageTransition::START_PAGE); } void DebuggerView::OnShow() { @@ -114,7 +115,7 @@ void DebuggerView::OnShow() { void DebuggerView::OnClose() { web_container_->SetTabContents(NULL); - delete web_contents_; + web_contents_->CloseContents(); } void DebuggerView::OpenURLFromTab(TabContents* source, diff --git a/chrome/browser/debugger/devtools_manager.cc b/chrome/browser/debugger/devtools_manager.cc index 6afa7a3..f048689 100644 --- a/chrome/browser/debugger/devtools_manager.cc +++ b/chrome/browser/debugger/devtools_manager.cc @@ -35,18 +35,22 @@ void DevToolsManager::Observe(NotificationType type, return; } - // Active tab contents disconnecting from its renderer means that the tab - // is closing. - client_host->InspectedTabClosing(); - UnregisterDevToolsClientHost(client_host, &src->controller()); + NavigationController* controller = src->controller(); + bool active = (controller->tab_contents() == src.ptr()); + if (active) { + // Active tab contents disconnecting from its renderer means that the tab + // is closing. + client_host->InspectedTabClosing(); + UnregisterDevToolsClientHost(client_host, controller); + } } } DevToolsClientHost* DevToolsManager::GetDevToolsClientHostFor( const WebContents& web_contents) { - const NavigationController& navigation_controller = web_contents.controller(); + NavigationController* navigation_controller = web_contents.controller(); ClientHostMap::const_iterator it = - navcontroller_to_client_host_.find(&navigation_controller); + navcontroller_to_client_host_.find(navigation_controller); if (it != navcontroller_to_client_host_.end()) { return it->second; } @@ -54,11 +58,11 @@ DevToolsClientHost* DevToolsManager::GetDevToolsClientHostFor( } void DevToolsManager::RegisterDevToolsClientHostFor( - WebContents& web_contents, + const WebContents& web_contents, DevToolsClientHost* client_host) { DCHECK(!GetDevToolsClientHostFor(web_contents)); - NavigationController* navigation_controller = &web_contents.controller(); + NavigationController* navigation_controller = web_contents.controller(); navcontroller_to_client_host_[navigation_controller] = client_host; client_host_to_navcontroller_[client_host] = navigation_controller; client_host->set_close_listener(this); diff --git a/chrome/browser/debugger/devtools_manager.h b/chrome/browser/debugger/devtools_manager.h index 77ffaf5..fe6fc4f 100644 --- a/chrome/browser/debugger/devtools_manager.h +++ b/chrome/browser/debugger/devtools_manager.h @@ -36,7 +36,7 @@ class DevToolsManager : public NotificationObserver, // Registers new DevToolsClientHost for |web_contents|. There must be no // other DevToolsClientHosts registered for the WebContents at the moment. - void RegisterDevToolsClientHostFor(WebContents& web_contents, + void RegisterDevToolsClientHostFor(const WebContents& web_contents, DevToolsClientHost* client_host); void ForwardToDevToolsAgent(const RenderViewHost& client_rvh, diff --git a/chrome/browser/debugger/devtools_view.cc b/chrome/browser/debugger/devtools_view.cc index 179eb7e..483f87f 100644 --- a/chrome/browser/debugger/devtools_view.cc +++ b/chrome/browser/debugger/devtools_view.cc @@ -50,6 +50,7 @@ void DevToolsView::Init() { Profile* profile = BrowserList::GetLastActive()->profile(); web_contents_ = new WebContents(profile, NULL, MSG_ROUTING_NONE, NULL); + web_contents_->SetupController(profile); web_contents_->set_delegate(this); web_container_->SetTabContents(web_contents_); web_contents_->render_view_host()->AllowDOMUIBindings(); @@ -58,8 +59,8 @@ void DevToolsView::Init() { GURL contents(std::string(chrome::kChromeUIDevToolsURL) + "devtools.html"); // this will call CreateRenderView to create renderer process - web_contents_->controller().LoadURL(contents, GURL(), - PageTransition::START_PAGE); + web_contents_->controller()->LoadURL(contents, GURL(), + PageTransition::START_PAGE); } void DevToolsView::OnWindowClosing() { @@ -69,7 +70,7 @@ void DevToolsView::OnWindowClosing() { web_container_->SetTabContents(NULL); // Destroy the tab and navigation controller. - delete web_contents_; + web_contents_->CloseContents(); web_contents_ = NULL; } } diff --git a/chrome/browser/debugger/inspectable_tab_proxy.cc b/chrome/browser/debugger/inspectable_tab_proxy.cc index a329d6a..e141bca 100644 --- a/chrome/browser/debugger/inspectable_tab_proxy.cc +++ b/chrome/browser/debugger/inspectable_tab_proxy.cc @@ -86,9 +86,9 @@ const InspectableTabProxy::ControllersMap& end = BrowserList::end(); it != end; ++it) { TabStripModel* model = (*it)->tabstrip_model(); for (int i = 0, size = model->count(); i < size; ++i) { - NavigationController& controller = + NavigationController* controller = model->GetTabContentsAt(i)->controller(); - controllers_map_[controller.session_id().id()] = &controller; + controllers_map_[controller->session_id().id()] = controller; } } } diff --git a/chrome/browser/dom_ui/dom_ui_unittest.cc b/chrome/browser/dom_ui/dom_ui_unittest.cc index e108c23..51b045a 100644 --- a/chrome/browser/dom_ui/dom_ui_unittest.cc +++ b/chrome/browser/dom_ui/dom_ui_unittest.cc @@ -16,7 +16,7 @@ class DOMUITest : public RenderViewHostTestHarness { // ID that we should use is passed as a parameter. We'll use the next two // values. This must be increasing for the life of the tests. static void DoNavigationTest(WebContents* contents, int page_id) { - NavigationController* controller = &contents->controller(); + NavigationController* controller = contents->controller(); // Start a pending load. GURL new_tab_url(chrome::kChromeUINewTabURL); @@ -88,19 +88,23 @@ TEST_F(DOMUITest, DOMUIToStandard) { // slightly different than the very-first-navigation case since the // SiteInstance will be the same (the original WebContents must still be // alive), which will trigger different behavior in RenderViewHostManager. - TestWebContents contents2(profile_.get(), NULL); + WebContents* contents2 = new TestWebContents(profile_.get(), NULL); + NavigationController* controller2 = + new NavigationController(contents2, profile_.get()); + contents2->set_controller(controller2); - DoNavigationTest(&contents2, 101); + DoNavigationTest(contents2, 101); + contents2->CloseContents(); } TEST_F(DOMUITest, DOMUIToDOMUI) { // Do a load (this state is tested above). GURL new_tab_url(chrome::kChromeUINewTabURL); - controller().LoadURL(new_tab_url, GURL(), PageTransition::LINK); + controller()->LoadURL(new_tab_url, GURL(), PageTransition::LINK); rvh()->SendNavigate(1, new_tab_url); // Start another pending load of the new tab page. - controller().LoadURL(new_tab_url, GURL(), PageTransition::LINK); + controller()->LoadURL(new_tab_url, GURL(), PageTransition::LINK); rvh()->SendNavigate(2, new_tab_url); // The flags should be the same as the non-pending state. @@ -113,7 +117,7 @@ TEST_F(DOMUITest, DOMUIToDOMUI) { TEST_F(DOMUITest, StandardToDOMUI) { // Start a pending navigation to a regular page. GURL std_url("http://google.com/"); - controller().LoadURL(std_url, GURL(), PageTransition::LINK); + controller()->LoadURL(std_url, GURL(), PageTransition::LINK); // The state should now reflect the default. EXPECT_TRUE(contents()->ShouldDisplayURL()); @@ -130,7 +134,7 @@ TEST_F(DOMUITest, StandardToDOMUI) { // Start a pending load for a DOMUI. GURL new_tab_url(chrome::kChromeUINewTabURL); - controller().LoadURL(new_tab_url, GURL(), PageTransition::LINK); + controller()->LoadURL(new_tab_url, GURL(), PageTransition::LINK); EXPECT_FALSE(contents()->ShouldDisplayURL()); EXPECT_TRUE(contents()->ShouldDisplayFavIcon()); EXPECT_FALSE(contents()->IsBookmarkBarAlwaysVisible()); diff --git a/chrome/browser/dom_ui/new_tab_ui.cc b/chrome/browser/dom_ui/new_tab_ui.cc index 90bf3dc..cd9b5d9 100644 --- a/chrome/browser/dom_ui/new_tab_ui.cc +++ b/chrome/browser/dom_ui/new_tab_ui.cc @@ -890,8 +890,9 @@ RecentlyClosedTabsHandler::~RecentlyClosedTabsHandler() { } void RecentlyClosedTabsHandler::HandleReopenTab(const Value* content) { + NavigationController* controller = dom_ui_->web_contents()->controller(); Browser* browser = Browser::GetBrowserForController( - &dom_ui_->web_contents()->controller(), NULL); + controller, NULL); if (!browser) return; @@ -1052,7 +1053,9 @@ void HistoryHandler::HandleSearchHistoryPage(const Value* content) { #if defined(OS_WIN) // TODO(port): include this once history is converted to HTML - dom_ui_->web_contents()->controller().LoadURL( + NavigationController* controller = + dom_ui_->web_contents()->controller(); + controller->LoadURL( HistoryUI::GetHistoryURLWithSearchText(wstring_value), GURL(), PageTransition::LINK); diff --git a/chrome/browser/download/download_request_manager.cc b/chrome/browser/download/download_request_manager.cc index 6078fce..98efb19 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,7 +193,13 @@ void DownloadRequestManager::CanDownloadOnIOThread(int render_process_host_id, } void DownloadRequestManager::OnUserGesture(TabContents* tab) { - TabDownloadState* state = GetDownloadState(&tab->controller(), NULL, false); + NavigationController* controller = tab->controller(); + if (!controller) { + NOTREACHED(); + return; + } + + TabDownloadState* state = GetDownloadState(controller, NULL, false); if (!state) return; @@ -250,8 +256,10 @@ void DownloadRequestManager::CanDownloadImpl( effective_tab->delegate()->GetConstrainingContents(effective_tab); } + NavigationController* controller = effective_tab->controller(); + DCHECK(controller); TabDownloadState* state = GetDownloadState( - &effective_tab->controller(), &originating_tab->controller(), true); + 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 a8ece7a..943b439 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())); } diff --git a/chrome/browser/extensions/extension_tabs_module.cc b/chrome/browser/extensions/extension_tabs_module.cc index 06bbcb5..27cf982 100644 --- a/chrome/browser/extensions/extension_tabs_module.cc +++ b/chrome/browser/extensions/extension_tabs_module.cc @@ -24,11 +24,11 @@ int ExtensionTabUtil::GetWindowId(const Browser* browser) { } int ExtensionTabUtil::GetTabId(const TabContents* tab_contents) { - return tab_contents->controller().session_id().id(); + return tab_contents->controller()->session_id().id(); } int ExtensionTabUtil::GetWindowIdOfTab(const TabContents* tab_contents) { - return tab_contents->controller().window_id().id(); + return tab_contents->controller()->window_id().id(); } bool GetWindowsFunction::RunImpl() { @@ -150,6 +150,9 @@ bool GetTabFunction::RunImpl() { if (!GetIndexOfTabId(tab_strip, tab_id, &tab_index)) return false; + TabContents* tab_contents = tab_strip->GetTabContentsAt(tab_index); + NavigationController* controller = tab_contents->controller(); + DCHECK(controller); result_.reset(CreateTabValue(tab_strip, tab_index)); return true; } @@ -175,7 +178,8 @@ bool UpdateTabFunction::RunImpl() { return false; TabContents* tab_contents = tab_strip->GetTabContentsAt(tab_index); - NavigationController& controller = tab_contents->controller(); + NavigationController* controller = tab_contents->controller(); + DCHECK(controller); // TODO(rafaelw): handle setting remaining tab properties: // -title @@ -186,7 +190,7 @@ bool UpdateTabFunction::RunImpl() { if (args->GetString(L"url", &url)) { GURL new_gurl(url); if (new_gurl.is_valid()) { - controller.LoadURL(new_gurl, GURL(), PageTransition::TYPED); + controller->LoadURL(new_gurl, GURL(), PageTransition::TYPED); } else { // TODO(rafaelw): return some reasonable error? } @@ -223,6 +227,10 @@ bool MoveTabFunction::RunImpl() { if (!GetIndexOfTabId(tab_strip, tab_id, &tab_index)) return false; + TabContents* tab_contents = tab_strip->GetTabContentsAt(tab_index); + NavigationController* controller = tab_contents->controller(); + DCHECK(controller); + // TODO(rafaelw): support moving tabs between windows // -windowId @@ -265,7 +273,10 @@ bool RemoveTabFunction::RunImpl() { int tab_index; TabStripModel* tab_strip = browser->tabstrip_model(); if (GetIndexOfTabId(tab_strip, tab_id, &tab_index)) { - browser->CloseContents(tab_strip->GetTabContentsAt(tab_index)); + TabContents* tab_contents = tab_strip->GetTabContentsAt(tab_index); + NavigationController* controller = tab_contents->controller(); + DCHECK(controller); + browser->CloseContents(tab_contents); return true; } @@ -303,6 +314,8 @@ static ListValue* CreateTabList(Browser* browser) { static DictionaryValue* CreateTabValue(TabStripModel* tab_strip, int tab_index) { TabContents* contents = tab_strip->GetTabContentsAt(tab_index); + NavigationController* controller = contents->controller(); + DCHECK(controller); // TODO(aa): Is this a valid assumption? DictionaryValue* result = new DictionaryValue(); result->SetInteger(L"id", ExtensionTabUtil::GetTabId(contents)); @@ -312,7 +325,7 @@ static DictionaryValue* CreateTabValue(TabStripModel* tab_strip, result->SetString(L"title", UTF16ToWide(contents->GetTitle())); result->SetBoolean(L"selected", tab_index == tab_strip->selected_index()); - NavigationEntry* entry = contents->controller().GetActiveEntry(); + NavigationEntry* entry = controller->GetActiveEntry(); if (entry) { if (entry->favicon().is_valid()) result->SetString(L"favIconUrl", entry->favicon().url().spec()); @@ -325,7 +338,10 @@ static bool GetIndexOfTabId(const TabStripModel* tab_strip, int tab_id, int* tab_index) { for (int i = 0; i < tab_strip->count(); ++i) { TabContents* tab_contents = tab_strip->GetTabContentsAt(i); - if (tab_contents->controller().session_id().id() == tab_id) { + NavigationController* controller = tab_contents->controller(); + DCHECK(controller); // TODO(aa): Is this a valid assumption? + + if (controller->session_id().id() == tab_id) { *tab_index = i; return true; } diff --git a/chrome/browser/external_tab_container.cc b/chrome/browser/external_tab_container.cc index 230c670..d9d375c 100644 --- a/chrome/browser/external_tab_container.cc +++ b/chrome/browser/external_tab_container.cc @@ -68,6 +68,7 @@ bool ExternalTabContainer::Init(Profile* profile, HWND parent, focus_manager->AddKeystrokeListener(this); tab_contents_ = new WebContents(profile, NULL, MSG_ROUTING_NONE, NULL); + tab_contents_->SetupController(profile); tab_contents_->set_delegate(this); tab_contents_->render_view_host()->AllowExternalHostBindings(); @@ -90,7 +91,8 @@ bool ExternalTabContainer::Init(Profile* profile, HWND parent, DCHECK(dummy->IsFocusable()); root_view_.AddChildView(dummy); - NavigationController* controller = &tab_contents_->controller(); + NavigationController* controller = tab_contents_->controller(); + DCHECK(controller); registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED, Source<NavigationController>(controller)); registrar_.Add(this, NotificationType::FAIL_PROVISIONAL_LOAD_WITH_ERROR, @@ -129,12 +131,18 @@ bool ExternalTabContainer::Uninitialize(HWND window) { root_view_.RemoveAllChildViews(true); if (tab_contents_) { + NavigationController* controller = tab_contents_->controller(); + DCHECK(controller); + NotificationService::current()->Notify( NotificationType::EXTERNAL_TAB_CLOSED, - Source<NavigationController>(&tab_contents_->controller()), + Source<NavigationController>(controller), Details<ExternalTabContainer>(this)); - delete tab_contents_; + tab_contents_->set_delegate(NULL); + tab_contents_->CloseContents(); + // WARNING: tab_contents_ has likely been deleted. + tab_contents_ = NULL; } return true; @@ -301,7 +309,7 @@ void ExternalTabContainer::Observe(NotificationType type, automation_->Send(new AutomationMsg_DidNavigate( 0, commit->type, commit->previous_entry_index - - tab_contents_->controller().last_committed_entry_index(), + tab_contents_->controller()->last_committed_entry_index(), commit->entry->url())); } break; diff --git a/chrome/browser/fav_icon_helper.cc b/chrome/browser/fav_icon_helper.cc index a3c3fcc..3b23e3b 100644 --- a/chrome/browser/fav_icon_helper.cc +++ b/chrome/browser/fav_icon_helper.cc @@ -129,7 +129,7 @@ void FavIconHelper::UpdateFavIcon(NavigationEntry* entry, } NavigationEntry* FavIconHelper::GetEntry() { - NavigationEntry* entry = web_contents_->controller().GetActiveEntry(); + NavigationEntry* entry = web_contents_->controller()->GetActiveEntry(); if (entry && entry->url() == url_ && web_contents_->IsActiveEntry(entry->page_id())) { return entry; diff --git a/chrome/browser/find_backend_unittest.cc b/chrome/browser/find_backend_unittest.cc index 260e897..2a2dba4 100644 --- a/chrome/browser/find_backend_unittest.cc +++ b/chrome/browser/find_backend_unittest.cc @@ -15,13 +15,16 @@ TEST_F(FindBackendTest, InternalState) { EXPECT_EQ(string16(), contents()->find_text()); // Get another WebContents object ready. - TestWebContents contents2(profile_.get(), NULL); + WebContents* contents2 = new TestWebContents(profile_.get(), NULL); + NavigationController* controller2 = + new NavigationController(contents2, profile_.get()); + contents2->set_controller(controller2); // No search has still been issued, strings should be blank. EXPECT_EQ(string16(), contents()->find_prepopulate_text()); EXPECT_EQ(string16(), contents()->find_text()); - EXPECT_EQ(string16(), contents2.find_prepopulate_text()); - EXPECT_EQ(string16(), contents2.find_text()); + EXPECT_EQ(string16(), contents2->find_prepopulate_text()); + EXPECT_EQ(string16(), contents2->find_text()); string16 search_term1 = L" I had a 401K "; string16 search_term2 = L" but the economy "; @@ -34,18 +37,18 @@ TEST_F(FindBackendTest, InternalState) { // should not. EXPECT_EQ(search_term1, contents()->find_prepopulate_text()); EXPECT_EQ(search_term1, contents()->find_text()); - EXPECT_EQ(search_term1, contents2.find_prepopulate_text()); - EXPECT_EQ(string16(), contents2.find_text()); + EXPECT_EQ(search_term1, contents2->find_prepopulate_text()); + EXPECT_EQ(string16(), contents2->find_text()); // Now search in the other WebContents. - contents2.StartFinding(search_term2, true); // true=forward. + contents2->StartFinding(search_term2, true); // true=forward. // Again, pre-populate string should always match between the two, but // find_text should not. EXPECT_EQ(search_term2, contents()->find_prepopulate_text()); EXPECT_EQ(search_term1, contents()->find_text()); - EXPECT_EQ(search_term2, contents2.find_prepopulate_text()); - EXPECT_EQ(search_term2, contents2.find_text()); + EXPECT_EQ(search_term2, contents2->find_prepopulate_text()); + EXPECT_EQ(search_term2, contents2->find_text()); // Search again in the first WebContents. contents()->StartFinding(search_term3, true); // true=forward. @@ -54,6 +57,8 @@ TEST_F(FindBackendTest, InternalState) { // find_text should not. EXPECT_EQ(search_term3, contents()->find_prepopulate_text()); EXPECT_EQ(search_term3, contents()->find_text()); - EXPECT_EQ(search_term3, contents2.find_prepopulate_text()); - EXPECT_EQ(search_term2, contents2.find_text()); + EXPECT_EQ(search_term3, contents2->find_prepopulate_text()); + EXPECT_EQ(search_term2, contents2->find_text()); + + contents2->CloseContents(); } diff --git a/chrome/browser/find_bar_controller.cc b/chrome/browser/find_bar_controller.cc index fb5cf3b..bdcb94b 100644 --- a/chrome/browser/find_bar_controller.cc +++ b/chrome/browser/find_bar_controller.cc @@ -55,7 +55,7 @@ void FindBarController::ChangeWebContents(WebContents* contents) { Source<TabContents>(web_contents_)); NotificationService::current()->RemoveObserver( this, NotificationType::NAV_ENTRY_COMMITTED, - Source<NavigationController>(&web_contents_->controller())); + Source<NavigationController>(web_contents_->controller())); find_bar_->StopAnimation(); } @@ -74,7 +74,7 @@ void FindBarController::ChangeWebContents(WebContents* contents) { Source<TabContents>(web_contents_)); NotificationService::current()->AddObserver( this, NotificationType::NAV_ENTRY_COMMITTED, - Source<NavigationController>(&web_contents_->controller())); + Source<NavigationController>(web_contents_->controller())); // Find out what we should show in the find text box. Usually, this will be // the last search in this tab, but if no search has been issued in this tab @@ -125,7 +125,7 @@ void FindBarController::Observe(NotificationType type, } else if (type == NotificationType::NAV_ENTRY_COMMITTED) { NavigationController* source_controller = Source<NavigationController>(source).ptr(); - if (source_controller == &web_contents_->controller()) { + if (source_controller == web_contents_->controller()) { NavigationController::LoadCommittedDetails* commit_details = Details<NavigationController::LoadCommittedDetails>(details).ptr(); PageTransition::Type transition_type = diff --git a/chrome/browser/gtk/tab_contents_container_gtk.cc b/chrome/browser/gtk/tab_contents_container_gtk.cc index 086d5c0..9e73389 100644 --- a/chrome/browser/gtk/tab_contents_container_gtk.cc +++ b/chrome/browser/gtk/tab_contents_container_gtk.cc @@ -79,7 +79,7 @@ void TabContentsContainerGtk::AddObservers() { // we crash after that NOTIMPLEMENTED(), we'll need it. NotificationService::current()->AddObserver( this, NotificationType::RENDER_VIEW_HOST_CHANGED, - Source<NavigationController>(&tab_contents_->controller())); + Source<NavigationController>(tab_contents_->controller())); } NotificationService::current()->AddObserver( this, @@ -93,7 +93,7 @@ void TabContentsContainerGtk::RemoveObservers() { NotificationService::current()->RemoveObserver( this, NotificationType::RENDER_VIEW_HOST_CHANGED, - Source<NavigationController>(&tab_contents_->controller())); + Source<NavigationController>(tab_contents_->controller())); } NotificationService::current()->RemoveObserver( this, diff --git a/chrome/browser/login_prompt.cc b/chrome/browser/login_prompt.cc index c7b30c9..ae2cc4d 100644 --- a/chrome/browser/login_prompt.cc +++ b/chrome/browser/login_prompt.cc @@ -266,7 +266,7 @@ class LoginHandlerImpl : public LoginHandler, if (!requesting_contents) return; - NavigationController* controller = &requesting_contents->controller(); + NavigationController* controller = requesting_contents->controller(); if (!WasAuthHandled(false)) { LoginNotificationDetails details(this); diff --git a/chrome/browser/memory_details.cc b/chrome/browser/memory_details.cc index a6dc4dc..1e7b29a 100644 --- a/chrome/browser/memory_details.cc +++ b/chrome/browser/memory_details.cc @@ -231,9 +231,9 @@ void MemoryDetails::CollectChildInfoOnUIThread() { // // Either the pending or last committed entries can be NULL. const NavigationEntry* pending_entry = - contents->controller().pending_entry(); + contents->controller()->pending_entry(); const NavigationEntry* last_committed_entry = - contents->controller().GetLastCommittedEntry(); + contents->controller()->GetLastCommittedEntry(); if ((last_committed_entry && LowerCaseEqualsASCII(last_committed_entry->display_url().spec(), chrome::kAboutMemoryURL)) || diff --git a/chrome/browser/navigation_controller_unittest.cc b/chrome/browser/navigation_controller_unittest.cc index 419ad2f..06ecf00 100644 --- a/chrome/browser/navigation_controller_unittest.cc +++ b/chrome/browser/navigation_controller_unittest.cc @@ -63,8 +63,8 @@ class NavigationControllerHistoryTest : public NavigationControllerTest { service->SetWindowType(window_id, Browser::TYPE_NORMAL); service->SetWindowBounds(window_id, gfx::Rect(0, 1, 2, 3), false); service->SetTabIndexInWindow(window_id, - controller().session_id(), 0); - controller().SetWindowID(window_id); + controller()->session_id(), 0); + controller()->SetWindowID(window_id); session_helper_.set_service(service); } @@ -103,8 +103,8 @@ class NavigationControllerHistoryTest : public NavigationControllerTest { } void GetLastSession() { - profile()->GetSessionService()->TabClosed(controller().window_id(), - controller().session_id()); + profile()->GetSessionService()->TabClosed(controller()->window_id(), + controller()->session_id()); ReopenDatabase(); Time close_time; @@ -146,35 +146,36 @@ void RegisterForAllNavNotifications(TestNotificationTracker* tracker, // ----------------------------------------------------------------------------- TEST_F(NavigationControllerTest, Defaults) { - EXPECT_FALSE(controller().pending_entry()); - EXPECT_FALSE(controller().GetLastCommittedEntry()); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_EQ(controller().last_committed_entry_index(), -1); - EXPECT_EQ(controller().entry_count(), 0); - EXPECT_FALSE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_TRUE(controller()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_FALSE(controller()->GetLastCommittedEntry()); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_EQ(controller()->last_committed_entry_index(), -1); + EXPECT_EQ(controller()->entry_count(), 0); + EXPECT_FALSE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); } TEST_F(NavigationControllerTest, LoadURL) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url1("http://foo1"); const GURL url2("http://foo2"); - controller().LoadURL(url1, GURL(), PageTransition::TYPED); + controller()->LoadURL(url1, GURL(), PageTransition::TYPED); // Creating a pending notification should not have issued any of the // notifications we're listening for. EXPECT_EQ(0U, notifications.size()); // The load should now be pending. - EXPECT_EQ(controller().entry_count(), 0); - EXPECT_EQ(controller().last_committed_entry_index(), -1); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_FALSE(controller().GetLastCommittedEntry()); - EXPECT_TRUE(controller().pending_entry()); - EXPECT_FALSE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 0); + EXPECT_EQ(controller()->last_committed_entry_index(), -1); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_FALSE(controller()->GetLastCommittedEntry()); + EXPECT_TRUE(controller()->pending_entry()); + EXPECT_FALSE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); EXPECT_EQ(contents()->GetMaxPageID(), -1); // We should have gotten no notifications from the preceeding checks. @@ -185,27 +186,27 @@ TEST_F(NavigationControllerTest, LoadURL) { NotificationType::NAV_ENTRY_COMMITTED)); // The load should now be committed. - EXPECT_EQ(controller().entry_count(), 1); - EXPECT_EQ(controller().last_committed_entry_index(), 0); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_FALSE(controller().pending_entry()); - EXPECT_FALSE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 1); + EXPECT_EQ(controller()->last_committed_entry_index(), 0); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_FALSE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); EXPECT_EQ(contents()->GetMaxPageID(), 0); // Load another... - controller().LoadURL(url2, GURL(), PageTransition::TYPED); + controller()->LoadURL(url2, GURL(), PageTransition::TYPED); // The load should now be pending. - EXPECT_EQ(controller().entry_count(), 1); - EXPECT_EQ(controller().last_committed_entry_index(), 0); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_TRUE(controller().pending_entry()); + EXPECT_EQ(controller()->entry_count(), 1); + EXPECT_EQ(controller()->last_committed_entry_index(), 0); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_TRUE(controller()->pending_entry()); // TODO(darin): maybe this should really be true? - EXPECT_FALSE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_FALSE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); EXPECT_EQ(contents()->GetMaxPageID(), 0); rvh()->SendNavigate(1, url2); @@ -213,13 +214,13 @@ TEST_F(NavigationControllerTest, LoadURL) { NotificationType::NAV_ENTRY_COMMITTED)); // The load should now be committed. - EXPECT_EQ(controller().entry_count(), 2); - EXPECT_EQ(controller().last_committed_entry_index(), 1); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_FALSE(controller().pending_entry()); - EXPECT_TRUE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 2); + EXPECT_EQ(controller()->last_committed_entry_index(), 1); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_TRUE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); EXPECT_EQ(contents()->GetMaxPageID(), 1); } @@ -229,69 +230,69 @@ TEST_F(NavigationControllerTest, LoadURL) { // the load commits (because WebCore didn't actually make a new entry). TEST_F(NavigationControllerTest, LoadURL_SamePage) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url1("http://foo1"); - controller().LoadURL(url1, GURL(), PageTransition::TYPED); + controller()->LoadURL(url1, GURL(), PageTransition::TYPED); EXPECT_EQ(0U, notifications.size()); rvh()->SendNavigate(0, url1); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - controller().LoadURL(url1, GURL(), PageTransition::TYPED); + controller()->LoadURL(url1, GURL(), PageTransition::TYPED); EXPECT_EQ(0U, notifications.size()); rvh()->SendNavigate(0, url1); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); // We should not have produced a new session history entry. - EXPECT_EQ(controller().entry_count(), 1); - EXPECT_EQ(controller().last_committed_entry_index(), 0); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_FALSE(controller().pending_entry()); - EXPECT_FALSE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 1); + EXPECT_EQ(controller()->last_committed_entry_index(), 0); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_FALSE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); } // Tests loading a URL but discarding it before the load commits. TEST_F(NavigationControllerTest, LoadURL_Discarded) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url1("http://foo1"); const GURL url2("http://foo2"); - controller().LoadURL(url1, GURL(), PageTransition::TYPED); + controller()->LoadURL(url1, GURL(), PageTransition::TYPED); EXPECT_EQ(0U, notifications.size()); rvh()->SendNavigate(0, url1); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - controller().LoadURL(url2, GURL(), PageTransition::TYPED); - controller().DiscardNonCommittedEntries(); + controller()->LoadURL(url2, GURL(), PageTransition::TYPED); + controller()->DiscardNonCommittedEntries(); EXPECT_EQ(0U, notifications.size()); // Should not have produced a new session history entry. - EXPECT_EQ(controller().entry_count(), 1); - EXPECT_EQ(controller().last_committed_entry_index(), 0); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_FALSE(controller().pending_entry()); - EXPECT_FALSE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 1); + EXPECT_EQ(controller()->last_committed_entry_index(), 0); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_FALSE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); } // Tests navigations that come in unrequested. This happens when the user // navigates from the web page, and here we test that there is no pending entry. TEST_F(NavigationControllerTest, LoadURL_NoPending) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); // First make an existing committed entry. const GURL kExistingURL1("http://eh"); - controller().LoadURL(kExistingURL1, GURL(), + controller()->LoadURL(kExistingURL1, GURL(), PageTransition::TYPED); rvh()->SendNavigate(0, kExistingURL1); EXPECT_TRUE(notifications.Check1AndReset( @@ -305,9 +306,9 @@ TEST_F(NavigationControllerTest, LoadURL_NoPending) { // just made should be committed. EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - EXPECT_EQ(-1, controller().pending_entry_index()); - EXPECT_EQ(1, controller().last_committed_entry_index()); - EXPECT_EQ(kNewURL, controller().GetActiveEntry()->url()); + EXPECT_EQ(-1, controller()->pending_entry_index()); + EXPECT_EQ(1, controller()->last_committed_entry_index()); + EXPECT_EQ(kNewURL, controller()->GetActiveEntry()->url()); } // Tests navigating to a new URL when there is a new pending navigation that is @@ -316,11 +317,11 @@ TEST_F(NavigationControllerTest, LoadURL_NoPending) { // commits. TEST_F(NavigationControllerTest, LoadURL_NewPending) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); // First make an existing committed entry. const GURL kExistingURL1("http://eh"); - controller().LoadURL(kExistingURL1, GURL(), + controller()->LoadURL(kExistingURL1, GURL(), PageTransition::TYPED); rvh()->SendNavigate(0, kExistingURL1); EXPECT_TRUE(notifications.Check1AndReset( @@ -328,7 +329,7 @@ TEST_F(NavigationControllerTest, LoadURL_NewPending) { // Make a pending entry to somewhere new. const GURL kExistingURL2("http://bee"); - controller().LoadURL(kExistingURL2, GURL(), + controller()->LoadURL(kExistingURL2, GURL(), PageTransition::TYPED); EXPECT_EQ(0U, notifications.size()); @@ -340,9 +341,9 @@ TEST_F(NavigationControllerTest, LoadURL_NewPending) { // just made should be committed. EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - EXPECT_EQ(-1, controller().pending_entry_index()); - EXPECT_EQ(1, controller().last_committed_entry_index()); - EXPECT_EQ(kNewURL, controller().GetActiveEntry()->url()); + EXPECT_EQ(-1, controller()->pending_entry_index()); + EXPECT_EQ(1, controller()->last_committed_entry_index()); + EXPECT_EQ(kNewURL, controller()->GetActiveEntry()->url()); } // Tests navigating to a new URL when there is a pending back/forward @@ -350,18 +351,18 @@ TEST_F(NavigationControllerTest, LoadURL_NewPending) { // they navigate somewhere new. TEST_F(NavigationControllerTest, LoadURL_ExistingPending) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); // First make some history. const GURL kExistingURL1("http://eh"); - controller().LoadURL(kExistingURL1, GURL(), + controller()->LoadURL(kExistingURL1, GURL(), PageTransition::TYPED); rvh()->SendNavigate(0, kExistingURL1); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); const GURL kExistingURL2("http://bee"); - controller().LoadURL(kExistingURL2, GURL(), + controller()->LoadURL(kExistingURL2, GURL(), PageTransition::TYPED); rvh()->SendNavigate(1, kExistingURL2); EXPECT_TRUE(notifications.Check1AndReset( @@ -369,10 +370,10 @@ TEST_F(NavigationControllerTest, LoadURL_ExistingPending) { // Now make a pending back/forward navigation. The zeroth entry should be // pending. - controller().GoBack(); + controller()->GoBack(); EXPECT_EQ(0U, notifications.size()); - EXPECT_EQ(0, controller().pending_entry_index()); - EXPECT_EQ(1, controller().last_committed_entry_index()); + EXPECT_EQ(0, controller()->pending_entry_index()); + EXPECT_EQ(1, controller()->last_committed_entry_index()); // Before that commits, do a new navigation. const GURL kNewURL("http://see"); @@ -383,63 +384,63 @@ TEST_F(NavigationControllerTest, LoadURL_ExistingPending) { // just made should be committed. EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - EXPECT_EQ(-1, controller().pending_entry_index()); - EXPECT_EQ(2, controller().last_committed_entry_index()); - EXPECT_EQ(kNewURL, controller().GetActiveEntry()->url()); + EXPECT_EQ(-1, controller()->pending_entry_index()); + EXPECT_EQ(2, controller()->last_committed_entry_index()); + EXPECT_EQ(kNewURL, controller()->GetActiveEntry()->url()); } TEST_F(NavigationControllerTest, Reload) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url1("http://foo1"); - controller().LoadURL(url1, GURL(), PageTransition::TYPED); + controller()->LoadURL(url1, GURL(), PageTransition::TYPED); EXPECT_EQ(0U, notifications.size()); rvh()->SendNavigate(0, url1); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - controller().Reload(true); + controller()->Reload(true); EXPECT_EQ(0U, notifications.size()); // The reload is pending. - EXPECT_EQ(controller().entry_count(), 1); - EXPECT_EQ(controller().last_committed_entry_index(), 0); - EXPECT_EQ(controller().pending_entry_index(), 0); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_TRUE(controller().pending_entry()); - EXPECT_FALSE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 1); + EXPECT_EQ(controller()->last_committed_entry_index(), 0); + EXPECT_EQ(controller()->pending_entry_index(), 0); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_TRUE(controller()->pending_entry()); + EXPECT_FALSE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); rvh()->SendNavigate(0, url1); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); // Now the reload is committed. - EXPECT_EQ(controller().entry_count(), 1); - EXPECT_EQ(controller().last_committed_entry_index(), 0); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_FALSE(controller().pending_entry()); - EXPECT_FALSE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 1); + EXPECT_EQ(controller()->last_committed_entry_index(), 0); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_FALSE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); } // Tests what happens when a reload navigation produces a new page. TEST_F(NavigationControllerTest, Reload_GeneratesNewPage) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url1("http://foo1"); const GURL url2("http://foo2"); - controller().LoadURL(url1, GURL(), PageTransition::TYPED); + controller()->LoadURL(url1, GURL(), PageTransition::TYPED); rvh()->SendNavigate(0, url1); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - controller().Reload(true); + controller()->Reload(true); EXPECT_EQ(0U, notifications.size()); rvh()->SendNavigate(1, url2); @@ -447,19 +448,19 @@ TEST_F(NavigationControllerTest, Reload_GeneratesNewPage) { NotificationType::NAV_ENTRY_COMMITTED)); // Now the reload is committed. - EXPECT_EQ(controller().entry_count(), 2); - EXPECT_EQ(controller().last_committed_entry_index(), 1); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_FALSE(controller().pending_entry()); - EXPECT_TRUE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 2); + EXPECT_EQ(controller()->last_committed_entry_index(), 1); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_TRUE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); } // Tests what happens when we navigate back successfully TEST_F(NavigationControllerTest, Back) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url1("http://foo1"); rvh()->SendNavigate(0, url1); @@ -471,62 +472,62 @@ TEST_F(NavigationControllerTest, Back) { EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - controller().GoBack(); + controller()->GoBack(); EXPECT_EQ(0U, notifications.size()); // We should now have a pending navigation to go back. - EXPECT_EQ(controller().entry_count(), 2); - EXPECT_EQ(controller().last_committed_entry_index(), 1); - EXPECT_EQ(controller().pending_entry_index(), 0); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_TRUE(controller().pending_entry()); - EXPECT_FALSE(controller().CanGoBack()); - EXPECT_TRUE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 2); + EXPECT_EQ(controller()->last_committed_entry_index(), 1); + EXPECT_EQ(controller()->pending_entry_index(), 0); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_TRUE(controller()->pending_entry()); + EXPECT_FALSE(controller()->CanGoBack()); + EXPECT_TRUE(controller()->CanGoForward()); rvh()->SendNavigate(0, url2); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); // The back navigation completed successfully. - EXPECT_EQ(controller().entry_count(), 2); - EXPECT_EQ(controller().last_committed_entry_index(), 0); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_FALSE(controller().pending_entry()); - EXPECT_FALSE(controller().CanGoBack()); - EXPECT_TRUE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 2); + EXPECT_EQ(controller()->last_committed_entry_index(), 0); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_FALSE(controller()->CanGoBack()); + EXPECT_TRUE(controller()->CanGoForward()); } // Tests what happens when a back navigation produces a new page. TEST_F(NavigationControllerTest, Back_GeneratesNewPage) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url1("http://foo1"); const GURL url2("http://foo2"); const GURL url3("http://foo3"); - controller().LoadURL(url1, GURL(), PageTransition::TYPED); + controller()->LoadURL(url1, GURL(), PageTransition::TYPED); rvh()->SendNavigate(0, url1); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - controller().LoadURL(url2, GURL(), PageTransition::TYPED); + controller()->LoadURL(url2, GURL(), PageTransition::TYPED); rvh()->SendNavigate(1, url2); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - controller().GoBack(); + controller()->GoBack(); EXPECT_EQ(0U, notifications.size()); // We should now have a pending navigation to go back. - EXPECT_EQ(controller().entry_count(), 2); - EXPECT_EQ(controller().last_committed_entry_index(), 1); - EXPECT_EQ(controller().pending_entry_index(), 0); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_TRUE(controller().pending_entry()); - EXPECT_FALSE(controller().CanGoBack()); - EXPECT_TRUE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 2); + EXPECT_EQ(controller()->last_committed_entry_index(), 1); + EXPECT_EQ(controller()->pending_entry_index(), 0); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_TRUE(controller()->pending_entry()); + EXPECT_FALSE(controller()->CanGoBack()); + EXPECT_TRUE(controller()->CanGoForward()); rvh()->SendNavigate(2, url3); EXPECT_TRUE(notifications.Check1AndReset( @@ -534,19 +535,19 @@ TEST_F(NavigationControllerTest, Back_GeneratesNewPage) { // The back navigation resulted in a completely new navigation. // TODO(darin): perhaps this behavior will be confusing to users? - EXPECT_EQ(controller().entry_count(), 3); - EXPECT_EQ(controller().last_committed_entry_index(), 2); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_FALSE(controller().pending_entry()); - EXPECT_TRUE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 3); + EXPECT_EQ(controller()->last_committed_entry_index(), 2); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_TRUE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); } // Receives a back message when there is a new pending navigation entry. TEST_F(NavigationControllerTest, Back_NewPending) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL kUrl1("http://foo1"); const GURL kUrl2("http://foo2"); @@ -557,21 +558,21 @@ TEST_F(NavigationControllerTest, Back_NewPending) { EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - //controller().LoadURL(kUrl2, PageTransition::TYPED); + //controller()->LoadURL(kUrl2, PageTransition::TYPED); rvh()->SendNavigate(1, kUrl2); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); // Now start a new pending navigation and go back before it commits. - controller().LoadURL(kUrl3, GURL(), PageTransition::TYPED); - EXPECT_EQ(-1, controller().pending_entry_index()); - EXPECT_EQ(kUrl3, controller().pending_entry()->url()); - controller().GoBack(); + controller()->LoadURL(kUrl3, GURL(), PageTransition::TYPED); + EXPECT_EQ(-1, controller()->pending_entry_index()); + EXPECT_EQ(kUrl3, controller()->pending_entry()->url()); + controller()->GoBack(); // The pending navigation should now be the "back" item and the new one // should be gone. - EXPECT_EQ(0, controller().pending_entry_index()); - EXPECT_EQ(kUrl1, controller().pending_entry()->url()); + EXPECT_EQ(0, controller()->pending_entry_index()); + EXPECT_EQ(kUrl1, controller()->pending_entry()->url()); } // Receives a back message when there is a different renavigation already @@ -592,39 +593,39 @@ TEST_F(NavigationControllerTest, Back_OtherBackPending) { // We know all the entries have the same site instance, so we can just grab // a random one for looking up other entries. SiteInstance* site_instance = - controller().GetLastCommittedEntry()->site_instance(); + controller()->GetLastCommittedEntry()->site_instance(); // That second URL should be the last committed and it should have gotten the // new title. - EXPECT_EQ(kUrl2, controller().GetEntryWithPageID(site_instance, 1)->url()); - EXPECT_EQ(1, controller().last_committed_entry_index()); - EXPECT_EQ(-1, controller().pending_entry_index()); + EXPECT_EQ(kUrl2, controller()->GetEntryWithPageID(site_instance, 1)->url()); + EXPECT_EQ(1, controller()->last_committed_entry_index()); + EXPECT_EQ(-1, controller()->pending_entry_index()); // Now go forward to the last item again and say it was committed. - controller().GoForward(); + controller()->GoForward(); rvh()->SendNavigate(2, kUrl3); // Now start going back one to the second page. It will be pending. - controller().GoBack(); - EXPECT_EQ(1, controller().pending_entry_index()); - EXPECT_EQ(2, controller().last_committed_entry_index()); + controller()->GoBack(); + EXPECT_EQ(1, controller()->pending_entry_index()); + EXPECT_EQ(2, controller()->last_committed_entry_index()); // Not synthesize a totally new back event to the first page. This will not // match the pending one. rvh()->SendNavigate(0, kUrl1); // The navigation should not have affected the pending entry. - EXPECT_EQ(1, controller().pending_entry_index()); + EXPECT_EQ(1, controller()->pending_entry_index()); // But the navigated entry should be the last committed. - EXPECT_EQ(0, controller().last_committed_entry_index()); - EXPECT_EQ(kUrl1, controller().GetLastCommittedEntry()->url()); + EXPECT_EQ(0, controller()->last_committed_entry_index()); + EXPECT_EQ(kUrl1, controller()->GetLastCommittedEntry()->url()); } // Tests what happens when we navigate forward successfully. TEST_F(NavigationControllerTest, Forward) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url1("http://foo1"); const GURL url2("http://foo2"); @@ -637,40 +638,40 @@ TEST_F(NavigationControllerTest, Forward) { EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - controller().GoBack(); + controller()->GoBack(); rvh()->SendNavigate(0, url1); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - controller().GoForward(); + controller()->GoForward(); // We should now have a pending navigation to go forward. - EXPECT_EQ(controller().entry_count(), 2); - EXPECT_EQ(controller().last_committed_entry_index(), 0); - EXPECT_EQ(controller().pending_entry_index(), 1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_TRUE(controller().pending_entry()); - EXPECT_TRUE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 2); + EXPECT_EQ(controller()->last_committed_entry_index(), 0); + EXPECT_EQ(controller()->pending_entry_index(), 1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_TRUE(controller()->pending_entry()); + EXPECT_TRUE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); rvh()->SendNavigate(1, url2); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); // The forward navigation completed successfully. - EXPECT_EQ(controller().entry_count(), 2); - EXPECT_EQ(controller().last_committed_entry_index(), 1); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_FALSE(controller().pending_entry()); - EXPECT_TRUE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 2); + EXPECT_EQ(controller()->last_committed_entry_index(), 1); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_TRUE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); } // Tests what happens when a forward navigation produces a new page. TEST_F(NavigationControllerTest, Forward_GeneratesNewPage) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url1("http://foo1"); const GURL url2("http://foo2"); @@ -683,42 +684,42 @@ TEST_F(NavigationControllerTest, Forward_GeneratesNewPage) { EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - controller().GoBack(); + controller()->GoBack(); rvh()->SendNavigate(0, url1); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - controller().GoForward(); + controller()->GoForward(); EXPECT_EQ(0U, notifications.size()); // Should now have a pending navigation to go forward. - EXPECT_EQ(controller().entry_count(), 2); - EXPECT_EQ(controller().last_committed_entry_index(), 0); - EXPECT_EQ(controller().pending_entry_index(), 1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_TRUE(controller().pending_entry()); - EXPECT_TRUE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 2); + EXPECT_EQ(controller()->last_committed_entry_index(), 0); + EXPECT_EQ(controller()->pending_entry_index(), 1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_TRUE(controller()->pending_entry()); + EXPECT_TRUE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); rvh()->SendNavigate(2, url3); EXPECT_TRUE(notifications.Check2AndReset( NotificationType::NAV_LIST_PRUNED, NotificationType::NAV_ENTRY_COMMITTED)); - EXPECT_EQ(controller().entry_count(), 2); - EXPECT_EQ(controller().last_committed_entry_index(), 1); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_FALSE(controller().pending_entry()); - EXPECT_TRUE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 2); + EXPECT_EQ(controller()->last_committed_entry_index(), 1); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_TRUE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); } // Tests navigation via link click within a subframe. A new navigation entry // should be created. TEST_F(NavigationControllerTest, NewSubframe) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url1("http://foo1"); rvh()->SendNavigate(0, url1); @@ -735,7 +736,7 @@ TEST_F(NavigationControllerTest, NewSubframe) { params.is_post = false; NavigationController::LoadCommittedDetails details; - EXPECT_TRUE(controller().RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller()->RendererDidNavigate(params, &details)); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); EXPECT_EQ(url1, details.previous_url); @@ -744,7 +745,7 @@ TEST_F(NavigationControllerTest, NewSubframe) { EXPECT_FALSE(details.is_main_frame); // The new entry should be appended. - EXPECT_EQ(2, controller().entry_count()); + EXPECT_EQ(2, controller()->entry_count()); // New entry should refer to the new page, but the old URL (entries only // reflect the toplevel URL). @@ -757,7 +758,7 @@ TEST_F(NavigationControllerTest, NewSubframe) { // just get thrown on the ground, but we shouldn't crash. TEST_F(NavigationControllerTest, SubframeOnEmptyPage) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); // Navigation controller currently has no entries. const GURL url("http://foo2"); @@ -770,7 +771,7 @@ TEST_F(NavigationControllerTest, SubframeOnEmptyPage) { params.is_post = false; NavigationController::LoadCommittedDetails details; - EXPECT_FALSE(controller().RendererDidNavigate(params, &details)); + EXPECT_FALSE(controller()->RendererDidNavigate(params, &details)); EXPECT_EQ(0U, notifications.size()); } @@ -778,7 +779,7 @@ TEST_F(NavigationControllerTest, SubframeOnEmptyPage) { // not create new navigation entries. TEST_F(NavigationControllerTest, AutoSubframe) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url1("http://foo1"); rvh()->SendNavigate(0, url1); @@ -796,17 +797,17 @@ TEST_F(NavigationControllerTest, AutoSubframe) { // Navigating should do nothing. NavigationController::LoadCommittedDetails details; - EXPECT_FALSE(controller().RendererDidNavigate(params, &details)); + EXPECT_FALSE(controller()->RendererDidNavigate(params, &details)); EXPECT_EQ(0U, notifications.size()); // There should still be only one entry. - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); } // Tests navigation and then going back to a subframe navigation. TEST_F(NavigationControllerTest, BackSubframe) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); // Main page. const GURL url1("http://foo1"); @@ -826,45 +827,45 @@ TEST_F(NavigationControllerTest, BackSubframe) { // This should generate a new entry. NavigationController::LoadCommittedDetails details; - EXPECT_TRUE(controller().RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller()->RendererDidNavigate(params, &details)); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - EXPECT_EQ(2, controller().entry_count()); + EXPECT_EQ(2, controller()->entry_count()); // Second manual subframe navigation should also make a new entry. const GURL url3("http://foo3"); params.page_id = 2; params.url = url3; - EXPECT_TRUE(controller().RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller()->RendererDidNavigate(params, &details)); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - EXPECT_EQ(3, controller().entry_count()); - EXPECT_EQ(2, controller().GetCurrentEntryIndex()); + EXPECT_EQ(3, controller()->entry_count()); + EXPECT_EQ(2, controller()->GetCurrentEntryIndex()); // Go back one. - controller().GoBack(); + controller()->GoBack(); params.url = url2; params.page_id = 1; - EXPECT_TRUE(controller().RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller()->RendererDidNavigate(params, &details)); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - EXPECT_EQ(3, controller().entry_count()); - EXPECT_EQ(1, controller().GetCurrentEntryIndex()); + EXPECT_EQ(3, controller()->entry_count()); + EXPECT_EQ(1, controller()->GetCurrentEntryIndex()); // Go back one more. - controller().GoBack(); + controller()->GoBack(); params.url = url1; params.page_id = 0; - EXPECT_TRUE(controller().RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller()->RendererDidNavigate(params, &details)); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - EXPECT_EQ(3, controller().entry_count()); - EXPECT_EQ(0, controller().GetCurrentEntryIndex()); + EXPECT_EQ(3, controller()->entry_count()); + EXPECT_EQ(0, controller()->GetCurrentEntryIndex()); } TEST_F(NavigationControllerTest, LinkClick) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url1("http://foo1"); const GURL url2("http://foo2"); @@ -878,18 +879,18 @@ TEST_F(NavigationControllerTest, LinkClick) { NotificationType::NAV_ENTRY_COMMITTED)); // Should not have produced a new session history entry. - EXPECT_EQ(controller().entry_count(), 2); - EXPECT_EQ(controller().last_committed_entry_index(), 1); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_FALSE(controller().pending_entry()); - EXPECT_TRUE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(controller()->entry_count(), 2); + EXPECT_EQ(controller()->last_committed_entry_index(), 1); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_TRUE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); } TEST_F(NavigationControllerTest, InPage) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); // Main page. Note that we need "://" so this URL is treated as "standard" // which are the only ones that can have a ref. @@ -910,50 +911,50 @@ TEST_F(NavigationControllerTest, InPage) { // This should generate a new entry. NavigationController::LoadCommittedDetails details; - EXPECT_TRUE(controller().RendererDidNavigate(params, &details)); + EXPECT_TRUE(controller()->RendererDidNavigate(params, &details)); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - EXPECT_EQ(2, controller().entry_count()); + EXPECT_EQ(2, controller()->entry_count()); // Go back one. ViewHostMsg_FrameNavigate_Params back_params(params); - controller().GoBack(); + controller()->GoBack(); back_params.url = url1; back_params.page_id = 0; - EXPECT_TRUE(controller().RendererDidNavigate(back_params, + EXPECT_TRUE(controller()->RendererDidNavigate(back_params, &details)); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - EXPECT_EQ(2, controller().entry_count()); - EXPECT_EQ(0, controller().GetCurrentEntryIndex()); - EXPECT_EQ(back_params.url, controller().GetActiveEntry()->url()); + EXPECT_EQ(2, controller()->entry_count()); + EXPECT_EQ(0, controller()->GetCurrentEntryIndex()); + EXPECT_EQ(back_params.url, controller()->GetActiveEntry()->url()); // Go forward ViewHostMsg_FrameNavigate_Params forward_params(params); - controller().GoForward(); + controller()->GoForward(); forward_params.url = url2; forward_params.page_id = 1; - EXPECT_TRUE(controller().RendererDidNavigate(forward_params, + EXPECT_TRUE(controller()->RendererDidNavigate(forward_params, &details)); EXPECT_TRUE(notifications.Check1AndReset( NotificationType::NAV_ENTRY_COMMITTED)); - EXPECT_EQ(2, controller().entry_count()); - EXPECT_EQ(1, controller().GetCurrentEntryIndex()); + EXPECT_EQ(2, controller()->entry_count()); + EXPECT_EQ(1, controller()->GetCurrentEntryIndex()); EXPECT_EQ(forward_params.url, - controller().GetActiveEntry()->url()); + controller()->GetActiveEntry()->url()); // Now go back and forward again. This is to work around a bug where we would // compare the incoming URL with the last committed entry rather than the // one identified by an existing page ID. This would result in the second URL // losing the reference fragment when you navigate away from it and then back. - controller().GoBack(); - EXPECT_TRUE(controller().RendererDidNavigate(back_params, + controller()->GoBack(); + EXPECT_TRUE(controller()->RendererDidNavigate(back_params, &details)); - controller().GoForward(); - EXPECT_TRUE(controller().RendererDidNavigate(forward_params, + controller()->GoForward(); + EXPECT_TRUE(controller()->RendererDidNavigate(forward_params, &details)); EXPECT_EQ(forward_params.url, - controller().GetActiveEntry()->url()); + controller()->GetActiveEntry()->url()); } namespace { @@ -1002,18 +1003,18 @@ TEST_F(NavigationControllerTest, EnforceMaxNavigationCount) { // Load up to the max count, all entries should be there. for (url_index = 0; url_index < kMaxEntryCount; url_index++) { GURL url(StringPrintf("http://www.a.com/%d", url_index)); - controller().LoadURL(url, GURL(), PageTransition::TYPED); + controller()->LoadURL(url, GURL(), PageTransition::TYPED); rvh()->SendNavigate(url_index, url); } - EXPECT_EQ(controller().entry_count(), kMaxEntryCount); + EXPECT_EQ(controller()->entry_count(), kMaxEntryCount); // Created a PrunedListener to observe prune notifications. - PrunedListener listener(&controller()); + PrunedListener listener(controller()); // Navigate some more. GURL url(StringPrintf("http://www.a.com/%d", url_index)); - controller().LoadURL(url, GURL(), PageTransition::TYPED); + controller()->LoadURL(url, GURL(), PageTransition::TYPED); rvh()->SendNavigate(url_index, url); url_index++; @@ -1023,19 +1024,19 @@ TEST_F(NavigationControllerTest, EnforceMaxNavigationCount) { EXPECT_EQ(1, listener.details_.count); // We expect http://www.a.com/0 to be gone. - EXPECT_EQ(controller().entry_count(), kMaxEntryCount); - EXPECT_EQ(controller().GetEntryAtIndex(0)->url(), + EXPECT_EQ(controller()->entry_count(), kMaxEntryCount); + EXPECT_EQ(controller()->GetEntryAtIndex(0)->url(), GURL("http:////www.a.com/1")); // More navigations. for (int i = 0; i < 3; i++) { url = GURL(StringPrintf("http:////www.a.com/%d", url_index)); - controller().LoadURL(url, GURL(), PageTransition::TYPED); + controller()->LoadURL(url, GURL(), PageTransition::TYPED); rvh()->SendNavigate(url_index, url); url_index++; } - EXPECT_EQ(controller().entry_count(), kMaxEntryCount); - EXPECT_EQ(controller().GetEntryAtIndex(0)->url(), + EXPECT_EQ(controller()->entry_count(), kMaxEntryCount); + EXPECT_EQ(controller()->GetEntryAtIndex(0)->url(), GURL("http:////www.a.com/4")); NavigationController::set_max_entry_count(original_count); @@ -1051,16 +1052,15 @@ TEST_F(NavigationControllerTest, RestoreNavigate) { navigations.push_back(TabNavigation(0, url, GURL(), ASCIIToUTF16("Title"), "state", PageTransition::LINK)); - WebContents our_contents(profile(), NULL, MSG_ROUTING_NONE, NULL); - NavigationController& our_controller = our_contents.controller(); - our_controller.RestoreFromState(navigations, 0); - our_controller.GoToIndex(0); + NavigationController* our_controller = + new NavigationController(profile(), navigations, 0); + our_controller->GoToIndex(0); // We should now have one entry, and it should be "pending". - EXPECT_EQ(1, our_controller.entry_count()); - EXPECT_EQ(our_controller.GetEntryAtIndex(0), - our_controller.pending_entry()); - EXPECT_EQ(0, our_controller.GetEntryAtIndex(0)->page_id()); + EXPECT_EQ(1, our_controller->entry_count()); + EXPECT_EQ(our_controller->GetEntryAtIndex(0), + our_controller->pending_entry()); + EXPECT_EQ(0, our_controller->GetEntryAtIndex(0)->page_id()); // Say we navigated to that entry. ViewHostMsg_FrameNavigate_Params params; @@ -1071,29 +1071,32 @@ TEST_F(NavigationControllerTest, RestoreNavigate) { params.gesture = NavigationGestureUser; params.is_post = false; NavigationController::LoadCommittedDetails details; - our_controller.RendererDidNavigate(params, &details); + our_controller->RendererDidNavigate(params, &details); // There should be no longer any pending entry and one committed one. This // means that we were able to locate the entry, assign its site instance, and // commit it properly. - EXPECT_EQ(1, our_controller.entry_count()); - EXPECT_EQ(0, our_controller.last_committed_entry_index()); - EXPECT_FALSE(our_controller.pending_entry()); + EXPECT_EQ(1, our_controller->entry_count()); + EXPECT_EQ(0, our_controller->last_committed_entry_index()); + EXPECT_FALSE(our_controller->pending_entry()); EXPECT_EQ(url, - our_controller.GetLastCommittedEntry()->site_instance()->site()); + our_controller->GetLastCommittedEntry()->site_instance()->site()); + + // Clean up the navigation controller. + our_controller->Destroy(); } // Make sure that the page type and stuff is correct after an interstitial. TEST_F(NavigationControllerTest, Interstitial) { // First navigate somewhere normal. const GURL url1("http://foo"); - controller().LoadURL(url1, GURL(), PageTransition::TYPED); + controller()->LoadURL(url1, GURL(), PageTransition::TYPED); rvh()->SendNavigate(0, url1); // Now navigate somewhere with an interstitial. const GURL url2("http://bar"); - controller().LoadURL(url1, GURL(), PageTransition::TYPED); - controller().pending_entry()->set_page_type( + controller()->LoadURL(url1, GURL(), PageTransition::TYPED); + controller()->pending_entry()->set_page_type( NavigationEntry::INTERSTITIAL_PAGE); // At this point the interstitial will be displayed and the load will still @@ -1101,9 +1104,9 @@ TEST_F(NavigationControllerTest, Interstitial) { rvh()->SendNavigate(1, url2); // The page should be a normal page again. - EXPECT_EQ(url2, controller().GetLastCommittedEntry()->url()); + EXPECT_EQ(url2, controller()->GetLastCommittedEntry()->url()); EXPECT_EQ(NavigationEntry::NORMAL_PAGE, - controller().GetLastCommittedEntry()->page_type()); + controller()->GetLastCommittedEntry()->page_type()); } TEST_F(NavigationControllerTest, RemoveEntry) { @@ -1115,60 +1118,60 @@ TEST_F(NavigationControllerTest, RemoveEntry) { const GURL pending_url("http://pending"); const GURL default_url("http://default"); - controller().LoadURL(url1, GURL(), PageTransition::TYPED); + controller()->LoadURL(url1, GURL(), PageTransition::TYPED); rvh()->SendNavigate(0, url1); - controller().LoadURL(url2, GURL(), PageTransition::TYPED); + controller()->LoadURL(url2, GURL(), PageTransition::TYPED); rvh()->SendNavigate(1, url2); - controller().LoadURL(url3, GURL(), PageTransition::TYPED); + controller()->LoadURL(url3, GURL(), PageTransition::TYPED); rvh()->SendNavigate(2, url3); - controller().LoadURL(url4, GURL(), PageTransition::TYPED); + controller()->LoadURL(url4, GURL(), PageTransition::TYPED); rvh()->SendNavigate(3, url4); - controller().LoadURL(url5, GURL(), PageTransition::TYPED); + controller()->LoadURL(url5, GURL(), PageTransition::TYPED); rvh()->SendNavigate(4, url5); // Remove the last entry. - controller().RemoveEntryAtIndex( - controller().entry_count() - 1, default_url); - EXPECT_EQ(4, controller().entry_count()); - EXPECT_EQ(3, controller().last_committed_entry_index()); - NavigationEntry* pending_entry = controller().pending_entry(); + controller()->RemoveEntryAtIndex( + controller()->entry_count() - 1, default_url); + EXPECT_EQ(4, controller()->entry_count()); + EXPECT_EQ(3, controller()->last_committed_entry_index()); + NavigationEntry* pending_entry = controller()->pending_entry(); EXPECT_TRUE(pending_entry && pending_entry->url() == url4); // Add a pending entry. - controller().LoadURL(pending_url, GURL(), PageTransition::TYPED); + controller()->LoadURL(pending_url, GURL(), PageTransition::TYPED); // Now remove the last entry. - controller().RemoveEntryAtIndex( - controller().entry_count() - 1, default_url); + controller()->RemoveEntryAtIndex( + controller()->entry_count() - 1, default_url); // The pending entry should have been discarded and the last committed entry // removed. - EXPECT_EQ(3, controller().entry_count()); - EXPECT_EQ(2, controller().last_committed_entry_index()); - pending_entry = controller().pending_entry(); + EXPECT_EQ(3, controller()->entry_count()); + EXPECT_EQ(2, controller()->last_committed_entry_index()); + pending_entry = controller()->pending_entry(); EXPECT_TRUE(pending_entry && pending_entry->url() == url3); // Remove an entry which is not the last committed one. - controller().RemoveEntryAtIndex(0, default_url); - EXPECT_EQ(2, controller().entry_count()); - EXPECT_EQ(1, controller().last_committed_entry_index()); + controller()->RemoveEntryAtIndex(0, default_url); + EXPECT_EQ(2, controller()->entry_count()); + EXPECT_EQ(1, controller()->last_committed_entry_index()); // No navigation should have been initiated since we did not remove the // current entry. - EXPECT_FALSE(controller().pending_entry()); + EXPECT_FALSE(controller()->pending_entry()); // Remove the 2 remaining entries. - controller().RemoveEntryAtIndex(1, default_url); - controller().RemoveEntryAtIndex(0, default_url); + controller()->RemoveEntryAtIndex(1, default_url); + controller()->RemoveEntryAtIndex(0, default_url); // This should have created a pending default entry. - EXPECT_EQ(0, controller().entry_count()); - EXPECT_EQ(-1, controller().last_committed_entry_index()); - pending_entry = controller().pending_entry(); + EXPECT_EQ(0, controller()->entry_count()); + EXPECT_EQ(-1, controller()->last_committed_entry_index()); + pending_entry = controller()->pending_entry(); EXPECT_TRUE(pending_entry && pending_entry->url() == default_url); } // Tests the transient entry, making sure it goes away with all navigations. TEST_F(NavigationControllerTest, TransientEntry) { TestNotificationTracker notifications; - RegisterForAllNavNotifications(¬ifications, &controller()); + RegisterForAllNavNotifications(¬ifications, controller()); const GURL url0("http://foo0"); const GURL url1("http://foo1"); @@ -1177,9 +1180,9 @@ TEST_F(NavigationControllerTest, TransientEntry) { const GURL url4("http://foo4"); const GURL transient_url("http://transient"); - controller().LoadURL(url0, GURL(), PageTransition::TYPED); + controller()->LoadURL(url0, GURL(), PageTransition::TYPED); rvh()->SendNavigate(0, url0); - controller().LoadURL(url1, GURL(), PageTransition::TYPED); + controller()->LoadURL(url1, GURL(), PageTransition::TYPED); rvh()->SendNavigate(1, url1); notifications.Reset(); @@ -1187,103 +1190,103 @@ TEST_F(NavigationControllerTest, TransientEntry) { // Adding a transient with no pending entry. NavigationEntry* transient_entry = new NavigationEntry; transient_entry->set_url(transient_url); - controller().AddTransientEntry(transient_entry); + controller()->AddTransientEntry(transient_entry); // We should not have received any notifications. EXPECT_EQ(0U, notifications.size()); // Check our state. - EXPECT_EQ(transient_url, controller().GetActiveEntry()->url()); - EXPECT_EQ(controller().entry_count(), 3); - EXPECT_EQ(controller().last_committed_entry_index(), 1); - EXPECT_EQ(controller().pending_entry_index(), -1); - EXPECT_TRUE(controller().GetLastCommittedEntry()); - EXPECT_FALSE(controller().pending_entry()); - EXPECT_TRUE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); + EXPECT_EQ(transient_url, controller()->GetActiveEntry()->url()); + EXPECT_EQ(controller()->entry_count(), 3); + EXPECT_EQ(controller()->last_committed_entry_index(), 1); + EXPECT_EQ(controller()->pending_entry_index(), -1); + EXPECT_TRUE(controller()->GetLastCommittedEntry()); + EXPECT_FALSE(controller()->pending_entry()); + EXPECT_TRUE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); EXPECT_EQ(contents()->GetMaxPageID(), 1); // Navigate. - controller().LoadURL(url2, GURL(), PageTransition::TYPED); + controller()->LoadURL(url2, GURL(), PageTransition::TYPED); rvh()->SendNavigate(2, url2); // We should have navigated, transient entry should be gone. - EXPECT_EQ(url2, controller().GetActiveEntry()->url()); - EXPECT_EQ(controller().entry_count(), 3); + EXPECT_EQ(url2, controller()->GetActiveEntry()->url()); + EXPECT_EQ(controller()->entry_count(), 3); // Add a transient again, then navigate with no pending entry this time. transient_entry = new NavigationEntry; transient_entry->set_url(transient_url); - controller().AddTransientEntry(transient_entry); - EXPECT_EQ(transient_url, controller().GetActiveEntry()->url()); + controller()->AddTransientEntry(transient_entry); + EXPECT_EQ(transient_url, controller()->GetActiveEntry()->url()); rvh()->SendNavigate(3, url3); // Transient entry should be gone. - EXPECT_EQ(url3, controller().GetActiveEntry()->url()); - EXPECT_EQ(controller().entry_count(), 4); + EXPECT_EQ(url3, controller()->GetActiveEntry()->url()); + EXPECT_EQ(controller()->entry_count(), 4); // Initiate a navigation, add a transient then commit navigation. - controller().LoadURL(url4, GURL(), PageTransition::TYPED); + controller()->LoadURL(url4, GURL(), PageTransition::TYPED); transient_entry = new NavigationEntry; transient_entry->set_url(transient_url); - controller().AddTransientEntry(transient_entry); - EXPECT_EQ(transient_url, controller().GetActiveEntry()->url()); + controller()->AddTransientEntry(transient_entry); + EXPECT_EQ(transient_url, controller()->GetActiveEntry()->url()); rvh()->SendNavigate(4, url4); - EXPECT_EQ(url4, controller().GetActiveEntry()->url()); - EXPECT_EQ(controller().entry_count(), 5); + EXPECT_EQ(url4, controller()->GetActiveEntry()->url()); + EXPECT_EQ(controller()->entry_count(), 5); // Add a transient and go back. This should simply remove the transient. transient_entry = new NavigationEntry; transient_entry->set_url(transient_url); - controller().AddTransientEntry(transient_entry); - EXPECT_EQ(transient_url, controller().GetActiveEntry()->url()); - EXPECT_TRUE(controller().CanGoBack()); - EXPECT_FALSE(controller().CanGoForward()); - controller().GoBack(); + controller()->AddTransientEntry(transient_entry); + EXPECT_EQ(transient_url, controller()->GetActiveEntry()->url()); + EXPECT_TRUE(controller()->CanGoBack()); + EXPECT_FALSE(controller()->CanGoForward()); + controller()->GoBack(); // Transient entry should be gone. - EXPECT_EQ(url4, controller().GetActiveEntry()->url()); - EXPECT_EQ(controller().entry_count(), 5); + EXPECT_EQ(url4, controller()->GetActiveEntry()->url()); + EXPECT_EQ(controller()->entry_count(), 5); rvh()->SendNavigate(3, url3); // Add a transient and go to an entry before the current one. transient_entry = new NavigationEntry; transient_entry->set_url(transient_url); - controller().AddTransientEntry(transient_entry); - EXPECT_EQ(transient_url, controller().GetActiveEntry()->url()); - controller().GoToIndex(1); + controller()->AddTransientEntry(transient_entry); + EXPECT_EQ(transient_url, controller()->GetActiveEntry()->url()); + controller()->GoToIndex(1); // The navigation should have been initiated, transient entry should be gone. - EXPECT_EQ(url1, controller().GetActiveEntry()->url()); + EXPECT_EQ(url1, controller()->GetActiveEntry()->url()); rvh()->SendNavigate(1, url1); // Add a transient and go to an entry after the current one. transient_entry = new NavigationEntry; transient_entry->set_url(transient_url); - controller().AddTransientEntry(transient_entry); - EXPECT_EQ(transient_url, controller().GetActiveEntry()->url()); - controller().GoToIndex(3); + controller()->AddTransientEntry(transient_entry); + EXPECT_EQ(transient_url, controller()->GetActiveEntry()->url()); + controller()->GoToIndex(3); // The navigation should have been initiated, transient entry should be gone. // Because of the transient entry that is removed, going to index 3 makes us // land on url2. - EXPECT_EQ(url2, controller().GetActiveEntry()->url()); + EXPECT_EQ(url2, controller()->GetActiveEntry()->url()); rvh()->SendNavigate(2, url2); // Add a transient and go forward. transient_entry = new NavigationEntry; transient_entry->set_url(transient_url); - controller().AddTransientEntry(transient_entry); - EXPECT_EQ(transient_url, controller().GetActiveEntry()->url()); - EXPECT_TRUE(controller().CanGoForward()); - controller().GoForward(); + controller()->AddTransientEntry(transient_entry); + EXPECT_EQ(transient_url, controller()->GetActiveEntry()->url()); + EXPECT_TRUE(controller()->CanGoForward()); + controller()->GoForward(); // We should have navigated, transient entry should be gone. - EXPECT_EQ(url3, controller().GetActiveEntry()->url()); + EXPECT_EQ(url3, controller()->GetActiveEntry()->url()); rvh()->SendNavigate(3, url3); // Ensure the URLS are correct. - EXPECT_EQ(controller().entry_count(), 5); - EXPECT_EQ(controller().GetEntryAtIndex(0)->url(), url0); - EXPECT_EQ(controller().GetEntryAtIndex(1)->url(), url1); - EXPECT_EQ(controller().GetEntryAtIndex(2)->url(), url2); - EXPECT_EQ(controller().GetEntryAtIndex(3)->url(), url3); - EXPECT_EQ(controller().GetEntryAtIndex(4)->url(), url4); + EXPECT_EQ(controller()->entry_count(), 5); + EXPECT_EQ(controller()->GetEntryAtIndex(0)->url(), url0); + EXPECT_EQ(controller()->GetEntryAtIndex(1)->url(), url1); + EXPECT_EQ(controller()->GetEntryAtIndex(2)->url(), url2); + EXPECT_EQ(controller()->GetEntryAtIndex(3)->url(), url3); + EXPECT_EQ(controller()->GetEntryAtIndex(4)->url(), url4); } // Tests that IsInPageNavigation returns appropriate results. Prevents @@ -1294,21 +1297,21 @@ TEST_F(NavigationControllerTest, IsInPageNavigation) { rvh()->SendNavigate(0, url); // Reloading the page is not an in-page navigation. - EXPECT_FALSE(controller().IsURLInPageNavigation(url)); + EXPECT_FALSE(controller()->IsURLInPageNavigation(url)); const GURL other_url("http://www.google.com/add.html"); - EXPECT_FALSE(controller().IsURLInPageNavigation(other_url)); + EXPECT_FALSE(controller()->IsURLInPageNavigation(other_url)); const GURL url_with_ref("http://www.google.com/home.html#my_ref"); - EXPECT_TRUE(controller().IsURLInPageNavigation(url_with_ref)); + EXPECT_TRUE(controller()->IsURLInPageNavigation(url_with_ref)); // Navigate to URL with refs. rvh()->SendNavigate(1, url_with_ref); // Reloading the page is not an in-page navigation. - EXPECT_FALSE(controller().IsURLInPageNavigation(url_with_ref)); - EXPECT_FALSE(controller().IsURLInPageNavigation(url)); - EXPECT_FALSE(controller().IsURLInPageNavigation(other_url)); + EXPECT_FALSE(controller()->IsURLInPageNavigation(url_with_ref)); + EXPECT_FALSE(controller()->IsURLInPageNavigation(url)); + EXPECT_FALSE(controller()->IsURLInPageNavigation(other_url)); const GURL other_url_with_ref("http://www.google.com/home.html#my_other_ref"); - EXPECT_TRUE(controller().IsURLInPageNavigation( + EXPECT_TRUE(controller()->IsURLInPageNavigation( other_url_with_ref)); } @@ -1322,8 +1325,8 @@ TEST_F(NavigationControllerTest, SameSubframe) { rvh()->SendNavigate(0, url); // We should be at the first navigation entry. - EXPECT_EQ(controller().entry_count(), 1); - EXPECT_EQ(controller().last_committed_entry_index(), 0); + EXPECT_EQ(controller()->entry_count(), 1); + EXPECT_EQ(controller()->last_committed_entry_index(), 0); // Navigate a subframe that would normally count as in-page. const GURL subframe("http://www.google.com/#"); @@ -1335,11 +1338,11 @@ TEST_F(NavigationControllerTest, SameSubframe) { params.gesture = NavigationGestureAuto; params.is_post = false; NavigationController::LoadCommittedDetails details; - EXPECT_FALSE(controller().RendererDidNavigate(params, &details)); + EXPECT_FALSE(controller()->RendererDidNavigate(params, &details)); // Nothing should have changed. - EXPECT_EQ(controller().entry_count(), 1); - EXPECT_EQ(controller().last_committed_entry_index(), 0); + EXPECT_EQ(controller()->entry_count(), 1); + EXPECT_EQ(controller()->last_committed_entry_index(), 0); } /* TODO(brettw) These test pass on my local machine but fail on the XP buildbot @@ -1349,7 +1352,7 @@ TEST_F(NavigationControllerTest, SameSubframe) { // A basic test case. Navigates to a single url, and make sure the history // db matches. TEST_F(NavigationControllerHistoryTest, Basic) { - controller().LoadURL(url0, GURL(), PageTransition::LINK); + controller()->LoadURL(url0, GURL(), PageTransition::LINK); rvh()->SendNavigate(0, url0); GetLastSession(); @@ -1369,7 +1372,7 @@ TEST_F(NavigationControllerHistoryTest, NavigationThenBack) { rvh()->SendNavigate(1, url1); rvh()->SendNavigate(2, url2); - controller().GoBack(); + controller()->GoBack(); rvh()->SendNavigate(1, url1); GetLastSession(); @@ -1396,10 +1399,10 @@ TEST_F(NavigationControllerHistoryTest, NavigationPruning) { rvh()->SendNavigate(1, url1); rvh()->SendNavigate(2, url2); - controller().GoBack(); + controller()->GoBack(); rvh()->SendNavigate(1, url1); - controller().GoBack(); + controller()->GoBack(); rvh()->SendNavigate(0, url0); rvh()->SendNavigate(3, url2); diff --git a/chrome/browser/password_manager/password_manager.cc b/chrome/browser/password_manager/password_manager.cc index 2144806..769c08e 100644 --- a/chrome/browser/password_manager/password_manager.cc +++ b/chrome/browser/password_manager/password_manager.cc @@ -98,9 +98,8 @@ PasswordManager::~PasswordManager() { } void PasswordManager::ProvisionallySavePassword(PasswordForm form) { - if (!web_contents_->profile() || - web_contents_->profile()->IsOffTheRecord() || - !*password_manager_enabled_) + if (!web_contents_->controller() || !web_contents_->profile() || + web_contents_->profile()->IsOffTheRecord() || !*password_manager_enabled_) return; // No password to save? Then don't. @@ -135,7 +134,7 @@ void PasswordManager::ProvisionallySavePassword(PasswordForm form) { return; form.ssl_valid = form.origin.SchemeIsSecure() && - !web_contents_->controller().ssl_manager()-> + !web_contents_->controller()->ssl_manager()-> ProcessedSSLErrorFromRequest(); form.preferred = true; manager->ProvisionallySave(form); @@ -167,6 +166,9 @@ void PasswordManager::DidStopLoading() { if (!web_contents_->profile() || !web_contents_->profile()->GetWebDataService(Profile::IMPLICIT_ACCESS)) return; + if (!web_contents_->controller()) + return; + if (provisional_save_manager_->IsNewLogin()) { web_contents_->AddInfoBar( new SavePasswordInfoBarDelegate(web_contents_, @@ -184,11 +186,13 @@ void PasswordManager::PasswordFormsSeen( if (!web_contents_->profile() || !web_contents_->profile()->GetWebDataService(Profile::EXPLICIT_ACCESS)) return; + if (!web_contents_->controller()) + return; if (!*password_manager_enabled_) return; // Ask the SSLManager for current security. - bool had_ssl_error = web_contents_->controller().ssl_manager()-> + bool had_ssl_error = web_contents_->controller()->ssl_manager()-> ProcessedSSLErrorFromRequest(); std::vector<PasswordForm>::const_iterator iter; diff --git a/chrome/browser/printing/print_view_manager.cc b/chrome/browser/printing/print_view_manager.cc index 4bbc4ce..b8d1095 100644 --- a/chrome/browser/printing/print_view_manager.cc +++ b/chrome/browser/printing/print_view_manager.cc @@ -29,6 +29,9 @@ PrintViewManager::PrintViewManager(WebContents& owner) } PrintViewManager::~PrintViewManager() { +} + +void PrintViewManager::Destroy() { DisconnectFromCurrentPrintJob(); } @@ -122,7 +125,7 @@ std::wstring PrintViewManager::RenderSourceName() { } GURL PrintViewManager::RenderSourceUrl() { - NavigationEntry* entry = owner_.controller().GetActiveEntry(); + NavigationEntry* entry = owner_.controller()->GetActiveEntry(); if (entry) return entry->display_url(); else diff --git a/chrome/browser/renderer_host/render_view_host_unittest.cc b/chrome/browser/renderer_host/render_view_host_unittest.cc index b70293a..9e91318 100644 --- a/chrome/browser/renderer_host/render_view_host_unittest.cc +++ b/chrome/browser/renderer_host/render_view_host_unittest.cc @@ -12,6 +12,6 @@ class RenderViewHostTest : public RenderViewHostTestHarness { // See RenderViewHost::OnMsgNavigate for a discussion. TEST_F(RenderViewHostTest, FilterAbout) { rvh()->SendNavigate(1, GURL("about:cache")); - ASSERT_TRUE(controller().GetActiveEntry()); - EXPECT_EQ(GURL("about:blank"), controller().GetActiveEntry()->url()); + ASSERT_TRUE(controller()->GetActiveEntry()); + EXPECT_EQ(GURL("about:blank"), controller()->GetActiveEntry()->url()); } diff --git a/chrome/browser/renderer_host/resource_dispatcher_host.cc b/chrome/browser/renderer_host/resource_dispatcher_host.cc index 5a211d4..db852b4 100644 --- a/chrome/browser/renderer_host/resource_dispatcher_host.cc +++ b/chrome/browser/renderer_host/resource_dispatcher_host.cc @@ -1293,7 +1293,7 @@ class NotificationTask : public Task { // Issue the notification. NotificationService::current()->Notify( type_, - Source<NavigationController>(&tab_contents->controller()), + Source<NavigationController>(tab_contents->controller()), Details<ResourceRequestDetails>(details_.get())); } } diff --git a/chrome/browser/renderer_host/test_render_view_host.cc b/chrome/browser/renderer_host/test_render_view_host.cc index 4255393..abc5699 100644 --- a/chrome/browser/renderer_host/test_render_view_host.cc +++ b/chrome/browser/renderer_host/test_render_view_host.cc @@ -70,7 +70,7 @@ BackingStore* TestRenderWidgetHostView::AllocBackingStore( } void RenderViewHostTestHarness::NavigateAndCommit(const GURL& url) { - controller().LoadURL(url, GURL(), 0); + controller()->LoadURL(url, GURL(), 0); rvh()->SendNavigate(process()->max_page_id() + 1, url); } @@ -87,8 +87,11 @@ void RenderViewHostTestHarness::SetUp() { } void RenderViewHostTestHarness::TearDown() { - if (contents_) - delete contents_; + if (contents_) { + contents_->CloseContents(); + contents_ = NULL; + } + controller_ = NULL; // Make sure that we flush any messages related to WebContents destruction // before we destroy the profile. diff --git a/chrome/browser/renderer_host/test_render_view_host.h b/chrome/browser/renderer_host/test_render_view_host.h index cdcf325..63e6da6 100644 --- a/chrome/browser/renderer_host/test_render_view_host.h +++ b/chrome/browser/renderer_host/test_render_view_host.h @@ -184,7 +184,7 @@ class RenderViewHostTestHarness : public testing::Test { controller_(NULL) {} virtual ~RenderViewHostTestHarness() {} - NavigationController& controller() { + NavigationController* controller() { return contents_->controller(); } diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc index fd07b9d6..0c1e95e 100644 --- a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc +++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc @@ -85,7 +85,7 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage( unsafe_resources_(unsafe_resources) { if (!is_main_frame_) { navigation_entry_index_to_remove_ = - tab()->controller().last_committed_entry_index(); + tab()->controller()->last_committed_entry_index(); } else { navigation_entry_index_to_remove_ = -1; } @@ -402,8 +402,8 @@ void SafeBrowsingBlockingPage::DontProceed() { // would trigger a navigation that would cause trouble as the render view host // for the tab has by then already been destroyed. if (navigation_entry_index_to_remove_ != -1 && !tab()->is_being_destroyed()) { - tab()->controller().RemoveEntryAtIndex(navigation_entry_index_to_remove_, - GURL(chrome::kChromeUINewTabURL)); + tab()->controller()->RemoveEntryAtIndex(navigation_entry_index_to_remove_, + GURL(chrome::kChromeUINewTabURL)); navigation_entry_index_to_remove_ = -1; } InterstitialPage::DontProceed(); diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc index 32f3ab6..e8311db 100644 --- a/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc +++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc @@ -154,7 +154,7 @@ class SafeBrowsingBlockingPageTest : public RenderViewHostTestHarness, // Tests showing a blocking page for a malware page and not proceeding. TEST_F(SafeBrowsingBlockingPageTest, MalwarePageDontProceed) { // Start a load. - controller().LoadURL(GURL(kBadURL), GURL(), PageTransition::TYPED); + controller()->LoadURL(GURL(kBadURL), GURL(), PageTransition::TYPED); // Simulate the load causing a safe browsing interstitial to be shown. ShowInterstitial(ResourceType::MAIN_FRAME, kBadURL); @@ -169,13 +169,13 @@ TEST_F(SafeBrowsingBlockingPageTest, MalwarePageDontProceed) { EXPECT_FALSE(GetSafeBrowsingBlockingPage()); // We did not proceed, the pending entry should be gone. - EXPECT_FALSE(controller().pending_entry()); + EXPECT_FALSE(controller()->pending_entry()); } // Tests showing a blocking page for a malware page and then proceeding. TEST_F(SafeBrowsingBlockingPageTest, MalwarePageProceed) { // Start a load. - controller().LoadURL(GURL(kBadURL), GURL(), PageTransition::TYPED); + controller()->LoadURL(GURL(kBadURL), GURL(), PageTransition::TYPED); // Simulate the load causing a safe browsing interstitial to be shown. ShowInterstitial(ResourceType::MAIN_FRAME, kBadURL); @@ -215,8 +215,8 @@ TEST_F(SafeBrowsingBlockingPageTest, PageWithMalwareResourceDontProceed) { // We did not proceed, we should be back to the first page, the 2nd one should // have been removed from the navigation controller. - ASSERT_EQ(1, controller().entry_count()); - EXPECT_EQ(kGoogleURL, controller().GetActiveEntry()->url().spec()); + ASSERT_EQ(1, controller()->entry_count()); + EXPECT_EQ(kGoogleURL, controller()->GetActiveEntry()->url().spec()); } // Tests showing a blocking page for a page that contains malware subresources @@ -237,8 +237,8 @@ TEST_F(SafeBrowsingBlockingPageTest, PageWithMalwareResourceProceed) { EXPECT_FALSE(GetSafeBrowsingBlockingPage()); // We did proceed, we should be back to showing the page. - ASSERT_EQ(1, controller().entry_count()); - EXPECT_EQ(kGoodURL, controller().GetActiveEntry()->url().spec()); + ASSERT_EQ(1, controller()->entry_count()); + EXPECT_EQ(kGoodURL, controller()->GetActiveEntry()->url().spec()); } // Tests showing a blocking page for a page that contains multiple malware @@ -270,8 +270,8 @@ TEST_F(SafeBrowsingBlockingPageTest, // We did not proceed, we should be back to the first page, the 2nd one should // have been removed from the navigation controller. - ASSERT_EQ(1, controller().entry_count()); - EXPECT_EQ(kGoogleURL, controller().GetActiveEntry()->url().spec()); + ASSERT_EQ(1, controller()->entry_count()); + EXPECT_EQ(kGoogleURL, controller()->GetActiveEntry()->url().spec()); } // Tests showing a blocking page for a page that contains multiple malware @@ -313,8 +313,8 @@ TEST_F(SafeBrowsingBlockingPageTest, // We did not proceed, we should be back to the first page, the 2nd one should // have been removed from the navigation controller. - ASSERT_EQ(1, controller().entry_count()); - EXPECT_EQ(kGoogleURL, controller().GetActiveEntry()->url().spec()); + ASSERT_EQ(1, controller()->entry_count()); + EXPECT_EQ(kGoogleURL, controller()->GetActiveEntry()->url().spec()); } // Tests showing a blocking page for a page that contains multiple malware @@ -350,6 +350,6 @@ TEST_F(SafeBrowsingBlockingPageTest, PageWithMultipleMalwareResourceProceed) { EXPECT_EQ(OK, user_response()); // We did proceed, we should be back to the initial page. - ASSERT_EQ(1, controller().entry_count()); - EXPECT_EQ(kGoodURL, controller().GetActiveEntry()->url().spec()); + ASSERT_EQ(1, controller()->entry_count()); + EXPECT_EQ(kGoodURL, controller()->GetActiveEntry()->url().spec()); } diff --git a/chrome/browser/safe_browsing/safe_browsing_service.cc b/chrome/browser/safe_browsing/safe_browsing_service.cc index 6369b3d..1a1c88a3 100644 --- a/chrome/browser/safe_browsing/safe_browsing_service.cc +++ b/chrome/browser/safe_browsing/safe_browsing_service.cc @@ -285,9 +285,11 @@ void SafeBrowsingService::DoDisplayBlockingPage( resource.threat_type == SafeBrowsingService::URL_MALWARE) { GURL page_url = wc->GetURL(); GURL referrer_url; - NavigationEntry* entry = wc->controller().GetActiveEntry(); - if (entry) - referrer_url = entry->referrer(); + if (wc->controller()) { + NavigationEntry* entry = wc->controller()->GetActiveEntry(); + if (entry) + referrer_url = entry->referrer(); + } io_loop_->PostTask(FROM_HERE, NewRunnableMethod(this, &SafeBrowsingService::ReportMalware, diff --git a/chrome/browser/sessions/session_restore.cc b/chrome/browser/sessions/session_restore.cc index f05a444..47fbad9 100644 --- a/chrome/browser/sessions/session_restore.cc +++ b/chrome/browser/sessions/session_restore.cc @@ -342,10 +342,10 @@ class SessionRestoreImpl : public NotificationObserver { std::min(selected_index, static_cast<int>(tab.navigations.size() - 1))); tab_loader_->AddTab( - &browser->AddRestoredTab(tab.navigations, - static_cast<int>(i - window.tabs.begin()), - selected_index, - false)->controller()); + browser->AddRestoredTab(tab.navigations, + static_cast<int>(i - window.tabs.begin()), + selected_index, + false)); } } @@ -383,7 +383,7 @@ class SessionRestoreImpl : public NotificationObserver { void NotifySessionServiceOfRestoredTabs(Browser* browser, int initial_count) { SessionService* session_service = profile_->GetSessionService(); for (int i = initial_count; i < browser->tab_count(); ++i) - session_service->TabRestored(&browser->GetTabContentsAt(i)->controller()); + session_service->TabRestored(browser->GetTabContentsAt(i)->controller()); } // The profile to create the sessions for. diff --git a/chrome/browser/sessions/session_service.cc b/chrome/browser/sessions/session_service.cc index c0b8158..ba473c9 100644 --- a/chrome/browser/sessions/session_service.cc +++ b/chrome/browser/sessions/session_service.cc @@ -938,7 +938,7 @@ void SessionService::BuildCommandsForBrowser( TabContents* tab = browser->GetTabContentsAt(i); DCHECK(tab); if (tab->profile() == profile()) { - BuildCommandsForTab(browser->session_id(), &tab->controller(), i, + BuildCommandsForTab(browser->session_id(), tab->controller(), i, commands, tab_to_available_range); if (windows_to_track && !added_to_windows_to_track) { windows_to_track->insert(browser->session_id().id()); diff --git a/chrome/browser/sessions/tab_restore_service.cc b/chrome/browser/sessions/tab_restore_service.cc index 5892013..340bf34 100644 --- a/chrome/browser/sessions/tab_restore_service.cc +++ b/chrome/browser/sessions/tab_restore_service.cc @@ -146,7 +146,7 @@ void TabRestoreService::BrowserClosing(Browser* browser) { size_t entry_index = 0; for (int tab_index = 0; tab_index < browser->tab_count(); ++tab_index) { PopulateTabFromController( - &browser->GetTabContentsAt(tab_index)->controller(), + browser->GetTabContentsAt(tab_index)->controller(), &(window->tabs[entry_index])); if (window->tabs[entry_index].navigations.empty()) window->tabs.erase(window->tabs.begin() + entry_index); @@ -229,13 +229,13 @@ void TabRestoreService::RestoreEntryById(Browser* browser, Browser* browser = Browser::Create(profile()); for (size_t tab_i = 0; tab_i < window->tabs.size(); ++tab_i) { const Tab& tab = window->tabs[tab_i]; - TabContents* restored_tab = + NavigationController* restored_controller = browser->AddRestoredTab(tab.navigations, browser->tab_count(), tab.current_navigation_index, (static_cast<int>(tab_i) == window->selected_tab_index)); - if (restored_tab) - restored_tab->controller().LoadIfNecessary(); + if (restored_controller) + restored_controller->LoadIfNecessary(); } browser->window()->Show(); } else { diff --git a/chrome/browser/sessions/tab_restore_service_unittest.cc b/chrome/browser/sessions/tab_restore_service_unittest.cc index 7a24a91..9be6429 100644 --- a/chrome/browser/sessions/tab_restore_service_unittest.cc +++ b/chrome/browser/sessions/tab_restore_service_unittest.cc @@ -41,9 +41,9 @@ class TabRestoreServiceTest : public RenderViewHostTestHarness { void NavigateToIndex(int index) { // Navigate back. We have to do this song and dance as NavigationController // isn't happy if you navigate immediately while going back. - controller().GoToIndex(index); - rvh()->SendNavigate(controller().pending_entry()->page_id(), - controller().pending_entry()->url()); + controller()->GoToIndex(index); + rvh()->SendNavigate(controller()->pending_entry()->page_id(), + controller()->pending_entry()->url()); } void RecreateService() { @@ -92,7 +92,7 @@ TEST_F(TabRestoreServiceTest, Basic) { AddThreeNavigations(); // Have the service record the tab. - service_->CreateHistoricalTab(&controller()); + service_->CreateHistoricalTab(controller()); // Make sure an entry was created. ASSERT_EQ(1U, service_->entries().size()); @@ -110,7 +110,7 @@ TEST_F(TabRestoreServiceTest, Basic) { NavigateToIndex(1); // And check again. - service_->CreateHistoricalTab(&controller()); + service_->CreateHistoricalTab(controller()); // There should be two entries now. ASSERT_EQ(2U, service_->entries().size()); @@ -129,7 +129,7 @@ TEST_F(TabRestoreServiceTest, Basic) { // Make sure TabRestoreService doesn't create an entry for a tab with no // navigations. TEST_F(TabRestoreServiceTest, DontCreateEmptyTab) { - service_->CreateHistoricalTab(&controller()); + service_->CreateHistoricalTab(controller()); EXPECT_TRUE(service_->entries().empty()); } @@ -138,7 +138,7 @@ TEST_F(TabRestoreServiceTest, Restore) { AddThreeNavigations(); // Have the service record the tab. - service_->CreateHistoricalTab(&controller()); + service_->CreateHistoricalTab(controller()); // Recreate the service and have it load the tabs. RecreateService(); @@ -162,7 +162,7 @@ TEST_F(TabRestoreServiceTest, DontLoadRestoredTab) { AddThreeNavigations(); // Have the service record the tab. - service_->CreateHistoricalTab(&controller()); + service_->CreateHistoricalTab(controller()); ASSERT_EQ(1U, service_->entries().size()); // Restore the tab. @@ -179,12 +179,12 @@ TEST_F(TabRestoreServiceTest, DontLoadRestoredTab) { // Make sure we persist entries to disk that have post data. TEST_F(TabRestoreServiceTest, DontPersistPostData) { AddThreeNavigations(); - controller().GetEntryAtIndex(0)->set_has_post_data(true); - controller().GetEntryAtIndex(1)->set_has_post_data(true); - controller().GetEntryAtIndex(2)->set_has_post_data(true); + controller()->GetEntryAtIndex(0)->set_has_post_data(true); + controller()->GetEntryAtIndex(1)->set_has_post_data(true); + controller()->GetEntryAtIndex(2)->set_has_post_data(true); // Have the service record the tab. - service_->CreateHistoricalTab(&controller()); + service_->CreateHistoricalTab(controller()); ASSERT_EQ(1U, service_->entries().size()); // Recreate the service and have it load the tabs. @@ -209,7 +209,7 @@ TEST_F(TabRestoreServiceTest, DontLoadTwice) { AddThreeNavigations(); // Have the service record the tab. - service_->CreateHistoricalTab(&controller()); + service_->CreateHistoricalTab(controller()); ASSERT_EQ(1U, service_->entries().size()); // Recreate the service and have it load the tabs. @@ -276,7 +276,7 @@ TEST_F(TabRestoreServiceTest, LoadPreviousSessionAndTabs) { AddThreeNavigations(); - service_->CreateHistoricalTab(&controller()); + service_->CreateHistoricalTab(controller()); RecreateService(); @@ -317,7 +317,7 @@ TEST_F(TabRestoreServiceTest, ManyWindowsInSessionService) { AddThreeNavigations(); - service_->CreateHistoricalTab(&controller()); + service_->CreateHistoricalTab(controller()); RecreateService(); diff --git a/chrome/browser/site_instance_unittest.cc b/chrome/browser/site_instance_unittest.cc index 92af43f..83e3baf 100644 --- a/chrome/browser/site_instance_unittest.cc +++ b/chrome/browser/site_instance_unittest.cc @@ -107,11 +107,15 @@ TEST_F(SiteInstanceTest, SiteInstanceDestructor) { TestSiteInstance::CreateTestSiteInstance(profile.get(), &siteDeleteCounter, &browsingDeleteCounter); - { - WebContents contents(profile.get(), instance, MSG_ROUTING_NONE, NULL); - EXPECT_EQ(1, siteDeleteCounter); - EXPECT_EQ(1, browsingDeleteCounter); - } + WebContents* contents = new WebContents( + profile.get(), instance, MSG_ROUTING_NONE, NULL); + contents->SetupController(profile.get()); + EXPECT_EQ(1, siteDeleteCounter); + EXPECT_EQ(1, browsingDeleteCounter); + + contents->CloseContents(); + // Make sure that we flush any messages related to WebContents destruction. + MessageLoop::current()->RunAllPending(); EXPECT_EQ(2, siteDeleteCounter); EXPECT_EQ(2, browsingDeleteCounter); diff --git a/chrome/browser/ssl/ssl_blocking_page.cc b/chrome/browser/ssl/ssl_blocking_page.cc index ce9d9da..5abf71a 100644 --- a/chrome/browser/ssl/ssl_blocking_page.cc +++ b/chrome/browser/ssl/ssl_blocking_page.cc @@ -81,7 +81,7 @@ void SSLBlockingPage::UpdateEntry(NavigationEntry* entry) { entry->ssl().set_security_bits(ssl_info.security_bits); NotificationService::current()->Notify( NotificationType::SSL_VISIBLE_STATE_CHANGED, - Source<NavigationController>(&web->controller()), + Source<NavigationController>(web->controller()), NotificationService::NoDetails()); } diff --git a/chrome/browser/ssl/ssl_manager.cc b/chrome/browser/ssl/ssl_manager.cc index afc48ce..bbdf0e2 100644 --- a/chrome/browser/ssl/ssl_manager.cc +++ b/chrome/browser/ssl/ssl_manager.cc @@ -292,7 +292,7 @@ void SSLManager::ErrorHandler::Dispatch() { } // Hand ourselves off to the SSLManager. - manager_ = web_contents->controller().ssl_manager(); + manager_ = web_contents->controller()->ssl_manager(); OnDispatched(); } diff --git a/chrome/browser/ssl/ssl_policy.cc b/chrome/browser/ssl/ssl_policy.cc index ebe5e60..3cb5975 100644 --- a/chrome/browser/ssl/ssl_policy.cc +++ b/chrome/browser/ssl/ssl_policy.cc @@ -136,7 +136,7 @@ static void ShowErrorPage(SSLPolicy* policy, SSLManager::CertError* error) { true, error->request_url(), security_info); - tab->controller().GetActiveEntry()->set_page_type( + tab->controller()->GetActiveEntry()->set_page_type( NavigationEntry::ERROR_PAGE); } diff --git a/chrome/browser/tab_contents/infobar_delegate.cc b/chrome/browser/tab_contents/infobar_delegate.cc index 9a466fb..6925dda 100644 --- a/chrome/browser/tab_contents/infobar_delegate.cc +++ b/chrome/browser/tab_contents/infobar_delegate.cc @@ -29,7 +29,7 @@ InfoBarDelegate::InfoBarDelegate(TabContents* contents) } void InfoBarDelegate::StoreActiveEntryUniqueID(TabContents* contents) { - NavigationEntry* active_entry = contents->controller().GetActiveEntry(); + NavigationEntry* active_entry = contents->controller()->GetActiveEntry(); contents_unique_id_ = active_entry ? active_entry->unique_id() : 0; } diff --git a/chrome/browser/tab_contents/interstitial_page.cc b/chrome/browser/tab_contents/interstitial_page.cc index 1ffff17..206bc34 100644 --- a/chrome/browser/tab_contents/interstitial_page.cc +++ b/chrome/browser/tab_contents/interstitial_page.cc @@ -121,7 +121,7 @@ InterstitialPage::InterstitialPage(WebContents* tab, // (which is the case when the interstitial was triggered by a sub-resource on // a page) when we have a pending entry (in the process of loading a new top // frame). - DCHECK(new_navigation || !tab->controller().pending_entry()); + DCHECK(new_navigation || !tab->controller()->pending_entry()); } InterstitialPage::~InterstitialPage() { @@ -169,7 +169,7 @@ void InterstitialPage::Show() { // Give sub-classes a chance to set some states on the navigation entry. UpdateEntry(entry); - tab_->controller().AddTransientEntry(entry); + tab_->controller()->AddTransientEntry(entry); } DCHECK(!render_view_host_); @@ -183,9 +183,9 @@ void InterstitialPage::Show() { notification_registrar_.Add(this, NotificationType::TAB_CONTENTS_DESTROYED, Source<TabContents>(tab_)); notification_registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED, - Source<NavigationController>(&tab_->controller())); + Source<NavigationController>(tab_->controller())); notification_registrar_.Add(this, NotificationType::NAV_ENTRY_PENDING, - Source<NavigationController>(&tab_->controller())); + Source<NavigationController>(tab_->controller())); } void InterstitialPage::Hide() { @@ -194,7 +194,7 @@ void InterstitialPage::Hide() { if (tab_->interstitial_page()) tab_->remove_interstitial_page(); // Let's revert to the original title if necessary. - NavigationEntry* entry = tab_->controller().GetActiveEntry(); + NavigationEntry* entry = tab_->controller()->GetActiveEntry(); if (!new_navigation_ && should_revert_tab_title_) { entry->set_title(WideToUTF16Hack(original_tab_title_)); tab_->NotifyNavigationStateChanged(TabContents::INVALIDATE_TITLE); @@ -321,7 +321,7 @@ void InterstitialPage::DontProceed() { // explicitely. Note that by calling DiscardNonCommittedEntries() we also // discard the pending entry, which is what we want, since the navigation is // cancelled. - tab_->controller().DiscardNonCommittedEntries(); + tab_->controller()->DiscardNonCommittedEntries(); } Hide(); @@ -383,7 +383,7 @@ void InterstitialPage::UpdateTitle(RenderViewHost* render_view_host, int32 page_id, const std::wstring& title) { DCHECK(render_view_host == render_view_host_); - NavigationEntry* entry = tab_->controller().GetActiveEntry(); + NavigationEntry* entry = tab_->controller()->GetActiveEntry(); // If this interstitial is shown on an existing navigation entry, we'll need // to remember its title so we can revert to it when hidden. if (!new_navigation_ && !should_revert_tab_title_) { diff --git a/chrome/browser/tab_contents/navigation_controller.cc b/chrome/browser/tab_contents/navigation_controller.cc index 268eadb..77cf714 100644 --- a/chrome/browser/tab_contents/navigation_controller.cc +++ b/chrome/browser/tab_contents/navigation_controller.cc @@ -15,6 +15,7 @@ #include "chrome/browser/tab_contents/navigation_entry.h" #include "chrome/browser/tab_contents/repost_form_warning.h" #include "chrome/browser/tab_contents/site_instance.h" +#include "chrome/browser/tab_contents/web_contents.h" #include "chrome/common/navigation_types.h" #include "chrome/common/notification_service.h" #include "chrome/common/render_messages.h" @@ -120,34 +121,45 @@ NavigationController::NavigationController(TabContents* contents, ALLOW_THIS_IN_INITIALIZER_LIST(ssl_manager_(this, NULL)), needs_reload_(false), load_pending_entry_when_active_(false) { + if (contents) + contents->set_controller(this); DCHECK(profile_); } -NavigationController::~NavigationController() { - DiscardNonCommittedEntriesInternal(); - - NotificationService::current()->Notify( - NotificationType::TAB_CLOSED, - Source<NavigationController>(this), - NotificationService::NoDetails()); -} - -void NavigationController::RestoreFromState( +NavigationController::NavigationController( + Profile* profile, const std::vector<TabNavigation>& navigations, - int selected_navigation) { - // Verify that this controller is unused and that the input is valid. - DCHECK(entry_count() == 0 && !pending_entry()); + int selected_navigation) + : profile_(profile), + pending_entry_(NULL), + last_committed_entry_index_(-1), + pending_entry_index_(-1), + transient_entry_index_(-1), + tab_contents_(NULL), + max_restored_page_id_(-1), + ALLOW_THIS_IN_INITIALIZER_LIST(ssl_manager_(this, NULL)), + needs_reload_(true), + load_pending_entry_when_active_(false) { + DCHECK(profile_); DCHECK(selected_navigation >= 0 && selected_navigation < static_cast<int>(navigations.size())); // Populate entries_ from the supplied TabNavigations. - needs_reload_ = true; CreateNavigationEntriesFromTabNavigations(navigations, &entries_); // And finish the restore. FinishRestore(selected_navigation); } +NavigationController::~NavigationController() { + DiscardNonCommittedEntriesInternal(); + + NotificationService::current()->Notify( + NotificationType::TAB_CLOSED, + Source<NavigationController>(this), + NotificationService::NoDetails()); +} + void NavigationController::Reload(bool check_for_repost) { // Reloading a transient entry does nothing. if (transient_entry_index_ != -1) @@ -339,6 +351,19 @@ void NavigationController::RemoveEntryAtIndex(int index, } } +void NavigationController::Destroy() { + // TODO(brettw) the destruction of TabContents/NavigationController makes no + // sense (see TabContentsWasDestroyed) + tab_contents_->Destroy(); + // We are now deleted. +} + +void NavigationController::TabContentsWasDestroyed() { + // TODO(brettw) the destruction of TabContents/NavigationController makes no + // sense (see Destroy). + delete this; +} + NavigationEntry* NavigationController::CreateNavigationEntry( const GURL& url, const GURL& referrer, PageTransition::Type transition) { // Allow the browser URL handler to rewrite the URL. This will, for example, @@ -396,7 +421,7 @@ void NavigationController::LoadURLLazily(const GURL& url, load_pending_entry_when_active_ = true; } -bool NavigationController::LoadingURLLazily() const { +bool NavigationController::LoadingURLLazily() { return load_pending_entry_when_active_; } @@ -716,7 +741,6 @@ bool NavigationController::RendererDidNavigateAutoSubframe( return false; } -// TODO(brettw) I think this function is unnecessary. void NavigationController::CommitPendingEntry() { DiscardTransientEntry(); @@ -779,22 +803,6 @@ bool NavigationController::IsURLInPageNavigation(const GURL& url) const { return AreURLsInPageNavigation(last_committed->url(), url); } -void NavigationController::CopyStateFrom(const NavigationController& source) { - // Verify that we look new. - DCHECK(entry_count() == 0 && !pending_entry()); - - if (source.entry_count() == 0) - return; // Nothing new to do. - - needs_reload_ = true; - for (int i = 0; i < source.entry_count(); i++) { - entries_.push_back(linked_ptr<NavigationEntry>( - new NavigationEntry(*source.entries_[i]))); - } - - FinishRestore(source.last_committed_entry_index_); -} - void NavigationController::DiscardNonCommittedEntries() { bool transient = transient_entry_index_ != -1; DiscardNonCommittedEntriesInternal(); @@ -859,6 +867,9 @@ void NavigationController::NavigateToPendingEntry(bool reload) { pending_entry_ = entries_[pending_entry_index_].get(); } + tab_contents_ = GetTabContentsCreateIfNecessary(*pending_entry_); + + NavigationEntry temp_entry(*pending_entry_); if (!tab_contents_->NavigateToPendingEntry(reload)) DiscardNonCommittedEntries(); } @@ -878,6 +889,17 @@ void NavigationController::NotifyNavigationEntryCommitted( Details<LoadCommittedDetails>(details)); } +TabContents* NavigationController::GetTabContentsCreateIfNecessary( + const NavigationEntry& entry) { + if (tab_contents_) + return tab_contents_; + + tab_contents_ = new WebContents(profile_, entry.site_instance(), + MSG_ROUTING_NONE, NULL); + tab_contents_->set_controller(this); + return tab_contents_; // TODO(brettw) it's stupid to both set and return it. +} + // static void NavigationController::DisablePromptOnRepost() { check_for_repost_ = false; @@ -916,6 +938,25 @@ void NavigationController::NotifyEntryChanged(const NavigationEntry* entry, Details<EntryChangedDetails>(&det)); } +NavigationController* NavigationController::Clone() { + NavigationController* nc = new NavigationController(NULL, profile_); + + if (entry_count() == 0) + return nc; + + nc->needs_reload_ = true; + + nc->entries_.reserve(entries_.size()); + for (int i = 0, c = entry_count(); i < c; ++i) { + nc->entries_.push_back(linked_ptr<NavigationEntry>( + new NavigationEntry(*GetEntryAtIndex(i)))); + } + + nc->FinishRestore(last_committed_entry_index_); + + return nc; +} + void NavigationController::FinishRestore(int selected_index) { DCHECK(selected_index >= 0 && selected_index < entry_count()); ConfigureEntriesForRestore(&entries_); @@ -923,6 +964,9 @@ void NavigationController::FinishRestore(int selected_index) { set_max_restored_page_id(entry_count()); last_committed_entry_index_ = selected_index; + + // Callers assume we have an active_contents after restoring, so set it now. + tab_contents_ = GetTabContentsCreateIfNecessary(*entries_[selected_index]); } void NavigationController::DiscardNonCommittedEntriesInternal() { diff --git a/chrome/browser/tab_contents/navigation_controller.h b/chrome/browser/tab_contents/navigation_controller.h index e42f4ad..af7cf60 100644 --- a/chrome/browser/tab_contents/navigation_controller.h +++ b/chrome/browser/tab_contents/navigation_controller.h @@ -123,21 +123,34 @@ class NavigationController { // --------------------------------------------------------------------------- - NavigationController(TabContents* tab_contents, Profile* profile); + NavigationController(TabContents* initial_contents, Profile* profile); + + // Creates a NavigationController from the specified history. Processing + // for this is asynchronous and handled via the RestoreHelper (in + // navigation_controller.cc). + NavigationController( + Profile* profile, + const std::vector<TabNavigation>& navigations, + int selected_navigation); ~NavigationController(); + // Begin the destruction sequence for this NavigationController and all its + // registered tabs. The sequence is as follows: + // 1. All tabs are asked to Destroy themselves. + // 2. When each tab is finished Destroying, it will notify the controller. + // 3. Once all tabs are Destroyed, the NavigationController deletes itself. + // This ensures that all the TabContents outlive the NavigationController. + void Destroy(); + + // Clone the receiving navigation controller. Only the active tab contents is + // duplicated. + NavigationController* Clone(); + // Returns the profile for this controller. It can never be NULL. Profile* profile() const { return profile_; } - // Initializes this NavigationController with the given saved navigations, - // using selected_navigation as the currently loaded entry. Before this call - // the controller should be unused (there should be no current entry). This is - // used for session restore. - void RestoreFromState(const std::vector<TabNavigation>& navigations, - int selected_navigation); - // Active entry -------------------------------------------------------------- // Returns the active entry, which is the transient entry if any, the pending @@ -282,6 +295,10 @@ class NavigationController { // TabContents --------------------------------------------------------------- + // Notifies the controller that a TabContents that it owns has been destroyed. + // This is part of the NavigationController's Destroy sequence. + void TabContentsWasDestroyed(); + // Returns the tab contents associated with this controller. Non-NULL except // during set-up of the tab. TabContents* tab_contents() const { @@ -329,17 +346,13 @@ class NavigationController { // refs without reload, only change to "#" which we don't count as empty). bool IsURLInPageNavigation(const GURL& url) const; - // Copies the navigation state from the given controller to this one. This - // one should be empty (just created). - void CopyStateFrom(const NavigationController& source); - // Random data --------------------------------------------------------------- // Returns true if this NavigationController is is configured to load a URL // lazily. If true, use GetLazyTitle() and GetLazyFavIcon() to discover the // titles and favicons. Since no request was made, this is the only info // we have about this page. This feature is used by web application clusters. - bool LoadingURLLazily() const; + bool LoadingURLLazily(); const string16& GetLazyTitle() const; const SkBitmap& GetLazyFavIcon() const; @@ -412,6 +425,11 @@ class NavigationController { // committed. This will fill in the active entry to the details structure. void NotifyNavigationEntryCommitted(LoadCommittedDetails* details); + // Returns the TabContents for the |entry|'s type. If the TabContents + // doesn't yet exist, it is created. If a new TabContents is created, its + // parent is |parent|. Becomes part of |entry|'s SiteInstance. + TabContents* GetTabContentsCreateIfNecessary(const NavigationEntry& entry); + // Sets the max restored page ID this NavigationController has seen, if it // was restored from a previous session. void set_max_restored_page_id(int max_id) { max_restored_page_id_ = max_id; } diff --git a/chrome/browser/tab_contents/render_view_context_menu.cc b/chrome/browser/tab_contents/render_view_context_menu.cc index 698a65a..fd94e13 100644 --- a/chrome/browser/tab_contents/render_view_context_menu.cc +++ b/chrome/browser/tab_contents/render_view_context_menu.cc @@ -208,10 +208,10 @@ bool RenderViewContextMenu::IsItemCommandEnabled(int id) const { switch (id) { case IDS_CONTENT_CONTEXT_BACK: - return source_web_contents_->controller().CanGoBack(); + return source_web_contents_->controller()->CanGoBack(); case IDS_CONTENT_CONTEXT_FORWARD: - return source_web_contents_->controller().CanGoForward(); + return source_web_contents_->controller()->CanGoForward(); case IDS_CONTENT_CONTEXT_VIEWPAGESOURCE: case IDS_CONTENT_CONTEXT_VIEWFRAMESOURCE: @@ -283,7 +283,7 @@ bool RenderViewContextMenu::IsItemCommandEnabled(int id) const { return !params_.misspelled_word.empty(); case IDS_CONTENT_CONTEXT_VIEWPAGEINFO: - return (source_web_contents_->controller().GetActiveEntry() != NULL); + return (source_web_contents_->controller()->GetActiveEntry() != NULL); case IDS_CONTENT_CONTEXT_RELOAD: case IDS_CONTENT_CONTEXT_COPYIMAGE: @@ -393,11 +393,11 @@ void RenderViewContextMenu::ExecuteItemCommand(int id) { break; case IDS_CONTENT_CONTEXT_BACK: - source_web_contents_->controller().GoBack(); + source_web_contents_->controller()->GoBack(); break; case IDS_CONTENT_CONTEXT_FORWARD: - source_web_contents_->controller().GoForward(); + source_web_contents_->controller()->GoForward(); break; case IDS_CONTENT_CONTEXT_SAVEPAGEAS: @@ -405,7 +405,7 @@ void RenderViewContextMenu::ExecuteItemCommand(int id) { break; case IDS_CONTENT_CONTEXT_RELOAD: - source_web_contents_->controller().Reload(true); + source_web_contents_->controller()->Reload(true); break; case IDS_CONTENT_CONTEXT_PRINT: @@ -424,7 +424,7 @@ void RenderViewContextMenu::ExecuteItemCommand(int id) { case IDS_CONTENT_CONTEXT_VIEWPAGEINFO: { #if defined(OS_WIN) NavigationEntry* nav_entry = - source_web_contents_->controller().GetActiveEntry(); + source_web_contents_->controller()->GetActiveEntry(); PageInfoWindow::CreatePageInfo( source_web_contents_->profile(), nav_entry, @@ -581,7 +581,7 @@ bool RenderViewContextMenu::IsDevCommandEnabled(int id) const { return true; NavigationEntry *active_entry = - source_web_contents_->controller().GetActiveEntry(); + source_web_contents_->controller()->GetActiveEntry(); if (!active_entry) return false; diff --git a/chrome/browser/tab_contents/render_view_host_delegate_helper.cc b/chrome/browser/tab_contents/render_view_host_delegate_helper.cc index 9268850..9846a77 100644 --- a/chrome/browser/tab_contents/render_view_host_delegate_helper.cc +++ b/chrome/browser/tab_contents/render_view_host_delegate_helper.cc @@ -29,6 +29,7 @@ void RenderViewHostDelegateViewHelper::CreateNewWindow(int route_id, site, route_id, modal_dialog_event); + new_contents->SetupController(profile); WebContentsView* new_view = new_contents->view(); // TODO(brettw) it seems bogus that we have to call this function on the diff --git a/chrome/browser/tab_contents/render_view_host_manager.cc b/chrome/browser/tab_contents/render_view_host_manager.cc index 8b2c276..a178356 100644 --- a/chrome/browser/tab_contents/render_view_host_manager.cc +++ b/chrome/browser/tab_contents/render_view_host_manager.cc @@ -37,13 +37,9 @@ RenderViewHostManager::RenderViewHostManager( } RenderViewHostManager::~RenderViewHostManager() { - if (pending_render_view_host_) - CancelPending(); - - // We should always have a main RenderViewHost. - RenderViewHost* render_view_host = render_view_host_; - render_view_host_ = NULL; - render_view_host->Shutdown(); + // Shutdown should have been called which should have cleaned these up. + DCHECK(!render_view_host_); + DCHECK(!pending_render_view_host_); } void RenderViewHostManager::Init(Profile* profile, @@ -59,6 +55,16 @@ void RenderViewHostManager::Init(Profile* profile, site_instance, render_view_delegate_, routing_id, modal_dialog_event); } +void RenderViewHostManager::Shutdown() { + if (pending_render_view_host_) + CancelPending(); + + // We should always have a main RenderViewHost. + RenderViewHost* render_view_host = render_view_host_; + render_view_host_ = NULL; + render_view_host->Shutdown(); +} + RenderViewHost* RenderViewHostManager::Navigate(const NavigationEntry& entry) { // This will possibly create (set to NULL) a DOM UI object for the pending // page. We'll use this later to give the page special access. This must @@ -99,7 +105,7 @@ RenderViewHost* RenderViewHostManager::Navigate(const NavigationEntry& entry) { NotificationService::current()->Notify( NotificationType::RENDER_VIEW_HOST_CHANGED, Source<NavigationController>( - &delegate_->GetControllerForRenderManager()), + delegate_->GetControllerForRenderManager()), Details<RenderViewHostSwitchedDetails>(&details)); } } @@ -350,12 +356,12 @@ SiteInstance* RenderViewHostManager::GetSiteInstanceForEntry( // For now, though, we're in a hybrid model where you only switch // SiteInstances if you type in a cross-site URL. This means we have to // compare the entry's URL to the last committed entry's URL. - NavigationController& controller = delegate_->GetControllerForRenderManager(); - NavigationEntry* curr_entry = controller.GetLastCommittedEntry(); + NavigationController* controller = delegate_->GetControllerForRenderManager(); + NavigationEntry* curr_entry = controller->GetLastCommittedEntry(); if (interstitial_page_) { // The interstitial is currently the last committed entry, but we want to // compare against the last non-interstitial entry. - curr_entry = controller.GetEntryAtOffset(-1); + curr_entry = controller->GetEntryAtOffset(-1); } // If there is no last non-interstitial entry (and curr_instance already // has a site), then we must have been opened from another tab. We want @@ -380,7 +386,7 @@ SiteInstance* RenderViewHostManager::GetSiteInstanceForEntry( // that page, we want to explicity ignore that BrowsingInstance and make a // new process. return SiteInstance::CreateSiteInstance( - delegate_->GetControllerForRenderManager().profile()); + delegate_->GetControllerForRenderManager()->profile()); } else { // Start the new renderer in a new SiteInstance, but in the current // BrowsingInstance. It is important to immediately give this new @@ -393,7 +399,7 @@ SiteInstance* RenderViewHostManager::GetSiteInstanceForEntry( bool RenderViewHostManager::CreatePendingRenderView(SiteInstance* instance) { NavigationEntry* curr_entry = - delegate_->GetControllerForRenderManager().GetLastCommittedEntry(); + delegate_->GetControllerForRenderManager()->GetLastCommittedEntry(); if (curr_entry) { DCHECK(!curr_entry->content_state().empty()); // TODO(creis): Should send a message to the RenderView to let it know @@ -460,7 +466,7 @@ void RenderViewHostManager::CommitPending() { details.old_host = old_render_view_host; NotificationService::current()->Notify( NotificationType::RENDER_VIEW_HOST_CHANGED, - Source<NavigationController>(&delegate_->GetControllerForRenderManager()), + Source<NavigationController>(delegate_->GetControllerForRenderManager()), Details<RenderViewHostSwitchedDetails>(&details)); old_render_view_host->Shutdown(); diff --git a/chrome/browser/tab_contents/render_view_host_manager.h b/chrome/browser/tab_contents/render_view_host_manager.h index 15ee081..b3a5b25 100644 --- a/chrome/browser/tab_contents/render_view_host_manager.h +++ b/chrome/browser/tab_contents/render_view_host_manager.h @@ -47,7 +47,7 @@ class RenderViewHostManager : public NotificationObserver { RenderViewHost* render_view_host) = 0; virtual void UpdateRenderViewSizeForRenderManager() = 0; virtual void NotifySwappedFromRenderManager() = 0; - virtual NavigationController& GetControllerForRenderManager() = 0; + virtual NavigationController* GetControllerForRenderManager() = 0; // Creates a DOMUI object for the given URL if one applies. Ownership of the // returned pointer will be passed to the caller. If no DOMUI applies, @@ -64,7 +64,8 @@ class RenderViewHostManager : public NotificationObserver { // They must outlive this class. The RenderViewHostDelegate is what will be // installed into all RenderViewHosts that are created. // - // You must call Init() before using this class. + // You must call Init() before using this class and Shutdown() before + // deleting it. RenderViewHostManager(RenderViewHostDelegate* render_view_delegate, Delegate* delegate); ~RenderViewHostManager(); @@ -75,6 +76,9 @@ class RenderViewHostManager : public NotificationObserver { int routing_id, base::WaitableEvent* modal_dialog_event); + // Schedules all RenderViewHosts for destruction. + void Shutdown(); + // Returns the currently actuive RenderViewHost. // // This will be non-NULL between Init() and Shutdown(). You may want to NULL diff --git a/chrome/browser/tab_contents/render_view_host_manager_unittest.cc b/chrome/browser/tab_contents/render_view_host_manager_unittest.cc index f0caccd..02f5473 100644 --- a/chrome/browser/tab_contents/render_view_host_manager_unittest.cc +++ b/chrome/browser/tab_contents/render_view_host_manager_unittest.cc @@ -23,22 +23,27 @@ TEST_F(RenderViewHostManagerTest, NewTabPageProcesses) { NavigateAndCommit(dest); // Make a second tab. - TestWebContents contents2(profile_.get(), NULL); + WebContents* contents2 = new TestWebContents(profile_.get(), NULL); + NavigationController* controller2 = + new NavigationController(contents2, profile_.get()); + contents2->set_controller(controller2); // Load the two URLs in the second tab. Note that the first navigation creates // a RVH that's not pending (since there is no cross-site transition), so // we use the committed one, but the second one is the opposite. - contents2.controller().LoadURL(ntp, GURL(), PageTransition::LINK); - static_cast<TestRenderViewHost*>(contents2.render_manager()-> + contents2->controller()->LoadURL(ntp, GURL(), PageTransition::LINK); + static_cast<TestRenderViewHost*>(contents2->render_manager()-> current_host())->SendNavigate(100, ntp); - contents2.controller().LoadURL(dest, GURL(), PageTransition::LINK); - static_cast<TestRenderViewHost*>(contents2.render_manager()-> + contents2->controller()->LoadURL(dest, GURL(), PageTransition::LINK); + static_cast<TestRenderViewHost*>(contents2->render_manager()-> pending_render_view_host())->SendNavigate(101, dest); // The two RVH's should be different in every way. - EXPECT_NE(rvh()->process(), contents2.render_view_host()->process()); + EXPECT_NE(rvh()->process(), contents2->render_view_host()->process()); EXPECT_NE(rvh()->site_instance(), - contents2.render_view_host()->site_instance()); + contents2->render_view_host()->site_instance()); EXPECT_NE(rvh()->site_instance()->browsing_instance(), - contents2.render_view_host()->site_instance()->browsing_instance()); + contents2->render_view_host()->site_instance()->browsing_instance()); + + contents2->CloseContents(); } diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc index 226ce5b..ea5a9b5 100644 --- a/chrome/browser/tab_contents/tab_contents.cc +++ b/chrome/browser/tab_contents/tab_contents.cc @@ -24,9 +24,9 @@ #include "chrome/views/controls/scrollbar/native_scroll_bar.h" #endif -TabContents::TabContents(Profile* profile) +TabContents::TabContents() : delegate_(NULL), - controller_(this, profile), + controller_(NULL), is_loading_(false), is_crashed_(false), waiting_for_response_(false), @@ -45,13 +45,71 @@ void TabContents::RegisterUserPrefs(PrefService* prefs) { prefs->RegisterBooleanPref(prefs::kBlockPopups, false); } +void TabContents::CloseContents() { + // Destroy our NavigationController, which will Destroy all tabs it owns. + controller_->Destroy(); + // Note that the controller may have deleted us at this point, + // so don't touch any member variables here. +} + +void TabContents::Destroy() { + DCHECK(!is_being_destroyed_); + is_being_destroyed_ = true; + + // First cleanly close all child windows. + // TODO(mpcomplete): handle case if MaybeCloseChildWindows() already asked + // some of these to close. CloseWindows is async, so it might get called + // twice before it runs. + int size = static_cast<int>(child_windows_.size()); + for (int i = size - 1; i >= 0; --i) { + ConstrainedWindow* window = child_windows_[i]; + if (window) + window->CloseConstrainedWindow(); + } + + // Notify any lasting InfobarDelegates that have not yet been removed that + // whatever infobar they were handling in this TabContents has closed, + // because the TabContents is going away entirely. + for (int i = 0; i < infobar_delegate_count(); ++i) { + InfoBarDelegate* delegate = GetInfoBarDelegateAt(i); + delegate->InfoBarClosed(); + } + infobar_delegates_.clear(); + + // Notify any observer that have a reference on this tab contents. + NotificationService::current()->Notify( + NotificationType::TAB_CONTENTS_DESTROYED, + Source<TabContents>(this), + NotificationService::NoDetails()); + +#if defined(OS_WIN) + // If we still have a window handle, destroy it. GetNativeView can return + // NULL if this contents was part of a window that closed. + if (GetNativeView()) + ::DestroyWindow(GetNativeView()); +#endif + + // Notify our NavigationController. Make sure we are deleted first, so + // that the controller is the last to die. + NavigationController* controller = controller_; + + delete this; + + controller->TabContentsWasDestroyed(); +} + +void TabContents::SetupController(Profile* profile) { + DCHECK(!controller_); + controller_ = new NavigationController(this, profile); +} + bool TabContents::SupportsURL(GURL* url) { return true; } const GURL& TabContents::GetURL() const { // We may not have a navigation entry yet - NavigationEntry* entry = controller_.GetActiveEntry(); + NavigationEntry* entry = controller_->GetActiveEntry(); return entry ? entry->display_url() : GURL::EmptyGURL(); } @@ -82,22 +140,22 @@ const std::wstring TabContents::GetDefaultTitle() const { SkBitmap TabContents::GetFavIcon() const { // Like GetTitle(), we also want to use the favicon for the last committed // entry rather than a pending navigation entry. - NavigationEntry* entry = controller_.GetTransientEntry(); + NavigationEntry* entry = controller_->GetTransientEntry(); if (entry) return entry->favicon().bitmap(); - entry = controller_.GetLastCommittedEntry(); + entry = controller_->GetLastCommittedEntry(); if (entry) return entry->favicon().bitmap(); - else if (controller_.LoadingURLLazily()) - return controller_.GetLazyFavIcon(); + else if (controller_->LoadingURLLazily()) + return controller_->GetLazyFavIcon(); return SkBitmap(); } #if defined(OS_WIN) SecurityStyle TabContents::GetSecurityStyle() const { // We may not have a navigation entry yet. - NavigationEntry* entry = controller_.GetActiveEntry(); + NavigationEntry* entry = controller_->GetActiveEntry(); return entry ? entry->ssl().security_style() : SECURITY_STYLE_UNKNOWN; } @@ -107,7 +165,7 @@ bool TabContents::GetSSLEVText(std::wstring* ev_text, ev_text->clear(); ev_tooltip_text->clear(); - NavigationEntry* entry = controller_.GetActiveEntry(); + NavigationEntry* entry = controller_->GetActiveEntry(); if (!entry || net::IsCertStatusError(entry->ssl().cert_status()) || ((entry->ssl().cert_status() & net::CERT_STATUS_IS_EV) == 0)) @@ -153,8 +211,8 @@ void TabContents::OpenURL(const GURL& url, const GURL& referrer, bool TabContents::NavigateToPendingEntry(bool reload) { // Our benavior is just to report that the entry was committed. string16 default_title = WideToUTF16Hack(GetDefaultTitle()); - controller_.pending_entry()->set_title(default_title); - controller_.CommitPendingEntry(); + controller()->pending_entry()->set_title(default_title); + controller()->CommitPendingEntry(); return true; } @@ -244,8 +302,9 @@ void TabContents::AddInfoBar(InfoBarDelegate* delegate) { // added. We use this notification to expire InfoBars that need to expire on // page transitions. if (infobar_delegates_.size() == 1) { + DCHECK(controller()); registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED, - Source<NavigationController>(&controller_)); + Source<NavigationController>(controller())); } } @@ -263,7 +322,7 @@ void TabContents::RemoveInfoBar(InfoBarDelegate* delegate) { // Remove ourselves as an observer if we are tracking no more InfoBars. if (infobar_delegates_.empty()) { registrar_.Remove(this, NotificationType::NAV_ENTRY_COMMITTED, - Source<NavigationController>(&controller_)); + Source<NavigationController>(controller())); } } } diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h index aa636f3..26dda95 100644 --- a/chrome/browser/tab_contents/tab_contents.h +++ b/chrome/browser/tab_contents/tab_contents.h @@ -44,8 +44,23 @@ class SkBitmap; class SiteInstance; class WebContents; -// Describes what goes in the main content area of a tab. WebContents is -// the only type of TabContents, and these should be merged together. +// Describes what goes in the main content area of a tab. For example, +// the WebContents is one such thing. +// +// When instantiating a new TabContents explicitly, the TabContents will not +// have an associated NavigationController. To setup a NavigationController +// for the TabContents, its SetupController method should be called. +// +// Once they reside within a NavigationController, TabContents objects are +// owned by that NavigationController. When the active TabContents within that +// NavigationController is closed, that TabContents destroys the +// NavigationController, which then destroys all of the TabContentses in it. +// +// NOTE: When the NavigationController is navigated to an URL corresponding to +// a different type of TabContents (see the TabContents::TypeForURL method), +// the NavigationController makes the active TabContents inactive, notifies the +// TabContentsDelegate that the TabContents is being replaced, and then +// activates the new TabContents. class TabContents : public PageNavigator, public NotificationObserver { public: @@ -62,10 +77,23 @@ class TabContents : public PageNavigator, INVALIDATE_EVERYTHING = 0xFFFFFFFF }; - virtual ~TabContents(); - static void RegisterUserPrefs(PrefService* prefs); + // Creation & destruction ---------------------------------------------------- + + // Request this tab to shut down. This kills the tab's NavigationController, + // which then Destroy()s all tabs it controls. + void CloseContents(); + + // Unregister/shut down any pending tasks involving this tab. + // This is called as the tab is shutting down, before the + // NavigationController (and consequently profile) are gone. + // + // If you override this, be sure to call this implementation at the end + // of yours. + // See also Close(). + virtual void Destroy() = 0; + // Intrinsic tab state ------------------------------------------------------- // Returns the property bag for this tab contents, where callers can add @@ -88,13 +116,27 @@ class TabContents : public PageNavigator, TabContentsDelegate* delegate() const { return delegate_; } void set_delegate(TabContentsDelegate* d) { delegate_ = d; } - // Gets the controller for this tab contents. - NavigationController& controller() { return controller_; } - const NavigationController& controller() const { return controller_; } + // This can only be null if the TabContents has been created but + // SetupController has not been called. The controller should always outlive + // its TabContents. + NavigationController* controller() const { return controller_; } + void set_controller(NavigationController* c) { controller_ = c; } + + // Sets up a new NavigationController for this TabContents. + // |profile| is the user profile that should be associated with + // the new controller. + // + // TODO(brettw) this seems bogus and I couldn't find any legitimate need for + // it. I think it should be passed in the constructor. + void SetupController(Profile* profile); // Returns the user profile associated with this TabContents (via the - // NavigationController). - Profile* profile() const { return controller_.profile(); } + // NavigationController). This will return NULL if there isn't yet a + // NavigationController on this TabContents. + // TODO(darin): make it so that controller_ can never be null + Profile* profile() const { + return controller_ ? controller_->profile() : NULL; + } // Returns whether this tab contents supports the provided URL.This method // matches the tab contents type with the result of TypeForURL(). |url| points @@ -231,10 +273,6 @@ class TabContents : public PageNavigator, // Called on a TabContents when it isn't a popup, but a new window. virtual void DisassociateFromPopupCount() = 0; - // Creates a new TabContents with the same state as this one. The returned - // heap-allocated pointer is owned by the caller. - virtual TabContents* Clone() = 0; - // Window management --------------------------------------------------------- #if defined(OS_WIN) @@ -348,7 +386,15 @@ class TabContents : public PageNavigator, // automation purposes. friend class AutomationProvider; - TabContents(Profile* profile); + TabContents(); + + // NOTE: the TabContents destructor can run after the NavigationController + // has gone away, so any complicated unregistering that expects the profile + // or other shared objects to still be around does not belong in a + // destructor. + // For those purposes, instead see Destroy(). + // Protected so that others don't try to delete this directly. + virtual ~TabContents(); // Changes the IsLoading state and notifies delegate as needed // |details| is used to provide details on the load that just finished @@ -386,7 +432,7 @@ class TabContents : public PageNavigator, // Data ---------------------------------------------------------------------- TabContentsDelegate* delegate_; - NavigationController controller_; + NavigationController* controller_; PropertyBag property_bag_; diff --git a/chrome/browser/tab_contents/web_contents.cc b/chrome/browser/tab_contents/web_contents.cc index 8592522..bf70542 100644 --- a/chrome/browser/tab_contents/web_contents.cc +++ b/chrome/browser/tab_contents/web_contents.cc @@ -203,8 +203,7 @@ WebContents::WebContents(Profile* profile, SiteInstance* site_instance, int routing_id, base::WaitableEvent* modal_dialog_event) - : TabContents(profile), - view_(WebContentsView::Create(this)), + : view_(WebContentsView::Create(this)), ALLOW_THIS_IN_INITIALIZER_LIST(render_manager_(this, this)), printing_(*this), notify_disconnection_(false), @@ -235,12 +234,14 @@ WebContents::WebContents(Profile* profile, } // Register for notifications about URL starredness changing on any profile. - registrar_.Add(this, NotificationType::URLS_STARRED, - NotificationService::AllSources()); - registrar_.Add(this, NotificationType::BOOKMARK_MODEL_LOADED, - NotificationService::AllSources()); - registrar_.Add(this, NotificationType::RENDER_WIDGET_HOST_DESTROYED, - NotificationService::AllSources()); + NotificationService::current()->AddObserver( + this, NotificationType::URLS_STARRED, NotificationService::AllSources()); + NotificationService::current()->AddObserver( + this, NotificationType::BOOKMARK_MODEL_LOADED, + NotificationService::AllSources()); + NotificationService::current()->AddObserver( + this, NotificationType::RENDER_WIDGET_HOST_DESTROYED, + NotificationService::AllSources()); // Keep a global copy of the previous search string (if any). static string16 global_last_search = string16(); @@ -248,60 +249,11 @@ WebContents::WebContents(Profile* profile, } WebContents::~WebContents() { - is_being_destroyed_ = true; - - // We don't want any notifications while we're runnign our destructor. - registrar_.RemoveAll(); - - // Unregister the notifications of all observed prefs change. - PrefService* prefs = profile()->GetPrefs(); - if (prefs) { - for (int i = 0; i < kPrefsToObserveLength; ++i) - prefs->RemovePrefObserver(kPrefsToObserve[i], this); - } - - // Clean up subwindows like plugins and the find in page bar. - view_->OnContentsDestroy(); - - NotifyDisconnected(); - HungRendererWarning::HideForWebContents(this); - if (pending_install_.callback_functor) pending_install_.callback_functor->Cancel(); - - // First cleanly close all child windows. - // TODO(mpcomplete): handle case if MaybeCloseChildWindows() already asked - // some of these to close. CloseWindows is async, so it might get called - // twice before it runs. - int size = static_cast<int>(child_windows_.size()); - for (int i = size - 1; i >= 0; --i) { - ConstrainedWindow* window = child_windows_[i]; - if (window) - window->CloseConstrainedWindow(); - } - - // Notify any lasting InfobarDelegates that have not yet been removed that - // whatever infobar they were handling in this TabContents has closed, - // because the TabContents is going away entirely. - for (int i = 0; i < infobar_delegate_count(); ++i) { - InfoBarDelegate* delegate = GetInfoBarDelegateAt(i); - delegate->InfoBarClosed(); - } - infobar_delegates_.clear(); - - // Notify any observer that have a reference on this tab contents. - NotificationService::current()->Notify( - NotificationType::TAB_CONTENTS_DESTROYED, - Source<TabContents>(this), - NotificationService::NoDetails()); - - // TODO(brettw) this should be moved to the view. -#if defined(OS_WIN) - // If we still have a window handle, destroy it. GetNativeView can return - // NULL if this contents was part of a window that closed. - if (GetNativeView()) - ::DestroyWindow(GetNativeView()); -#endif + NotificationService::current()->RemoveObserver( + this, NotificationType::RENDER_WIDGET_HOST_DESTROYED, + NotificationService::AllSources()); } // static @@ -378,6 +330,78 @@ PluginInstaller* WebContents::GetPluginInstaller() { return plugin_installer_.get(); } +void WebContents::Destroy() { + DCHECK(!is_being_destroyed_); + is_being_destroyed_ = true; + + // Tell the notification service we no longer want notifications. + NotificationService::current()->RemoveObserver( + this, NotificationType::URLS_STARRED, NotificationService::AllSources()); + NotificationService::current()->RemoveObserver( + this, NotificationType::BOOKMARK_MODEL_LOADED, + NotificationService::AllSources()); + + // Destroy the print manager right now since a Print command may be pending. + printing_.Destroy(); + + // Unregister the notifications of all observed prefs change. + PrefService* prefs = profile()->GetPrefs(); + if (prefs) { + for (int i = 0; i < kPrefsToObserveLength; ++i) + prefs->RemovePrefObserver(kPrefsToObserve[i], this); + } + + cancelable_consumer_.CancelAllRequests(); + + // Clean up subwindows like plugins and the find in page bar. + view_->OnContentsDestroy(); + + NotifyDisconnected(); + HungRendererWarning::HideForWebContents(this); + render_manager_.Shutdown(); + + // First cleanly close all child windows. + // TODO(mpcomplete): handle case if MaybeCloseChildWindows() already asked + // some of these to close. CloseWindows is async, so it might get called + // twice before it runs. + int size = static_cast<int>(child_windows_.size()); + for (int i = size - 1; i >= 0; --i) { + ConstrainedWindow* window = child_windows_[i]; + if (window) + window->CloseConstrainedWindow(); + } + + // Notify any lasting InfobarDelegates that have not yet been removed that + // whatever infobar they were handling in this TabContents has closed, + // because the TabContents is going away entirely. + for (int i = 0; i < infobar_delegate_count(); ++i) { + InfoBarDelegate* delegate = GetInfoBarDelegateAt(i); + delegate->InfoBarClosed(); + } + infobar_delegates_.clear(); + + // Notify any observer that have a reference on this tab contents. + NotificationService::current()->Notify( + NotificationType::TAB_CONTENTS_DESTROYED, + Source<TabContents>(this), + NotificationService::NoDetails()); + +#if defined(OS_WIN) + // If we still have a window handle, destroy it. GetNativeView can return + // NULL if this contents was part of a window that closed. + if (GetNativeView()) + ::DestroyWindow(GetNativeView()); +#endif + + // Notify our NavigationController. Make sure we are deleted first, so + // that the controller is the last to die. + NavigationController* controller = controller_; + + delete this; + + controller->TabContentsWasDestroyed(); +} + const string16& WebContents::GetTitle() const { DOMUI* our_dom_ui = render_manager_.pending_dom_ui() ? render_manager_.pending_dom_ui() : render_manager_.dom_ui(); @@ -394,15 +418,15 @@ const string16& WebContents::GetTitle() const { // title. // The exception is with transient pages, for which we really want to use // their title, as they are not committed. - NavigationEntry* entry = controller_.GetTransientEntry(); + NavigationEntry* entry = controller_->GetTransientEntry(); if (entry) - return entry->GetTitleForDisplay(&controller_); + return entry->GetTitleForDisplay(controller_); - entry = controller_.GetLastCommittedEntry(); + entry = controller_->GetLastCommittedEntry(); if (entry) - return entry->GetTitleForDisplay(&controller_); - else if (controller_.LoadingURLLazily()) - return controller_.GetLazyTitle(); + return entry->GetTitleForDisplay(controller_); + else if (controller_->LoadingURLLazily()) + return controller_->GetLazyTitle(); return EmptyString16(); } @@ -419,7 +443,7 @@ bool WebContents::ShouldDisplayURL() { bool WebContents::ShouldDisplayFavIcon() { // Always display a throbber during pending loads. - if (controller_.GetLastCommittedEntry() && controller_.pending_entry()) + if (controller()->GetLastCommittedEntry() && controller()->pending_entry()) return true; DOMUI* dom_ui = GetDOMUIForCurrentState(); @@ -456,7 +480,7 @@ std::wstring WebContents::GetStatusText() const { } bool WebContents::NavigateToPendingEntry(bool reload) { - const NavigationEntry& entry = *controller_.pending_entry(); + const NavigationEntry& entry = *controller()->pending_entry(); RenderViewHost* dest_render_view_host = render_manager_.Navigate(entry); if (!dest_render_view_host) @@ -519,19 +543,15 @@ void WebContents::DisassociateFromPopupCount() { render_view_host()->DisassociateFromPopupCount(); } -TabContents* WebContents::Clone() { - // We create a new SiteInstance so that the new tab won't share processes - // with the old one. This can be changed in the future if we need it to share - // processes for some reason. - TabContents* tc = new WebContents(profile(), - SiteInstance::CreateSiteInstance(profile()), - MSG_ROUTING_NONE, NULL); - tc->controller().CopyStateFrom(controller_); - return tc; -} - void WebContents::DidBecomeSelected() { - controller_.SetActive(true); + if (controller_) + controller_->SetActive(true); + +#if defined(OS_WIN) + // Invalidate all descendants. (take care to exclude invalidating ourselves!) + // TODO(brettw) this should be moved to the view! +// EnumChildWindows(GetNativeView(), InvalidateWindow, 0); +#endif if (render_widget_host_view()) render_widget_host_view()->DidBecomeSelected(); @@ -592,7 +612,7 @@ bool WebContents::IsBookmarkBarAlwaysVisible() { // See GetDOMUIForCurrentState() comment for more info. This case is very // similar, but for non-first loads, we want to use the committed entry. This // is so the bookmarks bar disappears at the same time the page does. - if (controller_.GetLastCommittedEntry()) { + if (controller()->GetLastCommittedEntry()) { // Not the first load, always use the committed DOM UI. if (render_manager_.dom_ui()) return render_manager_.dom_ui()->force_bookmark_bar_visible(); @@ -657,7 +677,7 @@ void WebContents::GetContainerBounds(gfx::Rect *out) const { } void WebContents::CreateShortcut() { - NavigationEntry* entry = controller_.GetLastCommittedEntry(); + NavigationEntry* entry = controller()->GetLastCommittedEntry(); if (!entry) return; @@ -775,7 +795,7 @@ bool WebContents::PrintNow() { } bool WebContents::IsActiveEntry(int32 page_id) { - NavigationEntry* active_entry = controller_.GetActiveEntry(); + NavigationEntry* active_entry = controller()->GetActiveEntry(); return (active_entry != NULL && active_entry->site_instance() == GetSiteInstance() && active_entry->page_id() == page_id); @@ -815,7 +835,7 @@ void WebContents::SetIsLoading(bool is_loading, if (details) det = Details<LoadNotificationDetails>(details); NotificationService::current()->Notify(type, - Source<NavigationController>(&controller_), + Source<NavigationController>(this->controller()), det); } @@ -832,7 +852,9 @@ Profile* WebContents::GetProfile() const { } void WebContents::RenderViewCreated(RenderViewHost* render_view_host) { - NavigationEntry* entry = controller_.GetActiveEntry(); + if (!controller()) + return; + NavigationEntry* entry = controller()->GetActiveEntry(); if (!entry) return; @@ -884,6 +906,10 @@ void WebContents::DidNavigate(RenderViewHost* rvh, if (PageTransition::IsMainFrame(params.transition)) render_manager_.DidNavigateMainFrame(rvh); + // We can't do anything about navigations when we're inactive. + if (!controller()) + return; + // Update the site of the SiteInstance if it doesn't have one yet. if (!GetSiteInstance()->has_site()) GetSiteInstance()->SetSite(params.url); @@ -900,7 +926,7 @@ void WebContents::DidNavigate(RenderViewHost* rvh, contents_mime_type_ = params.contents_mime_type; NavigationController::LoadCommittedDetails details; - if (!controller_.RendererDidNavigate(params, &details)) + if (!controller()->RendererDidNavigate(params, &details)) return; // No navigation happened. // DO NOT ADD MORE STUFF TO THIS FUNCTION! Your component should either listen @@ -918,6 +944,8 @@ void WebContents::UpdateState(RenderViewHost* rvh, int32 page_id, const std::string& state) { DCHECK(rvh == render_view_host()); + if (!controller()) + return; // We must be prepared to handle state updates for any page, these occur // when the user is scrolling and entering form data, as well as when we're @@ -925,43 +953,49 @@ void WebContents::UpdateState(RenderViewHost* rvh, // the next page. The navigation controller will look up the appropriate // NavigationEntry and update it when it is notified via the delegate. - int entry_index = controller_.GetEntryIndexWithPageID( + int entry_index = controller()->GetEntryIndexWithPageID( GetSiteInstance(), page_id); if (entry_index < 0) return; - NavigationEntry* entry = controller_.GetEntryAtIndex(entry_index); + NavigationEntry* entry = controller()->GetEntryAtIndex(entry_index); if (state == entry->content_state()) return; // Nothing to update. entry->set_content_state(state); - controller_.NotifyEntryChanged(entry, entry_index); + controller()->NotifyEntryChanged(entry, entry_index); } void WebContents::UpdateTitle(RenderViewHost* rvh, int32 page_id, const std::wstring& title) { + if (!controller()) + return; + // If we have a title, that's a pretty good indication that we've started // getting useful data. SetNotWaitingForResponse(); DCHECK(rvh == render_view_host()); - NavigationEntry* entry = controller_.GetEntryWithPageID(GetSiteInstance(), + NavigationEntry* entry = controller()->GetEntryWithPageID(GetSiteInstance(), page_id); if (!entry || !UpdateTitleForEntry(entry, title)) return; // Broadcast notifications when the UI should be updated. - if (entry == controller_.GetEntryAtOffset(0)) + if (entry == controller()->GetEntryAtOffset(0)) NotifyNavigationStateChanged(INVALIDATE_TITLE); } void WebContents::UpdateFeedList( RenderViewHost* rvh, const ViewHostMsg_UpdateFeedList_Params& params) { + if (!controller()) + return; + // We might have an old RenderViewHost sending messages, and we should ignore // those messages. if (rvh != render_view_host()) return; - NavigationEntry* entry = controller_.GetEntryWithPageID(GetSiteInstance(), + NavigationEntry* entry = controller()->GetEntryWithPageID(GetSiteInstance(), params.page_id); if (!entry) return; @@ -969,7 +1003,7 @@ void WebContents::UpdateFeedList( entry->set_feedlist(params.feedlist); // Broadcast notifications when the UI should be updated. - if (entry == controller_.GetEntryAtOffset(0)) + if (entry == controller()->GetEntryAtOffset(0)) NotifyNavigationStateChanged(INVALIDATE_FEEDLIST); } @@ -1011,23 +1045,24 @@ void WebContents::DidStartLoading(RenderViewHost* rvh, int32 page_id) { void WebContents::DidStopLoading(RenderViewHost* rvh, int32 page_id) { scoped_ptr<LoadNotificationDetails> details; - - NavigationEntry* entry = controller_.GetActiveEntry(); - // An entry may not exist for a stop when loading an initial blank page or - // if an iframe injected by script into a blank page finishes loading. - if (entry) { - scoped_ptr<base::ProcessMetrics> metrics( - base::ProcessMetrics::CreateProcessMetrics( - process()->process().handle())); - - TimeDelta elapsed = TimeTicks::Now() - current_load_start_; - - details.reset(new LoadNotificationDetails( - entry->display_url(), - entry->transition_type(), - elapsed, - &controller_, - controller_.GetCurrentEntryIndex())); + if (controller()) { + NavigationEntry* entry = controller()->GetActiveEntry(); + // An entry may not exist for a stop when loading an initial blank page or + // if an iframe injected by script into a blank page finishes loading. + if (entry) { + scoped_ptr<base::ProcessMetrics> metrics( + base::ProcessMetrics::CreateProcessMetrics( + process()->process().handle())); + + TimeDelta elapsed = TimeTicks::Now() - current_load_start_; + + details.reset(new LoadNotificationDetails( + entry->display_url(), + entry->transition_type(), + elapsed, + controller(), + controller()->GetCurrentEntryIndex())); + } } // Tell PasswordManager we've finished a page load, which serves as a @@ -1042,11 +1077,11 @@ void WebContents::DidStartProvisionalLoadForFrame( bool is_main_frame, const GURL& url) { ProvisionalLoadDetails details(is_main_frame, - controller_.IsURLInPageNavigation(url), + controller()->IsURLInPageNavigation(url), url, std::string(), false); NotificationService::current()->Notify( NotificationType::FRAME_PROVISIONAL_LOAD_START, - Source<NavigationController>(&controller_), + Source<NavigationController>(controller()), Details<ProvisionalLoadDetails>(&details)); } @@ -1055,9 +1090,9 @@ void WebContents::DidRedirectProvisionalLoad(int32 page_id, const GURL& target_url) { NavigationEntry* entry; if (page_id == -1) - entry = controller_.pending_entry(); + entry = controller()->pending_entry(); else - entry = controller_.GetEntryWithPageID(GetSiteInstance(), page_id); + entry = controller()->GetEntryWithPageID(GetSiteInstance(), page_id); if (!entry || entry->url() != source_url) return; entry->set_url(target_url); @@ -1068,6 +1103,9 @@ void WebContents::DidLoadResourceFromMemoryCache( const std::string& frame_origin, const std::string& main_frame_origin, const std::string& security_info) { + if (!controller()) + return; + // Send out a notification that we loaded a resource from our memory cache. int cert_id = 0, cert_status = 0, security_bits = 0; SSLManager::DeserializeSecurityInfo(security_info, @@ -1078,7 +1116,7 @@ void WebContents::DidLoadResourceFromMemoryCache( NotificationService::current()->Notify( NotificationType::LOAD_FROM_MEMORY_CACHE, - Source<NavigationController>(&controller_), + Source<NavigationController>(controller()), Details<LoadFromMemoryCacheDetails>(&details)); } @@ -1088,6 +1126,9 @@ void WebContents::DidFailProvisionalLoadWithError( int error_code, const GURL& url, bool showing_repost_interstitial) { + if (!controller()) + return; + if (net::ERR_ABORTED == error_code) { // EVIL HACK ALERT! Ignore failed loads when we're showing interstitials. // This means that the interstitial won't be torn down properly, which is @@ -1116,22 +1157,22 @@ void WebContents::DidFailProvisionalLoadWithError( // decided to download the file instead of load it). Only discard the // pending entry if the URLs match, otherwise the user initiated a navigate // before the page loaded so that the discard would discard the wrong entry. - NavigationEntry* pending_entry = controller_.pending_entry(); + NavigationEntry* pending_entry = controller()->pending_entry(); if (pending_entry && pending_entry->url() == url) - controller_.DiscardNonCommittedEntries(); + controller()->DiscardNonCommittedEntries(); render_manager_.RendererAbortedProvisionalLoad(render_view_host); } // Send out a notification that we failed a provisional load with an error. ProvisionalLoadDetails details(is_main_frame, - controller_.IsURLInPageNavigation(url), + controller()->IsURLInPageNavigation(url), url, std::string(), false); details.set_error_code(error_code); NotificationService::current()->Notify( NotificationType::FAIL_PROVISIONAL_LOAD_WITH_ERROR, - Source<NavigationController>(&controller_), + Source<NavigationController>(controller()), Details<ProvisionalLoadDetails>(&details)); } @@ -1203,14 +1244,21 @@ void WebContents::ProcessExternalHostMessage(const std::string& message, } void WebContents::GoToEntryAtOffset(int offset) { - controller_.GoToOffset(offset); + if (!controller()) + return; + controller()->GoToOffset(offset); } void WebContents::GetHistoryListCount(int* back_list_count, int* forward_list_count) { - int current_index = controller_.last_committed_entry_index(); - *back_list_count = current_index; - *forward_list_count = controller_.entry_count() - current_index - 1; + *back_list_count = 0; + *forward_list_count = 0; + + if (controller()) { + int current_index = controller()->last_committed_entry_index(); + *back_list_count = current_index; + *forward_list_count = controller()->entry_count() - current_index - 1; + } } void WebContents::RunFileChooser(bool multiple_files, @@ -1306,7 +1354,7 @@ void WebContents::PageHasOSDD(RenderViewHost* render_view_host, bool autodetected) { // Make sure page_id is the current page, and the TemplateURLModel is loaded. DCHECK(url.is_valid()); - if (!IsActiveEntry(page_id)) + if (!controller() || !IsActiveEntry(page_id)) return; TemplateURLModel* url_model = profile()->GetTemplateURLModel(); if (!url_model) @@ -1321,18 +1369,18 @@ void WebContents::PageHasOSDD(RenderViewHost* render_view_host, if (profile()->IsOffTheRecord()) return; - const NavigationEntry* entry = controller_.GetLastCommittedEntry(); + const NavigationEntry* entry = controller()->GetLastCommittedEntry(); DCHECK(entry); const NavigationEntry* base_entry = entry; if (IsFormSubmit(base_entry)) { // If the current page is a form submit, find the last page that was not // a form submit and use its url to generate the keyword from. - int index = controller_.last_committed_entry_index() - 1; - while (index >= 0 && IsFormSubmit(controller_.GetEntryAtIndex(index))) + int index = controller()->last_committed_entry_index() - 1; + while (index >= 0 && IsFormSubmit(controller()->GetEntryAtIndex(index))) index--; if (index >= 0) - base_entry = controller_.GetEntryAtIndex(index); + base_entry = controller()->GetEntryAtIndex(index); else base_entry = NULL; } @@ -1585,7 +1633,9 @@ DOMUI* WebContents::CreateDOMUIForRenderManager(const GURL& url) { NavigationEntry* WebContents::GetLastCommittedNavigationEntryForRenderManager() { - return controller_.GetLastCommittedEntry(); + if (!controller()) + return NULL; + return controller()->GetLastCommittedEntry(); } bool WebContents::CreateRenderViewForRenderManager( @@ -1644,7 +1694,7 @@ void WebContents::Observe(NotificationType type, break; case NotificationType::NAV_ENTRY_COMMITTED: { - DCHECK(&controller_ == Source<NavigationController>(source).ptr()); + DCHECK(controller() == Source<NavigationController>(source).ptr()); NavigationController::LoadCommittedDetails& committed_details = *(Details<NavigationController::LoadCommittedDetails>(details).ptr()); @@ -1778,7 +1828,7 @@ void WebContents::UpdateWebPreferences() { void WebContents::OnGearsCreateShortcutDone( const GearsShortcutData2& shortcut_data, bool success) { - NavigationEntry* current_entry = controller_.GetLastCommittedEntry(); + NavigationEntry* current_entry = controller()->GetLastCommittedEntry(); bool same_page = current_entry && pending_install_.page_id == current_entry->page_id(); @@ -1802,7 +1852,7 @@ void WebContents::UpdateMaxPageIDIfNecessary(SiteInstance* site_instance, // Note that it is ok for conflicting page IDs to exist in another tab // (i.e., NavigationController), but if any page ID is larger than the max, // the back/forward list will get confused. - int max_restored_page_id = controller_.max_restored_page_id(); + int max_restored_page_id = controller()->max_restored_page_id(); if (max_restored_page_id > 0) { int curr_max_page_id = site_instance->max_page_id(); if (max_restored_page_id > curr_max_page_id) { @@ -1917,13 +1967,14 @@ void WebContents::NotifyDisconnected() { void WebContents::GenerateKeywordIfNecessary( const ViewHostMsg_FrameNavigate_Params& params) { + DCHECK(controller()); if (!params.searchable_form_url.is_valid()) return; if (profile()->IsOffTheRecord()) return; - int last_index = controller_.last_committed_entry_index(); + int last_index = controller()->last_committed_entry_index(); // When there was no previous page, the last index will be 0. This is // normally due to a form submit that opened in a new tab. // TODO(brettw) bug 916126: we should support keywords when form submits @@ -1931,7 +1982,7 @@ void WebContents::GenerateKeywordIfNecessary( if (last_index <= 0) return; const NavigationEntry* previous_entry = - controller_.GetEntryAtIndex(last_index - 1); + controller()->GetEntryAtIndex(last_index - 1); if (IsFormSubmit(previous_entry)) { // Only generate a keyword if the previous page wasn't itself a form // submit. @@ -1972,9 +2023,9 @@ void WebContents::GenerateKeywordIfNecessary( new_url->set_short_name(keyword); new_url->SetURL(url, 0, 0); new_url->add_input_encoding(params.searchable_form_encoding); - DCHECK(controller_.GetLastCommittedEntry()); + DCHECK(controller()->GetLastCommittedEntry()); const GURL& favicon_url = - controller_.GetLastCommittedEntry()->favicon().url(); + controller()->GetLastCommittedEntry()->favicon().url(); if (favicon_url.is_valid()) { new_url->SetFavIconURL(favicon_url); } else { @@ -2022,8 +2073,8 @@ DOMUI* WebContents::GetDOMUIForCurrentState() { // // - Normal state with no load: committed nav entry + no pending nav entry: // -> Use committed DOM UI. - if (controller_.pending_entry() && - (controller_.GetLastCommittedEntry() || + if (controller()->pending_entry() && + (controller()->GetLastCommittedEntry() || render_manager_.pending_dom_ui())) return render_manager_.pending_dom_ui(); return render_manager_.dom_ui(); diff --git a/chrome/browser/tab_contents/web_contents.h b/chrome/browser/tab_contents/web_contents.h index 9128868..b89efd3 100644 --- a/chrome/browser/tab_contents/web_contents.h +++ b/chrome/browser/tab_contents/web_contents.h @@ -73,8 +73,6 @@ class WebContents : public TabContents, int routing_id, base::WaitableEvent* modal_dialog_event); - virtual ~WebContents(); - static void RegisterUserPrefs(PrefService* prefs); // Getters ------------------------------------------------------------------- @@ -128,6 +126,7 @@ class WebContents : public TabContents, // TabContents (public overrides) -------------------------------------------- + virtual void Destroy(); virtual WebContents* AsWebContents() { return this; } const string16& GetTitle() const; virtual SiteInstance* GetSiteInstance() const; @@ -140,7 +139,6 @@ class WebContents : public TabContents, virtual void Copy(); virtual void Paste(); virtual void DisassociateFromPopupCount(); - virtual TabContents* Clone(); virtual void DidBecomeSelected(); virtual void WasHidden(); virtual void ShowContents(); @@ -294,6 +292,9 @@ class WebContents : public TabContents, } protected: + // Should be deleted via CloseContents. + virtual ~WebContents(); + RenderWidgetHostView* render_widget_host_view() const { return render_manager_.current_view(); } @@ -452,7 +453,7 @@ class WebContents : public TabContents, virtual void NotifySwappedFromRenderManager() { NotifySwapped(); } - virtual NavigationController& GetControllerForRenderManager() { + virtual NavigationController* GetControllerForRenderManager() { return controller(); } virtual DOMUI* CreateDOMUIForRenderManager(const GURL& url); diff --git a/chrome/browser/tab_contents/web_contents_unittest.cc b/chrome/browser/tab_contents/web_contents_unittest.cc index 207a724..c5e58e1 100644 --- a/chrome/browser/tab_contents/web_contents_unittest.cc +++ b/chrome/browser/tab_contents/web_contents_unittest.cc @@ -206,7 +206,7 @@ TEST_F(WebContentsTest, UpdateTitle) { InitNavigateParams(¶ms, 0, GURL("about:blank")); NavigationController::LoadCommittedDetails details; - controller().RendererDidNavigate(params, &details); + controller()->RendererDidNavigate(params, &details); contents()->UpdateTitle(rvh(), 0, L" Lots O' Whitespace\n"); EXPECT_EQ(std::wstring(L"Lots O' Whitespace"), @@ -221,12 +221,12 @@ TEST_F(WebContentsTest, SimpleNavigation) { // Navigate to URL const GURL url("http://www.google.com"); - controller().LoadURL(url, GURL(), PageTransition::TYPED); + controller()->LoadURL(url, GURL(), PageTransition::TYPED); EXPECT_FALSE(contents()->cross_navigation_pending()); EXPECT_EQ(instance1, orig_rvh->site_instance()); // Controller's pending entry will have a NULL site instance until we assign // it in DidNavigate. - EXPECT_TRUE(controller().GetActiveEntry()->site_instance() == NULL); + EXPECT_TRUE(controller()->GetActiveEntry()->site_instance() == NULL); // DidNavigate from the page ViewHostMsg_FrameNavigate_Params params; @@ -237,7 +237,7 @@ TEST_F(WebContentsTest, SimpleNavigation) { EXPECT_EQ(instance1, orig_rvh->site_instance()); // Controller's entry should now have the SiteInstance, or else we won't be // able to find it later. - EXPECT_EQ(instance1, controller().GetActiveEntry()->site_instance()); + EXPECT_EQ(instance1, controller()->GetActiveEntry()->site_instance()); } // Test that navigating across a site boundary creates a new RenderViewHost @@ -251,7 +251,7 @@ TEST_F(WebContentsTest, CrossSiteBoundaries) { // Navigate to URL. First URL should use first RenderViewHost. const GURL url("http://www.google.com"); - controller().LoadURL(url, GURL(), PageTransition::TYPED); + controller()->LoadURL(url, GURL(), PageTransition::TYPED); ViewHostMsg_FrameNavigate_Params params1; InitNavigateParams(¶ms1, 1, url); contents()->TestDidNavigate(orig_rvh, params1); @@ -261,7 +261,7 @@ TEST_F(WebContentsTest, CrossSiteBoundaries) { // Navigate to new site const GURL url2("http://www.yahoo.com"); - controller().LoadURL(url2, GURL(), PageTransition::TYPED); + controller()->LoadURL(url2, GURL(), PageTransition::TYPED); EXPECT_TRUE(contents()->cross_navigation_pending()); TestRenderViewHost* pending_rvh = contents()->pending_rvh(); int pending_rvh_delete_count = 0; @@ -281,7 +281,7 @@ TEST_F(WebContentsTest, CrossSiteBoundaries) { // Going back should switch SiteInstances again. The first SiteInstance is // stored in the NavigationEntry, so it should be the same as at the start. - controller().GoBack(); + controller()->GoBack(); TestRenderViewHost* goback_rvh = contents()->pending_rvh(); EXPECT_TRUE(contents()->cross_navigation_pending()); @@ -304,7 +304,7 @@ TEST_F(WebContentsTest, CrossSiteBoundariesAfterCrash) { // Navigate to URL. First URL should use first RenderViewHost. const GURL url("http://www.google.com"); - controller().LoadURL(url, GURL(), PageTransition::TYPED); + controller()->LoadURL(url, GURL(), PageTransition::TYPED); ViewHostMsg_FrameNavigate_Params params1; InitNavigateParams(¶ms1, 1, url); contents()->TestDidNavigate(orig_rvh, params1); @@ -317,7 +317,7 @@ TEST_F(WebContentsTest, CrossSiteBoundariesAfterCrash) { // Navigate to new site. We should not go into PENDING. const GURL url2("http://www.yahoo.com"); - controller().LoadURL(url2, GURL(), PageTransition::TYPED); + controller()->LoadURL(url2, GURL(), PageTransition::TYPED); TestRenderViewHost* new_rvh = rvh(); EXPECT_FALSE(contents()->cross_navigation_pending()); EXPECT_TRUE(contents()->pending_rvh() == NULL); @@ -345,23 +345,24 @@ TEST_F(WebContentsTest, NavigateTwoTabsCrossSite) { // Navigate to URL. First URL should use first RenderViewHost. const GURL url("http://www.google.com"); - controller().LoadURL(url, GURL(), PageTransition::TYPED); + controller()->LoadURL(url, GURL(), PageTransition::TYPED); ViewHostMsg_FrameNavigate_Params params1; InitNavigateParams(¶ms1, 1, url); contents()->TestDidNavigate(orig_rvh, params1); // Open a new tab with the same SiteInstance, navigated to the same site. - TestWebContents contents2(profile(), instance1); + TestWebContents* contents2 = new TestWebContents(profile(), instance1); params1.page_id = 2; // Need this since the site instance is the same (which // is the scope of page IDs) and we want to consider // this a new page. - contents2.transition_cross_site = true; - contents2.controller().LoadURL(url, GURL(), PageTransition::TYPED); - contents2.TestDidNavigate(contents2.render_view_host(), params1); + contents2->transition_cross_site = true; + contents2->SetupController(profile()); + contents2->controller()->LoadURL(url, GURL(), PageTransition::TYPED); + contents2->TestDidNavigate(contents2->render_view_host(), params1); // Navigate first tab to a new site const GURL url2a("http://www.yahoo.com"); - controller().LoadURL(url2a, GURL(), PageTransition::TYPED); + controller()->LoadURL(url2a, GURL(), PageTransition::TYPED); TestRenderViewHost* pending_rvh_a = contents()->pending_rvh(); ViewHostMsg_FrameNavigate_Params params2a; InitNavigateParams(¶ms2a, 1, url2a); @@ -371,10 +372,10 @@ TEST_F(WebContentsTest, NavigateTwoTabsCrossSite) { // Navigate second tab to the same site as the first tab const GURL url2b("http://mail.yahoo.com"); - contents2.controller().LoadURL(url2b, GURL(), PageTransition::TYPED); - TestRenderViewHost* pending_rvh_b = contents2.pending_rvh(); + contents2->controller()->LoadURL(url2b, GURL(), PageTransition::TYPED); + TestRenderViewHost* pending_rvh_b = contents2->pending_rvh(); EXPECT_TRUE(pending_rvh_b != NULL); - EXPECT_TRUE(contents2.cross_navigation_pending()); + EXPECT_TRUE(contents2->cross_navigation_pending()); // NOTE(creis): We used to be in danger of showing a sad tab page here if the // second tab hadn't navigated somewhere first (bug 1145430). That case is @@ -382,12 +383,14 @@ TEST_F(WebContentsTest, NavigateTwoTabsCrossSite) { ViewHostMsg_FrameNavigate_Params params2b; InitNavigateParams(¶ms2b, 2, url2b); - contents2.TestDidNavigate(pending_rvh_b, params2b); - SiteInstance* instance2b = contents2.GetSiteInstance(); + contents2->TestDidNavigate(pending_rvh_b, params2b); + SiteInstance* instance2b = contents2->GetSiteInstance(); EXPECT_NE(instance1, instance2b); // Both tabs should now be in the same SiteInstance. EXPECT_EQ(instance2a, instance2b); + + contents2->CloseContents(); } // Tests that WebContents uses the current URL, not the SiteInstance's site, to @@ -399,27 +402,28 @@ TEST_F(WebContentsTest, CrossSiteComparesAgainstCurrentPage) { // Navigate to URL. const GURL url("http://www.google.com"); - controller().LoadURL(url, GURL(), PageTransition::TYPED); + controller()->LoadURL(url, GURL(), PageTransition::TYPED); ViewHostMsg_FrameNavigate_Params params1; InitNavigateParams(¶ms1, 1, url); contents()->TestDidNavigate(orig_rvh, params1); // Open a related tab to a second site. - TestWebContents contents2(profile(), instance1); - contents2.transition_cross_site = true; + TestWebContents* contents2 = new TestWebContents(profile(), instance1); + contents2->transition_cross_site = true; + contents2->SetupController(profile()); const GURL url2("http://www.yahoo.com"); - contents2.controller().LoadURL(url2, GURL(), PageTransition::TYPED); + contents2->controller()->LoadURL(url2, GURL(), PageTransition::TYPED); // The first RVH in contents2 isn't live yet, so we shortcut the cross site // pending. TestRenderViewHost* rvh2 = static_cast<TestRenderViewHost*>( - contents2.render_view_host()); - EXPECT_FALSE(contents2.cross_navigation_pending()); + contents2->render_view_host()); + EXPECT_FALSE(contents2->cross_navigation_pending()); ViewHostMsg_FrameNavigate_Params params2; InitNavigateParams(¶ms2, 2, url2); - contents2.TestDidNavigate(rvh2, params2); - SiteInstance* instance2 = contents2.GetSiteInstance(); + contents2->TestDidNavigate(rvh2, params2); + SiteInstance* instance2 = contents2->GetSiteInstance(); EXPECT_NE(instance1, instance2); - EXPECT_FALSE(contents2.cross_navigation_pending()); + EXPECT_FALSE(contents2->cross_navigation_pending()); // Simulate a link click in first tab to second site. Doesn't switch // SiteInstances, because we don't intercept WebKit navigations. @@ -433,13 +437,15 @@ TEST_F(WebContentsTest, CrossSiteComparesAgainstCurrentPage) { // Navigate to the new site. Doesn't switch SiteInstancees, because we // compare against the current URL, not the SiteInstance's site. const GURL url3("http://mail.yahoo.com"); - controller().LoadURL(url3, GURL(), PageTransition::TYPED); + controller()->LoadURL(url3, GURL(), PageTransition::TYPED); EXPECT_FALSE(contents()->cross_navigation_pending()); ViewHostMsg_FrameNavigate_Params params4; InitNavigateParams(¶ms4, 3, url3); contents()->TestDidNavigate(orig_rvh, params4); SiteInstance* instance4 = contents()->GetSiteInstance(); EXPECT_EQ(instance1, instance4); + + contents2->CloseContents(); } // Test that the onbeforeunload and onunload handlers run when navigating @@ -451,7 +457,7 @@ TEST_F(WebContentsTest, CrossSiteUnloadHandlers) { // Navigate to URL. First URL should use first RenderViewHost. const GURL url("http://www.google.com"); - controller().LoadURL(url, GURL(), PageTransition::TYPED); + controller()->LoadURL(url, GURL(), PageTransition::TYPED); ViewHostMsg_FrameNavigate_Params params1; InitNavigateParams(¶ms1, 1, url); contents()->TestDidNavigate(orig_rvh, params1); @@ -460,13 +466,13 @@ TEST_F(WebContentsTest, CrossSiteUnloadHandlers) { // Navigate to new site, but simulate an onbeforeunload denial. const GURL url2("http://www.yahoo.com"); - controller().LoadURL(url2, GURL(), PageTransition::TYPED); + controller()->LoadURL(url2, GURL(), PageTransition::TYPED); orig_rvh->TestOnMessageReceived(ViewHostMsg_ShouldClose_ACK(0, false)); EXPECT_FALSE(contents()->cross_navigation_pending()); EXPECT_EQ(orig_rvh, contents()->render_view_host()); // Navigate again, but simulate an onbeforeunload approval. - controller().LoadURL(url2, GURL(), PageTransition::TYPED); + controller()->LoadURL(url2, GURL(), PageTransition::TYPED); orig_rvh->TestOnMessageReceived(ViewHostMsg_ShouldClose_ACK(0, true)); EXPECT_TRUE(contents()->cross_navigation_pending()); TestRenderViewHost* pending_rvh = static_cast<TestRenderViewHost*>( @@ -494,34 +500,34 @@ TEST_F(WebContentsTest, NavigationEntryContentState) { // Navigate to URL. There should be no committed entry yet. const GURL url("http://www.google.com"); - controller().LoadURL(url, GURL(), PageTransition::TYPED); - NavigationEntry* entry = controller().GetLastCommittedEntry(); + controller()->LoadURL(url, GURL(), PageTransition::TYPED); + NavigationEntry* entry = controller()->GetLastCommittedEntry(); EXPECT_TRUE(entry == NULL); // Committed entry should have content state after DidNavigate. ViewHostMsg_FrameNavigate_Params params1; InitNavigateParams(¶ms1, 1, url); contents()->TestDidNavigate(orig_rvh, params1); - entry = controller().GetLastCommittedEntry(); + entry = controller()->GetLastCommittedEntry(); EXPECT_FALSE(entry->content_state().empty()); // Navigate to same site. const GURL url2("http://images.google.com"); - controller().LoadURL(url2, GURL(), PageTransition::TYPED); - entry = controller().GetLastCommittedEntry(); + controller()->LoadURL(url2, GURL(), PageTransition::TYPED); + entry = controller()->GetLastCommittedEntry(); EXPECT_FALSE(entry->content_state().empty()); // Committed entry should have content state after DidNavigate. ViewHostMsg_FrameNavigate_Params params2; InitNavigateParams(¶ms2, 2, url2); contents()->TestDidNavigate(orig_rvh, params2); - entry = controller().GetLastCommittedEntry(); + entry = controller()->GetLastCommittedEntry(); EXPECT_FALSE(entry->content_state().empty()); // Now go back. Committed entry should still have content state. - controller().GoBack(); + controller()->GoBack(); contents()->TestDidNavigate(orig_rvh, params1); - entry = controller().GetLastCommittedEntry(); + entry = controller()->GetLastCommittedEntry(); EXPECT_FALSE(entry->content_state().empty()); } @@ -539,7 +545,7 @@ TEST_F(WebContentsTest, NavigationEntryContentStateNewWindow) { contents()->TestDidNavigate(orig_rvh, params1); // Should have a content state here. - NavigationEntry* entry = controller().GetLastCommittedEntry(); + NavigationEntry* entry = controller()->GetLastCommittedEntry(); EXPECT_FALSE(entry->content_state().empty()); } @@ -571,10 +577,10 @@ TEST_F(WebContentsTest, // Navigate to a page. GURL url1("http://www.google.com"); rvh()->SendNavigate(1, url1); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); // Initiate a browser navigation that will trigger the interstitial - controller().LoadURL(GURL("http://www.evil.com"), GURL(), + controller()->LoadURL(GURL("http://www.evil.com"), GURL(), PageTransition::TYPED); // Show an interstitial. @@ -595,7 +601,7 @@ TEST_F(WebContentsTest, EXPECT_TRUE(interstitial->is_showing()); EXPECT_TRUE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == interstitial); - NavigationEntry* entry = controller().GetActiveEntry(); + NavigationEntry* entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); EXPECT_TRUE(entry->url() == url2); @@ -605,10 +611,10 @@ TEST_F(WebContentsTest, EXPECT_EQ(TestInterstitialPage::CANCELED, state); EXPECT_FALSE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == NULL); - entry = controller().GetActiveEntry(); + entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); EXPECT_TRUE(entry->url() == url1); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); } // Test navigating to a page (with the navigation initiated from the renderer, @@ -619,7 +625,7 @@ TEST_F(WebContentsTest, // Navigate to a page. GURL url1("http://www.google.com"); rvh()->SendNavigate(1, url1); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); // Show an interstitial (no pending entry, the interstitial would have been // triggered by clicking on a link). @@ -640,7 +646,7 @@ TEST_F(WebContentsTest, EXPECT_TRUE(interstitial->is_showing()); EXPECT_TRUE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == interstitial); - NavigationEntry* entry = controller().GetActiveEntry(); + NavigationEntry* entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); EXPECT_TRUE(entry->url() == url2); @@ -650,10 +656,10 @@ TEST_F(WebContentsTest, EXPECT_EQ(TestInterstitialPage::CANCELED, state); EXPECT_FALSE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == NULL); - entry = controller().GetActiveEntry(); + entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); EXPECT_TRUE(entry->url() == url1); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); } // Test navigating to a page that shows an interstitial without creating a new @@ -663,7 +669,7 @@ TEST_F(WebContentsTest, ShowInterstitialNoNewNavigationDontProceed) { // Navigate to a page. GURL url1("http://www.google.com"); rvh()->SendNavigate(1, url1); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); // Show an interstitial. TestInterstitialPage::InterstitialState state = @@ -683,7 +689,7 @@ TEST_F(WebContentsTest, ShowInterstitialNoNewNavigationDontProceed) { EXPECT_TRUE(interstitial->is_showing()); EXPECT_TRUE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == interstitial); - NavigationEntry* entry = controller().GetActiveEntry(); + NavigationEntry* entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); // The URL specified to the interstitial should have been ignored. EXPECT_TRUE(entry->url() == url1); @@ -694,10 +700,10 @@ TEST_F(WebContentsTest, ShowInterstitialNoNewNavigationDontProceed) { EXPECT_EQ(TestInterstitialPage::CANCELED, state); EXPECT_FALSE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == NULL); - entry = controller().GetActiveEntry(); + entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); EXPECT_TRUE(entry->url() == url1); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); } // Test navigating to a page (with the navigation initiated from the browser, @@ -708,10 +714,10 @@ TEST_F(WebContentsTest, // Navigate to a page. GURL url1("http://www.google.com"); rvh()->SendNavigate(1, url1); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); // Initiate a browser navigation that will trigger the interstitial - controller().LoadURL(GURL("http://www.evil.com"), GURL(), + controller()->LoadURL(GURL("http://www.evil.com"), GURL(), PageTransition::TYPED); // Show an interstitial. @@ -732,7 +738,7 @@ TEST_F(WebContentsTest, EXPECT_TRUE(interstitial->is_showing()); EXPECT_TRUE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == interstitial); - NavigationEntry* entry = controller().GetActiveEntry(); + NavigationEntry* entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); EXPECT_TRUE(entry->url() == url2); @@ -752,11 +758,11 @@ TEST_F(WebContentsTest, EXPECT_TRUE(deleted); EXPECT_FALSE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == NULL); - entry = controller().GetActiveEntry(); + entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); EXPECT_TRUE(entry->url() == url3); - EXPECT_EQ(2, controller().entry_count()); + EXPECT_EQ(2, controller()->entry_count()); } // Test navigating to a page (with the navigation initiated from the renderer, @@ -767,7 +773,7 @@ TEST_F(WebContentsTest, // Navigate to a page. GURL url1("http://www.google.com"); rvh()->SendNavigate(1, url1); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); // Show an interstitial. TestInterstitialPage::InterstitialState state = @@ -787,7 +793,7 @@ TEST_F(WebContentsTest, EXPECT_TRUE(interstitial->is_showing()); EXPECT_TRUE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == interstitial); - NavigationEntry* entry = controller().GetActiveEntry(); + NavigationEntry* entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); EXPECT_TRUE(entry->url() == url2); @@ -807,11 +813,11 @@ TEST_F(WebContentsTest, EXPECT_TRUE(deleted); EXPECT_FALSE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == NULL); - entry = controller().GetActiveEntry(); + entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); EXPECT_TRUE(entry->url() == url3); - EXPECT_EQ(2, controller().entry_count()); + EXPECT_EQ(2, controller()->entry_count()); } // Test navigating to a page that shows an interstitial without creating a new @@ -821,7 +827,7 @@ TEST_F(WebContentsTest, ShowInterstitialNoNewNavigationProceed) { // Navigate to a page so we have a navigation entry in the controller. GURL url1("http://www.google.com"); rvh()->SendNavigate(1, url1); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); // Show an interstitial. TestInterstitialPage::InterstitialState state = @@ -841,7 +847,7 @@ TEST_F(WebContentsTest, ShowInterstitialNoNewNavigationProceed) { EXPECT_TRUE(interstitial->is_showing()); EXPECT_TRUE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == interstitial); - NavigationEntry* entry = controller().GetActiveEntry(); + NavigationEntry* entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); // The URL specified to the interstitial should have been ignored. EXPECT_TRUE(entry->url() == url1); @@ -854,11 +860,11 @@ TEST_F(WebContentsTest, ShowInterstitialNoNewNavigationProceed) { EXPECT_EQ(TestInterstitialPage::OKED, state); EXPECT_FALSE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == NULL); - entry = controller().GetActiveEntry(); + entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); EXPECT_TRUE(entry->url() == url1); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); } // Test navigating to a page that shows an interstitial, then navigating away. @@ -896,7 +902,7 @@ TEST_F(WebContentsTest, ShowInterstitialThenCloseTab) { interstitial->TestDidNavigate(1, url); // Now close the tab. - delete contents(); + contents()->CloseContents(); ContentsCleanedUp(); EXPECT_TRUE(deleted); EXPECT_EQ(TestInterstitialPage::CANCELED, state); @@ -908,7 +914,7 @@ TEST_F(WebContentsTest, ShowInterstitialProceedMultipleCommands) { // Navigate to a page so we have a navigation entry in the controller. GURL url1("http://www.google.com"); rvh()->SendNavigate(1, url1); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); // Show an interstitial. TestInterstitialPage::InterstitialState state = @@ -942,7 +948,7 @@ TEST_F(WebContentsTest, ShowInterstitialOnInterstitial) { // Navigate to a page so we have a navigation entry in the controller. GURL start_url("http://www.google.com"); rvh()->SendNavigate(1, start_url); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); // Show an interstitial. TestInterstitialPage::InterstitialState state1 = @@ -980,10 +986,10 @@ TEST_F(WebContentsTest, ShowInterstitialOnInterstitial) { EXPECT_TRUE(deleted2); EXPECT_FALSE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == NULL); - NavigationEntry* entry = controller().GetActiveEntry(); + NavigationEntry* entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); EXPECT_TRUE(entry->url() == landing_url); - EXPECT_EQ(2, controller().entry_count()); + EXPECT_EQ(2, controller()->entry_count()); } // Test showing an interstitial, proceeding and then navigating to another @@ -992,7 +998,7 @@ TEST_F(WebContentsTest, ShowInterstitialProceedShowInterstitial) { // Navigate to a page so we have a navigation entry in the controller. GURL start_url("http://www.google.com"); rvh()->SendNavigate(1, start_url); - EXPECT_EQ(1, controller().entry_count()); + EXPECT_EQ(1, controller()->entry_count()); // Show an interstitial. TestInterstitialPage::InterstitialState state1 = @@ -1035,10 +1041,10 @@ TEST_F(WebContentsTest, ShowInterstitialProceedShowInterstitial) { EXPECT_TRUE(deleted2); EXPECT_FALSE(contents()->showing_interstitial_page()); EXPECT_TRUE(contents()->interstitial_page() == NULL); - NavigationEntry* entry = controller().GetActiveEntry(); + NavigationEntry* entry = controller()->GetActiveEntry(); ASSERT_TRUE(entry != NULL); EXPECT_TRUE(entry->url() == landing_url); - EXPECT_EQ(2, controller().entry_count()); + EXPECT_EQ(2, controller()->entry_count()); } // Test that navigating away from an interstitial while it's loading cause it @@ -1058,7 +1064,7 @@ TEST_F(WebContentsTest, NavigateBeforeInterstitialShows) { // Let's simulate a navigation initiated from the browser before the // interstitial finishes loading. const GURL url("http://www.google.com"); - controller().LoadURL(url, GURL(), PageTransition::TYPED); + controller()->LoadURL(url, GURL(), PageTransition::TYPED); ASSERT_FALSE(deleted); EXPECT_FALSE(interstitial->is_showing()); diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc index 0470efa..2fc7e51 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,8 +403,10 @@ bool TabStripModel::IsContextMenuCommandEnabled( case CommandCloseTabsToRight: return context_index < (count() - 1); case CommandCloseTabsOpenedBy: { - int next_index = GetIndexOfNextTabContentsOpenedBy( - &GetTabContentsAt(context_index)->controller(), context_index, true); + NavigationController* opener = + GetTabContentsAt(context_index)->controller(); + int next_index = + GetIndexOfNextTabContentsOpenedBy(opener, context_index, true); return next_index != kNoTab; } case CommandDuplicate: @@ -427,7 +429,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_); @@ -455,7 +457,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)) @@ -476,7 +478,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); @@ -509,7 +511,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, @@ -532,9 +534,9 @@ bool TabStripModel::InternalCloseTabContentsAt(int index, if (create_historical_tab) delegate_->CreateHistoricalTab(detached_contents); - // Deleting the TabContents will call back to us via NotificationObserver - // and detach it. - delete detached_contents; + detached_contents->CloseContents(); + // Closing the TabContents will later call back to us via + // NotificationObserver and detach it. } return true; } @@ -562,7 +564,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 1db0525..0c6326a 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 656cba1..6b1f3d0 100644 --- a/chrome/browser/tabs/tab_strip_model_unittest.cc +++ b/chrome/browser/tabs/tab_strip_model_unittest.cc @@ -68,21 +68,23 @@ class TabStripDummyDelegate : public TabStripModelDelegate { class TabStripModelTest : public RenderViewHostTestHarness { public: TabContents* CreateTabContents() { - return new WebContents(profile(), NULL, 0, NULL); + WebContents* con = new WebContents(profile(), NULL, 0, NULL); + con->SetupController(profile()); + return con; } // 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) { @@ -358,8 +360,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 @@ -412,7 +414,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(); @@ -445,7 +447,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)); @@ -511,7 +513,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 @@ -590,7 +592,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(); @@ -668,7 +670,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(); diff --git a/chrome/browser/task_manager_resource_providers.cc b/chrome/browser/task_manager_resource_providers.cc index 5364b4d..0a992dd 100644 --- a/chrome/browser/task_manager_resource_providers.cc +++ b/chrome/browser/task_manager_resource_providers.cc @@ -40,6 +40,11 @@ TaskManagerWebContentsResource::~TaskManagerWebContentsResource() { } std::wstring TaskManagerWebContentsResource::GetTitle() const { + // GetTitle() and GetURL() can only be invoked when the WebContents has a + // controller. + if (!web_contents_->controller()) + return std::wstring(); + // Fall back on the URL if there's no title. std::wstring tab_title(UTF16ToWideHack(web_contents_->GetTitle())); if (tab_title.empty()) { diff --git a/chrome/browser/views/blocked_popup_container.cc b/chrome/browser/views/blocked_popup_container.cc index 3906d5b..ca5b996 100644 --- a/chrome/browser/views/blocked_popup_container.cc +++ b/chrome/browser/views/blocked_popup_container.cc @@ -254,12 +254,12 @@ void BlockedPopupContainer::AddTabContents(TabContents* blocked_contents, const gfx::Rect& bounds) { if (has_been_dismissed_) { // We simply bounce this popup without notice. - delete blocked_contents; + blocked_contents->CloseContents(); return; } if (blocked_popups_.size() > kImpossibleNumberOfPopups) { - delete blocked_contents; + blocked_contents->CloseContents(); LOG(INFO) << "Warning: Renderer is sending more popups to us then should be" " possible. Renderer compromised?"; return; @@ -477,7 +477,7 @@ void BlockedPopupContainer::SetPosition() { void BlockedPopupContainer::CloseEachTabContents() { while (!blocked_popups_.empty()) { blocked_popups_.back().first->set_delegate(NULL); - delete blocked_popups_.back().first; + blocked_popups_.back().first->CloseContents(); blocked_popups_.pop_back(); } diff --git a/chrome/browser/views/blocked_popup_container.h b/chrome/browser/views/blocked_popup_container.h index c634308..e469da6 100644 --- a/chrome/browser/views/blocked_popup_container.h +++ b/chrome/browser/views/blocked_popup_container.h @@ -220,7 +220,7 @@ class BlockedPopupContainer : public ConstrainedWindow, // change. void SetPosition(); - // Deletes each contents in |blocked_popups_|. + // Sends a CloseContents() to each message in |blocked_popups_|. void CloseEachTabContents(); // The TabContents that owns and constrains this BlockedPopupContainer. diff --git a/chrome/browser/views/bug_report_view.cc b/chrome/browser/views/bug_report_view.cc index 647dfc1..1144065 100644 --- a/chrome/browser/views/bug_report_view.cc +++ b/chrome/browser/views/bug_report_view.cc @@ -482,7 +482,7 @@ void BugReportView::SendReport() { } void BugReportView::ReportPhishing() { - tab_->controller().LoadURL( + tab_->controller()->LoadURL( safe_browsing_util::GeneratePhishingReportUrl( kReportPhishingUrl, WideToUTF8(page_url_text_->GetText())), GURL(), diff --git a/chrome/browser/views/dom_view.cc b/chrome/browser/views/dom_view.cc index ffa9c1d..3e1c777 100644 --- a/chrome/browser/views/dom_view.cc +++ b/chrome/browser/views/dom_view.cc @@ -11,8 +11,11 @@ DOMView::DOMView() : initialized_(false), web_contents_(NULL) { } DOMView::~DOMView() { - if (web_contents_.get()) + if (web_contents_) { Detach(); + web_contents_->Destroy(); + web_contents_ = NULL; + } } bool DOMView::Init(Profile* profile, SiteInstance* instance) { @@ -20,13 +23,13 @@ bool DOMView::Init(Profile* profile, SiteInstance* instance) { return true; initialized_ = true; - web_contents_.reset(new WebContents(profile, instance, - MSG_ROUTING_NONE, NULL)); + web_contents_ = new WebContents(profile, instance, MSG_ROUTING_NONE, NULL); views::HWNDView::Attach(web_contents_->GetNativeView()); + web_contents_->SetupController(profile); return true; } void DOMView::LoadURL(const GURL& url) { DCHECK(initialized_); - web_contents_->controller().LoadURL(url, GURL(), PageTransition::START_PAGE); + web_contents_->controller()->LoadURL(url, GURL(), PageTransition::START_PAGE); } diff --git a/chrome/browser/views/dom_view.h b/chrome/browser/views/dom_view.h index 132a444..bfa6d13 100644 --- a/chrome/browser/views/dom_view.h +++ b/chrome/browser/views/dom_view.h @@ -8,7 +8,6 @@ #ifndef CHROME_BROWSER_VIEWS_DOM_VIEW_H_ #define CHROME_BROWSER_VIEWS_DOM_VIEW_H_ -#include "base/scoped_ptr.h" #include "chrome/views/controls/hwnd_view.h" #include "googleurl/src/gurl.h" @@ -35,7 +34,7 @@ class DOMView : public views::HWNDView { protected: virtual bool CanProcessTabKeyEvents() { return true; } - scoped_ptr<WebContents> web_contents_; + WebContents* web_contents_; private: bool initialized_; diff --git a/chrome/browser/views/frame/browser_view.cc b/chrome/browser/views/frame/browser_view.cc index 2cd0823..2505ff8 100644 --- a/chrome/browser/views/frame/browser_view.cc +++ b/chrome/browser/views/frame/browser_view.cc @@ -769,10 +769,10 @@ void BrowserView::ShowReportBugDialog() { BugReportView* bug_report_view = new BugReportView(browser_->profile(), current_tab); - if (current_tab->controller().GetLastCommittedEntry()) { + if (current_tab->controller()->GetLastCommittedEntry()) { // URL for the current page bug_report_view->SetUrl( - current_tab->controller().GetActiveEntry()->url()); + current_tab->controller()->GetActiveEntry()->url()); } // retrieve the application version info diff --git a/chrome/browser/views/location_bar_view.cc b/chrome/browser/views/location_bar_view.cc index 59bce3b..c1b96b9 100644 --- a/chrome/browser/views/location_bar_view.cc +++ b/chrome/browser/views/location_bar_view.cc @@ -989,7 +989,7 @@ bool LocationBarView::SecurityImageView::OnMousePressed( const views::MouseEvent& event) { NavigationEntry* nav_entry = BrowserList::GetLastActive()->GetSelectedTabContents()-> - controller().GetActiveEntry(); + controller()->GetActiveEntry(); if (!nav_entry) { NOTREACHED(); return true; @@ -1031,7 +1031,7 @@ bool LocationBarView::RssImageView::OnMousePressed( const views::MouseEvent& event) { NavigationEntry* entry = BrowserList::GetLastActive()->GetSelectedTabContents()-> - controller().GetActiveEntry(); + controller()->GetActiveEntry(); if (!entry) { NOTREACHED(); return true; diff --git a/chrome/browser/views/tab_contents_container_view.cc b/chrome/browser/views/tab_contents_container_view.cc index 04c9354..4b721da 100644 --- a/chrome/browser/views/tab_contents_container_view.cc +++ b/chrome/browser/views/tab_contents_container_view.cc @@ -181,7 +181,7 @@ void TabContentsContainerView::AddObservers() { // the focus subclass on the shown HWND so we intercept focus change events. NotificationService::current()->AddObserver( this, NotificationType::RENDER_VIEW_HOST_CHANGED, - Source<NavigationController>(&tab_contents_->controller())); + Source<NavigationController>(tab_contents_->controller())); } NotificationService::current()->AddObserver( this, @@ -195,7 +195,7 @@ void TabContentsContainerView::RemoveObservers() { NotificationService::current()->RemoveObserver( this, NotificationType::RENDER_VIEW_HOST_CHANGED, - Source<NavigationController>(&tab_contents_->controller())); + Source<NavigationController>(tab_contents_->controller())); } NotificationService::current()->RemoveObserver( this, diff --git a/chrome/browser/views/tabs/tab_strip.cc b/chrome/browser/views/tabs/tab_strip.cc index 5328927..51baf95 100644 --- a/chrome/browser/views/tabs/tab_strip.cc +++ b/chrome/browser/views/tabs/tab_strip.cc @@ -703,8 +703,8 @@ int TabStrip::OnPerformDrop(const DropTargetEvent& event) { } else { UserMetrics::RecordAction(L"Tab_DropURLOnTab", model_->profile()); - model_->GetTabContentsAt(drop_index)->controller().LoadURL( - url, GURL(), PageTransition::GENERATED); + model_->GetTabContentsAt(drop_index)->controller()-> + LoadURL(url, GURL(), PageTransition::GENERATED); model_->SelectTabContentsAt(drop_index, true); } diff --git a/chrome/test/browser_with_test_window_test.cc b/chrome/test/browser_with_test_window_test.cc index b0d6a84..1881a54 100644 --- a/chrome/test/browser_with_test_window_test.cc +++ b/chrome/test/browser_with_test_window_test.cc @@ -51,7 +51,7 @@ void BrowserWithTestWindowTest::AddTab(Browser* browser, const GURL& url) { TabContents* new_tab = browser->AddTabWithURL(url, GURL(), PageTransition::TYPED, true, 0, NULL); - CommitPendingLoadAsNewNavigation(&new_tab->controller(), url); + CommitPendingLoadAsNewNavigation(new_tab->controller(), url); } void BrowserWithTestWindowTest::CommitPendingLoadAsNewNavigation( @@ -73,5 +73,5 @@ void BrowserWithTestWindowTest::NavigateAndCommit( } void BrowserWithTestWindowTest::NavigateAndCommitActiveTab(const GURL& url) { - NavigateAndCommit(&browser()->GetSelectedTabContents()->controller(), url); + NavigateAndCommit(browser()->GetSelectedTabContents()->controller(), url); } diff --git a/chrome/test/in_process_browser_test.cc b/chrome/test/in_process_browser_test.cc index 25cc6ee..47e0e81 100644 --- a/chrome/test/in_process_browser_test.cc +++ b/chrome/test/in_process_browser_test.cc @@ -175,7 +175,7 @@ Browser* InProcessBrowserTest::CreateBrowser(Profile* profile) { // Wait for the page to finish loading. ui_test_utils::WaitForNavigation( - &browser->GetSelectedTabContents()->controller()); + browser->GetSelectedTabContents()->controller()); browser->window()->Show(); diff --git a/chrome/test/ui_test_utils.cc b/chrome/test/ui_test_utils.cc index 2d91bcc..be86d17 100644 --- a/chrome/test/ui_test_utils.cc +++ b/chrome/test/ui_test_utils.cc @@ -93,7 +93,7 @@ void NavigateToURLBlockUntilNavigationsComplete(Browser* browser, const GURL& url, int number_of_navigations) { NavigationController* controller = - &browser->GetSelectedTabContents()->controller(); + browser->GetSelectedTabContents()->controller(); browser->OpenURLFromTab(browser->GetSelectedTabContents(), url, GURL(), CURRENT_TAB, PageTransition::TYPED); WaitForNavigations(controller, number_of_navigations); |