diff options
author | brettw@chromium.org <brettw@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-04-20 19:55:57 +0000 |
---|---|---|
committer | brettw@chromium.org <brettw@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-04-20 19:55:57 +0000 |
commit | ce3fa3c831cba1f44d34c1c66020f830be33a068 (patch) | |
tree | da18dbdfb081c17949dca2dcfa0633142a63a868 /chrome/browser | |
parent | 6ee6368c466235c5919c9b4d5cfe11f8e48b29ca (diff) | |
download | chromium_src-ce3fa3c831cba1f44d34c1c66020f830be33a068.zip chromium_src-ce3fa3c831cba1f44d34c1c66020f830be33a068.tar.gz chromium_src-ce3fa3c831cba1f44d34c1c66020f830be33a068.tar.bz2 |
Re-land my change to clean up TabContents/WebContents ownership. This
is the same except in tab_strip_model_unittest I fixed a leak by making a
WebContents on the stack, I added a factory to the SiteInstance unittest to
prevent another leak, and I re-added a NULL set to the external_tab_container.
Fix the ownership model of TabContents and NavigationController. Previously the
NavigationController owned the TabContents, and there were extra steps required
at creation and destruction to clean everything up properly.
NavigationController is now a member of TabContents, and there is no setup or
tear down necessary other than the constructor and destructor. I could remove
the tab contents creation in the NavigationController, as well as all the
weird destruction code in WebContents which got moved to the destructor.
I made the controller getter return a reference since the ownership is clear
and there is no possibility of NULL. This required changing a lot of tiles, but
many of them were simplified since they no longer have to NULL check.
Previous review URL: http://codereview.chromium.org/69043
Review URL: http://codereview.chromium.org/67294
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@14053 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser')
71 files changed, 1001 insertions, 1340 deletions
diff --git a/chrome/browser/app_modal_dialog.cc b/chrome/browser/app_modal_dialog.cc index 91cffdd..dac60e4 100644 --- a/chrome/browser/app_modal_dialog.cc +++ b/chrome/browser/app_modal_dialog.cc @@ -36,7 +36,8 @@ 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 ba4dfe4..1845fd7 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 380f70b..83b8e53 100644 --- a/chrome/browser/back_forward_menu_model.cc +++ b/chrome/browser/back_forward_menu_model.cc @@ -21,16 +21,14 @@ 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 = controller->entry_count() - - controller->GetCurrentEntryIndex() - 1; + items = contents->controller().entry_count() - + contents->controller().GetCurrentEntryIndex() - 1; } else { - items = controller->GetCurrentEntryIndex(); + items = contents->controller().GetCurrentEntryIndex(); } if (items > kMaxHistoryItems) @@ -43,10 +41,9 @@ 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 = controller->GetCurrentEntryIndex(); + int current_entry = contents->controller().GetCurrentEntryIndex(); if (history_items == kMaxHistoryItems) { int chapter_id = current_entry; @@ -91,9 +88,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. @@ -107,7 +104,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) { @@ -115,7 +112,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. @@ -125,7 +122,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. @@ -143,10 +140,7 @@ int BackForwardMenuModel::FindChapterStop(int offset, offset *= -1; TabContents* contents = GetTabContents(); - NavigationController* controller = contents->controller(); - - int entry = controller->GetCurrentEntryIndex() + offset; - + int entry = contents->controller().GetCurrentEntryIndex() + offset; for (int i = 0; i < skip + 1; i++) entry = GetIndexOfNextChapterStop(entry, forward); @@ -155,14 +149,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 @@ -174,16 +168,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 { @@ -217,7 +211,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 { @@ -249,8 +243,6 @@ 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); @@ -259,10 +251,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 += controller->GetCurrentEntryIndex(); + menu_id += contents->controller().GetCurrentEntryIndex(); } else { // Back menu is reverse. - menu_id = controller->GetCurrentEntryIndex() - menu_id; + menu_id = contents->controller().GetCurrentEntryIndex() - menu_id; } return menu_id; } @@ -281,11 +273,8 @@ 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 controller->GetEntryAtIndex(index); + return GetTabContents()->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 a171302..708847d 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 0a2ebad..a8501d9 100644 --- a/chrome/browser/browser.cc +++ b/chrome/browser/browser.cc @@ -536,6 +536,7 @@ 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(); @@ -543,35 +544,34 @@ TabContents* Browser::AddTabWithNavigationController( return tc; } -NavigationController* Browser::AddRestoredTab( +TabContents* Browser::AddRestoredTab( const std::vector<TabNavigation>& navigations, int tab_index, int selected_navigation, bool select) { - NavigationController* restored_controller = - BuildRestoredNavigationController(navigations, selected_navigation); + TabContents* new_tab = new WebContents(profile(), NULL, + MSG_ROUTING_NONE, NULL); + new_tab->controller().RestoreFromState(navigations, selected_navigation); - tabstrip_model_.InsertTabContentsAt( - tab_index, - restored_controller->tab_contents(), - select, false); + tabstrip_model_.InsertTabContentsAt(tab_index, new_tab, select, false); if (profile_->HasSessionService()) { SessionService* session_service = profile_->GetSessionService(); if (session_service) - session_service->TabRestored(restored_controller); + session_service->TabRestored(&new_tab->controller()); } - return restored_controller; + return new_tab; } void Browser::ReplaceRestoredTab( const std::vector<TabNavigation>& navigations, int selected_navigation) { - NavigationController* restored_controller = - BuildRestoredNavigationController(navigations, selected_navigation); + TabContents* replacement = new WebContents(profile(), NULL, + MSG_ROUTING_NONE, NULL); + replacement->controller().RestoreFromState(navigations, selected_navigation); tabstrip_model_.ReplaceNavigationControllerAt( tabstrip_model_.selected_index(), - restored_controller); + &replacement->controller()); } bool Browser::CanRestoreTab() { @@ -611,33 +611,34 @@ void Browser::GoBack(WindowOpenDisposition disposition) { return; } - if (current_tab->controller()->CanGoBack()) { - NavigationController* controller = 0; - if (disposition == NEW_FOREGROUND_TAB || disposition == NEW_BACKGROUND_TAB){ - controller = GetSelectedTabContents()->controller()->Clone(); + if (current_tab->controller().CanGoBack()) { + NavigationController* controller = NULL; + if (disposition == NEW_FOREGROUND_TAB || + disposition == NEW_BACKGROUND_TAB) { tabstrip_model_.AddTabContents( - controller->tab_contents(), -1, + GetSelectedTabContents()->Clone(), -1, PageTransition::LINK, disposition == NEW_FOREGROUND_TAB); } else { // Default disposition is CURRENT_TAB. - controller = current_tab->controller(); + controller = ¤t_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( - controller->tab_contents(), -1, + GetSelectedTabContents()->Clone(), -1, PageTransition::LINK, disp == NEW_FOREGROUND_TAB); } else { // Default disposition is CURRENT_TAB. - controller = GetSelectedTabContents()->controller(); + controller = &GetSelectedTabContents()->controller(); } controller->GoForward(); } @@ -651,7 +652,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; @@ -661,7 +662,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); } } @@ -797,7 +798,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(); @@ -822,7 +823,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); @@ -1390,23 +1391,19 @@ 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) { - TabContents* contents = GetTabContentsAt(index); - DCHECK(contents); - - NavigationController* nc = contents->controller(); - return nc ? (nc->tab_contents() && nc->GetLastCommittedEntry()) : false; + NavigationController& nc = GetTabContentsAt(index)->controller(); + return nc.tab_contents() && nc.GetLastCommittedEntry(); } void Browser::DuplicateContentsAt(int index) { @@ -1417,7 +1414,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->controller()->Clone()->tab_contents(); + new_contents = contents->Clone(); // 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 @@ -1446,14 +1443,14 @@ void Browser::DuplicateContentsAt(int index) { // The page transition below is only for the purpose of inserting the tab. new_contents = browser->AddTabWithNavigationController( - contents->controller()->Clone(), + &contents->Clone()->controller(), 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()); } } @@ -1479,7 +1476,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()); } } @@ -1510,7 +1507,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)); @@ -1527,11 +1524,9 @@ 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>(controller), + Source<NavigationController>(&contents->controller()), NotificationService::NoDetails()); // Sever the TabContents' connection back to us. @@ -1704,15 +1699,8 @@ void Browser::OpenURLFromTab(TabContents* source, } else if ((disposition == CURRENT_TAB) && current_tab) { tabstrip_model_.TabNavigating(current_tab, transition); - // 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(); + current_tab->controller().LoadURL(url, referrer, transition); + new_contents = current_tab; if (GetStatusBubble()) GetStatusBubble()->Hide(); @@ -1886,7 +1874,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); @@ -1994,7 +1982,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; @@ -2137,9 +2125,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, @@ -2151,7 +2139,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. @@ -2245,7 +2233,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. @@ -2273,8 +2261,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; } @@ -2331,7 +2319,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()) { @@ -2382,13 +2370,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); } } } } -NavigationController* Browser::BuildRestoredNavigationController( +TabContents* Browser::BuildRestoredTab( const std::vector<TabNavigation>& navigations, int selected_navigation) { if (!navigations.empty()) { @@ -2396,13 +2384,14 @@ NavigationController* Browser::BuildRestoredNavigationController( selected_navigation < static_cast<int>(navigations.size())); // Create a NavigationController. This constructor creates the appropriate // set of TabContents. - return new NavigationController(profile_, navigations, selected_navigation); + TabContents* new_tab = new WebContents(profile_, NULL, + MSG_ROUTING_NONE, NULL); + new_tab->controller().RestoreFromState(navigations, selected_navigation); + return new_tab; } else { // No navigations. Create a tab with about:blank. - TabContents* contents = - CreateTabContentsForURL(GURL("about:blank"), GURL(), profile_, - PageTransition::START_PAGE, false, NULL); - return new NavigationController(contents, profile_); + return CreateTabContentsForURL(GURL("about:blank"), GURL(), profile_, + PageTransition::START_PAGE, false, NULL); } } diff --git a/chrome/browser/browser.h b/chrome/browser/browser.h index af0fb52..d0f2d6f 100644 --- a/chrome/browser/browser.h +++ b/chrome/browser/browser.h @@ -227,15 +227,13 @@ 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. 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); + // 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); // Replaces the state of the currently selected tab with the session // history restored from the SessionRestore system. @@ -525,11 +523,10 @@ class Browser : public TabStripModelDelegate, void SyncHistoryWithTabs(int index); // Called from AddRestoredTab and ReplaceRestoredTab to build a - // NavigationController from an incoming vector of TabNavigations. - // Caller takes ownership of the returned NavigationController. - NavigationController* BuildRestoredNavigationController( - const std::vector<TabNavigation>& navigations, - int selected_navigation); + // TabContents from an incoming vector of TabNavigations. + // Caller takes ownership of the returned TabContents. + TabContents* BuildRestoredTab(const std::vector<TabNavigation>& navigations, + int selected_navigation); // OnBeforeUnload handling ////////////////////////////////////////////////// @@ -639,7 +636,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 ? current_tab->controller() : NULL; + return current_tab ? ¤t_tab->controller() : NULL; } private: diff --git a/chrome/browser/browser_commands_unittest.cc b/chrome/browser/browser_commands_unittest.cc index f55cb8e..5cecf0a 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 e22a391..1f98f4b8 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 c22bd7f..0beedce 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 3a72171..0c9f6f8 100644 --- a/chrome/browser/debugger/debugger_view.cc +++ b/chrome/browser/debugger/debugger_view.cc @@ -99,14 +99,13 @@ 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() { @@ -115,7 +114,7 @@ void DebuggerView::OnShow() { void DebuggerView::OnClose() { web_container_->SetTabContents(NULL); - web_contents_->CloseContents(); + delete web_contents_; } void DebuggerView::OpenURLFromTab(TabContents* source, diff --git a/chrome/browser/debugger/devtools_manager.cc b/chrome/browser/debugger/devtools_manager.cc index f048689..6afa7a3 100644 --- a/chrome/browser/debugger/devtools_manager.cc +++ b/chrome/browser/debugger/devtools_manager.cc @@ -35,22 +35,18 @@ void DevToolsManager::Observe(NotificationType type, return; } - 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); - } + // Active tab contents disconnecting from its renderer means that the tab + // is closing. + client_host->InspectedTabClosing(); + UnregisterDevToolsClientHost(client_host, &src->controller()); } } DevToolsClientHost* DevToolsManager::GetDevToolsClientHostFor( const WebContents& web_contents) { - NavigationController* navigation_controller = web_contents.controller(); + const 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; } @@ -58,11 +54,11 @@ DevToolsClientHost* DevToolsManager::GetDevToolsClientHostFor( } void DevToolsManager::RegisterDevToolsClientHostFor( - const WebContents& web_contents, + 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 fe6fc4f..77ffaf5 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(const WebContents& web_contents, + void RegisterDevToolsClientHostFor(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 483f87f..179eb7e 100644 --- a/chrome/browser/debugger/devtools_view.cc +++ b/chrome/browser/debugger/devtools_view.cc @@ -50,7 +50,6 @@ 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(); @@ -59,8 +58,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() { @@ -70,7 +69,7 @@ void DevToolsView::OnWindowClosing() { web_container_->SetTabContents(NULL); // Destroy the tab and navigation controller. - web_contents_->CloseContents(); + delete web_contents_; web_contents_ = NULL; } } diff --git a/chrome/browser/debugger/inspectable_tab_proxy.cc b/chrome/browser/debugger/inspectable_tab_proxy.cc index e141bca..a329d6a 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 51b045a..e108c23 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,23 +88,19 @@ 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. - WebContents* contents2 = new TestWebContents(profile_.get(), NULL); - NavigationController* controller2 = - new NavigationController(contents2, profile_.get()); - contents2->set_controller(controller2); + TestWebContents contents2(profile_.get(), NULL); - DoNavigationTest(contents2, 101); - contents2->CloseContents(); + DoNavigationTest(&contents2, 101); } 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. @@ -117,7 +113,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()); @@ -134,7 +130,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 cd9b5d9..90bf3dc 100644 --- a/chrome/browser/dom_ui/new_tab_ui.cc +++ b/chrome/browser/dom_ui/new_tab_ui.cc @@ -890,9 +890,8 @@ RecentlyClosedTabsHandler::~RecentlyClosedTabsHandler() { } void RecentlyClosedTabsHandler::HandleReopenTab(const Value* content) { - NavigationController* controller = dom_ui_->web_contents()->controller(); Browser* browser = Browser::GetBrowserForController( - controller, NULL); + &dom_ui_->web_contents()->controller(), NULL); if (!browser) return; @@ -1053,9 +1052,7 @@ void HistoryHandler::HandleSearchHistoryPage(const Value* content) { #if defined(OS_WIN) // TODO(port): include this once history is converted to HTML - NavigationController* controller = - dom_ui_->web_contents()->controller(); - controller->LoadURL( + dom_ui_->web_contents()->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 98efb19..6078fce 100644 --- a/chrome/browser/download/download_request_manager.cc +++ b/chrome/browser/download/download_request_manager.cc @@ -177,7 +177,7 @@ DownloadRequestManager::~DownloadRequestManager() { DownloadRequestManager::DownloadStatus DownloadRequestManager::GetDownloadStatus(TabContents* tab) { - TabDownloadState* state = GetDownloadState(tab->controller(), NULL, false); + TabDownloadState* state = GetDownloadState(&tab->controller(), NULL, false); return state ? state->download_status() : ALLOW_ONE_DOWNLOAD; } @@ -193,13 +193,7 @@ void DownloadRequestManager::CanDownloadOnIOThread(int render_process_host_id, } void DownloadRequestManager::OnUserGesture(TabContents* tab) { - NavigationController* controller = tab->controller(); - if (!controller) { - NOTREACHED(); - return; - } - - TabDownloadState* state = GetDownloadState(controller, NULL, false); + TabDownloadState* state = GetDownloadState(&tab->controller(), NULL, false); if (!state) return; @@ -256,10 +250,8 @@ void DownloadRequestManager::CanDownloadImpl( effective_tab->delegate()->GetConstrainingContents(effective_tab); } - NavigationController* controller = effective_tab->controller(); - DCHECK(controller); TabDownloadState* state = GetDownloadState( - controller, originating_tab->controller(), true); + &effective_tab->controller(), &originating_tab->controller(), true); switch (state->download_status()) { case ALLOW_ALL_DOWNLOADS: ScheduleNotification(callback, true); diff --git a/chrome/browser/download/download_request_manager_unittest.cc b/chrome/browser/download/download_request_manager_unittest.cc index 943b439..a8ece7a 100644 --- a/chrome/browser/download/download_request_manager_unittest.cc +++ b/chrome/browser/download/download_request_manager_unittest.cc @@ -38,7 +38,7 @@ class DownloadRequestManagerTest void CanDownload() { download_request_manager_->CanDownloadImpl( - controller()->tab_contents(), this); + controller().tab_contents(), this); } bool ShouldAllowDownload() { @@ -81,13 +81,13 @@ TEST_F(DownloadRequestManagerTest, Allow) { // All tabs should initially start at ALLOW_ONE_DOWNLOAD. ASSERT_EQ(DownloadRequestManager::ALLOW_ONE_DOWNLOAD, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); // Ask if the tab can do a download. This moves to PROMPT_BEFORE_DOWNLOAD. CanDownload(); ASSERT_EQ(DownloadRequestManager::PROMPT_BEFORE_DOWNLOAD, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); // We should have been told we can download. ASSERT_EQ(1, continue_count_); ASSERT_EQ(0, cancel_count_); @@ -102,7 +102,7 @@ TEST_F(DownloadRequestManagerTest, Allow) { ask_allow_count_ = 0; ASSERT_EQ(DownloadRequestManager::ALLOW_ALL_DOWNLOADS, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); // We should have been told we can download. ASSERT_EQ(1, continue_count_); ASSERT_EQ(0, cancel_count_); @@ -114,7 +114,7 @@ TEST_F(DownloadRequestManagerTest, Allow) { ASSERT_EQ(0, ask_allow_count_); ASSERT_EQ(DownloadRequestManager::ALLOW_ALL_DOWNLOADS, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); // We should have been told we can download. ASSERT_EQ(1, continue_count_); ASSERT_EQ(0, cancel_count_); @@ -131,7 +131,7 @@ TEST_F(DownloadRequestManagerTest, ResetOnNavigation) { ask_allow_count_ = continue_count_ = cancel_count_ = 0; ASSERT_EQ(DownloadRequestManager::ALLOW_ALL_DOWNLOADS, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); // Navigate to a new URL with the same host, which shouldn't reset the allow // all state. @@ -143,20 +143,20 @@ TEST_F(DownloadRequestManagerTest, ResetOnNavigation) { ask_allow_count_ = continue_count_ = cancel_count_ = 0; ASSERT_EQ(DownloadRequestManager::ALLOW_ALL_DOWNLOADS, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); // Do a user gesture, because we're at allow all, this shouldn't change the // state. - download_request_manager_->OnUserGesture(controller()->tab_contents()); + download_request_manager_->OnUserGesture(controller().tab_contents()); ASSERT_EQ(DownloadRequestManager::ALLOW_ALL_DOWNLOADS, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); // Navigate to a completely different host, which should reset the state. NavigateAndCommit(GURL("http://fooey.com")); ASSERT_EQ(DownloadRequestManager::ALLOW_ONE_DOWNLOAD, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); } TEST_F(DownloadRequestManagerTest, ResetOnUserGesture) { @@ -167,13 +167,13 @@ TEST_F(DownloadRequestManagerTest, ResetOnUserGesture) { ask_allow_count_ = continue_count_ = cancel_count_ = 0; ASSERT_EQ(DownloadRequestManager::PROMPT_BEFORE_DOWNLOAD, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); // Do a user gesture, which should reset back to allow one. - download_request_manager_->OnUserGesture(controller()->tab_contents()); + download_request_manager_->OnUserGesture(controller().tab_contents()); ASSERT_EQ(DownloadRequestManager::ALLOW_ONE_DOWNLOAD, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); // Ask twice, which triggers calling the delegate. Don't allow the download // so that we end up with not allowed. @@ -182,13 +182,13 @@ TEST_F(DownloadRequestManagerTest, ResetOnUserGesture) { CanDownload(); ASSERT_EQ(DownloadRequestManager::DOWNLOADS_NOT_ALLOWED, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); // A user gesture now should NOT change the state. - download_request_manager_->OnUserGesture(controller()->tab_contents()); + download_request_manager_->OnUserGesture(controller().tab_contents()); ASSERT_EQ(DownloadRequestManager::DOWNLOADS_NOT_ALLOWED, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); // And make sure we really can't download. ask_allow_count_ = continue_count_ = cancel_count_ = 0; CanDownload(); @@ -198,5 +198,5 @@ TEST_F(DownloadRequestManagerTest, ResetOnUserGesture) { // And the state shouldn't have changed. ASSERT_EQ(DownloadRequestManager::DOWNLOADS_NOT_ALLOWED, download_request_manager_->GetDownloadStatus( - controller()->tab_contents())); + controller().tab_contents())); } diff --git a/chrome/browser/extensions/extension_tabs_module.cc b/chrome/browser/extensions/extension_tabs_module.cc index 27cf982..06bbcb5 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,9 +150,6 @@ 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; } @@ -178,8 +175,7 @@ bool UpdateTabFunction::RunImpl() { return false; TabContents* tab_contents = tab_strip->GetTabContentsAt(tab_index); - NavigationController* controller = tab_contents->controller(); - DCHECK(controller); + NavigationController& controller = tab_contents->controller(); // TODO(rafaelw): handle setting remaining tab properties: // -title @@ -190,7 +186,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? } @@ -227,10 +223,6 @@ 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 @@ -273,10 +265,7 @@ bool RemoveTabFunction::RunImpl() { int tab_index; TabStripModel* tab_strip = browser->tabstrip_model(); if (GetIndexOfTabId(tab_strip, tab_id, &tab_index)) { - TabContents* tab_contents = tab_strip->GetTabContentsAt(tab_index); - NavigationController* controller = tab_contents->controller(); - DCHECK(controller); - browser->CloseContents(tab_contents); + browser->CloseContents(tab_strip->GetTabContentsAt(tab_index)); return true; } @@ -314,8 +303,6 @@ 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)); @@ -325,7 +312,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 = controller->GetActiveEntry(); + NavigationEntry* entry = contents->controller().GetActiveEntry(); if (entry) { if (entry->favicon().is_valid()) result->SetString(L"favIconUrl", entry->favicon().url().spec()); @@ -338,10 +325,7 @@ 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); - NavigationController* controller = tab_contents->controller(); - DCHECK(controller); // TODO(aa): Is this a valid assumption? - - if (controller->session_id().id() == tab_id) { + if (tab_contents->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 d9d375c..79a84fc 100644 --- a/chrome/browser/external_tab_container.cc +++ b/chrome/browser/external_tab_container.cc @@ -68,7 +68,6 @@ 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(); @@ -91,8 +90,7 @@ bool ExternalTabContainer::Init(Profile* profile, HWND parent, DCHECK(dummy->IsFocusable()); root_view_.AddChildView(dummy); - NavigationController* controller = tab_contents_->controller(); - DCHECK(controller); + NavigationController* controller = &tab_contents_->controller(); registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED, Source<NavigationController>(controller)); registrar_.Add(this, NotificationType::FAIL_PROVISIONAL_LOAD_WITH_ERROR, @@ -131,17 +129,12 @@ 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>(controller), + Source<NavigationController>(&tab_contents_->controller()), Details<ExternalTabContainer>(this)); - tab_contents_->set_delegate(NULL); - tab_contents_->CloseContents(); - // WARNING: tab_contents_ has likely been deleted. + delete tab_contents_; tab_contents_ = NULL; } @@ -309,7 +302,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 3b23e3b..a3c3fcc 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 2a2dba4..260e897 100644 --- a/chrome/browser/find_backend_unittest.cc +++ b/chrome/browser/find_backend_unittest.cc @@ -15,16 +15,13 @@ TEST_F(FindBackendTest, InternalState) { EXPECT_EQ(string16(), contents()->find_text()); // Get another WebContents object ready. - WebContents* contents2 = new TestWebContents(profile_.get(), NULL); - NavigationController* controller2 = - new NavigationController(contents2, profile_.get()); - contents2->set_controller(controller2); + TestWebContents contents2(profile_.get(), NULL); // 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 "; @@ -37,18 +34,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. @@ -57,8 +54,6 @@ 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()); - - contents2->CloseContents(); + EXPECT_EQ(search_term3, contents2.find_prepopulate_text()); + EXPECT_EQ(search_term2, contents2.find_text()); } diff --git a/chrome/browser/find_bar_controller.cc b/chrome/browser/find_bar_controller.cc index bdcb94b..fb5cf3b 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 9e73389..086d5c0 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 ae2cc4d..c7b30c9 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 1e7b29a..a6dc4dc 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 06ecf00..419ad2f 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,36 +146,35 @@ void RegisterForAllNavNotifications(TestNotificationTracker* tracker, // ----------------------------------------------------------------------------- TEST_F(NavigationControllerTest, Defaults) { - 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()); + 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. @@ -186,27 +185,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); @@ -214,13 +213,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); } @@ -230,69 +229,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( @@ -306,9 +305,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 @@ -317,11 +316,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( @@ -329,7 +328,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()); @@ -341,9 +340,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 @@ -351,18 +350,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( @@ -370,10 +369,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"); @@ -384,63 +383,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); @@ -448,19 +447,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); @@ -472,62 +471,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( @@ -535,19 +534,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"); @@ -558,21 +557,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 @@ -593,39 +592,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"); @@ -638,40 +637,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"); @@ -684,42 +683,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); @@ -736,7 +735,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); @@ -745,7 +744,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). @@ -758,7 +757,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"); @@ -771,7 +770,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()); } @@ -779,7 +778,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); @@ -797,17 +796,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"); @@ -827,45 +826,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"); @@ -879,18 +878,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. @@ -911,50 +910,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 { @@ -1003,18 +1002,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++; @@ -1024,19 +1023,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); @@ -1052,15 +1051,16 @@ TEST_F(NavigationControllerTest, RestoreNavigate) { navigations.push_back(TabNavigation(0, url, GURL(), ASCIIToUTF16("Title"), "state", PageTransition::LINK)); - NavigationController* our_controller = - new NavigationController(profile(), navigations, 0); - our_controller->GoToIndex(0); + WebContents our_contents(profile(), NULL, MSG_ROUTING_NONE, NULL); + NavigationController& our_controller = our_contents.controller(); + our_controller.RestoreFromState(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,32 +1071,29 @@ 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()); - - // Clean up the navigation controller. - our_controller->Destroy(); + our_controller.GetLastCommittedEntry()->site_instance()->site()); } // 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 @@ -1104,9 +1101,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) { @@ -1118,60 +1115,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"); @@ -1180,9 +1177,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(); @@ -1190,103 +1187,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 @@ -1297,21 +1294,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)); } @@ -1325,8 +1322,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/#"); @@ -1338,11 +1335,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 @@ -1352,7 +1349,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(); @@ -1372,7 +1369,7 @@ TEST_F(NavigationControllerHistoryTest, NavigationThenBack) { rvh()->SendNavigate(1, url1); rvh()->SendNavigate(2, url2); - controller()->GoBack(); + controller().GoBack(); rvh()->SendNavigate(1, url1); GetLastSession(); @@ -1399,10 +1396,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 769c08e..2144806 100644 --- a/chrome/browser/password_manager/password_manager.cc +++ b/chrome/browser/password_manager/password_manager.cc @@ -98,8 +98,9 @@ PasswordManager::~PasswordManager() { } void PasswordManager::ProvisionallySavePassword(PasswordForm form) { - if (!web_contents_->controller() || !web_contents_->profile() || - web_contents_->profile()->IsOffTheRecord() || !*password_manager_enabled_) + if (!web_contents_->profile() || + web_contents_->profile()->IsOffTheRecord() || + !*password_manager_enabled_) return; // No password to save? Then don't. @@ -134,7 +135,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); @@ -166,9 +167,6 @@ 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_, @@ -186,13 +184,11 @@ 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 b8d1095..4bbc4ce 100644 --- a/chrome/browser/printing/print_view_manager.cc +++ b/chrome/browser/printing/print_view_manager.cc @@ -29,9 +29,6 @@ PrintViewManager::PrintViewManager(WebContents& owner) } PrintViewManager::~PrintViewManager() { -} - -void PrintViewManager::Destroy() { DisconnectFromCurrentPrintJob(); } @@ -125,7 +122,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 9e91318..b70293a 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 a4cd3ba..631ea6c 100644 --- a/chrome/browser/renderer_host/resource_dispatcher_host.cc +++ b/chrome/browser/renderer_host/resource_dispatcher_host.cc @@ -1295,7 +1295,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 abc5699..4255393 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,11 +87,8 @@ void RenderViewHostTestHarness::SetUp() { } void RenderViewHostTestHarness::TearDown() { - if (contents_) { - contents_->CloseContents(); - contents_ = NULL; - } - controller_ = NULL; + if (contents_) + delete contents_; // 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 63e6da6..2d07975 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(); } @@ -200,7 +200,7 @@ class RenderViewHostTestHarness : public testing::Test { return profile_.get(); } - MockRenderProcessHost* process() { + MockRenderProcessHost* process() { return static_cast<MockRenderProcessHost*>(rvh()->process()); } diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc index 0c1e95e..fd07b9d6 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 e8311db..32f3ab6 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 1a1c88a3..6369b3d 100644 --- a/chrome/browser/safe_browsing/safe_browsing_service.cc +++ b/chrome/browser/safe_browsing/safe_browsing_service.cc @@ -285,11 +285,9 @@ void SafeBrowsingService::DoDisplayBlockingPage( resource.threat_type == SafeBrowsingService::URL_MALWARE) { GURL page_url = wc->GetURL(); GURL referrer_url; - if (wc->controller()) { - NavigationEntry* entry = wc->controller()->GetActiveEntry(); - if (entry) - referrer_url = entry->referrer(); - } + 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 47fbad9..f05a444 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)); + &browser->AddRestoredTab(tab.navigations, + static_cast<int>(i - window.tabs.begin()), + selected_index, + false)->controller()); } } @@ -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 ba473c9..c0b8158 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 340bf34..5892013 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]; - NavigationController* restored_controller = + TabContents* restored_tab = browser->AddRestoredTab(tab.navigations, browser->tab_count(), tab.current_navigation_index, (static_cast<int>(tab_i) == window->selected_tab_index)); - if (restored_controller) - restored_controller->LoadIfNecessary(); + if (restored_tab) + restored_tab->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 9be6429..7a24a91 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 83e3baf..883de56 100644 --- a/chrome/browser/site_instance_unittest.cc +++ b/chrome/browser/site_instance_unittest.cc @@ -5,6 +5,7 @@ #include "base/string16.h" #include "chrome/browser/renderer_host/browser_render_process_host.h" #include "chrome/browser/renderer_host/render_view_host.h" +#include "chrome/browser/renderer_host/test_render_view_host.h" #include "chrome/browser/tab_contents/navigation_entry.h" #include "chrome/browser/tab_contents/web_contents.h" #include "chrome/common/render_messages.h" @@ -68,6 +69,10 @@ class TestSiteInstance : public SiteInstance { // Test to ensure no memory leaks for SiteInstance objects. TEST_F(SiteInstanceTest, SiteInstanceDestructor) { + // The existance of these factories will cause WebContents to create our test + // one instead of the real one. + MockRenderProcessHostFactory rph_factory; + TestRenderViewHostFactory rvh_factory(&rph_factory); int siteDeleteCounter = 0; int browsingDeleteCounter = 0; const GURL url("test:foo"); @@ -107,15 +112,11 @@ TEST_F(SiteInstanceTest, SiteInstanceDestructor) { TestSiteInstance::CreateTestSiteInstance(profile.get(), &siteDeleteCounter, &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(); + { + WebContents contents(profile.get(), instance, MSG_ROUTING_NONE, NULL); + EXPECT_EQ(1, siteDeleteCounter); + EXPECT_EQ(1, browsingDeleteCounter); + } 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 5abf71a..ce9d9da 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 bbdf0e2..afc48ce 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 3cb5975..ebe5e60 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 6925dda..9a466fb 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 206bc34..1ffff17 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 77cf714..268eadb 100644 --- a/chrome/browser/tab_contents/navigation_controller.cc +++ b/chrome/browser/tab_contents/navigation_controller.cc @@ -15,7 +15,6 @@ #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" @@ -121,45 +120,34 @@ 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( - Profile* profile, +NavigationController::~NavigationController() { + DiscardNonCommittedEntriesInternal(); + + NotificationService::current()->Notify( + NotificationType::TAB_CLOSED, + Source<NavigationController>(this), + NotificationService::NoDetails()); +} + +void NavigationController::RestoreFromState( const std::vector<TabNavigation>& navigations, - 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_); + int selected_navigation) { + // Verify that this controller is unused and that the input is valid. + DCHECK(entry_count() == 0 && !pending_entry()); 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) @@ -351,19 +339,6 @@ 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, @@ -421,7 +396,7 @@ void NavigationController::LoadURLLazily(const GURL& url, load_pending_entry_when_active_ = true; } -bool NavigationController::LoadingURLLazily() { +bool NavigationController::LoadingURLLazily() const { return load_pending_entry_when_active_; } @@ -741,6 +716,7 @@ bool NavigationController::RendererDidNavigateAutoSubframe( return false; } +// TODO(brettw) I think this function is unnecessary. void NavigationController::CommitPendingEntry() { DiscardTransientEntry(); @@ -803,6 +779,22 @@ 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(); @@ -867,9 +859,6 @@ 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(); } @@ -889,17 +878,6 @@ 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; @@ -938,25 +916,6 @@ 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_); @@ -964,9 +923,6 @@ 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 af7cf60..e42f4ad 100644 --- a/chrome/browser/tab_contents/navigation_controller.h +++ b/chrome/browser/tab_contents/navigation_controller.h @@ -123,34 +123,21 @@ class NavigationController { // --------------------------------------------------------------------------- - 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(TabContents* tab_contents, Profile* profile); ~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 @@ -295,10 +282,6 @@ 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 { @@ -346,13 +329,17 @@ 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(); + bool LoadingURLLazily() const; const string16& GetLazyTitle() const; const SkBitmap& GetLazyFavIcon() const; @@ -425,11 +412,6 @@ 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 fd94e13..698a65a 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 9846a77..9268850 100644 --- a/chrome/browser/tab_contents/render_view_host_delegate_helper.cc +++ b/chrome/browser/tab_contents/render_view_host_delegate_helper.cc @@ -29,7 +29,6 @@ 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 a178356..8b2c276 100644 --- a/chrome/browser/tab_contents/render_view_host_manager.cc +++ b/chrome/browser/tab_contents/render_view_host_manager.cc @@ -37,9 +37,13 @@ RenderViewHostManager::RenderViewHostManager( } RenderViewHostManager::~RenderViewHostManager() { - // Shutdown should have been called which should have cleaned these up. - DCHECK(!render_view_host_); - DCHECK(!pending_render_view_host_); + 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(); } void RenderViewHostManager::Init(Profile* profile, @@ -55,16 +59,6 @@ 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 @@ -105,7 +99,7 @@ RenderViewHost* RenderViewHostManager::Navigate(const NavigationEntry& entry) { NotificationService::current()->Notify( NotificationType::RENDER_VIEW_HOST_CHANGED, Source<NavigationController>( - delegate_->GetControllerForRenderManager()), + &delegate_->GetControllerForRenderManager()), Details<RenderViewHostSwitchedDetails>(&details)); } } @@ -356,12 +350,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 @@ -386,7 +380,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 @@ -399,7 +393,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 @@ -466,7 +460,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 b3a5b25..15ee081 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,8 +64,7 @@ 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 and Shutdown() before - // deleting it. + // You must call Init() before using this class. RenderViewHostManager(RenderViewHostDelegate* render_view_delegate, Delegate* delegate); ~RenderViewHostManager(); @@ -76,9 +75,6 @@ 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 02f5473..f0caccd 100644 --- a/chrome/browser/tab_contents/render_view_host_manager_unittest.cc +++ b/chrome/browser/tab_contents/render_view_host_manager_unittest.cc @@ -23,27 +23,22 @@ TEST_F(RenderViewHostManagerTest, NewTabPageProcesses) { NavigateAndCommit(dest); // Make a second tab. - WebContents* contents2 = new TestWebContents(profile_.get(), NULL); - NavigationController* controller2 = - new NavigationController(contents2, profile_.get()); - contents2->set_controller(controller2); + TestWebContents contents2(profile_.get(), NULL); // 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->CloseContents(); + contents2.render_view_host()->site_instance()->browsing_instance()); } diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc index ea5a9b5..226ce5b 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() +TabContents::TabContents(Profile* profile) : delegate_(NULL), - controller_(NULL), + controller_(this, profile), is_loading_(false), is_crashed_(false), waiting_for_response_(false), @@ -45,71 +45,13 @@ 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(); } @@ -140,22 +82,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; } @@ -165,7 +107,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)) @@ -211,8 +153,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; } @@ -302,9 +244,8 @@ 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_)); } } @@ -322,7 +263,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 26dda95..aa636f3 100644 --- a/chrome/browser/tab_contents/tab_contents.h +++ b/chrome/browser/tab_contents/tab_contents.h @@ -44,23 +44,8 @@ class SkBitmap; class SiteInstance; class WebContents; -// 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. +// Describes what goes in the main content area of a tab. WebContents is +// the only type of TabContents, and these should be merged together. class TabContents : public PageNavigator, public NotificationObserver { public: @@ -77,22 +62,9 @@ class TabContents : public PageNavigator, INVALIDATE_EVERYTHING = 0xFFFFFFFF }; - 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(); + virtual ~TabContents(); - // 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; + static void RegisterUserPrefs(PrefService* prefs); // Intrinsic tab state ------------------------------------------------------- @@ -116,27 +88,13 @@ class TabContents : public PageNavigator, TabContentsDelegate* delegate() const { return delegate_; } void set_delegate(TabContentsDelegate* d) { delegate_ = d; } - // 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); + // Gets the controller for this tab contents. + NavigationController& controller() { return controller_; } + const NavigationController& controller() const { return controller_; } // Returns the user profile associated with this TabContents (via the - // 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; - } + // NavigationController). + Profile* profile() const { return controller_.profile(); } // Returns whether this tab contents supports the provided URL.This method // matches the tab contents type with the result of TypeForURL(). |url| points @@ -273,6 +231,10 @@ 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) @@ -386,15 +348,7 @@ class TabContents : public PageNavigator, // automation purposes. friend class AutomationProvider; - 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(); + TabContents(Profile* profile); // Changes the IsLoading state and notifies delegate as needed // |details| is used to provide details on the load that just finished @@ -432,7 +386,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 bf70542..8592522 100644 --- a/chrome/browser/tab_contents/web_contents.cc +++ b/chrome/browser/tab_contents/web_contents.cc @@ -203,7 +203,8 @@ WebContents::WebContents(Profile* profile, SiteInstance* site_instance, int routing_id, base::WaitableEvent* modal_dialog_event) - : view_(WebContentsView::Create(this)), + : TabContents(profile), + view_(WebContentsView::Create(this)), ALLOW_THIS_IN_INITIALIZER_LIST(render_manager_(this, this)), printing_(*this), notify_disconnection_(false), @@ -234,14 +235,12 @@ WebContents::WebContents(Profile* profile, } // Register for notifications about URL starredness changing on any profile. - 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()); + 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()); // Keep a global copy of the previous search string (if any). static string16 global_last_search = string16(); @@ -249,11 +248,60 @@ 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(); - NotificationService::current()->RemoveObserver( - this, NotificationType::RENDER_WIDGET_HOST_DESTROYED, - NotificationService::AllSources()); + + // 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 } // static @@ -330,78 +378,6 @@ 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(); @@ -418,15 +394,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(); } @@ -443,7 +419,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(); @@ -480,7 +456,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) @@ -543,15 +519,19 @@ void WebContents::DisassociateFromPopupCount() { render_view_host()->DisassociateFromPopupCount(); } -void WebContents::DidBecomeSelected() { - if (controller_) - controller_->SetActive(true); +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; +} -#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 +void WebContents::DidBecomeSelected() { + controller_.SetActive(true); if (render_widget_host_view()) render_widget_host_view()->DidBecomeSelected(); @@ -612,7 +592,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(); @@ -677,7 +657,7 @@ void WebContents::GetContainerBounds(gfx::Rect *out) const { } void WebContents::CreateShortcut() { - NavigationEntry* entry = controller()->GetLastCommittedEntry(); + NavigationEntry* entry = controller_.GetLastCommittedEntry(); if (!entry) return; @@ -795,7 +775,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); @@ -835,7 +815,7 @@ void WebContents::SetIsLoading(bool is_loading, if (details) det = Details<LoadNotificationDetails>(details); NotificationService::current()->Notify(type, - Source<NavigationController>(this->controller()), + Source<NavigationController>(&controller_), det); } @@ -852,9 +832,7 @@ Profile* WebContents::GetProfile() const { } void WebContents::RenderViewCreated(RenderViewHost* render_view_host) { - if (!controller()) - return; - NavigationEntry* entry = controller()->GetActiveEntry(); + NavigationEntry* entry = controller_.GetActiveEntry(); if (!entry) return; @@ -906,10 +884,6 @@ 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); @@ -926,7 +900,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 @@ -944,8 +918,6 @@ 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 @@ -953,49 +925,43 @@ 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; @@ -1003,7 +969,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); } @@ -1045,24 +1011,23 @@ void WebContents::DidStartLoading(RenderViewHost* rvh, int32 page_id) { void WebContents::DidStopLoading(RenderViewHost* rvh, int32 page_id) { scoped_ptr<LoadNotificationDetails> details; - 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())); - } + + 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 @@ -1077,11 +1042,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)); } @@ -1090,9 +1055,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); @@ -1103,9 +1068,6 @@ 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, @@ -1116,7 +1078,7 @@ void WebContents::DidLoadResourceFromMemoryCache( NotificationService::current()->Notify( NotificationType::LOAD_FROM_MEMORY_CACHE, - Source<NavigationController>(controller()), + Source<NavigationController>(&controller_), Details<LoadFromMemoryCacheDetails>(&details)); } @@ -1126,9 +1088,6 @@ 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 @@ -1157,22 +1116,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)); } @@ -1244,21 +1203,14 @@ void WebContents::ProcessExternalHostMessage(const std::string& message, } void WebContents::GoToEntryAtOffset(int offset) { - if (!controller()) - return; - controller()->GoToOffset(offset); + controller_.GoToOffset(offset); } void WebContents::GetHistoryListCount(int* back_list_count, int* forward_list_count) { - *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; - } + 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, @@ -1354,7 +1306,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 (!controller() || !IsActiveEntry(page_id)) + if (!IsActiveEntry(page_id)) return; TemplateURLModel* url_model = profile()->GetTemplateURLModel(); if (!url_model) @@ -1369,18 +1321,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; } @@ -1633,9 +1585,7 @@ DOMUI* WebContents::CreateDOMUIForRenderManager(const GURL& url) { NavigationEntry* WebContents::GetLastCommittedNavigationEntryForRenderManager() { - if (!controller()) - return NULL; - return controller()->GetLastCommittedEntry(); + return controller_.GetLastCommittedEntry(); } bool WebContents::CreateRenderViewForRenderManager( @@ -1694,7 +1644,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()); @@ -1828,7 +1778,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(); @@ -1852,7 +1802,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) { @@ -1967,14 +1917,13 @@ 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 @@ -1982,7 +1931,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. @@ -2023,9 +1972,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 { @@ -2073,8 +2022,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 b89efd3..9128868 100644 --- a/chrome/browser/tab_contents/web_contents.h +++ b/chrome/browser/tab_contents/web_contents.h @@ -73,6 +73,8 @@ class WebContents : public TabContents, int routing_id, base::WaitableEvent* modal_dialog_event); + virtual ~WebContents(); + static void RegisterUserPrefs(PrefService* prefs); // Getters ------------------------------------------------------------------- @@ -126,7 +128,6 @@ class WebContents : public TabContents, // TabContents (public overrides) -------------------------------------------- - virtual void Destroy(); virtual WebContents* AsWebContents() { return this; } const string16& GetTitle() const; virtual SiteInstance* GetSiteInstance() const; @@ -139,6 +140,7 @@ 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(); @@ -292,9 +294,6 @@ class WebContents : public TabContents, } protected: - // Should be deleted via CloseContents. - virtual ~WebContents(); - RenderWidgetHostView* render_widget_host_view() const { return render_manager_.current_view(); } @@ -453,7 +452,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 c5e58e1..207a724 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,24 +345,23 @@ 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 = new TestWebContents(profile(), instance1); + TestWebContents contents2(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->SetupController(profile()); - contents2->controller()->LoadURL(url, GURL(), PageTransition::TYPED); - contents2->TestDidNavigate(contents2->render_view_host(), params1); + contents2.transition_cross_site = true; + 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); @@ -372,10 +371,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 @@ -383,14 +382,12 @@ 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 @@ -402,28 +399,27 @@ 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 = new TestWebContents(profile(), instance1); - contents2->transition_cross_site = true; - contents2->SetupController(profile()); + TestWebContents contents2(profile(), instance1); + contents2.transition_cross_site = true; 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. @@ -437,15 +433,13 @@ 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 @@ -457,7 +451,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); @@ -466,13 +460,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*>( @@ -500,34 +494,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()); } @@ -545,7 +539,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()); } @@ -577,10 +571,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. @@ -601,7 +595,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); @@ -611,10 +605,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, @@ -625,7 +619,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). @@ -646,7 +640,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); @@ -656,10 +650,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 @@ -669,7 +663,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 = @@ -689,7 +683,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); @@ -700,10 +694,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, @@ -714,10 +708,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. @@ -738,7 +732,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); @@ -758,11 +752,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, @@ -773,7 +767,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 = @@ -793,7 +787,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); @@ -813,11 +807,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 @@ -827,7 +821,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 = @@ -847,7 +841,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); @@ -860,11 +854,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. @@ -902,7 +896,7 @@ TEST_F(WebContentsTest, ShowInterstitialThenCloseTab) { interstitial->TestDidNavigate(1, url); // Now close the tab. - contents()->CloseContents(); + delete contents(); ContentsCleanedUp(); EXPECT_TRUE(deleted); EXPECT_EQ(TestInterstitialPage::CANCELED, state); @@ -914,7 +908,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 = @@ -948,7 +942,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 = @@ -986,10 +980,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 @@ -998,7 +992,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 = @@ -1041,10 +1035,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 @@ -1064,7 +1058,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 2fc7e51..0470efa 100644 --- a/chrome/browser/tabs/tab_strip_model.cc +++ b/chrome/browser/tabs/tab_strip_model.cc @@ -94,7 +94,7 @@ void TabStripModel::InsertTabContentsAt(int index, ForgetAllOpeners(); } // Anything opened by a link we deem to have an opener. - data->SetGroup(selected_contents->controller()); + data->SetGroup(&selected_contents->controller()); } contents_data_.insert(contents_data_.begin() + index, data); @@ -198,7 +198,7 @@ int TabStripModel::GetIndexOfController( int index = 0; TabContentsDataVector::const_iterator iter = contents_data_.begin(); for (; iter != contents_data_.end(); ++iter, ++index) { - if ((*iter)->contents->controller() == controller) + if (&(*iter)->contents->controller() == controller) return index; } return kNoTab; @@ -403,10 +403,8 @@ bool TabStripModel::IsContextMenuCommandEnabled( case CommandCloseTabsToRight: return context_index < (count() - 1); case CommandCloseTabsOpenedBy: { - NavigationController* opener = - GetTabContentsAt(context_index)->controller(); - int next_index = - GetIndexOfNextTabContentsOpenedBy(opener, context_index, true); + int next_index = GetIndexOfNextTabContentsOpenedBy( + &GetTabContentsAt(context_index)->controller(), context_index, true); return next_index != kNoTab; } case CommandDuplicate: @@ -429,7 +427,7 @@ void TabStripModel::ExecuteContextMenuCommand( break; case CommandReload: UserMetrics::RecordAction(L"TabContextMenu_Reload", profile_); - GetContentsAt(context_index)->controller()->Reload(true); + GetContentsAt(context_index)->controller().Reload(true); break; case CommandDuplicate: UserMetrics::RecordAction(L"TabContextMenu_Duplicate", profile_); @@ -457,7 +455,7 @@ void TabStripModel::ExecuteContextMenuCommand( case CommandCloseTabsOpenedBy: { UserMetrics::RecordAction(L"TabContextMenu_CloseTabsOpenedBy", profile_); NavigationController* opener = - GetTabContentsAt(context_index)->controller(); + &GetTabContentsAt(context_index)->controller(); for (int i = count() - 1; i >= 0; --i) { if (OpenerMatches(contents_data_.at(i), opener, true)) @@ -478,7 +476,7 @@ void TabStripModel::ExecuteContextMenuCommand( std::vector<int> TabStripModel::GetIndexesOpenedBy(int index) const { std::vector<int> indices; - NavigationController* opener = GetTabContentsAt(index)->controller(); + NavigationController* opener = &GetTabContentsAt(index)->controller(); for (int i = count() - 1; i >= 0; --i) { if (OpenerMatches(contents_data_.at(i), opener, true)) indices.push_back(i); @@ -511,7 +509,7 @@ bool TabStripModel::IsNewTabAtEndOfTabStrip(TabContents* contents) const { return LowerCaseEqualsASCII(contents->GetURL().spec(), chrome::kChromeUINewTabURL) && contents == GetContentsAt(count() - 1) && - contents->controller()->entry_count() == 1; + contents->controller().entry_count() == 1; } bool TabStripModel::InternalCloseTabContentsAt(int index, @@ -534,9 +532,9 @@ bool TabStripModel::InternalCloseTabContentsAt(int index, if (create_historical_tab) delegate_->CreateHistoricalTab(detached_contents); - detached_contents->CloseContents(); - // Closing the TabContents will later call back to us via - // NotificationObserver and detach it. + // Deleting the TabContents will call back to us via NotificationObserver + // and detach it. + delete detached_contents; } return true; } @@ -564,7 +562,7 @@ void TabStripModel::ChangeSelectedContentsFrom( void TabStripModel::SetOpenerForContents(TabContents* contents, TabContents* opener) { int index = GetIndexOfTabContents(contents); - contents_data_.at(index)->opener = opener->controller(); + contents_data_.at(index)->opener = &opener->controller(); } // static diff --git a/chrome/browser/tabs/tab_strip_model_order_controller.cc b/chrome/browser/tabs/tab_strip_model_order_controller.cc index 0c6326a..1db0525 100644 --- a/chrome/browser/tabs/tab_strip_model_order_controller.cc +++ b/chrome/browser/tabs/tab_strip_model_order_controller.cc @@ -36,7 +36,7 @@ int TabStripModelOrderController::DetermineInsertionIndex( return tabstrip_->selected_index() + 1; } NavigationController* opener = - tabstrip_->GetSelectedTabContents()->controller(); + &tabstrip_->GetSelectedTabContents()->controller(); // Get the index of the next item opened by this tab, and insert before // it... int index = tabstrip_->GetIndexOfLastTabContentsOpenedBy( @@ -60,7 +60,7 @@ int TabStripModelOrderController::DetermineNewSelectedIndex( // want to select the first in that child group, not the next tab in the same // group of the removed tab. NavigationController* removed_controller = - tabstrip_->GetTabContentsAt(removing_index)->controller(); + &tabstrip_->GetTabContentsAt(removing_index)->controller(); int index = tabstrip_->GetIndexOfNextTabContentsOpenedBy(removed_controller, removing_index, false); @@ -109,8 +109,8 @@ void TabStripModelOrderController::TabSelectedAt(TabContents* old_contents, NavigationController* new_opener = tabstrip_->GetOpenerOfTabContentsAt(index); if (user_gesture && new_opener != old_opener && - new_opener != old_contents->controller() && - old_opener != new_contents->controller()) { + new_opener != &old_contents->controller() && + old_opener != &new_contents->controller()) { tabstrip_->ForgetAllOpeners(); } } diff --git a/chrome/browser/tabs/tab_strip_model_unittest.cc b/chrome/browser/tabs/tab_strip_model_unittest.cc index 6b1f3d0..8e052544 100644 --- a/chrome/browser/tabs/tab_strip_model_unittest.cc +++ b/chrome/browser/tabs/tab_strip_model_unittest.cc @@ -68,23 +68,21 @@ class TabStripDummyDelegate : public TabStripModelDelegate { class TabStripModelTest : public RenderViewHostTestHarness { public: TabContents* CreateTabContents() { - WebContents* con = new WebContents(profile(), NULL, 0, NULL); - con->SetupController(profile()); - return con; + return new WebContents(profile(), NULL, 0, NULL); } // Forwards a URL "load" request through to our dummy TabContents // implementation. void LoadURL(TabContents* con, const std::wstring& url) { - controller()->LoadURL(GURL(url), GURL(), PageTransition::LINK); + controller().LoadURL(GURL(url), GURL(), PageTransition::LINK); } void GoBack(TabContents* contents) { - controller()->GoBack(); + controller().GoBack(); } void GoForward(TabContents* contents) { - controller()->GoForward(); + controller().GoForward(); } void SwitchTabTo(TabContents* contents) { @@ -360,8 +358,8 @@ TEST_F(TabStripModelTest, TestBasicAPI) { EXPECT_EQ(contents1, tabstrip.GetTabContentsAt(1)); EXPECT_EQ(0, tabstrip.GetIndexOfTabContents(contents2)); EXPECT_EQ(1, tabstrip.GetIndexOfTabContents(contents1)); - EXPECT_EQ(0, tabstrip.GetIndexOfController(contents2->controller())); - EXPECT_EQ(1, tabstrip.GetIndexOfController(contents1->controller())); + EXPECT_EQ(0, tabstrip.GetIndexOfController(&contents2->controller())); + EXPECT_EQ(1, tabstrip.GetIndexOfController(&contents1->controller())); } // Test UpdateTabContentsStateAt @@ -414,7 +412,7 @@ TEST_F(TabStripModelTest, TestBasicOpenerAPI) { // background with opener_contents set as their opener. TabContents* opener_contents = CreateTabContents(); - NavigationController* opener = opener_contents->controller(); + NavigationController* opener = &opener_contents->controller(); tabstrip.AppendTabContents(opener_contents, true); TabContents* contents1 = CreateTabContents(); TabContents* contents2 = CreateTabContents(); @@ -447,7 +445,7 @@ TEST_F(TabStripModelTest, TestBasicOpenerAPI) { // For a tab that has opened no other tabs, the return value should always be // -1... - NavigationController* o1 = contents1->controller(); + NavigationController* o1 = &contents1->controller(); EXPECT_EQ(-1, tabstrip.GetIndexOfNextTabContentsOpenedBy(o1, 3, false)); EXPECT_EQ(-1, tabstrip.GetIndexOfLastTabContentsOpenedBy(o1, 3)); @@ -513,7 +511,7 @@ TEST_F(TabStripModelTest, TestInsertionIndexDetermination) { EXPECT_TRUE(tabstrip.empty()); TabContents* opener_contents = CreateTabContents(); - NavigationController* opener = opener_contents->controller(); + NavigationController* opener = &opener_contents->controller(); tabstrip.AppendTabContents(opener_contents, true); // Open some other random unrelated tab in the background to monkey with our @@ -592,7 +590,7 @@ TEST_F(TabStripModelTest, TestSelectOnClose) { EXPECT_TRUE(tabstrip.empty()); TabContents* opener_contents = CreateTabContents(); - NavigationController* opener = opener_contents->controller(); + NavigationController* opener = &opener_contents->controller(); tabstrip.AppendTabContents(opener_contents, true); TabContents* contents1 = CreateTabContents(); @@ -670,7 +668,7 @@ TEST_F(TabStripModelTest, TestContextMenuCloseCommands) { EXPECT_TRUE(tabstrip.empty()); TabContents* opener_contents = CreateTabContents(); - NavigationController* opener = opener_contents->controller(); + NavigationController* opener = &opener_contents->controller(); tabstrip.AppendTabContents(opener_contents, true); TabContents* contents1 = CreateTabContents(); @@ -945,8 +943,8 @@ TEST_F(TabStripModelTest, AddTabContents_ForgetOpeners) { // Added for http://b/issue?id=958960 TEST_F(TabStripModelTest, AppendContentsReselectionTest) { - TabContents* fake_destinations_tab = CreateTabContents(); - TabStripDummyDelegate delegate(fake_destinations_tab); + WebContents fake_destinations_tab(profile(), NULL, 0, NULL); + TabStripDummyDelegate delegate(&fake_destinations_tab); TabStripModel tabstrip(&delegate, profile()); EXPECT_TRUE(tabstrip.empty()); diff --git a/chrome/browser/task_manager_resource_providers.cc b/chrome/browser/task_manager_resource_providers.cc index 0a992dd..5364b4d 100644 --- a/chrome/browser/task_manager_resource_providers.cc +++ b/chrome/browser/task_manager_resource_providers.cc @@ -40,11 +40,6 @@ 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 ca5b996..3906d5b 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. - blocked_contents->CloseContents(); + delete blocked_contents; return; } if (blocked_popups_.size() > kImpossibleNumberOfPopups) { - blocked_contents->CloseContents(); + delete blocked_contents; 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); - blocked_popups_.back().first->CloseContents(); + delete blocked_popups_.back().first; blocked_popups_.pop_back(); } diff --git a/chrome/browser/views/blocked_popup_container.h b/chrome/browser/views/blocked_popup_container.h index e469da6..c634308 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(); - // Sends a CloseContents() to each message in |blocked_popups_|. + // Deletes each contents 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 1144065..647dfc1 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 3e1c777..ffa9c1d 100644 --- a/chrome/browser/views/dom_view.cc +++ b/chrome/browser/views/dom_view.cc @@ -11,11 +11,8 @@ DOMView::DOMView() : initialized_(false), web_contents_(NULL) { } DOMView::~DOMView() { - if (web_contents_) { + if (web_contents_.get()) Detach(); - web_contents_->Destroy(); - web_contents_ = NULL; - } } bool DOMView::Init(Profile* profile, SiteInstance* instance) { @@ -23,13 +20,13 @@ bool DOMView::Init(Profile* profile, SiteInstance* instance) { return true; initialized_ = true; - web_contents_ = new WebContents(profile, instance, MSG_ROUTING_NONE, NULL); + web_contents_.reset(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 bfa6d13..132a444 100644 --- a/chrome/browser/views/dom_view.h +++ b/chrome/browser/views/dom_view.h @@ -8,6 +8,7 @@ #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" @@ -34,7 +35,7 @@ class DOMView : public views::HWNDView { protected: virtual bool CanProcessTabKeyEvents() { return true; } - WebContents* web_contents_; + scoped_ptr<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 2505ff8..2cd0823 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 c1b96b9..59bce3b 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 4b721da..04c9354 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 51baf95..5328927 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); } |