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.
+  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);
+    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) {

[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,
     // 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));
-      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(),
                                             "\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,

[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.
   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 403 matching lines @@
 #elif defined(ENABLE_BROWSER_CDMS)
         cdm_manager_,
 #endif
         this);
   }
 
   return cdm_factory_;
 }
 
 }  // namespace content