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 6c1cf84ece6942b5a972edf6b76d3c96c14840fa..794416886d9cfadd89b0d3bc984a63013cd85a90 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -4372,27 +4372,35 @@ 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.
+  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) {
+      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);
+    }
+  } 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,
@@ -4401,24 +4409,34 @@ 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));
+    if (!entry) {
+      Send(new FrameHostMsg_DidDropNavigation(routing_id_));

[Charlie Reis, 2015/06/02 05:41:42]

This method doesn't feel like it handles early returns well.  We keep having to
reset the pending nav params and possibly send DidDropNavigation.  That's bad
for things like code duplication, potentially missing cases, and updating the
code in the future.

I think this is a case where it would be much easier to maintain if there were a
single block at the end that (1) either called loadRequest or sent
DidDropNavigation, and (2) reset the pending nav params (and did any other
cleanup we might need in the future).

Is it possible to structure it this way, maybe with else blocks or a bool to
decide which action to take for (1) if necessary?  (We could also use another
level of helper methods, though that seems a bit awkward.)

[clamy, 2015/06/02 13:30:13]

I've tried to do something with a boolean and if/else conditions. I'm just a bit
worried that we end up with lots of intricated if conditions.

+      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));
+
+    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);
+      pending_navigation_params_.reset();
+      return;
     }
-  } else if (!common_params.base_url_for_data_url.is_empty() ||
-             (browser_side_navigation &&
-              common_params.url.SchemeIs(url::kDataScheme))) {
-    LoadDataURL(common_params, frame_);
+
+    // TODO(clamy): this should be set to the HistoryItem sent by the browser
+    // once the HistoryController has moved to the browser.
+    // 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;

[Charlie Reis, 2015/06/02 05:41:42]

Worth noting that this won't fully work today (probably only for the main frame,
even if there are subframes), but it seems like it's in the right direction for
where we're heading.  Sounds ok to me for now, since it's just in PlzNavigate.

[clamy, 2015/06/02 13:30:13]

Acknowledged. I'm waiting for the HistoryController change before looking at the
situation with subframes in PlzNavigate.

   } 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,20 +4466,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();
 }