Use WebFrame::loadRequest for reloads and history navigations

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 c2abbffa402e075d28f6c724e74b01c634bd8d45..e152bed005d7470973c38c56abc607f3b7308442 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -4372,27 +4372,37 @@ void RenderFrameImpl::NavigateInternal(
   pending_navigation_params_.reset(
       new NavigationParams(common_params, start_params, request_params));
 
-  // If we are reloading, then Blink will use the history state of the current
-  // page, so we should just ignore any given history state.  Otherwise, if we
-  // have history state, then we need to navigate to it, which corresponds to a
-  // back/forward navigation event.
-  if (is_reload && !browser_side_navigation) {
-    // TODO(clamy): adapt this code for PlzNavigate. In particular the stream
-    // override should be given to the generated request.
-    bool reload_original_url =
-        (common_params.navigation_type ==
-         FrameMsg_Navigate_Type::RELOAD_ORIGINAL_REQUEST_URL);
+  // Create parameters for a standard navigation.
+  blink::WebFrameLoadType load_type = blink::Standard;
+  WebHistoryItem item_for_history_navigation;
+  WebURLRequest request = CreateURLRequestForNavigation(
+      common_params, stream_params.Pass(), frame_->isViewSourceModeEnabled());
+
+  // PlzNavigate: Make sure that Blink's loader will not try to use browser side
+  // navigation for this request (since it already went to the browser).
+  if (browser_side_navigation)
+    request.setCheckForBrowserSideNavigation(false);
+
+  // If we are reloading, then use the history state of the current frame.
+  // Otherwise, if we have history state, then we need to navigate to it, which
+  // corresponds to a back/forward navigation event. Update the parameters
+  // depending on the navigation type.
+  bool perform_load_url = false;

[Charlie Reis, 2015/06/02 16:27:45]

nit: should_load_request

Also move up higher, probably before load_type.  Right now it's under the reload
comment, but it's not specific to that.

[clamy, 2015/06/03 15:06:19]

Done.

+  if (is_reload) {
     bool ignore_cache = (common_params.navigation_type ==
                          FrameMsg_Navigate_Type::RELOAD_IGNORING_CACHE);
-
-    if (reload_original_url)
-      frame_->reloadWithOverrideURL(common_params.url, true);
-    else
-      frame_->reload(ignore_cache);
-  } else if (is_history_navigation && !browser_side_navigation) {
-    // TODO(clamy): adapt this code for PlzNavigate. In particular the stream
-    // override should be given to the generated request.
-
+    load_type = ignore_cache ? blink::ReloadFromOrigin : blink::Reload;
+
+    if (!browser_side_navigation) {

[Charlie Reis, 2015/06/02 16:27:45]

Sanity check: Is RELOAD_ORIGINAL_REQUEST_URL handled earlier for PlzNavigate? 
That would make sense to me.

[clamy, 2015/06/03 15:06:19]

That should be handled on the browser-side. We get here when the request as
already been made and the response is ready.

+      const GURL override_url =
+          (common_params.navigation_type ==
+           FrameMsg_Navigate_Type::RELOAD_ORIGINAL_REQUEST_URL)
+              ? common_params.url
+              : GURL();
+      request = frame_->requestForReload(load_type, override_url);
+    }
+    perform_load_url = true;
+  } else if (is_history_navigation) {
     // We must know the page ID of the page we are navigating back to.
     DCHECK_NE(request_params.page_id, -1);
     // We must know the nav entry ID of the page we are navigating back to,
@@ -4405,20 +4415,27 @@ void RenderFrameImpl::NavigateInternal(
       // 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));
-      scoped_ptr<NavigationParams> navigation_params(
-          new NavigationParams(*pending_navigation_params_.get()));
-      render_view_->history_controller()->GoToEntry(
-          entry.Pass(), navigation_params.Pass(), cache_policy);
+
+      if (!browser_side_navigation) {
+        scoped_ptr<NavigationParams> navigation_params(
+            new NavigationParams(*pending_navigation_params_.get()));
+        render_view_->history_controller()->GoToEntry(
+            entry.Pass(), navigation_params.Pass(), cache_policy);
+      } else {
+        // TODO(clamy): this should be set to the HistoryItem sent by the
+        // browser
+        // once the HistoryController has moved to the browser.

[Charlie Reis, 2015/06/02 16:27:45]

nit: Move to previous line.

[clamy, 2015/06/03 15:06:19]

Done.

+        // TODO(clamy): distinguish between different document and same document
+        // loads.
+        // TODO(clamy): update this for subframes history loads.
+        item_for_history_navigation =
+            entry->GetHistoryNodeForFrame(this)->item();
+        load_type = blink::BackForward;
+        perform_load_url = true;
+      }
     }
-  } else if (!common_params.base_url_for_data_url.is_empty() ||
-             (browser_side_navigation &&
-              common_params.url.SchemeIs(url::kDataScheme))) {
-    LoadDataURL(common_params, frame_);
   } else {
     // Navigate to the given URL.
-    WebURLRequest request = CreateURLRequestForNavigation(
-        common_params, stream_params.Pass(), frame_->isViewSourceModeEnabled());
-
     if (!start_params.extra_headers.empty() && !browser_side_navigation) {
       for (net::HttpUtil::HeadersIterator i(start_params.extra_headers.begin(),
                                             start_params.extra_headers.end(),
@@ -4448,18 +4465,24 @@ void RenderFrameImpl::NavigateInternal(
     // A session history navigation should have been accompanied by state.
     CHECK_EQ(request_params.page_id, -1);
 
-    // PlzNavigate: Make sure that Blink's loader will not try to use browser
-    // side navigation for this request (since it already went to the browser).
-    if (browser_side_navigation)
-      request.setCheckForBrowserSideNavigation(false);
-
     // 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_, request_params.browser_navigation_start,

[Charlie Reis, 2015/06/02 16:27:45]

What are the implications of calling this before the loadRequest call?

[clamy, 2015/06/03 15:06:19]

That could be a problem since it checks for the provisional data source of the
frame. Moved it to after the loadRequest call for normal navigations.

                                 renderer_navigation_start);
+    perform_load_url = true;
+  }
+
+  if (perform_load_url) {
+    // Perform a navigation to a data url if needed.
+    if (!common_params.base_url_for_data_url.is_empty() ||
+        (browser_side_navigation &&
+         common_params.url.SchemeIs(url::kDataScheme))) {
+      LoadDataURL(common_params, frame_);
+    } else {
+      // Load the request.
+      frame_->loadRequest(request, load_type, item_for_history_navigation);
+    }
   }

[Charlie Reis, 2015/06/02 16:27:45]

Ah.  Looks like you skipped the "else send DidDropNavigation" because of the
HistoryController::GoToEntry case.  I'm ok with that.  It's no worse than what
we have today, and we can possibly add it when GoToEntry moves to the browser
process.

[clamy, 2015/06/03 15:06:19]

Yes that was my thought process as well.

 
   // In case LoadRequest failed before didCreateDataSource was called.