Using WebContents::UpdateTitleForEntry() instead of NavigationEntry::SetTitle()

chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc

Index: chrome/browser/task_management/providers/web_contents/web_contents_task_provider.cc
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 f6862a33f3dd7e1330d7d9d3df792f75de86e2c4..d2015fbe98ace754a45f6ce177b7b857efa8a669 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
@@ -55,10 +55,11 @@ class WebContentsEntry : public content::WebContentsObserver {
   void WebContentsDestroyed() override;
   void RenderProcessGone(base::TerminationStatus status) override;
   void OnRendererUnresponsive(RenderWidgetHost* render_widget_host) override;
-  void DidNavigateMainFrame(
-      const content::LoadCommittedDetails& details,
-      const content::FrameNavigateParams& params) override;
+  void DidFinishNavigation(
+      content::NavigationHandle* navigation_handle) override;

[Charlie Reis, 2016/06/24 22:21:17]

Hooray!  Thanks for moving to the new NavHandle-based API.

[afakhry, 2016/06/27 14:29:18]

Yes, Nick told me DidNavigateMainFrame() will be deprecated.

   void TitleWasSet(content::NavigationEntry* entry, bool explicit_set) override;
+  void DidAttachInterstitialPage() override;
+  void DidDetachInterstitialPage() override;
 
  private:
   // Defines a callback for WebContents::ForEachFrame() to create a
@@ -163,49 +164,60 @@ void WebContentsEntry::RenderProcessGone(base::TerminationStatus status) {
 
 void WebContentsEntry::OnRendererUnresponsive(
     RenderWidgetHost* render_widget_host) {
-  auto itr = tasks_by_frames_.find(web_contents()->GetMainFrame());
-  if (itr == tasks_by_frames_.end())
+  RendererTask* task = GetTaskForFrame(web_contents()->GetMainFrame());
+  if (!task)
     return;
 
   DCHECK_EQ(render_widget_host->GetProcess(),
             web_contents()->GetMainFrame()->GetProcess());

[Charlie Reis, 2016/06/24 22:21:17]

This line caught my eye, since it looks buggy for OOPIFs.  And indeed, I get a
crash here in debug builds with the following repro steps:

1) Start Chrome with --site-per-process and open Task Manager.
2) Visit http://csreis.github.io/tests/cross-site-iframe.html.
3) Click "Go cross-site (simple page)"
4) Right click inside the iframe and open DevTools.  (Important, so that we're
inspecting the OOPIF's page.)
5) Type "while (1);" in the console.
6) Try to select text in the iframe and wait 20 seconds.

I'll file this as a separate bug so that we can keep this CL focused on titles.

[afakhry, 2016/06/27 14:29:18]

I saw the bug you filed, and I will handle it soon. Thanks for letting me know
about this issue.

 
-  provider_->NotifyObserverTaskUnresponsive(itr->second);
+  provider_->NotifyObserverTaskUnresponsive(task);
 }
 
-void WebContentsEntry::DidNavigateMainFrame(
-    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();
+void WebContentsEntry::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  RendererTask* task = GetTaskForFrame(web_contents()->GetMainFrame());
+  if (!task)
     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
   // the title here as well.
-  itr->second->UpdateTitle();
+  task->UpdateTitle();
 
   // Call RendererTask::UpdateFavicon() to set the current favicon to the
   // default favicon. If the page has a non-default favicon,
   // RendererTask::OnFaviconUpdated() will update the current favicon once
   // FaviconDriver figures out the correct favicon for the page.
-  itr->second->UpdateFavicon();
-  itr->second->UpdateRapporSampleName();
+  task->UpdateFavicon();
+  task->UpdateRapporSampleName();
 }
 
 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();
+  RendererTask* task = GetTaskForFrame(web_contents()->GetMainFrame());
+  if (!task)
+    return;
+
+  task->UpdateTitle();
+}
+
+void WebContentsEntry::DidAttachInterstitialPage() {

[Charlie Reis, 2016/06/24 22:21:17]

Not sure I understand-- why do we need DidAttachInterstitialPage and
DidDetachInterstitialPage in addition to the changes to how TitleWasSet is
called?

[afakhry, 2016/06/27 14:29:18]

Updating titles here alone is not enough. It will update the title to the link
"malware.testing.google.test/testing/malware" but not the actual title "Security
Error". We can probably skip the title update here, since now we have it in
TitleWasSet().

However, we need this for updating the Favicon.

[Charlie Reis, 2016/06/28 00:47:09]

I think I'm ok with that.  Just curious, would we still need it if we put a
UpdateFavicon in TitleWasSet?  That might be a bit cleaner and less brittle if
other places trigger TitleWasSet, assuming that titles and favicons often (not
always) change at the same time.

[afakhry, 2016/06/28 14:34:16]

No we won't need listening to the attach/detach events if we UpdateFavicon()
inside TitleWasSet().

Done.

+  RendererTask* task = GetTaskForFrame(web_contents()->GetMainFrame());
+  if (!task)
+    return;
+
+  task->UpdateTitle();
+  task->UpdateFavicon();
+}
+
+void WebContentsEntry::DidDetachInterstitialPage() {
+  RendererTask* task = GetTaskForFrame(web_contents()->GetMainFrame());
+  if (!task)
     return;
-  }
 
-  itr->second->UpdateTitle();
+  task->UpdateTitle();
+  task->UpdateFavicon();
 }
 
 void WebContentsEntry::CreateTaskForFrame(RenderFrameHost* render_frame_host) {