OOPIF: report both original and destination rfh in AboutToNavigateRenderFrame.

content/browser/devtools/render_view_devtools_agent_host.cc

 // Copyright (c) 2012 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 "content/browser/devtools/render_view_devtools_agent_host.h"
 
 #include "base/basictypes.h"
 #include "base/json/json_writer.h"
 #include "base/lazy_instance.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 27 matching lines @@
 #include "content/public/browser/render_widget_host_view.h"
 #endif
 
 namespace content {
 
 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>
 DevToolsAgentHost::GetOrCreateFor(WebContents* web_contents) {
   RenderViewDevToolsAgentHost* result = FindAgentHost(web_contents);
   if (!result)
     result = new RenderViewDevToolsAgentHost(web_contents->GetMainFrame());
   return result;
 }
 
@@ skipping 27 matching lines @@
   }
   std::vector<WebContents*> result(set.size());
   std::copy(set.begin(), set.end(), result.begin());
   return result;
 }
 
 // static
 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();
   agent_host->ConnectRenderFrameHost(current);
 }
 
 RenderViewDevToolsAgentHost::RenderViewDevToolsAgentHost(RenderFrameHost* rfh)
     : render_frame_host_(NULL),
       dom_handler_(new devtools::dom::DOMHandler()),
       input_handler_(new devtools::input::InputHandler()),
@@ skipping 136 matching lines @@
 }
 
 RenderViewDevToolsAgentHost::~RenderViewDevToolsAgentHost() {
   Instances::iterator it = std::find(g_instances.Get().begin(),
                                      g_instances.Get().end(),
                                      this);
   if (it != g_instances.Get().end())
     g_instances.Get().erase(it);
 }
 
 // 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);
   }
 }
 
 void
 RenderViewDevToolsAgentHost::ReattachToRenderFrameHost(RenderFrameHost* rfh) {
   DCHECK(!reattaching_);
   reattaching_ = true;
   DisconnectRenderFrameHost();
@@ skipping 88 matching lines @@
                                           const NotificationSource& source,
                                           const NotificationDetails& details) {
   if (type == content::NOTIFICATION_RENDER_WIDGET_VISIBILITY_CHANGED) {
     bool visible = *Details<bool>(details).ptr();
     page_handler_->OnVisibilityChanged(visible);
   }
 }
 
 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));
   RenderViewHostImpl* rvh = static_cast<RenderViewHostImpl*>(
       rfh->GetRenderViewHost());
   dom_handler_->SetRenderViewHost(rvh);
   input_handler_->SetRenderViewHost(rvh);
   network_handler_->SetRenderViewHost(rvh);
   page_handler_->SetRenderViewHost(rvh);
 
@@ skipping 82 matching lines @@
   page_handler_->OnSwapCompositorFrame(get<1>(param).metadata);
 }
 
 void RenderViewDevToolsAgentHost::SynchronousSwapCompositorFrame(
     const cc::CompositorFrameMetadata& frame_metadata) {
   if (!render_frame_host_)
     return;
   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_)
     return;
   state_ = state;
 }
 
 void RenderViewDevToolsAgentHost::OnDispatchOnInspectorFrontend(
     const std::string& message,
     uint32 total_size) {
   if (!IsAttached() || !render_frame_host_)
     return;
   ProcessChunkedMessageFromAgent(message, total_size);
 }
 
 void RenderViewDevToolsAgentHost::DispatchOnInspectorFrontend(
     const std::string& message) {
   if (!IsAttached() || !render_frame_host_)
     return;
   SendMessageToClient(message);
 }
 
 }  // namespace content