PlzNavigate: Improvements to RFHM commit logic.

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 442 matching lines @@
 }
 
 void RenderFrameHostManager::CommitPendingIfNecessary(
     RenderFrameHostImpl* render_frame_host,
     bool was_caused_by_user_gesture) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableBrowserSideNavigation)) {
     if (render_frame_host == speculative_render_frame_host_.get()) {
       CommitPending();
     } else if (render_frame_host == render_frame_host_.get()) {
-      // TODO(carlosk): this code doesn't properly handle in-page navigation or
-      // interwoven navigation requests.
-      DCHECK(!speculative_render_frame_host_);
+      DCHECK(!should_reuse_web_ui_ || web_ui_);
+      // TODO(carlosk): this code might not handle interwoven navigation

[clamy, 2015/03/31 11:40:42]

Could you add a comment explaining that even if there is no speculative RFH
there may be a speculative webui to commit?

[carlosk, 2015/03/31 14:19:39]

Done.

+      // requests properly.

[carlosk, 2015/03/27 17:11:29]

Due to the possibility of having multiple ongoing navigations and the
asynchronous nature of message passing there still are situations where the code
below won't do the right thing.

[clamy, 2015/03/31 11:40:42]

Yes we should maybe think about pausing a navigation request when we are in the
process of committing something just to make things clearer.

[carlosk, 2015/03/31 14:19:39]

Let's talk offline about this as I don't understand what you meant by "pausing a
navigation".

+      if (speculative_web_ui_ && !speculative_render_frame_host_) {
+        CommitPending();
+      } else {
+        CleanUpNavigation();
+      }
     } else {
       // No one else should be sending us a DidNavigate in this state.
       DCHECK(false);
     }
     DCHECK(!speculative_render_frame_host_);
     return;
   }
 
   if (!cross_navigation_pending_) {
     DCHECK(!pending_render_frame_host_);
@@ skipping 1154 matching lines @@
     if (pending_web_ui_) {
       web_ui_.reset(pending_web_ui_.release());
     } else if (!pending_and_current_web_ui_.get()) {
       web_ui_.reset();
     } else {
       DCHECK_EQ(pending_and_current_web_ui_.get(), web_ui_.get());
       pending_and_current_web_ui_.reset();
     }
   } else {
     // PlzNavigate
-    if (!should_reuse_web_ui_)
+    if (speculative_web_ui_) {
+      DCHECK(!should_reuse_web_ui_);
       web_ui_.reset(speculative_web_ui_.release());
+    } else if (should_reuse_web_ui_) {

[clamy, 2015/03/31 11:40:42]

At which point is should_reuse_web_ui_ reset?

[carlosk, 2015/03/31 14:19:39]

You are right to bring this up: this is the place.

+      CHECK(web_ui_);

[clamy, 2015/03/31 11:40:42]

I wonder if this should be a DCHECK seeing that all other webui related checks
are DCHECKs instead of CHECKs.

[carlosk, 2015/03/31 14:19:39]

Done.

+    } else {
+      web_ui_.reset();
+    }

[carlosk, 2015/03/27 17:11:30]

This code does basically the same as the previous one but looks way clearer. I
was really confused until I found out that speculative_web_ui_.release() won't
raise an exception if it points to NULL (and correctly caused the reset of
|web_ui_|).

     DCHECK(!speculative_web_ui_);
   }
 
   // It's possible for the pending_render_frame_host_ to be nullptr when we
   // aren't crossing process boundaries. If so, we just needed to handle the Web
   // UI committing above and we're done.
   if (!pending_render_frame_host_ && !speculative_render_frame_host_) {
     if (will_focus_location_bar)
       delegate_->SetFocusToLocationBar(false);
     return;
@@ skipping 367 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