Add a FrameHostMsg_BeginNavigation IPC

Add a FrameHostMsg_BeginNavigation IPC

As part of the project PlzNavigate, this CL introduces a new IPC,
FrameHostMsg_BeginNavigation that should be sent from the renderer to the
browser on renderer-initiated navigation, or in response to a
FrameMsg_RequestNavigation (that will be introduced in a later CL) for browser
initiated ones.

Upon reception of FrameHostMsg_BeginNavigation, the UI thread will forward the relevant information to the IO thread (to the ResourceDispatcherHost). 

BUG=376014
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283346

[clamy, 2014-07-01 11:59:16]

@ppi: PTAL

[clamy, 2014-07-02 09:50:41]

@ davidben, nasko: PTAL

[ppi, 2014-07-02 17:28:57]

Thanks!

A high-level note - this is not, strictly speaking, needed for the experimental
codepath we want this Q, right? Would we want to wait with this to the time we
start adding support for navigating existing renderers?

On another note, I think that alternative approach to scaffolding everything in
advance would be to start only with the core parameters needed to have the
simplest case working, and add subsequent IPC attributes as we add the code that
handles them.

Wdyt?

[davidben, 2014-07-02 21:05:13]

lgtm, though you'll need content/ and IPC stamps from nasko anyway. I don't
really have opinions as to what order we scaffold things in.

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

https://codereview.chromium.org/367653002/diff/20001/content/common/frame_mes...
content/common/frame_messages.h:279: IPC_STRUCT_MEMBER(ResourceType::Type,
resource_type)
Probably can remove this one since the browser will know what the RFH's type is
anyway.

[clamy, 2014-07-03 09:41:54]

@ ppi, davidben: thanks!

