PlzNavigate: Implement RequestNavigation on the renderer side

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 983f0723700b621224443f6f7fd914d05cb90418..2ea1a4a4784d576630d31fdffc43003149352d35 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -364,20 +364,47 @@ static bool IsNonLocalTopLevelNavigation(const GURL& url,
 }
 
 WebURLRequest CreateURLRequestForNavigation(
-    const CommonNavigationParams& params,
+    const CommonNavigationParams& common_params,
+    const RequestNavigationParams& request_params,
     scoped_ptr<StreamOverrideParameters> stream_override,
     bool is_view_source_mode_enabled) {
-  WebURLRequest request(params.url);
+  WebURLRequest request(common_params.url);
   if (is_view_source_mode_enabled)
     request.setCachePolicy(WebURLRequest::ReturnCacheDataElseLoad);
 
-  if (params.referrer.url.is_valid()) {
+  if (common_params.referrer.url.is_valid()) {
     WebString web_referrer = WebSecurityPolicy::generateReferrerHeader(
-        params.referrer.policy,
-        params.url,
-        WebString::fromUTF8(params.referrer.url.spec()));
+        common_params.referrer.policy,
+        common_params.url,
+        WebString::fromUTF8(common_params.referrer.url.spec()));
     if (!web_referrer.isEmpty())
-      request.setHTTPReferrer(web_referrer, params.referrer.policy);
+      request.setHTTPReferrer(web_referrer, common_params.referrer.policy);
+  }
+
+  if (!request_params.extra_headers.empty()) {
+    for (net::HttpUtil::HeadersIterator i(request_params.extra_headers.begin(),
+                                          request_params.extra_headers.end(),
+                                          "\n");
+         i.GetNext();) {
+      request.addHTTPHeaderField(WebString::fromUTF8(i.name()),
+                                 WebString::fromUTF8(i.values()));
+    }
+  }
+
+  if (request_params.is_post) {
+    request.setHTTPMethod(WebString::fromUTF8("POST"));
+
+    // Set post data.
+    WebHTTPBody http_body;
+    http_body.initialize();
+    const char* data = NULL;
+    if (request_params.browser_initiated_post_data.size()) {
+      data = reinterpret_cast<const char*>(
+          &request_params.browser_initiated_post_data.front());
+    }
+    http_body.appendData(
+        WebData(data, request_params.browser_initiated_post_data.size()));

[carlosk, 2014/10/08 14:28:55]

Shouldn't this line be moved inside the previous "if" scope? If that's the case
then |data| declaration should also be moved in there.

Edit: later on I noticed this is code was moved from elsewhere so do as you
like.

[clamy, 2014/10/10 02:33:11]

I think I'll keep it as the code originally was.

+    request.setHTTPBody(http_body);
   }
 
   RequestExtraData* extra_data = new RequestExtraData();
@@ -410,6 +437,35 @@ void UpdateFrameNavigationTiming(WebFrame* frame,
   }
 }
 
+// PlzNavigate
+FrameHostMsg_BeginNavigation_Params MakeBeginNavigationParams(
+    const blink::WebURLRequest& request) {
+  FrameHostMsg_BeginNavigation_Params params;
+  params.method = request.httpMethod().latin1();

[nasko, 2014/10/08 17:40:32]

Is latin1() the usual encoding to use?

[clamy, 2014/10/10 02:33:11]

It is the one used by WebURLLoaderImpl to get the method from the WebURLRequest
(https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_...).

+  params.headers = GetWebURLRequestHeaders(request);
+  params.load_flags = GetLoadFlagsForWebURLRequest(request);
+  // TODO(clamy): fill the http body.
+  params.has_user_gesture = request.hasUserGesture();
+  return params;
+}
+
+// PlzNavigate
+CommonNavigationParams MakeCommonNavigationParams(
+    const blink::WebURLRequest& request) {
+  const RequestExtraData kEmptyData;

[carlosk, 2014/10/08 14:28:55]

nit: you could declare this const member as static to avoid initializing it in
every call (unless that is what you want).

[clamy, 2014/10/10 02:33:11]

The use of static variables for non POD data is forbidden by the style guide
(see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nonmember,_St...).

[carlosk, 2014/10/10 08:44:09]

Acknowledged.

In fact for this specific case -- function scope static variables -- the risks
are somewhat different as the guide itself states:

"This prohibition does not apply to a static variable within function scope,
since its initialization order is well-defined and does not occur until control
passes through its declaration."

But they exist nonetheless:
http://stackoverflow.com/questions/246564/what-is-the-lifetime-of-a-static-va...

+  const RequestExtraData* extra_data =
+      static_cast<RequestExtraData*>(request.extraData());
+  if (!extra_data)
+    extra_data = &kEmptyData;
+  CommonNavigationParams params;
+  params.url = request.url();
+  params.referrer = Referrer(
+      GURL(request.httpHeaderField(WebString::fromUTF8("Referer")).latin1()),
+      request.referrerPolicy());
+  params.transition = extra_data->transition_type();
+  return params;
+}
+
 }  // namespace
 
 static RenderFrameImpl* (*g_create_render_frame_impl)(RenderViewImpl*, int32) =
