OOPIF: report both original and destination rfh in AboutToNavigateRenderFrame.

content/browser/devtools/render_view_devtools_agent_host.cc

Index: content/browser/devtools/render_view_devtools_agent_host.cc
diff --git a/content/browser/devtools/render_view_devtools_agent_host.cc b/content/browser/devtools/render_view_devtools_agent_host.cc
index e6a12ca401c62876e69d7a3fc87d643aaa09981d..a18e37c425d0491eb8495f4e18314f4d93bec8bc 100644
--- a/content/browser/devtools/render_view_devtools_agent_host.cc
+++ b/content/browser/devtools/render_view_devtools_agent_host.cc
@@ -45,19 +45,23 @@ typedef std::vector<RenderViewDevToolsAgentHost*> Instances;
 namespace {
 base::LazyInstance<Instances>::Leaky g_instances = LAZY_INSTANCE_INITIALIZER;
 
-// Returns RenderViewDevToolsAgentHost attached to any of RenderFrameHost
-// instances associated with |web_contents|
-static RenderViewDevToolsAgentHost* FindAgentHost(WebContents* web_contents) {
+static RenderViewDevToolsAgentHost* FindAgentHost(RenderFrameHost* host) {
   if (g_instances == NULL)
     return NULL;
   for (Instances::iterator it = g_instances.Get().begin();
        it != g_instances.Get().end(); ++it) {
-    if ((*it)->GetWebContents() == web_contents)
+    if ((*it)->HasRenderFrameHost(host))
       return *it;
   }
   return NULL;
 }
 
+// Returns RenderViewDevToolsAgentHost attached to any of RenderFrameHost

[dgozman, 2015/01/22 13:54:09]

The comment is wrong. It's only for the main frame. Since behavior changed, did
you regress something?

[pfeldman, 2015/01/22 22:10:05]

Done.

+// instances associated with |web_contents|
+static RenderViewDevToolsAgentHost* FindAgentHost(WebContents* web_contents) {
+  return FindAgentHost(web_contents->GetMainFrame());
+}
+
 }  // namespace
 
 scoped_refptr<DevToolsAgentHost>
@@ -105,11 +109,7 @@ std::vector<WebContents*> DevToolsAgentHostImpl::GetInspectableWebContents() {
 void RenderViewDevToolsAgentHost::OnCancelPendingNavigation(
     RenderFrameHost* pending,
     RenderFrameHost* current) {
-  if (current->GetParent())
-    return;
-  WebContents* web_contents =
-      WebContents::FromRenderFrameHost(pending);
-  RenderViewDevToolsAgentHost* agent_host = FindAgentHost(web_contents);
+  RenderViewDevToolsAgentHost* agent_host = FindAgentHost(current);

[dgozman, 2015/01/22 13:54:09]

pending

[pfeldman, 2015/01/22 22:10:05]

Done, but we'll have to carefully manage the state here as planned.

   if (!agent_host)
     return;
   agent_host->DisconnectRenderFrameHost();
@@ -268,30 +268,27 @@ RenderViewDevToolsAgentHost::~RenderViewDevToolsAgentHost() {
 
 // TODO(creis): Consider removing this in favor of RenderFrameHostChanged.

[dgozman, 2015/01/22 13:54:09]

Remove TODO? This (or similar) notification is not going anywhere.

[nasko, 2015/01/22 21:20:12]

I do believe that this notification will go away, so keep the TODO. I'm
currently working on cleaning up/fixing up the WebContentsObserver interfaces
for RenderFrameHosts, so it will be nice to investigate what the requirements
you have are, so I can find a good replacement for this one.

[pfeldman, 2015/01/22 22:10:05]

Leave the TODO for now, as agreed.

 void RenderViewDevToolsAgentHost::AboutToNavigateRenderFrame(
-    RenderFrameHost* render_frame_host) {
-  if (!render_frame_host_)
-    return;
-  if (render_frame_host->GetParent())
+    RenderFrameHost* old_host,
+    RenderFrameHost* new_host) {
+  if (render_frame_host_ != old_host)
     return;
 
   // TODO(creis): This will need to be updated for --site-per-process, since
   // RenderViewHost is going away and navigations could happen in any frame.
-  if (render_frame_host_ == render_frame_host) {
+  if (render_frame_host_ == new_host) {
     RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(
         render_frame_host_->GetRenderViewHost());
     if (rvh->render_view_termination_status() ==
             base::TERMINATION_STATUS_STILL_RUNNING)
       return;
   }
-  ReattachToRenderFrameHost(render_frame_host);
+  ReattachToRenderFrameHost(new_host);
 }
 
 void RenderViewDevToolsAgentHost::RenderFrameHostChanged(
     RenderFrameHost* old_host,
     RenderFrameHost* new_host) {
-  if (new_host->GetParent())
-    return;
-  if (new_host != render_frame_host_) {
+  if (old_host == render_frame_host_ && new_host != render_frame_host_) {
     // AboutToNavigateRenderFrame was not called for renderer-initiated

[nasko, 2015/01/22 21:20:12]

Since you already handle this case, why is AboutToNavigateRenderFrame needed?

[pfeldman, 2015/01/22 22:10:05]

It is too late, we've already failed to seed the right state in the renderer.
This one is a fallback basically and I'd need to look at the bug it was fixing
to find out the call stack.

     // navigation.
     ReattachToRenderFrameHost(new_host);
@@ -401,7 +398,6 @@ void RenderViewDevToolsAgentHost::Observe(int type,
 
 void RenderViewDevToolsAgentHost::SetRenderFrameHost(RenderFrameHost* rfh) {
   DCHECK(!render_frame_host_);
-  DCHECK(!rfh->GetParent());

[dgozman, 2015/01/22 13:54:09]

I think we can keep this until we support child frames.

[pfeldman, 2015/01/22 22:10:04]

Ack. But it is only called for GetMainFrame and I plan to follow up with the
experimental ability to inspect RFH shortly.

   render_frame_host_ = static_cast<RenderFrameHostImpl*>(rfh);
 
   WebContentsObserver::Observe(WebContents::FromRenderFrameHost(rfh));
@@ -504,6 +500,11 @@ void RenderViewDevToolsAgentHost::SynchronousSwapCompositorFrame(
   page_handler_->OnSwapCompositorFrame(frame_metadata);
 }
 
+bool RenderViewDevToolsAgentHost::HasRenderFrameHost(
+    RenderFrameHost* host) {
+  return host == render_frame_host_;
+}
+
 void RenderViewDevToolsAgentHost::OnSaveAgentRuntimeState(
     const std::string& state) {
   if (!render_frame_host_)