PlzNavigate: track pending commits

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 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/frame_host/render_frame_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 713 matching lines @@
 
 // PlzNavigate
 void RenderFrameHostManager::BeginNavigation(const NavigationRequest& request) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   // Clean up any state in case there's an ongoing navigation.
   // TODO(carlosk): remove this cleanup here once we properly cancel ongoing
   // 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));
 
   SiteInstance* current_site_instance = render_frame_host_->GetSiteInstance();
 
   scoped_refptr<SiteInstance> dest_site_instance = GetSiteInstanceForNavigation(
       request.common_params().url, request.source_site_instance(),
       request.dest_site_instance(), request.common_params().transition,
       request.restore_type() != NavigationEntryImpl::RESTORE_NONE,
       request.is_view_source());
@@ skipping 69 matching lines @@
   if (!navigation_rfh->IsRenderFrameLive()) {
     // Recreate the opener chain.
     int opener_route_id = delegate_->CreateOpenerRenderViewsForRenderManager(
         navigation_rfh->GetSiteInstance());
     if (!InitRenderView(navigation_rfh->render_view_host(), opener_route_id,
                         MSG_ROUTING_NONE, frame_tree_node_->IsMainFrame())) {
       return nullptr;
     }
   }
 
-  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;
 }
 
 // PlzNavigate
 void RenderFrameHostManager::CleanUpNavigation() {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   speculative_web_ui_.reset();
   should_reuse_web_ui_ = false;
   if (speculative_render_frame_host_)
@@ skipping 1178 matching lines @@
 void RenderFrameHostManager::DeleteRenderFrameProxyHost(
     SiteInstance* instance) {
   RenderFrameProxyHostMap::iterator iter = proxy_hosts_.find(instance->GetId());
   if (iter != proxy_hosts_.end()) {
     delete iter->second;
     proxy_hosts_.erase(iter);
   }
 }
 
 }  // namespace content