PlzNavigate: Implement RequestNavigation on the renderer side

PlzNavigate:  RequestNavigation on the renderer side

This CL provides a basic implementation of RequestNavigation on the renderer side, that allows all browser initiated navigations to proceed in the PlzNavigate world. Note that this implementation is missing a certain number of features to be fully functional, in particular the BeforeUnload event will not be executed here. This will be corrected in latter patches.

BUG=376014, 376006
Committed: https://crrev.com/79b3da6756024fea55906b1639e5fe9e92c098b7
Cr-Commit-Position: refs/heads/master@{#299447}

[clamy, 2014-10-07 20:21:15]

clamy@chromium.org changed reviewers:
+ carlosk@chromium.org, creis@chromium.org, nasko@chromium.org

[clamy, 2014-10-07 20:23:04]

@carlosk, creis, nasko: PTAL

This CL is based on the CommitNavigation one, and implements very basically
RequestNavigation on the renderer side. This allows for browser initiated
navigations to proceed when there is a live renderer.

[carlosk, 2014-10-08 14:28:56]

Found just a few minor things...

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:406: WebData(data,
request_params.browser_initiated_post_data.size()));
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.

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:455: const RequestExtraData kEmptyData;
nit: you could declare this const member as static to avoid initializing it in
every call (unless that is what you want).

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3508: // TODO(clamy): Make sure that
navigation request are not modified somewhere
nit: /are/is/

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

Nice to see the PlzNavigate picture getting filled in :).

https://codereview.chromium.org/559613002/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/559613002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1386: Send(new
FrameMsg_RequestNavigation(
Do we forsee this method growing in size? If not, we can just send the IPC
directly from NavigatorImpl.

https://codereview.chromium.org/559613002/diff/80001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/559613002/diff/80001/content/common/frame_mes...
content/common/frame_messages.h:472: content::RequestNavigationParams /*
request_params */)
nit: let's leave an empty line between the IPC messages to be consistent with
the rest.

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:444: params.method =
request.httpMethod().latin1();
Is latin1() the usual encoding to use?

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3506: willSendRequest(frame_, 0, request,
blink::WebURLResponse());
Shouldn't this go the same way as OnNaivate through frame->loadRequest()?

[clamy, 2014-10-10 02:33:12]

Thanks! Please find my answers to your comments below.

https://codereview.chromium.org/559613002/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/559613002/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:1386: Send(new
FrameMsg_RequestNavigation(
On 2014/10/08 17:40:32, nasko wrote:
> Do we forsee this method growing in size? If not, we can just send the IPC
> directly from NavigatorImpl.

Done.

https://codereview.chromium.org/559613002/diff/80001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/559613002/diff/80001/content/common/frame_mes...
content/common/frame_messages.h:472: content::RequestNavigationParams /*
request_params */)
On 2014/10/08 17:40:32, nasko wrote:
> nit: let's leave an empty line between the IPC messages to be consistent with
> the rest.

Done.

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:406: WebData(data,
request_params.browser_initiated_post_data.size()));
On 2014/10/08 14:28:55, carlosk wrote:
> 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.

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

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:444: params.method =
request.httpMethod().latin1();
On 2014/10/08 17:40:32, nasko wrote:
> Is latin1() the usual encoding to use?

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_...).

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:455: const RequestExtraData kEmptyData;
On 2014/10/08 14:28:55, carlosk wrote:
> nit: you could declare this const member as static to avoid initializing it in
> every call (unless that is what you want).

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...).

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3506: willSendRequest(frame_, 0, request,
blink::WebURLResponse());
On 2014/10/08 17:40:32, nasko wrote:
> Shouldn't this go the same way as OnNaivate through frame->loadRequest()?

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.

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3508: // TODO(clamy): Make sure that
navigation request are not modified somewhere
On 2014/10/08 14:28:55, carlosk wrote:
> nit: /are/is/

Changed it to navigation requests are

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

Cool, LGTM.

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:455: const RequestExtraData kEmptyData;
On 2014/10/10 02:33:11, clamy wrote:
> On 2014/10/08 14:28:55, carlosk wrote:
> > nit: you could declare this const member as static to avoid initializing it
in
> > every call (unless that is what you want).
> 
> 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...).

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...

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

LGTM, provided the comment is added.

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3506: willSendRequest(frame_, 0, request,
blink::WebURLResponse());
On 2014/10/10 02:33:11, clamy wrote:
> On 2014/10/08 17:40:32, nasko wrote:
> > Shouldn't this go the same way as OnNaivate through frame->loadRequest()?
> 
> 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.

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]

Thanks!

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/559613002/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3506: willSendRequest(frame_, 0, request,
blink::WebURLResponse());
On 2014/10/13 17:04:24, nasko wrote:
> On 2014/10/10 02:33:11, clamy wrote:
> > On 2014/10/08 17:40:32, nasko wrote:
> > > Shouldn't this go the same way as OnNaivate through frame->loadRequest()?
> > 
> > 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.
> 
> 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.

Done.

[clamy, 2014-10-13 19:13:16]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-10-13 19:14:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/559613002/310001

[commit-bot: I haz the power, 2014-10-13 20:08:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-13 20:08:25]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[clamy, 2014-10-14 07:58:59]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-10-14 07:59:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/559613002/580001

[commit-bot: I haz the power, 2014-10-14 08:50:45]

Committed patchset #8 (id:580001)

[commit-bot: I haz the power, 2014-10-14 08:51:29]

Patchset 8 (id:??) landed as
https://crrev.com/79b3da6756024fea55906b1639e5fe9e92c098b7
Cr-Commit-Position: refs/heads/master@{#299447}