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

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/task_management/providers/web_contents/web_contents_task_provider.h"
 
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/stl_util.h"
 #include "chrome/browser/task_management/providers/web_contents/subframe_task.h"
@@ skipping 37 matching lines @@
   RendererTask* GetTaskForFrame(RenderFrameHost* render_frame_host) const;
 
   // content::WebContentsObserver:
   void RenderFrameDeleted(RenderFrameHost* render_frame_host) override;
   void RenderFrameHostChanged(RenderFrameHost* old_host,
                               RenderFrameHost* new_host) override;
   void RenderViewReady() override;
   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
   // corresponding task for the given |render_frame_host| and notifying the
   // provider's observer of the new task.
   void CreateTaskForFrame(RenderFrameHost* render_frame_host);
 
   // Clears the task that corresponds to the given |render_frame_host| and
   // notifies the provider's observer of the tasks removal.
   void ClearTaskForFrame(RenderFrameHost* render_frame_host);
@@ skipping 84 matching lines @@
   ClearAllTasks(true);
   provider_->DeleteEntry(web_contents());
 }
 
 void WebContentsEntry::RenderProcessGone(base::TerminationStatus status) {
   ClearAllTasks(true);
 }
 
 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;
-  }
 
-  itr->second->UpdateTitle();
+  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;
+
+  task->UpdateTitle();
+  task->UpdateFavicon();
 }
 
 void WebContentsEntry::CreateTaskForFrame(RenderFrameHost* render_frame_host) {
   DCHECK(!ContainsKey(tasks_by_frames_, render_frame_host));
 
   content::SiteInstance* site_instance = render_frame_host->GetSiteInstance();
   if (!site_instance->GetProcess()->HasConnection())
     return;
 
   bool site_instance_exists =
@@ skipping 175 matching lines @@
 
   WebContentsEntry* entry = itr->second;
   entries_map_.erase(itr);
 
   // The entry we're about to delete is our caller, however its' still fine to
   // delete it.
   delete entry;
 }
 
 }  // namespace task_management