Use WebFrame::loadRequest for reloads and history navigations

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 4354 matching lines @@
   if (is_reload && !render_view_->history_controller()->GetCurrentEntry()) {
     // We cannot reload if we do not have any history state.  This happens, for
     // example, when recovering from a crash.
     is_reload = false;
     cache_policy = WebURLRequest::ReloadIgnoringCacheData;
   }
 
   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);
+    load_type = ignore_cache ? blink::ReloadFromOrigin : blink::Reload;
 
-    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.
-
+    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,
     // which should be the case because history navigations are routed via the
     // browser.
     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(),
                                             "\n");
            i.GetNext();) {
         request.addHTTPHeaderField(WebString::fromUTF8(i.name()),
                                    WebString::fromUTF8(i.values()));
       }
     }
 
     if (start_params.is_post && !browser_side_navigation) {
       request.setHTTPMethod(WebString::fromUTF8("POST"));
 
       // Set post data.
       WebHTTPBody http_body;
       http_body.initialize();
       const char* data = nullptr;
       if (start_params.browser_initiated_post_data.size()) {
         data = reinterpret_cast<const char*>(
             &start_params.browser_initiated_post_data.front());
       }
       http_body.appendData(
           WebData(data, start_params.browser_initiated_post_data.size()));
       request.setHTTPBody(http_body);
     }
 
     // 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();
 }
 
 void RenderFrameImpl::UpdateEncoding(WebFrame* frame,
                                      const std::string& encoding_name) {
   // Only update main frame's encoding_name.
   if (!frame->parent())
     Send(new FrameHostMsg_UpdateEncoding(routing_id_, encoding_name));
 }
@@ skipping 402 matching lines @@
 #elif defined(ENABLE_BROWSER_CDMS)
         cdm_manager_,
 #endif
         this);
   }
 
   return cdm_factory_;
 }
 
 }  // namespace content