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 842e93f2a4f24ae4df409185e0102c155120a101..f56f4a747340778f5d0622397edae471ca01a3d1 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -4415,27 +4415,42 @@ 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.
+  WebFrame::WebFrameLoadType load_type = WebFrame::WebFrameLoadTypeStandard;
+  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.
+  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 ? WebFrame::WebFrameLoadTypeReloadFromOrigin
+                             : WebFrame::WebFrameLoadTypeReload;
+
+    if (!browser_side_navigation) {
+      WebHistoryItem current_item = frame_->CurrentItem();
+      if (current_item.isNull()) {
+        Send(new FrameHostMsg_DidDropNavigation(routing_id_));

[Charlie Reis, 2015/05/27 23:34:36]

I don't understand this case.  When would this happen?

[clamy, 2015/05/29 14:47:01]

Following interface changes in the blink patch this is no longer needed.

+        pending_navigation_params_.reset();
+        return;
+      }
+      const GURL override_url =
+          (common_params.navigation_type ==
+           FrameMsg_Navigate_Type::RELOAD_ORIGINAL_REQUEST_URL)
+              ? common_params.url
+              : GURL();
+      request = frame_->RequestForReload(current_item, load_type, override_url);
+    }
+  } 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,
@@ -4444,24 +4459,29 @@ void RenderFrameImpl::NavigateInternal(
     DCHECK_NE(0, request_params.nav_entry_id);
     scoped_ptr<HistoryEntry> entry =
         PageStateToHistoryEntry(request_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));
-      scoped_ptr<NavigationParams> navigation_params(
-          new NavigationParams(*pending_navigation_params_.get()));
+    if (!entry) {
+      Send(new FrameHostMsg_DidDropNavigation(routing_id_));

[Charlie Reis, 2015/05/27 23:34:36]

It feels awkward to have this in multiple places, especially when we didn't need
it before.

[clamy, 2015/05/29 14:47:01]

I can remove it. It just seemed weird to me that the renderer did not inform the
browser it did not proceed with the navigation it was asked to do.

+      pending_navigation_params_.reset();
+      return;
+    }
+
+    // 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()));
+    if (!browser_side_navigation) {
       render_view_->history_controller()->GoToEntry(
           entry.Pass(), navigation_params.Pass(), cache_policy);
+      pending_navigation_params_.reset();
+      return;
+    } else {

[Charlie Reis, 2015/05/27 23:34:36]

No else needed after a return.

[clamy, 2015/05/29 14:47:01]

Done.

+      item_for_history_navigation =
+          render_view_->history_controller()->FindItemForFrame(

[Charlie Reis, 2015/05/27 23:34:36]

This isn't going to make sense soon, and I'm not sure I understand it now. 
Soon, the browser process will be telling each frame which history item to load
directly.

For now, we only get to NavigateInternal for the main frame, so this would only
find a single item for the main frame.  How would we handle the case of going
back in a subframe?

[clamy, 2015/05/29 14:47:01]

With PlzNavigate we also get to NavigateInternal on subframes (we get there when
the response for the navigation was received). If we get the history item from
the browser it's even better since we would just set item_for_history_navigation
to whatever we received in the meantime (and no need to find the relevant one).
Once you're done, I will just have to make sure that the proper history item is
sent to the subframes, hence the TODO.

I hope what we are doing is clearer in the new patchset with the call to
GetHistoryNodeForFrame.

+              entry.Pass(), this, cache_policy, &load_type);
     }
-  } 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(),
@@ -4491,20 +4511,25 @@ 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,
                                 renderer_navigation_start);
   }
 
+  // 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_);
+    pending_navigation_params_.reset();
+    return;
+  }
+
+  // Load the request.
+  frame_->loadRequest(request, load_type, item_for_history_navigation);
+
   // In case LoadRequest failed before didCreateDataSource was called.
   pending_navigation_params_.reset();
 }