Using WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle()
NavigationEntry::SetTitle() doesn't result in the emission of
WebContentsObserver::TitleWasSet() events. This used to cause listeners like
the task manager to show wrong titles of tabs (for example in the event of an
interstitial page).
BUG=607074
TEST=content_browsertests --gtest_filter=InterstitialPageImplTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2086423005
Cr-Commit-Position: refs/heads/master@{#405485}
diff --git a/chrome/browser/devtools/devtools_ui_bindings.cc b/chrome/browser/devtools/devtools_ui_bindings.cc
index e083cfb..2ad277b 100644
--- a/chrome/browser/devtools/devtools_ui_bindings.cc
+++ b/chrome/browser/devtools/devtools_ui_bindings.cc
@@ -44,7 +44,6 @@
#include "components/zoom/page_zoom.h"
#include "content/public/browser/devtools_external_agent_proxy.h"
#include "content/public/browser/devtools_external_agent_proxy_delegate.h"
-#include "content/public/browser/invalidate_type.h"
#include "content/public/browser/navigation_controller.h"
#include "content/public/browser/navigation_entry.h"
#include "content/public/browser/notification_source.h"
@@ -536,9 +535,8 @@
content::NavigationController& controller = web_contents()->GetController();
content::NavigationEntry* entry = controller.GetActiveEntry();
// DevTools UI is not localized.
- entry->SetTitle(
- base::UTF8ToUTF16(base::StringPrintf(kTitleFormat, url.c_str())));
- web_contents()->NotifyNavigationStateChanged(content::INVALIDATE_TYPE_TITLE);
+ web_contents()->UpdateTitleForEntry(
+ entry, base::UTF8ToUTF16(base::StringPrintf(kTitleFormat, url.c_str())));
}
void DevToolsUIBindings::LoadNetworkResource(const DispatchCallback& callback,
diff --git a/chrome/browser/media/tab_desktop_media_list_unittest.cc b/chrome/browser/media/tab_desktop_media_list_unittest.cc
index f1117f0..80c7a50f 100644
--- a/chrome/browser/media/tab_desktop_media_list_unittest.cc
+++ b/chrome/browser/media/tab_desktop_media_list_unittest.cc
@@ -139,7 +139,7 @@
entry = contents->GetController().GetTransientEntry();
}
- entry->SetTitle(base::UTF8ToUTF16("Test tab"));
+ contents->UpdateTitleForEntry(entry, base::ASCIIToUTF16("Test tab"));
content::FaviconStatus favicon_info;
favicon_info.image =
@@ -325,8 +325,9 @@
WebContents* contents =
tab_strip_model->GetWebContentsAt(kDefaultSourceCount - 1);
ASSERT_TRUE(contents);
- contents->GetController().GetTransientEntry()->SetTitle(
- base::UTF8ToUTF16("New test tab"));
+ const content::NavigationController& controller = contents->GetController();
+ contents->UpdateTitleForEntry(controller.GetTransientEntry(),
+ base::ASCIIToUTF16("New test tab"));
EXPECT_CALL(observer_, OnSourceNameChanged(list_.get(), 0))
.WillOnce(QuitMessageLoop());
diff --git a/chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc b/chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc
index f6862a3..452ce24 100644
--- a/chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc
+++ b/chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc
@@ -177,11 +177,8 @@
const content::LoadCommittedDetails& details,
const content::FrameNavigateParams& params) {
auto itr = tasks_by_frames_.find(web_contents()->GetMainFrame());
- if (itr == tasks_by_frames_.end()) {
- // TODO(afakhry): Validate whether this actually happens in practice.
- NOTREACHED();
+ if (itr == tasks_by_frames_.end())
return;
- }
// Listening to WebContentsObserver::TitleWasSet() only is not enough in
// some cases when the the webpage doesn't have a title. That's why we update
@@ -199,11 +196,8 @@
void WebContentsEntry::TitleWasSet(content::NavigationEntry* entry,
bool explicit_set) {
auto itr = tasks_by_frames_.find(web_contents()->GetMainFrame());
- if (itr == tasks_by_frames_.end()) {
- // TODO(afakhry): Validate whether this actually happens in practice.
- NOTREACHED();
+ if (itr == tasks_by_frames_.end())
return;
- }
itr->second->UpdateTitle();
}
diff --git a/chrome/browser/ui/browser_commands.cc b/chrome/browser/ui/browser_commands.cc
index d3558150..8a77489 100644
--- a/chrome/browser/ui/browser_commands.cc
+++ b/chrome/browser/ui/browser_commands.cc
@@ -1200,7 +1200,8 @@
last_committed_entry->SetPageState(page_state.RemoveScrollOffset());
// Do not restore title, derive it from the url.
- last_committed_entry->SetTitle(base::string16());
+ view_source_contents->UpdateTitleForEntry(last_committed_entry,
+ base::string16());
// Now show view-source entry.
if (browser->CanSupportWindowFeature(Browser::FEATURE_TABSTRIP)) {
diff --git a/chrome/browser/ui/search/search_tab_helper.cc b/chrome/browser/ui/search/search_tab_helper.cc
index d194d3b..01e6a7e 100644
--- a/chrome/browser/ui/search/search_tab_helper.cc
+++ b/chrome/browser/ui/search/search_tab_helper.cc
@@ -277,8 +277,10 @@
// prevents any flickering of the tab title.
content::NavigationEntry* entry =
web_contents_->GetController().GetPendingEntry();
- if (entry)
- entry->SetTitle(l10n_util::GetStringUTF16(IDS_NEW_TAB_TITLE));
+ if (entry) {
+ web_contents_->UpdateTitleForEntry(
+ entry, l10n_util::GetStringUTF16(IDS_NEW_TAB_TITLE));
+ }
}
}
@@ -306,7 +308,8 @@
if (entry && entry->GetTitle().empty() &&
(entry->GetVirtualURL() == GURL(chrome::kChromeUINewTabURL) ||
search::NavEntryIsInstantNTP(web_contents_, entry))) {
- entry->SetTitle(l10n_util::GetStringUTF16(IDS_NEW_TAB_TITLE));
+ web_contents_->UpdateTitleForEntry(
+ entry, l10n_util::GetStringUTF16(IDS_NEW_TAB_TITLE));
}
}
diff --git a/chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc b/chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc
index 03492e2..5d66a18 100644
--- a/chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc
+++ b/chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc
@@ -79,7 +79,8 @@
void LoadURLAndUpdateState(const char* url, const char* title) {
NavigateAndCommit(GURL(url));
- controller().GetLastCommittedEntry()->SetTitle(base::UTF8ToUTF16(title));
+ web_contents()->UpdateTitleForEntry(
+ controller().GetLastCommittedEntry(), base::UTF8ToUTF16(title));
}
// Navigate back or forward the given amount and commits the entry (which
diff --git a/chrome/test/base/browser_with_test_window_test.cc b/chrome/test/base/browser_with_test_window_test.cc
index b2dda01..cfb71ad 100644
--- a/chrome/test/base/browser_with_test_window_test.cc
+++ b/chrome/test/base/browser_with_test_window_test.cc
@@ -183,10 +183,11 @@
Browser* navigating_browser,
const GURL& url,
const base::string16& title) {
- NavigationController* controller = &navigating_browser->tab_strip_model()->
- GetActiveWebContents()->GetController();
+ WebContents* contents =
+ navigating_browser->tab_strip_model()->GetActiveWebContents();
+ NavigationController* controller = &contents->GetController();
NavigateAndCommit(controller, url);
- controller->GetActiveEntry()->SetTitle(title);
+ contents->UpdateTitleForEntry(controller->GetActiveEntry(), title);
}
void BrowserWithTestWindowTest::DestroyBrowserAndProfile() {
diff --git a/content/browser/frame_host/interstitial_page_impl.cc b/content/browser/frame_host/interstitial_page_impl.cc
index 47e0c48..be08682 100644
--- a/content/browser/frame_host/interstitial_page_impl.cc
+++ b/content/browser/frame_host/interstitial_page_impl.cc
@@ -298,11 +298,8 @@
controller_->delegate()->DetachInterstitialPage();
// Let's revert to the original title if necessary.
NavigationEntry* entry = controller_->GetVisibleEntry();
- if (entry && !new_navigation_ && should_revert_web_contents_title_) {
- entry->SetTitle(original_web_contents_title_);
- controller_->delegate()->NotifyNavigationStateChanged(
- INVALIDATE_TYPE_TITLE);
- }
+ if (entry && !new_navigation_ && should_revert_web_contents_title_)
+ web_contents_->UpdateTitleForEntry(entry, original_web_contents_title_);
static_cast<WebContentsImpl*>(web_contents_)->DidChangeVisibleSSLState();
@@ -411,8 +408,7 @@
}
// TODO(evan): make use of title_direction.
// http://code.google.com/p/chromium/issues/detail?id=27094
- entry->SetTitle(title);
- controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_TITLE);
+ web_contents_->UpdateTitleForEntry(entry, title);
}
InterstitialPage* InterstitialPageImpl::GetAsInterstitialPage() {
@@ -669,7 +665,7 @@
if (should_discard_pending_nav_entry_) {
// Since no navigation happens we have to discard the transient entry
- // explicitely. Note that by calling DiscardNonCommittedEntries() we also
+ // explicitly. Note that by calling DiscardNonCommittedEntries() we also
// discard the pending entry, which is what we want, since the navigation is
// cancelled.
controller_->DiscardNonCommittedEntries();
diff --git a/content/browser/frame_host/interstitial_page_impl_browsertest.cc b/content/browser/frame_host/interstitial_page_impl_browsertest.cc
index 2fbdda3..ab7203d 100644
--- a/content/browser/frame_host/interstitial_page_impl_browsertest.cc
+++ b/content/browser/frame_host/interstitial_page_impl_browsertest.cc
@@ -64,61 +64,6 @@
}
};
-// A title watcher for interstitial pages. The existing TitleWatcher does not
-// work for interstitial pages. Note that this title watcher waits for the
-// title update IPC message not the actual title update. So, the new title is
-// probably not propagated completely, yet.
-class InterstitialTitleUpdateWatcher : public BrowserMessageFilter {
- public:
- explicit InterstitialTitleUpdateWatcher(InterstitialPage* interstitial)
- : BrowserMessageFilter(FrameMsgStart) {
- interstitial->GetMainFrame()->GetProcess()->AddFilter(this);
- }
-
- void InitWait(const std::string& expected_title) {
- DCHECK(!run_loop_);
- expected_title_ = base::UTF8ToUTF16(expected_title);
- run_loop_.reset(new base::RunLoop());
- }
-
- void Wait() {
- DCHECK(run_loop_);
- run_loop_->Run();
- run_loop_.reset();
- }
-
- private:
- ~InterstitialTitleUpdateWatcher() override {}
-
- void OnTitleUpdateReceived(const base::string16& title) {
- DCHECK(run_loop_);
- if (title == expected_title_)
- run_loop_->Quit();
- }
-
- // BrowserMessageFilter:
- bool OnMessageReceived(const IPC::Message& message) override {
- if (!run_loop_)
- return false;
-
- if (message.type() == FrameHostMsg_UpdateTitle::ID) {
- FrameHostMsg_UpdateTitle::Param params;
- if (FrameHostMsg_UpdateTitle::Read(&message, ¶ms)) {
- BrowserThread::PostTask(
- BrowserThread::UI, FROM_HERE,
- base::Bind(&InterstitialTitleUpdateWatcher::OnTitleUpdateReceived,
- this, std::get<0>(params)));
- }
- }
- return false;
- }
-
- base::string16 expected_title_;
- std::unique_ptr<base::RunLoop> run_loop_;
-
- DISALLOW_COPY_AND_ASSIGN(InterstitialTitleUpdateWatcher);
-};
-
// A message filter that watches for WriteText and CommitWrite clipboard IPC
// messages to make sure cut/copy is working properly. It will mark these events
// as handled to prevent modification of the actual clipboard.
@@ -252,8 +197,6 @@
clipboard_message_watcher_ =
new ClipboardMessageWatcher(interstitial_.get());
- title_update_watcher_ =
- new InterstitialTitleUpdateWatcher(interstitial_.get());
// Wait until page loads completely.
ASSERT_TRUE(WaitForRenderFrameReady(interstitial_->GetMainFrame()));
@@ -292,12 +235,14 @@
std::string PerformCut() {
clipboard_message_watcher_->InitWait();
- title_update_watcher_->InitWait("TEXT_CHANGED");
+ const base::string16 expected_title = base::UTF8ToUTF16("TEXT_CHANGED");
+ content::TitleWatcher title_watcher(shell()->web_contents(),
+ expected_title);
RenderFrameHostImpl* rfh =
static_cast<RenderFrameHostImpl*>(interstitial_->GetMainFrame());
rfh->GetRenderWidgetHost()->delegate()->Cut();
clipboard_message_watcher_->WaitForWriteCommit();
- title_update_watcher_->Wait();
+ EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
return clipboard_message_watcher_->last_text();
}
@@ -311,19 +256,24 @@
}
void PerformPaste() {
- title_update_watcher_->InitWait("TEXT_CHANGED");
+ const base::string16 expected_title = base::UTF8ToUTF16("TEXT_CHANGED");
+ content::TitleWatcher title_watcher(shell()->web_contents(),
+ expected_title);
RenderFrameHostImpl* rfh =
static_cast<RenderFrameHostImpl*>(interstitial_->GetMainFrame());
rfh->GetRenderWidgetHost()->delegate()->Paste();
- title_update_watcher_->Wait();
+ EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
}
void PerformSelectAll() {
- title_update_watcher_->InitWait("SELECTION_CHANGED");
+ const base::string16 expected_title =
+ base::UTF8ToUTF16("SELECTION_CHANGED");
+ content::TitleWatcher title_watcher(shell()->web_contents(),
+ expected_title);
RenderFrameHostImpl* rfh =
static_cast<RenderFrameHostImpl*>(interstitial_->GetMainFrame());
rfh->GetRenderWidgetHost()->delegate()->SelectAll();
- title_update_watcher_->Wait();
+ EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle());
}
private:
@@ -344,7 +294,6 @@
std::unique_ptr<InterstitialPageImpl> interstitial_;
scoped_refptr<ClipboardMessageWatcher> clipboard_message_watcher_;
- scoped_refptr<InterstitialTitleUpdateWatcher> title_update_watcher_;
DISALLOW_COPY_AND_ASSIGN(InterstitialPageImplTest);
};
diff --git a/content/browser/frame_host/navigation_controller_impl_unittest.cc b/content/browser/frame_host/navigation_controller_impl_unittest.cc
index a2c74bd..1655c5f 100644
--- a/content/browser/frame_host/navigation_controller_impl_unittest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_unittest.cc
@@ -5039,7 +5039,8 @@
FaviconStatus favicon;
favicon.valid = true;
favicon.url = GURL("http://foo/favicon.ico");
- controller().GetLastCommittedEntry()->SetTitle(title);
+ contents()->UpdateTitleForEntry(
+ controller().GetLastCommittedEntry(), title);
controller().GetLastCommittedEntry()->GetFavicon() = favicon;
// history.pushState() is called.
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 9b445c8..8facc1c 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -3893,7 +3893,7 @@
max_restored_page_id);
}
-bool WebContentsImpl::UpdateTitleForEntry(NavigationEntryImpl* entry,
+void WebContentsImpl::UpdateTitleForEntry(NavigationEntry* entry,
const base::string16& title) {
// For file URLs without a title, use the pathname instead. In the case of a
// synthesized title, we don't want the update to count toward the "one set
@@ -3913,12 +3913,12 @@
// |page_title_when_no_navigation_entry_| will be used for page title.
if (entry) {
if (final_title == entry->GetTitle())
- return false; // Nothing changed, don't bother.
+ return; // Nothing changed, don't bother.
entry->SetTitle(final_title);
} else {
if (page_title_when_no_navigation_entry_ == final_title)
- return false; // Nothing changed, don't bother.
+ return; // Nothing changed, don't bother.
page_title_when_no_navigation_entry_ = final_title;
}
@@ -3929,7 +3929,9 @@
FOR_EACH_OBSERVER(WebContentsObserver, observers_,
TitleWasSet(entry, explicit_set));
- return true;
+ // Broadcast notifications when the UI should be updated.
+ if (entry == controller_.GetEntryAtOffset(0))
+ NotifyNavigationStateChanged(INVALIDATE_TYPE_TITLE);
}
void WebContentsImpl::SendChangeLoadProgress() {
@@ -4535,12 +4537,7 @@
// TODO(evan): make use of title_direction.
// http://code.google.com/p/chromium/issues/detail?id=27094
- if (!UpdateTitleForEntry(entry, title))
- return;
-
- // Broadcast notifications when the UI should be updated.
- if (entry == controller_.GetEntryAtOffset(0))
- NotifyNavigationStateChanged(INVALIDATE_TYPE_TITLE);
+ UpdateTitleForEntry(entry, title);
}
void WebContentsImpl::UpdateEncoding(RenderFrameHost* render_frame_host,
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
index dca061eb..c99ec23 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -278,6 +278,8 @@
bool IsFullAccessibilityModeForTesting() const override;
const PageImportanceSignals& GetPageImportanceSignals() const override;
const base::string16& GetTitle() const override;
+ void UpdateTitleForEntry(NavigationEntry* entry,
+ const base::string16& title) override;
int32_t GetMaxPageID() override;
int32_t GetMaxPageIDForSiteInstance(SiteInstance* site_instance) override;
SiteInstanceImpl* GetSiteInstance() const override;
@@ -803,9 +805,6 @@
// So |find_request_manager_| can be accessed for testing.
friend class FindRequestManagerTest;
- // So InterstitialPageImpl can access SetIsLoading.
- friend class InterstitialPageImpl;
-
// TODO(brettw) TestWebContents shouldn't exist!
friend class TestWebContents;
@@ -997,17 +996,6 @@
// have begun, to prevent any races in updating RenderView::next_page_id.
void UpdateMaxPageIDIfNecessary(RenderViewHost* rvh);
- // Saves the given title to the navigation entry and does associated work. It
- // will update history and the view for the new title, and also synthesize
- // titles for file URLs that have none (so we require that the URL of the
- // entry already be set).
- //
- // This is used as the backend for state updates, which include a new title,
- // or the dedicated set title message. It returns true if the new title is
- // different and was therefore updated.
- bool UpdateTitleForEntry(NavigationEntryImpl* entry,
- const base::string16& title);
-
// Helper for CreateNewWidget/CreateNewFullscreenWidget.
void CreateNewWidget(int32_t render_process_id,
int32_t route_id,
diff --git a/content/public/browser/navigation_entry.h b/content/public/browser/navigation_entry.h
index 92f020f9..87c17cd 100644
--- a/content/public/browser/navigation_entry.h
+++ b/content/public/browser/navigation_entry.h
@@ -87,6 +87,10 @@
// The caller is responsible for detecting when there is no title and
// displaying the appropriate "Untitled" label if this is being displayed to
// the user.
+ // Use WebContents::UpdateTitleForEntry() in most cases, since that notifies
+ // observers when the visible title changes. Only call
+ // NavigationEntry::SetTitle() below directly when this entry is known not to
+ // be visible.
virtual void SetTitle(const base::string16& title) = 0;
virtual const base::string16& GetTitle() const = 0;
diff --git a/content/public/browser/web_contents.h b/content/public/browser/web_contents.h
index 36996f0..65de3da 100644
--- a/content/public/browser/web_contents.h
+++ b/content/public/browser/web_contents.h
@@ -317,6 +317,12 @@
// download, in which case the URL would revert to what it was previously).
virtual const base::string16& GetTitle() const = 0;
+ // Saves the given title to the navigation entry and does associated work. It
+ // will update history and the view with the new title, and also synthesize
+ // titles for file URLs that have none. Thus |entry| must have a URL set.
+ virtual void UpdateTitleForEntry(NavigationEntry* entry,
+ const base::string16& title) = 0;
+
// The max page ID for any page that the current SiteInstance has loaded in
// this WebContents. Page IDs are specific to a given SiteInstance and
// WebContents, corresponding to a specific RenderView in the renderer.
diff --git a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc
index 13f7dc3..a58e021b 100644
--- a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc
+++ b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc
@@ -182,7 +182,8 @@
content::NavigationEntry* last_committed_entry =
embedder_web_contents()->GetController().GetLastCommittedEntry();
if (last_committed_entry) {
- last_committed_entry->SetTitle(source->GetTitle());
+ embedder_web_contents()->UpdateTitleForEntry(last_committed_entry,
+ source->GetTitle());
embedder_web_contents()->GetDelegate()->NavigationStateChanged(
embedder_web_contents(), changed_flags);
}