PlzNavigate: send history params at commit time to the renderer

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 1e3667ff83565f31db7f86e44e961680febabb36..76826ec6765827289f4200e441980ea51d720f63 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -1030,23 +1030,16 @@ void RenderFrameImpl::OnNavigate(const FrameMsg_Navigate_Params& params) {
 
   bool is_reload =
       RenderViewImpl::IsReload(params.common_params.navigation_type);
-  bool is_history_navigation = params.commit_params.page_state.IsValid();
+  bool is_history_navigation = params.history_params.page_state.IsValid();
   WebURLRequest::CachePolicy cache_policy =
       WebURLRequest::UseProtocolCachePolicy;
   if (!RenderFrameImpl::PrepareRenderViewForNavigation(
-      params.common_params.url, true, is_history_navigation,
-      params.current_history_list_offset, &is_reload, &cache_policy)) {
+          params.common_params.url, true, is_history_navigation,
+          params.history_params, &is_reload, &cache_policy)) {
     Send(new FrameHostMsg_DidDropNavigation(routing_id_));
     return;
   }
 
-  render_view_->history_list_offset_ = params.current_history_list_offset;

[clamy, 2015/03/03 18:02:08]

This was moved to PrepareRenderViewForNavigation, so that it is executed by
PlzNavigate as well.

-  render_view_->history_list_length_ = params.current_history_list_length;
-  if (params.should_clear_history_list) {
-    CHECK_EQ(-1, render_view_->history_list_offset_);
-    CHECK_EQ(0, render_view_->history_list_length_);
-  }
-
   GetContentClient()->SetActiveURL(params.common_params.url);
 
   WebFrame* frame = frame_;
@@ -1084,9 +1077,9 @@ void RenderFrameImpl::OnNavigate(const FrameMsg_Navigate_Params& params) {
       frame->reload(ignore_cache);
   } else if (is_history_navigation) {
     // We must know the page ID of the page we are navigating back to.
-    DCHECK_NE(params.page_id, -1);
+    DCHECK_NE(params.history_params.page_id, -1);
     scoped_ptr<HistoryEntry> entry =
-        PageStateToHistoryEntry(params.commit_params.page_state);
+        PageStateToHistoryEntry(params.history_params.page_state);
     if (entry) {
       // Ensure we didn't save the swapped out URL in UpdateState, since the
       // browser should never be telling us to navigate to swappedout://.
@@ -1129,7 +1122,7 @@ void RenderFrameImpl::OnNavigate(const FrameMsg_Navigate_Params& params) {
     }
 
     // A session history navigation should have been accompanied by state.
-    CHECK_EQ(params.page_id, -1);
+    CHECK_EQ(params.history_params.page_id, -1);
 
     // Record this before starting the load, we need a lower bound of this time
     // to sanitize the navigationStart override set below.
@@ -2412,11 +2405,13 @@ void RenderFrameImpl::didFailProvisionalLoad(blink::WebLocalFrame* frame,
   if (!navigation_state->is_content_initiated()) {
     render_view_->pending_navigation_params_.reset(
         new FrameMsg_Navigate_Params);
-    render_view_->pending_navigation_params_->page_id =
+    render_view_->pending_navigation_params_->history_params.page_id =

[Charlie Reis, 2015/03/05 05:01:04]

nit: This is getting a bit unwieldy.  Can you save
render_view_->pending_navigation_params_ in a local to make it easier to read?

[clamy, 2015/03/05 13:15:00]

Done.

         navigation_state->pending_page_id();
-    render_view_->pending_navigation_params_->pending_history_list_offset =
+    render_view_->pending_navigation_params_->history_params
+        .pending_history_list_offset =
         navigation_state->pending_history_list_offset();
-    render_view_->pending_navigation_params_->should_clear_history_list =
+    render_view_->pending_navigation_params_->history_params
+        .should_clear_history_list =
         navigation_state->history_list_was_cleared();
     render_view_->pending_navigation_params_->common_params.transition =
         navigation_state->transition_type();
@@ -3854,17 +3849,17 @@ void RenderFrameImpl::OnCommitNavigation(
     const ResourceResponseHead& response,
     const GURL& stream_url,
     const CommonNavigationParams& common_params,
-    const CommitNavigationParams& commit_params) {
+    const CommitNavigationParams& commit_params,
+    const HistoryNavigationParams& history_params) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   bool is_reload = false;
-  bool is_history_navigation = commit_params.page_state.IsValid();
+  bool is_history_navigation = history_params.page_state.IsValid();
   WebURLRequest::CachePolicy cache_policy =
       WebURLRequest::UseProtocolCachePolicy;
   if (!RenderFrameImpl::PrepareRenderViewForNavigation(
-      common_params.url, false /* check_for_stale_navigation */,
-      is_history_navigation, -1 /* current_history_list_offset; TODO(clamy)*/,
-      &is_reload, &cache_policy)) {
+          common_params.url, true, is_history_navigation, history_params,
+          &is_reload, &cache_policy)) {
     return;
   }
 
@@ -4294,7 +4289,7 @@ bool RenderFrameImpl::PrepareRenderViewForNavigation(
     const GURL& url,
     bool check_for_stale_navigation,
     bool is_history_navigation,

[Avi (use Gerrit), 2015/03/04 21:23:12]

This parameter is no longer needed, as you're passing in |true| for the
PlzNavigate code path.

[Charlie Reis, 2015/03/05 05:01:04]

You meant check_for_stale_navigation, right?

[Avi (use Gerrit), 2015/03/05 05:05:55]

D'oh. Yes, I meant to tag the previous line.

[clamy, 2015/03/05 13:15:00]

Done.

-    int current_history_list_offset,
+    const HistoryNavigationParams& history_params,
     bool* is_reload,
     WebURLRequest::CachePolicy* cache_policy) {
   MaybeHandleDebugURL(url);
@@ -4308,9 +4303,10 @@ bool RenderFrameImpl::PrepareRenderViewForNavigation(
   // didn't know about), ignore it. Only check if swapped in because if the
   // frame is swapped out, it won't commit before asking the browser.
   // TODO(clamy): remove check_for_stale_navigation

[Avi (use Gerrit), 2015/03/04 21:23:12]

Do this TODO (and remove it).

[clamy, 2015/03/05 13:15:00]

Done.

-  if (check_for_stale_navigation &&
-      !render_view_->is_swapped_out() && is_history_navigation &&
-      render_view_->history_list_offset_ != current_history_list_offset) {
+  if (check_for_stale_navigation && !render_view_->is_swapped_out() &&
+      is_history_navigation &&
+      render_view_->history_list_offset_ !=
+          history_params.current_history_list_offset) {
     return false;
   }
 
@@ -4337,6 +4333,16 @@ bool RenderFrameImpl::PrepareRenderViewForNavigation(
 
   render_view_->SetSwappedOut(false);
   is_swapped_out_ = false;
+
+  render_view_->history_list_offset_ =

[Charlie Reis, 2015/03/05 05:01:04]

Putting it here isn't equivalent to where it was before, since we might return
early before getting here.  In fact, we usually return early before getting
here.  (This last block is only about swapped out main frames.)

[clamy, 2015/03/05 13:15:00]

I moved it below the last return to false and added a test that would fail if an
equivalent bug was reintroduced.

+      history_params.current_history_list_offset;
+  render_view_->history_list_length_ =
+      history_params.current_history_list_length;
+  if (history_params.should_clear_history_list) {
+    CHECK_EQ(-1, render_view_->history_list_offset_);
+    CHECK_EQ(0, render_view_->history_list_length_);
+  }
+
   return true;
 }