PlzNavigate: send history params at commit time to the renderer

content/renderer/render_frame_impl.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/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 
 #include "base/auto_reset.h"
@@ skipping 1012 matching lines @@
   return handled;
 }
 
 void RenderFrameImpl::OnNavigate(const FrameMsg_Navigate_Params& params) {
   TRACE_EVENT2("navigation", "RenderFrameImpl::OnNavigate",
                "id", routing_id_,
                "url", params.common_params.url.possibly_invalid_spec());
 
   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_;
   if (!params.frame_to_navigate.empty()) {
     // TODO(nasko): Move this lookup to the browser process.
     frame = render_view_->webview()->findFrameByName(
         WebString::fromUTF8(params.frame_to_navigate));
     CHECK(frame) << "Invalid frame name passed: " << params.frame_to_navigate;
   }
 
@@ skipping 17 matching lines @@
          FrameMsg_Navigate_Type::RELOAD_ORIGINAL_REQUEST_URL);
     bool ignore_cache = (params.common_params.navigation_type ==
                          FrameMsg_Navigate_Type::RELOAD_IGNORING_CACHE);
 
     if (reload_original_url)
       frame->reloadWithOverrideURL(params.common_params.url, true);
     else
       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://.
       CHECK(entry->root().urlString() != WebString::fromUTF8(kSwappedOutURL));
       render_view_->history_controller()->GoToEntry(entry.Pass(), cache_policy);
     }
   } else if (!params.common_params.base_url_for_data_url.is_empty()) {
     LoadDataURL(params.common_params, frame);
   } else {
     // Navigate to the given URL.
@@ skipping 22 matching lines @@
       if (params.browser_initiated_post_data.size()) {
         data = reinterpret_cast<const char*>(
             &params.browser_initiated_post_data.front());
       }
       http_body.appendData(
           WebData(data, params.browser_initiated_post_data.size()));
       request.setHTTPBody(http_body);
     }
 
     // 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.
     base::TimeTicks renderer_navigation_start = base::TimeTicks::Now();
     frame->loadRequest(request);
 
     UpdateFrameNavigationTiming(
         frame, params.commit_params.browser_navigation_start,
         renderer_navigation_start);
   }
@@ skipping 1262 matching lines @@
   bool replace =
       navigation_state->pending_page_id() != -1 ||
       ui::PageTransitionCoreTypeIs(navigation_state->transition_type(),
                                ui::PAGE_TRANSITION_AUTO_SUBFRAME);
 
   // If we failed on a browser initiated request, then make sure that our error
   // page load is regarded as the same browser initiated request.
   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();
     render_view_->pending_navigation_params_->request_time =
         document_state->request_time();
     render_view_->pending_navigation_params_->should_replace_current_entry =
         replace;
   }
 
   // Load an error page.
@@ skipping 1417 matching lines @@
 void RenderFrameImpl::FocusedNodeChangedForAccessibility(const WebNode& node) {
   if (renderer_accessibility())
     renderer_accessibility()->AccessibilityFocusedNodeChanged(node);
 }
 
 // PlzNavigate
 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;
   }
 
   GetContentClient()->SetActiveURL(common_params.url);
 
   if (!common_params.base_url_for_data_url.is_empty() ||
       common_params.url.SchemeIs(url::kDataScheme)) {
     LoadDataURL(common_params, frame_);
     return;
   }
@@ skipping 408 matching lines @@
       new MediaStreamRendererFactory());
 #else
   return scoped_ptr<MediaStreamRendererFactory>(
       static_cast<MediaStreamRendererFactory*>(NULL));
 #endif
 }
 
 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);
   if (!render_view_->webview())
     return false;
 
   FOR_EACH_OBSERVER(
       RenderViewObserver, render_view_->observers_, Navigate(url));
 
   // If this is a stale back/forward (due to a recent navigation the browser
   // 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;
   }
 
   if (!is_swapped_out_ || frame_->parent())
     return true;
 
   // This is a swapped out main frame, so swap the renderer back in.
   // We marked the view as hidden when swapping the view out, so be sure to
   // reset the visibility state before navigating to the new URL.
   render_view_->webview()->setVisibilityState(
       render_view_->visibilityState(), false);
 
   // If this is an attempt to reload while we are swapped out, we should not
   // reload swappedout://, but the previous page, which is stored in
   // params.state.  Setting is_reload to false will treat this like a back
   // navigation to accomplish that.
   *is_reload = false;
   *cache_policy = WebURLRequest::ReloadIgnoringCacheData;
 
   // We refresh timezone when a view is swapped in since timezone
   // can get out of sync when the system timezone is updated while
   // the view is swapped out.
   RenderThreadImpl::NotifyTimezoneChange();
 
   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;
 }
 
 void RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   DCHECK(request);
   // TODO(clamy): Execute the beforeunload event.
 
   // Note: At this stage, the goal is to apply all the modifications the
@@ skipping 105 matching lines @@
 
 #if defined(ENABLE_BROWSER_CDMS)
 RendererCdmManager* RenderFrameImpl::GetCdmManager() {
   if (!cdm_manager_)
     cdm_manager_ = new RendererCdmManager(this);
   return cdm_manager_;
 }
 #endif  // defined(ENABLE_BROWSER_CDMS)
 
 }  // namespace content