@@ -847,6 +903,7 @@ bool RenderFrameImpl::OnMessageReceived(const IPC::Message& msg) {
     IPC_MESSAGE_HANDLER(FrameMsg_SetAccessibilityMode,
                         OnSetAccessibilityMode)
     IPC_MESSAGE_HANDLER(FrameMsg_DisownOpener, OnDisownOpener)
+    IPC_MESSAGE_HANDLER(FrameMsg_RequestNavigation, OnRequestNavigation)
     IPC_MESSAGE_HANDLER(FrameMsg_CommitNavigation, OnCommitNavigation)
 #if defined(OS_ANDROID)
     IPC_MESSAGE_HANDLER(FrameMsg_SelectPopupMenuItems, OnSelectPopupMenuItems)
@@ -958,40 +1015,15 @@ void RenderFrameImpl::OnNavigate(const FrameMsg_Navigate_Params& params) {
     }
   } else {
     // Navigate to the given URL.
-    WebURLRequest request = CreateURLRequestForNavigation(
-        params.common_params, scoped_ptr<StreamOverrideParameters>(),
-        frame->isViewSourceModeEnabled());
+    WebURLRequest request =
+        CreateURLRequestForNavigation(params.common_params,
+                                      params.request_params,
+                                      scoped_ptr<StreamOverrideParameters>(),
+                                      frame->isViewSourceModeEnabled());
 
     // A session history navigation should have been accompanied by state.
     CHECK_EQ(params.page_id, -1);
 
-    if (!params.request_params.extra_headers.empty()) {
-      for (net::HttpUtil::HeadersIterator i(
-               params.request_params.extra_headers.begin(),
-               params.request_params.extra_headers.end(),
-               "\n");
-           i.GetNext();) {
-        request.addHTTPHeaderField(WebString::fromUTF8(i.name()),
-                                   WebString::fromUTF8(i.values()));
-      }
-    }
-
-    if (params.request_params.is_post) {
-      request.setHTTPMethod(WebString::fromUTF8("POST"));
-
-      // Set post data.
-      WebHTTPBody http_body;
-      http_body.initialize();
-      const char* data = NULL;
-      if (params.request_params.browser_initiated_post_data.size()) {
-        data = reinterpret_cast<const char*>(
-            &params.request_params.browser_initiated_post_data.front());
-      }
-      http_body.appendData(WebData(
-          data, params.request_params.browser_initiated_post_data.size()));
-      request.setHTTPBody(http_body);
-    }
-
     // 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();
@@ -3460,6 +3492,29 @@ void RenderFrameImpl::FocusedNodeChanged(const WebNode& node) {
 }
 
 // PlzNavigate
+void RenderFrameImpl::OnRequestNavigation(
+    const CommonNavigationParams& common_params,
+    const RequestNavigationParams& request_params) {
+  CHECK(CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  // TODO(clamy): Execute the beforeunload event.
+  WebURLRequest request =
+      CreateURLRequestForNavigation(common_params,
+                                    request_params,
+                                    scoped_ptr<StreamOverrideParameters>(),
+                                    frame_->isViewSourceModeEnabled());
+  willSendRequest(frame_, 0, request, blink::WebURLResponse());

[nasko, 2014/10/08 17:40:32]

Shouldn't this go the same way as OnNaivate through frame->loadRequest()?

[clamy, 2014/10/10 02:33:11]

At this stage we are just trying to get whatever modifications Blink wants to
make on the navigation request, before sending it back to the browser, so that
we can start the actual request. Ideally, all modifications would take place in
RenderFrameImpl::willSendRequest, and the implementations of willSendRequest for
the various Inspectors in Devtools. We are not there yet, as we put that
refactoring on the back-burner while we focused on the navigation with no live
renderer case (but will resume soon :)). Therefore, after the call to these two
functions, we can send the BeginNavigation IPC and start the request on the
browser side.

We also need to execute the before unload event here, hence the TODO.

[nasko, 2014/10/13 17:04:24]

Let's put some of the explanation above in a TODO or a Note: as it will likely
be a question for whoever looks at this code next.

[clamy, 2014/10/13 19:13:10]

Done.

+  // TODO(clamy): Apply devtools override.
+  // TODO(clamy): Make sure that navigation request are not modified somewhere

[carlosk, 2014/10/08 14:28:55]

nit: /are/is/

[clamy, 2014/10/10 02:33:11]

Changed it to navigation requests are

+  // else in blink.
+  // TODO(clamy): Same-document navigations should not be sent back to the
+  // browser.
+  Send(new FrameHostMsg_BeginNavigation(routing_id_,
+                                        MakeBeginNavigationParams(request),
+                                        MakeCommonNavigationParams(request)));
+}
+
+// PlzNavigate
 void RenderFrameImpl::OnCommitNavigation(
     const ResourceResponseHead& response,
     const GURL& stream_url,
@@ -3483,8 +3538,11 @@ void RenderFrameImpl::OnCommitNavigation(
       new StreamOverrideParameters());
   stream_override->stream_url = stream_url;
   stream_override->response = response;
-  WebURLRequest request = CreateURLRequestForNavigation(
-      common_params, stream_override.Pass(), frame_->isViewSourceModeEnabled());
+  WebURLRequest request =
+      CreateURLRequestForNavigation(common_params,
+                                    RequestNavigationParams(),
+                                    stream_override.Pass(),
+                                    frame_->isViewSourceModeEnabled());
 
   // Record this before starting the load, we need a lower bound of this time
   // to sanitize the navigationStart override set below.