PlzNavigate: track pending commits

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 29771b2ab65da73c748a60643a0634acce2ec5a2..d8242acb803cacc26d40ab37983ea52e11827923 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -731,13 +731,15 @@ void RenderFrameHostManager::BeginNavigation(const NavigationRequest& request) {
   // navigations.
   CleanUpNavigation();
 
-  RenderFrameHostImpl* dest_rfh = GetFrameHostForNavigation(request);
+  RenderFrameHostImpl* dest_rfh = GetFrameHostForNavigation(request, false);
+  cross_navigation_pending_ = false;
   DCHECK(dest_rfh);
 }
 
 // PlzNavigate
 RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
-    const NavigationRequest& request) {
+    const NavigationRequest& request,
+    bool for_commit) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
 
@@ -827,7 +829,9 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
     }
   }
 
-  cross_navigation_pending_ = navigation_rfh != render_frame_host_.get();
+  if (for_commit)

[nasko, 2015/03/30 22:35:22]

Can't this be replaced by an earlier check for existence of speculative RFH?
Alternatively, it is called from only two places, the one passing in false
already is resetting it to false after this call.
It will be ideal if we don't have to add this new parameter to the method.

[clamy, 2015/03/31 12:58:17]

I'm now resetting it to false in BeginNavigation (the check on the speculative
RFH would not cover all possible cases).

[carlosk, 2015/03/31 13:01:32]

I think the explicit parameter is a better choice than the implicit knowledge
that this method will only be called twice. And even if relying on this fact I
don't think there's a way to detect if |cross_navigation_pending_| should be
true or false without adding an extra member variable (bool
has_already_been_called).

So IMO the problem is in fact that BeginNavigation is also setting  to false.

In this case, there's a nit: instead of the if-check here, this could be
replaced by having this line:
cross_navigation_pending_ = for_commit;
inside the if-block above where the speculative RFH is created.

+    cross_navigation_pending_ = navigation_rfh != render_frame_host_.get();
+
   return navigation_rfh;
 }