@ ppi: in order for the experimental path to work, we will need to create a
proper net::URLRequest. As you can see in
ResourceDispatcherHostImpl::BeginRequest(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/loader/resource_dispatcher_host_impl.cc&q=resourcedispatcherhostimpl&type=cs&sq=package:chromium&l=961),
this means giving it all parameters normally found in a ResourceHostMsg_Request
(https://code.google.com/p/chromium/codesearch#chromium/src/content/common/res...).
So we need to somehow have all of them in ResourceDispatcherHostImpl.

I think we can divide them in 3 groups:
- the ones that are always the same in the case of a navigation request (eg
priority). These ones can be set directly in ResourceDispatcherHostImpl.
- the ones that the UI thread has authority on (eg is_main_frame). Those ones
should be set in a common code path for both browser-initiated and
renderer-initiated navigations.
- the ones that directly depend on the renderer in renderer-initiated
navigations (eg method). Those ones will be received via the IPC introduced in
this CL during renderer-initiated navigations and browser-initiated navigations
that have a live renderer. They will need to be generated in the browser for a
browser-initiated navigation that has not live renderer, based on default values
for the parameters and information provided by the Navigator.

The goal of this CL is to draw the distinction between group 3 and the rest.
Then, in the next CL I will implement the common code path that takes a
FrameHostMsg_BeginNavigation_Params (or an equivalent struct), and sends it to
the IO thread, along with the parameters known to the UI thread. A further CL
will generate the FrameHostMsg_BeginNavigation_Params (or an equivalent struct),
and pass it to the common code path for handling, in the case we try to navigate
while not having a live renderer.

It would be of course possible to just provide default values for everything in
one place, but I feel that it makes more sense that way.

https://chromiumcodereview.appspot.com/367653002/diff/20001/content/common/fr...
File content/common/frame_messages.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/20001/content/common/fr...
content/common/frame_messages.h:279: IPC_STRUCT_MEMBER(ResourceType::Type,
resource_type)
On 2014/07/02 21:05:12, David Benjamin wrote:
> Probably can remove this one since the browser will know what the RFH's type
is
> anyway.

Done. We may have to put it back in place if we happen to merge OpenURL and this
IPC together though.

[ppi, 2014-07-03 09:52:59]

Sounds reasonable, thanks for explanations! Lgtm.

[nasko, 2014-07-03 10:26:09]

Including jam@ for explicit visibility that we are starting to commit code for
this.

In general, we avoid CLs that add code that is unused. I think it will make
sense to fold this into either the CL that will send the IPC or into a CL that
will process it (probably the former).

Otherwise the code is fine.

https://chromiumcodereview.appspot.com/367653002/diff/40001/content/browser/f...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/367653002/diff/40001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:346:
IPC_MESSAGE_HANDLER(FrameHostMsg_BeginNavigation,
nit: Keep this after _UpdateEncoding, as that's where the method is also in the
header and here.

https://chromiumcodereview.appspot.com/367653002/diff/40001/content/browser/f...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/40001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:252: void OnBeginNavigation(
nit: No need for empty line above.

[clamy, 2014-07-03 11:42:58]

On 2014/07/03 10:26:09, nasko wrote:
> Including jam@ for explicit visibility that we are starting to commit code for
> this.
> 
> In general, we avoid CLs that add code that is unused. I think it will make
> sense to fold this into either the CL that will send the IPC or into a CL that
> will process it (probably the former).
> 
> Otherwise the code is fine.
> 
>
https://chromiumcodereview.appspot.com/367653002/diff/40001/content/browser/f...
> File content/browser/frame_host/render_frame_host_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/367653002/diff/40001/content/browser/f...
> content/browser/frame_host/render_frame_host_impl.cc:346:
> IPC_MESSAGE_HANDLER(FrameHostMsg_BeginNavigation,
> nit: Keep this after _UpdateEncoding, as that's where the method is also in
the
> header and here.
> 
>
https://chromiumcodereview.appspot.com/367653002/diff/40001/content/browser/f...
> File content/browser/frame_host/render_frame_host_impl.h (right):
> 
>
https://chromiumcodereview.appspot.com/367653002/diff/40001/content/browser/f...
> content/browser/frame_host/render_frame_host_impl.h:252: void
OnBeginNavigation(
> nit: No need for empty line above.

Thanks for the review! I have a CL that processes the IPC in the works, so I
will fold these two together.

[clamy, 2014-07-03 15:10:51]

Here is a draft version that sends the info received from the IPC to the
ResourceDispatcherHostImpl. I will add some unit tests to properly exercise the
code. In the meantime, if you have some time, would you care to have a look at
it and see if you are happy with the general design?

https://chromiumcodereview.appspot.com/367653002/diff/40001/content/browser/f...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/367653002/diff/40001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:346:
IPC_MESSAGE_HANDLER(FrameHostMsg_BeginNavigation,
On 2014/07/03 10:26:09, nasko wrote:
> nit: Keep this after _UpdateEncoding, as that's where the method is also in
the
> header and here.

Done.

https://chromiumcodereview.appspot.com/367653002/diff/40001/content/browser/f...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/40001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:252: void OnBeginNavigation(
On 2014/07/03 10:26:09, nasko wrote:
> nit: No need for empty line above.

Done.

https://chromiumcodereview.appspot.com/367653002/diff/80001/content/browser/f...
File content/browser/frame_host/navigation_request.cc (right):

https://chromiumcodereview.appspot.com/367653002/diff/80001/content/browser/f...
content/browser/frame_host/navigation_request.cc:18:
scoped_refptr<ResourceRequestBody> request_body,
request_body is used as a separate parameter in this method (instead of being
part of the struct) to be properly passed between threads.

https://chromiumcodereview.appspot.com/367653002/diff/80001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/80001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.h:306: void
BeginNavigation(const FrameHostMsg_BeginNavigation_Params& params);
I think it would also be possible to put this method in the FrameTreeNode, but I
think it makes more sense to have it in RenderFrameHostManager. Wdyt?

https://chromiumcodereview.appspot.com/367653002/diff/80001/content/browser/l...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/80001/content/browser/l...
content/browser/loader/resource_dispatcher_host_impl.h:243: void
NavigationRequest(const NavigationRequestInfo& info,
We did not define what the method to call in ResourceDispatcherHostImpl to start
a navigation would look like. I think this is probably the bare minimum we need
to provide the loader with.

https://chromiumcodereview.appspot.com/367653002/diff/80001/content/common/re...
File content/common/resource_request_body.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/80001/content/common/re...
content/common/resource_request_body.h:26: : public
base::RefCountedThreadSafe<ResourceRequestBody>,
We now pass a scoped_refptr<ResourceRequestBody> between the UI and IO thread.

https://chromiumcodereview.appspot.com/367653002/diff/80001/content/common/re...
content/common/resource_request_body.h:32: virtual ~ResourceRequestBody();
This is needed for compilation of a RefCountedThreadSafe object.

[clamy, 2014-07-04 15:07:20]

@ davidben, nasko, ppi: I have added a very basic implementation of the IPC
handler, as well as a unit test to exercise it. PTAL.

The implementation does not handle a certain number of interesting cases/corner
cases (eg canceling a previously made request if a new one comes in). Since the
plan for this quarter is to get a prototype, I intend to add support for them
later.

[nasko, 2014-07-07 11:35:10]

A few comments. In general, if we plan to add something later, it needs to have
a TODO note added, so other developers are aware of what changes are coming and
why certain things aren't implemented.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
File content/browser/frame_host/navigation_request.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/navigation_request.h:20: // navigation, please see
the PlzNavigate design doc.
While I love the PlzNavigate codename, I'm not sure it is the best thing to use
in comments. Some of the content of that document will have to be described in a
comment somewhere in the codebase, though not sure what the best spot is right
now.

I think the important details concerning this object should be described here.
It will be fine to put a TODO to add those comments later.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/navigation_request.h:27: NavigationRequestInfo info()
const { return info_; }
Wouldn't this return a full copy? Is this the intention here? It is also only
used for tests, so it should indicate this using a suffix in the function name.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/navigation_request.h:30: // Called on the UI thread
by the RenderFrameHostManager that owns the
nit: s/that/which/

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
File content/browser/frame_host/navigation_request_info.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/navigation_request_info.h:27: // with no live
renderer.
Does the "live renderer" detail matter? How would it be different if I typed in
the omnibox an URL that will end up in the same renderer process as the
currently used one?

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/navigation_request_info.h:36: int load_flags;
Some basic comments on the fields in this struct will be useful.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:576: FrameTreeNode* node
= render_frame_host_->frame_tree_node();
RFHM has the frame_tree_node_ as private member, no need to go to the RFH for
it.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:180: // Returns the
current navigation request (used in the PlzNavigate navigation
Now that you mention it like this, it might not be too bad of a token to use and
grep for at the end of the project, so we can clean up comments ; ).

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:182: NavigationRequest*
navigation_request() const {
This is only used for tests, so it should be named with _for_testing suffix and
possibly made private.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/common/r...
File content/common/resource_request_body.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/common/r...
content/common/resource_request_body.h:26: : public
base::RefCountedThreadSafe<ResourceRequestBody>,
Is there a different pattern we might use for sending it from the UI to the IO
thread?

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/common/r...
content/common/resource_request_body.h:32: virtual ~ResourceRequestBody();
Why is the destructor now public?

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/common/r...
content/common/resource_request_body.h:55: friend class
base::RefCounted<ResourceRequestBody>;
Shouldn't this be RefCountedThreadSafe?

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/test/tes...
File content/test/test_render_frame_host.cc (right):

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/test/tes...
content/test/test_render_frame_host.cc:162: params.url = url;
Why not fill in the rest of the params with some sane default values?

[clamy, 2014-07-07 13:36:23]

Thanks for the review!

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
File content/browser/frame_host/navigation_request.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/navigation_request.h:20: // navigation, please see
the PlzNavigate design doc.
On 2014/07/07 11:35:07, nasko wrote:
> While I love the PlzNavigate codename, I'm not sure it is the best thing to
use
> in comments. Some of the content of that document will have to be described in
a
> comment somewhere in the codebase, though not sure what the best spot is right
> now.
> 
> I think the important details concerning this object should be described here.
> It will be fine to put a TODO to add those comments later.

Done.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/navigation_request.h:27: NavigationRequestInfo info()
const { return info_; }
On 2014/07/07 11:35:07, nasko wrote:
> Wouldn't this return a full copy? Is this the intention here? It is also only
> used for tests, so it should indicate this using a suffix in the function
name.

Done.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/navigation_request.h:30: // Called on the UI thread
by the RenderFrameHostManager that owns the
On 2014/07/07 11:35:07, nasko wrote:
> nit: s/that/which/

Done.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
File content/browser/frame_host/navigation_request_info.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/navigation_request_info.h:27: // with no live
renderer.
On 2014/07/07 11:35:07, nasko wrote:
> Does the "live renderer" detail matter? How would it be different if I typed
in
> the omnibox an URL that will end up in the same renderer process as the
> currently used one?

I removed this point of detail.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/navigation_request_info.h:36: int load_flags;
On 2014/07/07 11:35:08, nasko wrote:
> Some basic comments on the fields in this struct will be useful.

Done.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:576: FrameTreeNode* node
= render_frame_host_->frame_tree_node();
On 2014/07/07 11:35:08, nasko wrote:
> RFHM has the frame_tree_node_ as private member, no need to go to the RFH for
> it.

Done.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:182: NavigationRequest*
navigation_request() const {
On 2014/07/07 11:35:09, nasko wrote:
> This is only used for tests, so it should be named with _for_testing suffix
and
> possibly made private.

Done.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/common/r...
File content/common/resource_request_body.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/common/r...
content/common/resource_request_body.h:26: : public
base::RefCountedThreadSafe<ResourceRequestBody>,
On 2014/07/07 11:35:09, nasko wrote:
> Is there a different pattern we might use for sending it from the UI to the IO
> thread?

I'm afraid I don't see a better one :).

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/common/r...
content/common/resource_request_body.h:32: virtual ~ResourceRequestBody();
On 2014/07/07 11:35:09, nasko wrote:
> Why is the destructor now public?

Since I forgot to change the friend class, it would not compile otherwise :).
This is now corrected.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/common/r...
content/common/resource_request_body.h:55: friend class
base::RefCounted<ResourceRequestBody>;
On 2014/07/07 11:35:09, nasko wrote:
> Shouldn't this be RefCountedThreadSafe?

Done.

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/test/tes...
File content/test/test_render_frame_host.cc (right):

https://chromiumcodereview.appspot.com/367653002/diff/100001/content/test/tes...
content/test/test_render_frame_host.cc:162: params.url = url;
On 2014/07/07 11:35:10, nasko wrote:
> Why not fill in the rest of the params with some sane default values?

Done.

[nasko, 2014-07-07 13:49:26]

LGTM

[ppi, 2014-07-07 14:10:05]

Thanks! Some remarks below.

https://chromiumcodereview.appspot.com/367653002/diff/120001/content/browser/...
File content/browser/frame_host/navigation_request_info.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/120001/content/browser/...
content/browser/frame_host/navigation_request_info.h:37: int referrer_policy;
In content we have
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...
that wraps together the url and the policy - this should be used wherever
possible.

Is it possible here?

https://chromiumcodereview.appspot.com/367653002/diff/120001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/367653002/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1855:
TEST_F(RenderFrameHostManagerTest,PlzNavigateBeginNavigation) {
nit: missing space after comma

https://chromiumcodereview.appspot.com/367653002/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1875:
DCHECK(subframe_request);
We probably should be using ASSERT_* instead of (D)CHECK in tests.

https://chromiumcodereview.appspot.com/367653002/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1888:
DCHECK(main_request);
See above.

[clamy, 2014-07-08 14:21:40]

Thanks for the review!

https://chromiumcodereview.appspot.com/367653002/diff/120001/content/browser/...
File content/browser/frame_host/navigation_request_info.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/120001/content/browser/...
content/browser/frame_host/navigation_request_info.h:37: int referrer_policy;
On 2014/07/07 14:10:05, ppi wrote:
> In content we have
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...
> that wraps together the url and the policy - this should be used wherever
> possible.
> 
> Is it possible here?

Done.

https://chromiumcodereview.appspot.com/367653002/diff/120001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/367653002/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1855:
TEST_F(RenderFrameHostManagerTest,PlzNavigateBeginNavigation) {
On 2014/07/07 14:10:05, ppi wrote:
> nit: missing space after comma

Done.

https://chromiumcodereview.appspot.com/367653002/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1875:
DCHECK(subframe_request);
On 2014/07/07 14:10:05, ppi wrote:
> We probably should be using ASSERT_* instead of (D)CHECK in tests.

Done.

https://chromiumcodereview.appspot.com/367653002/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1888:
DCHECK(main_request);
On 2014/07/07 14:10:05, ppi wrote:
> See above.

Done.

[ppi, 2014-07-08 14:29:02]

Lgtm, thanks!

[clamy, 2014-07-09 15:11:19]

@ppi: thanks for the review!

@davidben: are you okay with the changes in content/browser/loader?

@jam: PTAL. This patch introduces the second IPC for PlzNavigate and a barebones
implementation of its handler.

[davidben, 2014-07-09 15:14:28]

LGTM with some minor comments

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:34: // it has not commited yet.
(I suspect this'll require that NavigationRequests have their own IDs, but that
can come later.)

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:31: // NavigationRequest.
This probably wants a comment that, after calling this, |body| can no longer be
manipulated on the UI thread. Even though the reference count is thread-safe,
ResourceRequestBody itself isn't. (Presumably we're not going to be modifying it
anyway, but worth a mention.)

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:775: const
FrameHostMsg_BeginNavigation_Params& params) {
Should we guard at this point behind a switch or something, in case we skimp on
this or that security check mid-development? Probably good to avoid problems in
case of a compromised renderer.

https://codereview.chromium.org/367653002/diff/160001/content/test/test_rende...
File content/test/test_render_frame_host.cc (right):

https://codereview.chromium.org/367653002/diff/160001/content/test/test_rende...
content/test/test_render_frame_host.cc:165: params.method="GET";
Nit: spaces around the =.

[ppi, 2014-07-10 16:03:34]

Thanks, two additional nits on second reading.

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:306: void
BeginNavigation(const FrameHostMsg_BeginNavigation_Params& params);
We might want to call it transitively: OnBeginNavigation().

Otherwise we will end up with both "BeginNavigation" and "RequestNavigation"
here, which could be confusing in terms of the flow of interactions.

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/common/f...
File content/common/frame_messages.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/common/f...
content/common/frame_messages.h:257: // Parameters for a navigation request,
nit: "," -> "."

[jam, 2014-07-15 00:05:44]

lgtm

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:13: struct
NavigationRequestInfo;
nit: not needed because of the header include

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:23: explicit
NavigationRequest(const NavigationRequestInfo& info,
nit: don't need explicit since it has more than one parameter

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/navigation_request_info.h (right):

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
content/browser/frame_host/navigation_request_info.h:26: //
FrameHostMsg_BeginNavigation_Params.
did you consider having a FrameHostMsg_BeginNavigation_Params as a member here
since it's almost the same as these first 9 parameters? is the presence of two
unused parameters in that struct important?

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/367653002/diff/160001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1853: //
Project PlzNavigate: Test that a proper NavigationRequest is created by
nit: years later, few will know what PlzNavigate codename was. better to make it
self-descriptive, i.e. For browser-side navigation, ...

https://codereview.chromium.org/367653002/diff/160001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/367653002/diff/160001/content/common/frame_me...
content/common/frame_messages.h:257: // Parameters for a navigation request,
nit: this comment is necessary

[clamy, 2014-07-15 15:32:15]

Thanks for the review!

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
File content/browser/frame_host/navigation_request.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
content/browser/frame_host/navigation_request.h:13: struct
NavigationRequestInfo;
On 2014/07/15 00:05:43, jam wrote:
> nit: not needed because of the header include

Done.

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
content/browser/frame_host/navigation_request.h:23: explicit
NavigationRequest(const NavigationRequestInfo& info,
On 2014/07/15 00:05:43, jam wrote:
> nit: don't need explicit since it has more than one parameter

Done.

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
content/browser/frame_host/navigation_request.h:31: // NavigationRequest.
On 2014/07/09 15:14:27, David Benjamin wrote:
> This probably wants a comment that, after calling this, |body| can no longer
be
> manipulated on the UI thread. Even though the reference count is thread-safe,
> ResourceRequestBody itself isn't. (Presumably we're not going to be modifying
it
> anyway, but worth a mention.)

Done.

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
File content/browser/frame_host/navigation_request_info.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
content/browser/frame_host/navigation_request_info.h:26: //
FrameHostMsg_BeginNavigation_Params.
On 2014/07/15 00:05:43, jam wrote:
> did you consider having a FrameHostMsg_BeginNavigation_Params as a member here
> since it's almost the same as these first 9 parameters? is the presence of two
> unused parameters in that struct important?

Done.

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:775: const
FrameHostMsg_BeginNavigation_Params& params) {
On 2014/07/09 15:14:28, David Benjamin wrote:
> Should we guard at this point behind a switch or something, in case we skimp
on
> this or that security check mid-development? Probably good to avoid problems
in
> case of a compromised renderer.

Good point. I put it behind a compile flag.

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:306: void
BeginNavigation(const FrameHostMsg_BeginNavigation_Params& params);
On 2014/07/10 16:03:34, ppi wrote:
> We might want to call it transitively: OnBeginNavigation().
> 
> Otherwise we will end up with both "BeginNavigation" and "RequestNavigation"
> here, which could be confusing in terms of the flow of interactions.

Done.

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1853: //
Project PlzNavigate: Test that a proper NavigationRequest is created by
On 2014/07/15 00:05:43, jam wrote:
> nit: years later, few will know what PlzNavigate codename was. better to make
it
> self-descriptive, i.e. For browser-side navigation, ...

Done.

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/common/f...
File content/common/frame_messages.h (right):

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/common/f...
content/common/frame_messages.h:257: // Parameters for a navigation request,
On 2014/07/15 00:05:43, jam wrote:
> nit: this comment is necessary

I removed it entirely.

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/test/tes...
File content/test/test_render_frame_host.cc (right):

https://chromiumcodereview.appspot.com/367653002/diff/160001/content/test/tes...
content/test/test_render_frame_host.cc:165: params.method="GET";
On 2014/07/09 15:14:28, David Benjamin wrote:
> Nit: spaces around the =.

Done.

[clamy, 2014-07-15 15:32:21]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-07-15 15:33:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/clamy@chromium.org/367653002/180001

[commit-bot: I haz the power, 2014-07-16 01:14:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-16 05:02:12]

Change committed as 283346