PlzNavigate: implement CommitNavigation on the browser side

PlzNavigate: implement CommitNavigation on the browser side

Part of the PlzNavigate navigation refactoring project. This CL implements CommitNavigation on the browser side, so that the proper parameters can be passed to the renderer when navigations are ready to commit.

BUG=376091
Committed: https://crrev.com/9bfeef40d0a1597a6cbf5835f0bc2c2553818975
Cr-Commit-Position: refs/heads/master@{#297495}

[clamy, 2014-08-26 15:44:54]

Patchset #1 (id:1) has been deleted

[clamy, 2014-08-26 15:45:02]

Patchset #1 (id:20001) has been deleted

[clamy, 2014-08-26 16:02:50]

Patchset #1 (id:40001) has been deleted

[clamy, 2014-08-26 16:10:12]

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

[clamy, 2014-08-26 16:18:37]

@nasko: PTAL
@carlosk: FYI

This CL extends the RenderFrameHostManager::CommitNavigation method introduce in
the previous CL so that the RenderFrameHost that we selected there can send a
FrameMsg_CommitNavigation to the renderer with proper parameters. I plan to have
the implementation of the FrameMsg_CommitNavigation handler in RenderFrame in a
next CL.

I will also expand the unit tests to test the changes that this CL introduce in
the next patch set. In the meantime, I'd be happy to get your opinion on the
proposed changes.

[(Do not use) nasko, 2014-08-28 16:39:09]

nasko@google.com changed reviewers:
+ nasko@google.com

[(Do not use) nasko, 2014-08-28 16:39:09]

I like how things are coming along step by step! Lots of comments on this first
round with the main confusion on my side is why we need to create commit
parameters so early in the timeline?

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/navigator_impl.cc:93:
navigator->controller_->FillHistoryParametersForNavigationEntry(
Do we need navigator for anything else than the controller_? 
It doesn't make much sense to have a static method that takes the pointer of the
same type - just make it a regular method on the class.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
File content/browser/frame_host/navigator_impl.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/navigator_impl.h:28: // PlzNavigate: Returns a
FramsMsg_commit_Params filled in based on the
nit: Uppercase _commit_.

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

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:1208:
render_view_host_->SetState(RenderViewHostImpl::STATE_DEFAULT);
Setting this unconditionally can be problematic. Why not check/dcheck state
first just to make sure we don't have any bugs that creep up?

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

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:285: // PlzNavigate:
Indicates that a navigation is about to commit in this
Is it about to commit? Or has it committed and we just need to hand it off to
the renderer for processing?

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.cc:55: case
FrameMsg_Navigate_Type::RELOAD :
nit: no need for space between the constant and ":"

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

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.h:321:
scoped_ptr<FrameMsg_CommitNavigation_Params> commit_navigation_params,
Why do we need to have commit parameters at the RequestNavigation time?

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:392: void
StartBrowserSideNavigation(const GURL& url,
nit: In general we talk about navigating in a node of the frame tree. It is
really not much different than level of indication, but might be nice to take
FTN instead of RFHM.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:412:
*entry.get(),
nit: no need to call .get() on a scoped_ptr. It has operator* and operator->
overloaded to get the internal pointer.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:424:
FrameMsg_CommitNavigation_Params* GetCommitParamsForRenderFrameManager(
nit: The name of this method is almost as long as the actual code. Is it worth
it? I have harder time reading the name than the actual code inside it.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1769:
StartBrowserSideNavigation(kUrl2, main_frame_manager);
Reading this as Start and then next line as Send, they seem like they do the
same thing. Maybe rename the initial one to be SendRequestNavigation*, so it
matches the SendBeginNavigation and form the pattern of Send<IPC Name>*.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1841: //
renderer when the navigation can commit.
Do we need to pass the command line flag to exercise these tests? If yes, we
should probably put that in the comments, so other devs can easily understand
why those aren't run in default case.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1849:
StartBrowserSideNavigationWithParameters(
It sounds wrong to reload when we have no committed navigation. Can we ensure we
properly handle this case?

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1865: // Now do
a shift+reload.
Shouldn't we commit the reload before issuing another one? Unless the intent is
to interleave the two.

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

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/common/fr...
content/common/frame_messages.h:324: IPC_STRUCT_MEMBER(int,
pending_history_list_offset)
Do we really need to send history parameters down to the renderer?

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/common/fr...
content/common/frame_messages.h:539:
IPC_MESSAGE_ROUTED1(FrameMsg_CommitNavigation, FrameMsg_CommitNavigation_Params)
If we are adding an IPC, let's add a handler in the renderer process that either
does a NOTREACHED or just logs something. Leaving unhandled IPCs is not a good
practice.

[clamy, 2014-09-02 18:25:20]

Thanks! The problem with the commit parameters is that they depend on parameters
that are in the NavigationEntry. In the previous CL, we decided that we could
not rely on the pending entry of the NavigationController. So the only moment we
have access to the NavigationEntry is in NavigatorImpl::NavigateToEntry, which
will call RequestNavigation.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/navigator_impl.cc:93:
navigator->controller_->FillHistoryParametersForNavigationEntry(
On 2014/08/28 16:39:08, (Do not use) nasko wrote:
> Do we need navigator for anything else than the controller_? 
> It doesn't make much sense to have a static method that takes the pointer of
the
> same type - just make it a regular method on the class.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
File content/browser/frame_host/navigator_impl.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/navigator_impl.h:28: // PlzNavigate: Returns a
FramsMsg_commit_Params filled in based on the
On 2014/08/28 16:39:08, (Do not use) nasko wrote:
> nit: Uppercase _commit_.

Done.

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

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:1208:
render_view_host_->SetState(RenderViewHostImpl::STATE_DEFAULT);
On 2014/08/28 16:39:08, (Do not use) nasko wrote:
> Setting this unconditionally can be problematic. Why not check/dcheck state
> first just to make sure we don't have any bugs that creep up?

Done.

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

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:285: // PlzNavigate:
Indicates that a navigation is about to commit in this
On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> Is it about to commit? Or has it committed and we just need to hand it off to
> the renderer for processing?

Is it clearer this way?

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.cc:55: case
FrameMsg_Navigate_Type::RELOAD :
On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> nit: no need for space between the constant and ":"

Done.

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

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.h:321:
scoped_ptr<FrameMsg_CommitNavigation_Params> commit_navigation_params,
On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> Why do we need to have commit parameters at the RequestNavigation time?

Because they depend on information stored in the NavigationEntry, and in the
discussion about the previous CL, we agreed that we should not depend on the
pending entry of the NavigationController. Which means that the only moment
where we have access to the NavigationEntry is when we are asked to navigate,
that is on RequestNavigation.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:392: void
StartBrowserSideNavigation(const GURL& url,
On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> nit: In general we talk about navigating in a node of the frame tree. It is
> really not much different than level of indication, but might be nice to take
> FTN instead of RFHM.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:412:
*entry.get(),
On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> nit: no need to call .get() on a scoped_ptr. It has operator* and operator->
> overloaded to get the internal pointer.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:424:
FrameMsg_CommitNavigation_Params* GetCommitParamsForRenderFrameManager(
On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> nit: The name of this method is almost as long as the actual code. Is it worth
> it? I have harder time reading the name than the actual code inside it.

Done.

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

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/common/fr...
content/common/frame_messages.h:324: IPC_STRUCT_MEMBER(int,
pending_history_list_offset)
On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> Do we really need to send history parameters down to the renderer?

I don't know. Right now the history gets updated in the handler of
FrameMsg_Navigate. In the new logic, the history may not have been updated,
because the renderer never got the equivalent of FrameMsg_Navigate.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/common/fr...
content/common/frame_messages.h:539:
IPC_MESSAGE_ROUTED1(FrameMsg_CommitNavigation, FrameMsg_CommitNavigation_Params)
On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> If we are adding an IPC, let's add a handler in the renderer process that
either
> does a NOTREACHED or just logs something. Leaving unhandled IPCs is not a good
> practice.

Done.

[nasko, 2014-09-03 16:30:27]

Thanks for the round of cleanup. 
I saw a few comments in render_frame_host_manager_unittest.cc that had no
response or changes. For some reason I've made those from nasko@google.com, so
they might have not shown up properly.

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

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:285: // PlzNavigate:
Indicates that a navigation is about to commit in this
On 2014/09/02 18:25:19, clamy wrote:
> On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> > Is it about to commit? Or has it committed and we just need to hand it off
to
> > the renderer for processing?
> 
> Is it clearer this way?

I think it is. Thanks!

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

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.h:321:
scoped_ptr<FrameMsg_CommitNavigation_Params> commit_navigation_params,
On 2014/09/02 18:25:19, clamy wrote:
> On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> > Why do we need to have commit parameters at the RequestNavigation time?
> 
> Because they depend on information stored in the NavigationEntry, and in the
> discussion about the previous CL, we agreed that we should not depend on the
> pending entry of the NavigationController. Which means that the only moment
> where we have access to the NavigationEntry is when we are asked to navigate,
> that is on RequestNavigation.

Yes, I agree that we shouldn't depend on NavigationEntry. One idea I was
discussing with Charlie is to have one structure that follows a full navigation
from the beginning to the end, carrying the common data that needs to be passed
around. Things like referrer and redirect chain can be part of it and we can
easily track it around. Each specific IPC can use that structure for the common
parts and additional params struct for the IPC specific parts. What do you
think?

https://chromiumcodereview.appspot.com/483773002/diff/90001/content/browser/f...
File content/browser/frame_host/navigator_impl.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/90001/content/browser/f...
content/browser/frame_host/navigator_impl.h:30:
scoped_ptr<FrameMsg_CommitNavigation_Params>
Now that these are no longer static, they should move lower in the file. We tend
to have inherited methods first, then class specific methods later.

[clamy, 2014-09-03 17:15:15]

Thanks. Yes some of the comments you made did not appear when I looked at
render_frame_host_manager_unittests in the first place. But I can see them now,
so I will be addressing them. I will be adding a struct as well, as suggested.
This way it should be clearer how parameters for the navigation are modified.

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

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.h:321:
scoped_ptr<FrameMsg_CommitNavigation_Params> commit_navigation_params,
On 2014/09/03 16:30:26, nasko wrote:
> On 2014/09/02 18:25:19, clamy wrote:
> > On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> > > Why do we need to have commit parameters at the RequestNavigation time?
> > 
> > Because they depend on information stored in the NavigationEntry, and in the
> > discussion about the previous CL, we agreed that we should not depend on the
> > pending entry of the NavigationController. Which means that the only moment
> > where we have access to the NavigationEntry is when we are asked to
navigate,
> > that is on RequestNavigation.
> 
> Yes, I agree that we shouldn't depend on NavigationEntry. One idea I was
> discussing with Charlie is to have one structure that follows a full
navigation
> from the beginning to the end, carrying the common data that needs to be
passed
> around. Things like referrer and redirect chain can be part of it and we can
> easily track it around. Each specific IPC can use that structure for the
common
> parts and additional params struct for the IPC specific parts. What do you
> think?

I think that would work. This is essentially what the
FrameMsg_CommitNavigation_Params are doing here. Initially, I did not want to
add an additional structure (because we do have _plenty_ of structures already).
However, reading the code again, it may be more understandable than having the
FrameMsg_CommitNavigation_Params so early in the navigation.

[clamy, 2014-09-03 22:21:59]

clamy@chromium.org changed reviewers:
+ davidben@chromium.org

[clamy, 2014-09-03 22:23:44]

+davidben: FYI - this changes a bit the struct passed to the IO thread.
@nasko: We now have a separate class for tracking the parameters associated to
the navigation.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1769:
StartBrowserSideNavigation(kUrl2, main_frame_manager);
On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> Reading this as Start and then next line as Send, they seem like they do the
> same thing. Maybe rename the initial one to be SendRequestNavigation*, so it
> matches the SendBeginNavigation and form the pattern of Send<IPC Name>*.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1841: //
renderer when the navigation can commit.
On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> Do we need to pass the command line flag to exercise these tests? If yes, we
> should probably put that in the comments, so other devs can easily understand
> why those aren't run in default case.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1849:
StartBrowserSideNavigationWithParameters(
On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> It sounds wrong to reload when we have no committed navigation. Can we ensure
we
> properly handle this case?

Done.

https://chromiumcodereview.appspot.com/483773002/diff/70001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1865: // Now do
a shift+reload.
On 2014/08/28 16:39:09, (Do not use) nasko wrote:
> Shouldn't we commit the reload before issuing another one? Unless the intent
is
> to interleave the two.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/90001/content/browser/f...
File content/browser/frame_host/navigator_impl.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/90001/content/browser/f...
content/browser/frame_host/navigator_impl.h:30:
scoped_ptr<FrameMsg_CommitNavigation_Params>
On 2014/09/03 16:30:26, nasko wrote:
> Now that these are no longer static, they should move lower in the file. We
tend
> to have inherited methods first, then class specific methods later.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
File content/browser/frame_host/navigation_parameters.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.h:29: class
NavigationParameters {
Disclaimer: if you have a great name for that class I'd be happy to hear it :).

[(Do not use) nasko, 2014-09-05 17:04:49]

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_controller_impl.cc:1738: void
NavigationControllerImpl::FillHistoryParametersForNavigationEntry(
nit: This seems to be filling params "from" navigation entry, not "for" one.

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
File content/browser/frame_host/navigation_parameters.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.cc:115: }
nit: extra empty line needed

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
File content/browser/frame_host/navigation_parameters.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.h:26: // This class holds
parameters for a Navigation. It is first created when the
nit: "navigation", no need to uppercase.

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.h:27: // the navigation is
requested, then updated when the navigation begins and when
In the case of renderer initiated, there will be no "request" for navigation,
right?

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.h:29: class
NavigationParameters {
On 2014/09/03 22:23:44, clamy wrote:
> Disclaimer: if you have a great name for that class I'd be happy to hear it
:).

I don't have anything greater than what you've already come up with : ).

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.h:42: void
UpdateFromBeginNavigationParams(
nit: I'd drop the "Params" part of the name, it is clear in the parameter list.

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.h:47:
FrameHostMsg_BeginNavigation_Params SimulateRendererResponse() const;
Is this intended for tests? Or temporary? It should be clear what the purpose
is, as I doubt we want this to stick around in the long term.

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.h:52: // The URL to navigate
to. May be modified when th enavigation is ready to
"the navigation"

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.h:56: // The URL to request to
the browser to get access to the stream of data.
nit: "The URL the renderer should request from the browser ..."

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.h:59: bool is_post_;
comment?

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.h:67: // Informs the RenderView
the session history should be cleared. In that
nit: Let's start using RenderFrame in all new code/comments. Even if we
currently use RenderView, at least comments should mention RenderFrame, since
later refactor can miss the comments and they will diverge.

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.h:106: base::TimeTicks
browser_navigation_start_;
This is mostly a copy of FrameMsg_Navigate_Params. The initial thought was to
split parameters that are sent through IPCs in two sets - one specific to each
event (begin/commit) and the other being the common parameters we always send
around. This way we can follow the flow of data between processes and avoid
duplicating it all.

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigator_impl.cc:672: NavigationParameters*
NavigatorImpl::MakeNavigationParametersForTest(
I know this is for testing, but it seems that it will be good for this to return
a scoped_ptr, so the test doesn't have to explicitly delete it. We shouldn't be
leaking memory even in tests, otherwise leak detection can only be done in real
Chrome instance, which is not ideal.

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

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/common/f...
content/common/frame_messages.h:303: IPC_STRUCT_MEMBER(GURL, url)
Yeah, this is the point I was trying to make. This message has a huge overlap
with FrameMsg_Navigate_Params. We can always pass two structs in an IPC:

Send(new FrameMsg_Navigate(FrameMsg_NavigationParams, FrameMsg_Commit_Params))

This way we can "compose" all parameters based on the common and specific ones.

[clamy, 2014-09-05 18:21:25]

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
File content/browser/frame_host/navigation_parameters.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_parameters.h:106: base::TimeTicks
browser_navigation_start_;
On 2014/09/05 17:04:48, (Do not use) nasko wrote:
> This is mostly a copy of FrameMsg_Navigate_Params. The initial thought was to
> split parameters that are sent through IPCs in two sets - one specific to each
> event (begin/commit) and the other being the common parameters we always send
> around. This way we can follow the flow of data between processes and avoid
> duplicating it all.

So if we split up the IPCs in having some sort of core parameter (that contains
thing like history parameters, page id etc...) would you prefer if we store one
of those here rather than the separate parameters themselves?

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

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/common/f...
content/common/frame_messages.h:303: IPC_STRUCT_MEMBER(GURL, url)
On 2014/09/05 17:04:49, (Do not use) nasko wrote:
> Yeah, this is the point I was trying to make. This message has a huge overlap
> with FrameMsg_Navigate_Params. We can always pass two structs in an IPC:
> 
> Send(new FrameMsg_Navigate(FrameMsg_NavigationParams, FrameMsg_Commit_Params))
> 
> This way we can "compose" all parameters based on the common and specific
ones.

So should this be split up in something like FrameMsg_CoreNavigation_Params,
FrameMsg_CommitNavigation_Params and FrameMsg_Navigate_Params? Or maybe define a
content::NavigationCoreParams and have both FrameMsg_CommitNavigation_Params and
FrameMsg_Navigate_Params be IPC struct with parent as is done for
FrameHostMsg_DidCommitProvisionalLoad_Params?

[nasko, 2014-09-05 18:36:40]

On 2014/09/05 18:21:25, clamy wrote:
>
https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
> File content/browser/frame_host/navigation_parameters.h (right):
> 
>
https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
> content/browser/frame_host/navigation_parameters.h:106: base::TimeTicks
> browser_navigation_start_;
> On 2014/09/05 17:04:48, (Do not use) nasko wrote:
> > This is mostly a copy of FrameMsg_Navigate_Params. The initial thought was
to
> > split parameters that are sent through IPCs in two sets - one specific to
each
> > event (begin/commit) and the other being the common parameters we always
send
> > around. This way we can follow the flow of data between processes and avoid
> > duplicating it all.
> 
> So if we split up the IPCs in having some sort of core parameter (that
contains
> thing like history parameters, page id etc...) would you prefer if we store
one
> of those here rather than the separate parameters themselves?

Let's store the struct in a scoped_ptr and not have individual parameters used.
Whether we need a class to wrap them or not I'm not clear yet, so let's look at
how it looks once it is moved over.

>
https://chromiumcodereview.appspot.com/483773002/diff/110001/content/common/f...
> File content/common/frame_messages.h (right):
> 
>
https://chromiumcodereview.appspot.com/483773002/diff/110001/content/common/f...
> content/common/frame_messages.h:303: IPC_STRUCT_MEMBER(GURL, url)
> On 2014/09/05 17:04:49, (Do not use) nasko wrote:
> > Yeah, this is the point I was trying to make. This message has a huge
overlap
> > with FrameMsg_Navigate_Params. We can always pass two structs in an IPC:
> > 
> > Send(new FrameMsg_Navigate(FrameMsg_NavigationParams,
FrameMsg_Commit_Params))
> > 
> > This way we can "compose" all parameters based on the common and specific
> ones.
> 
> So should this be split up in something like FrameMsg_CoreNavigation_Params,
> FrameMsg_CommitNavigation_Params and FrameMsg_Navigate_Params? Or maybe define
a
> content::NavigationCoreParams and have both FrameMsg_CommitNavigation_Params
and
> FrameMsg_Navigate_Params be IPC struct with parent as is done for
> FrameHostMsg_DidCommitProvisionalLoad_Params?

I like the latter approach better. It will be good to see if we can either
extend content::FrameNavigateParams or reuse it somehow in a
content::NavigationParams struct, so we don't get more copies of the same
params. In general, I'd like us to get to a state where we can compose
structures instead of having copies of the same in various places.

Also, thanks for sticking through with this. I know the CL is dragging longer
than needed, but I'd like us to nail this early, so we can have cleaner code to
work with and less technical debt for the future.

[clamy, 2014-09-08 18:36:35]

Patchset #5 (id:130001) has been deleted

[clamy, 2014-09-08 18:37:12]

Here is the version with structs+inheritance. PTAL

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigation_controller_impl.cc:1738: void
NavigationControllerImpl::FillHistoryParametersForNavigationEntry(
On 2014/09/05 17:04:48, (Do not use) nasko wrote:
> nit: This seems to be filling params "from" navigation entry, not "for" one.

Removed the function since the new patchset only has a single caller for that
code.

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/110001/content/browser/...
content/browser/frame_host/navigator_impl.cc:672: NavigationParameters*
NavigatorImpl::MakeNavigationParametersForTest(
On 2014/09/05 17:04:48, (Do not use) nasko wrote:
> I know this is for testing, but it seems that it will be good for this to
return
> a scoped_ptr, so the test doesn't have to explicitly delete it. We shouldn't
be
> leaking memory even in tests, otherwise leak detection can only be done in
real
> Chrome instance, which is not ideal.

Done.

[clamy, 2014-09-15 17:43:09]

clamy@chromium.org changed reviewers:
+ creis@chromium.org

[clamy, 2014-09-15 17:45:46]

@creis: PTAL.

[Charlie Reis, 2014-09-15 21:19:40]

I have some meta-level comments.  I'll note that I have some of the context from
Nasko but probably don't know all the details, so feel free to let me know if
this CL is just a step toward answering some of these questions.

The main topic: I like that we're trying to find a way to unify the navigation
params, but I'm finding myself confused by them at the moment.  What's the goal
for these new structs in this CL, and in the future?  (Perhaps we should add it
to the CL description.)

Right now, I'm finding it difficult to determine which bits of data belong in
which struct, and what relationship the structs have to each other.

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

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
content/browser/frame_host/navigation_request_info.h:36: // Whether or not we
should allow the URL to download.
This comment is a bit unclear.  What value does this have when it's not a
download (the common case), and how can you tell whether it's a download or not?

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
content/browser/frame_host/navigation_request_info.h:37: bool allow_download;
This is also one of the "request info" struct items.  Why isn't that struct
being used here?

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

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1235:
render_view_host_->SetState(RenderViewHostImpl::STATE_DEFAULT);
I don't understand this state transition.  The destination RFH/RVH is either
already in the default state (e.g., if it's new), or we're swapping it back in. 
In that case, it seems like the state should match whether it's visible or not. 
We do the visual swap after the renderer tells us the navigation has committed,
right?  Or are we moving that earlier to this point in time?

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1237: // TODO(clamy): Check
if we have to be concerned about throbbing javascript
What does this mean?

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
content/browser/frame_host/render_frame_host_impl.h:290: // PlzNavigate:
Indicates that a navigation has committed and is ready for
"has committed" sounds wrong to me.  This is called when the navigation is ready
to commit in the RenderFrame, right?

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

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:94:
FrameMsg_CommitNavigation_Params MakeCommitNavigationParams(
This kind of conversion makes me think we may not have the right struct
definitions yet.  Perhaps it's necessary, but it feels awkward and can lead to
certain bits of information getting forgotten (in future CLs).  Maybe there's a
way we can use the same struct, or compose them without doing an item-for-item
copy?

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

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:541:
scoped_ptr<NavigationParamsWithRequestAndCommitInfo> navigation_params_;
I want to be careful before we add navigation-specific state to RFHM.  We've
just eliminated the old PendingNavigationParams, and PlzNavigate will let us
eliminate pending_render_frame_host_ as well.  It would be nice if RFHM just
managed the RenderFrameHosts and didn't know about the navigation details.

Is there another way to handle this data?  Can we avoid storing it as state at
all?  (Maybe this is the right place, but it would be nice to avoid if we can.)

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

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/common/f...
content/common/frame_messages.h:103:
IPC_STRUCT_TRAITS_BEGIN(content::CoreNavigationParams)
Why are these defined in reverse order?  Is that a constraint from the macros?

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/common/f...
content/common/frame_messages.h:203: IPC_STRUCT_MEMBER(GURL,
base_url_for_data_url)
Are the rest of these going to get migrated to the struct in the future, or is
there a reason they won't be included?  (If it's the former, we should put a
TODO on FrameMsg_Navigate_Params to eventually put everything in the struct.)

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/common/f...
content/common/frame_messages.h:251: IPC_STRUCT_MEMBER(std::string, method)
Same question.

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/common/n...
File content/common/navigation_params.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/common/n...
content/common/navigation_params.h:38: struct NavigationParamsWithCommitInfo :
CoreNavigationParams {
Please add a comment saying what stage(s) of navigation this data is needed for.
 Right now, I'm not sure where I would put a new bit of data if one came up
(e.g., should_replace_entry).

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/common/n...
content/common/navigation_params.h:75: struct
NavigationParamsWithRequestAndCommitInfo :
Please add a comment saying when this is used.

At the moment, I don't understand the inheritance relationship.  If "request
info" is things attached to the request and "commit info" is things we need to
know when it commits, then wouldn't we be tacking commit info onto request info
(once it's learned), rather than vice versa?

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/common/n...
content/common/navigation_params.h:79: bool is_post;
Please add a comment here.

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/common/n...
content/common/navigation_params.h:81: // Whether or not we should allow the URL
to download.
As noted before, what if it's not a download?  (And how can we tell?)

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/renderer...
File content/renderer/render_frame_impl.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/renderer...
content/renderer/render_frame_impl.h:535: //PlzNavigate
nit: Space

[clamy, 2014-09-15 21:35:18]

The main goal of the structs are to make sure that we avoid having duplicated
parameters in our IPCs. I was on verge between using inheritance for the
structs, or changing the IPCs to be composed of various structs (eg we could
have one for core parameters, one for request parameters). The latter may be
clearer, but it requires changes in all users of FrameMsg_Navigate. So I am not
completely sure what to use, especially as inheriting structs seemed to be used
by other IPCs.

As for storing the parameters, this is because we don't have access to the
NavigationEntry at CommitTime. Yet I think we still need some information coming
from it to properly commit the navigation (eg page_id,
is_overriding_user_agent). So I ended up storing these parameters when we have
access to them (RequestNavigation), and provide them to the renderer through the
FrameMsg_CommitNavigation.

[clamy, 2014-09-18 20:48:00]

Patchset #6 (id:170001) has been deleted

[clamy, 2014-09-18 20:50:39]

@creis, nasko: PTAL

I have changed the CL to use three separate structs for navigation parameters. I
think it makes understanding what is passed where easier (but comes with a
rewrite of about every single line that touches a FrameMsg_Navigate).

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

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
content/browser/frame_host/navigation_request_info.h:36: // Whether or not we
should allow the URL to download.
On 2014/09/15 21:19:39, Charlie Reis wrote:
> This comment is a bit unclear.  What value does this have when it's not a
> download (the common case), and how can you tell whether it's a download or
not?

This is required by the loader, and I think it is true by default. I am not sure
if it can actually be false for a navigation request.

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

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1235:
render_view_host_->SetState(RenderViewHostImpl::STATE_DEFAULT);
On 2014/09/15 21:19:39, Charlie Reis wrote:
> I don't understand this state transition.  The destination RFH/RVH is either
> already in the default state (e.g., if it's new), or we're swapping it back
in. 
> In that case, it seems like the state should match whether it's visible or
not. 
> We do the visual swap after the renderer tells us the navigation has
committed,
> right?  Or are we moving that earlier to this point in time?

Right now we are swapping it in RFHM::CommitNavigation. However we probably want
to do it after the renderer tells us that the navigation has committed (I was
planning to do that in another CL since this one is already a bit big).

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
content/browser/frame_host/render_frame_host_impl.h:290: // PlzNavigate:
Indicates that a navigation has committed and is ready for
On 2014/09/15 21:19:39, Charlie Reis wrote:
> "has committed" sounds wrong to me.  This is called when the navigation is
ready
> to commit in the RenderFrame, right?

Is it better now?

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

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:541:
scoped_ptr<NavigationParamsWithRequestAndCommitInfo> navigation_params_;
On 2014/09/15 21:19:40, Charlie Reis wrote:
> I want to be careful before we add navigation-specific state to RFHM.  We've
> just eliminated the old PendingNavigationParams, and PlzNavigate will let us
> eliminate pending_render_frame_host_ as well.  It would be nice if RFHM just
> managed the RenderFrameHosts and didn't know about the navigation details.
> 
> Is there another way to handle this data?  Can we avoid storing it as state at
> all?  (Maybe this is the right place, but it would be nice to avoid if we
can.)

We need some of this state when the navigation is ready to commit (eg page_id).
This is coming from the NavigationEntry. Since we discussed in the previous CL
that we can't depend on the pending_navigation_entry, I don't have that much
choice apart from storing the information when I get it (that is
RequestNavigation). However, I think the new format of structs allow for less of
it to be stored.

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/renderer...
File content/renderer/render_frame_impl.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/renderer...
content/renderer/render_frame_impl.h:535: //PlzNavigate
On 2014/09/15 21:19:40, Charlie Reis wrote:
> nit: Space

Done.

[Charlie Reis, 2014-09-19 23:12:32]

Thanks!  I think this will be useful to clearly see which groups of navigation
params are needed at which times.  Is it the goal for these to fully replace the
other navigation param structs / IPC messages that exist today?

Note: I'll probably hand the review back to Nasko now that he's returned, once
we resolve the questions below.

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

https://chromiumcodereview.appspot.com/483773002/diff/150001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1235:
render_view_host_->SetState(RenderViewHostImpl::STATE_DEFAULT);
On 2014/09/18 20:50:39, clamy wrote:
> On 2014/09/15 21:19:39, Charlie Reis wrote:
> > I don't understand this state transition.  The destination RFH/RVH is either
> > already in the default state (e.g., if it's new), or we're swapping it back
> in. 
> > In that case, it seems like the state should match whether it's visible or
> not. 
> > We do the visual swap after the renderer tells us the navigation has
> committed,
> > right?  Or are we moving that earlier to this point in time?
> 
> Right now we are swapping it in RFHM::CommitNavigation. However we probably
want
> to do it after the renderer tells us that the navigation has committed (I was
> planning to do that in another CL since this one is already a bit big).

Making that change in another CL is fine, but I do think it needs to be moved. 
Can we remove this line until then, or put a TODO to move it if it's needed in
the meantime?

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

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/navigation_request_info.h:39: // Whether or not we
should allow the URL to download (true by default).
Ah, I see the point of confusion now.  This description (which was copied from
existing code) makes it sounds like we already know it's a download, and that
the bool indicates whether it should be permitted.  Instead, it's about whether
future downloads of the URL are allowed.

Can we rephrase it to this?

// Whether or not we should allow this URL to be downloaded (true by default).

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/navigation_request_info.h:40: bool allow_download;
I still don't follow why this was moved from FrameHostMsg_BeginNavigation_Params
to here.  Is there a plan to move more of those params to here, or is this one
different?

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/navigator_impl.cc:90: // Avoid downloading when in
view-source mode.
This comment sounds like it belongs above line 84.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/navigator_impl.cc:377: RequestNavigationParams
request_params;
Why are the first two scoped_ptrs and this one isn't?  Please explain in a
comment.

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

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1339:
render_view_host_->SetState(RenderViewHostImpl::STATE_DEFAULT);
As mentioned, please either remove this or add a TODO to move it to DidCommit.

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

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:51: int
LoadFlagFromNavigationType(FrameMsg_Navigate_Type::Value navigation_type) {
Where is this code from?  (In other words, how does this work in the
non-PlzNavigate case?)

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:695:
*core_navigation_params_,
These are scoped_ptrs.  Shouldn't we be passing ownership of them and clearing
them out?

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

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:545: // PlzNavigate:
Holds info that should be sent to the renderer when the
Do these have the same lifetime as navigation_request_?  Should they live there
instead?  (If not, please clarify how their lifetime differs in the comment.)

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:548:
scoped_ptr<CommitNavigationParams> commit_navigation_params_;
(I'm still a bit nervous about storing request-specific state in RFHM, with the
risk of it being overwritten by a different navigation request.  If this data
lived with the request on the IO thread and was then passed back, then RFHM
would be stateless with respect to ResourceRequests.  The thread hop seems
problematic, though, and we probably don't want a map of requests indexed by
navigation_request_id.  Perhaps storing it here is ok for the time being.)

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

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:394:
FrameTreeNode* node) {
nit: Wrong indent.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1913:
BrowserSideNavigationReload) {
Fits on one line?

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1935: 1;
Move 1 to previous line?  (Same below)

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

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/f...
content/common/frame_messages.h:254: // TODO(clamy): See if it possible to
define acommon struct between this IPC
nits: it is, a common

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/f...
content/common/frame_messages.h:459: // The renderer should request the url to
get access to the stream containing
should request |stream_url| to get

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/f...
content/common/frame_messages.h:462: GURL,
Please name these params, as in the other messages (e.g., /* stream_url */).

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/n...
File content/common/navigation_params.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/n...
content/common/navigation_params.h:21: // The following structures hold
parameters used during a navigation. In
Thanks.  Despite the more verbose syntax to access the parameters, I think it's
now much clearer to understand that different parameters are relevant in
particular phases of the navigation timeline.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/n...
content/common/navigation_params.h:69: // succeeds, then this page_id will be
reflected in the resultant
nit: resultant -> resulting

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/renderer...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/renderer...
content/renderer/render_frame_impl.cc:817: "url",
nit: These don't need to be wrapped one-per-line.  (That's only necessary for
function declarations, not call sites.)

[nasko, 2014-09-22 23:13:02]

nasko@chromium.org changed reviewers:
- nasko@google.com

[nasko, 2014-09-22 23:13:04]

Some comments as I'm swapping back this CL in short term memory : ).

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

https://codereview.chromium.org/483773002/diff/190001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1225:
params.core_params.url = url;
nit: I'd put the core params before the commit ones.

https://codereview.chromium.org/483773002/diff/190001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1228:
params.commit_params.browser_navigation_start = base::TimeTicks::Now();
nit: move this up to the group of commit_* members

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

https://codereview.chromium.org/483773002/diff/190001/content/common/frame_me...
content/common/frame_messages.h:358: IPC_MESSAGE_ROUTED1(FrameMsg_Navigate ,
FrameMsg_Navigate_Params)
nit: extra space before the comma

https://codereview.chromium.org/483773002/diff/190001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/483773002/diff/190001/content/common/navigati...
content/common/navigation_params.h:26: struct CONTENT_EXPORT
CoreNavigationParams {
Reading through other files, the "Core" prefix doesn't seem quite right. I don't
a have a really good replacement. Do you think "Common" would work better?

https://codereview.chromium.org/483773002/diff/190001/content/common/navigati...
content/common/navigation_params.h:29: // The URL to navigate to.
nit: empty line above the comment should make it a bit more readable.

https://codereview.chromium.org/483773002/diff/190001/content/common/navigati...
content/common/navigation_params.h:43: // Whether or not we should allow the URL
to download.
nit: There was a long thread at some point about avoiding the word "we". I've
tried to follow it as much as I can and it mostly results in better comments.

https://codereview.chromium.org/483773002/diff/190001/content/common/navigati...
content/common/navigation_params.h:52: bool is_post;
nit: emtpy line between the destructor and member and a missing comment.

https://codereview.chromium.org/483773002/diff/190001/content/common/navigati...
content/common/navigation_params.h:73: // Informs the RenderView the session
history should be cleared. In that
nit: Since RenderView is going to go away, let's aim to use comments free of it.
I think it will be just as valid to say "Informs the renderer process ..." or
just "renderer".

https://codereview.chromium.org/483773002/diff/190001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/483773002/diff/190001/content/renderer/render...
content/renderer/render_frame_impl.cc:861:
params.commit_params.pending_history_list_offset;
This method is starting a browser initiated navigation. It looks incorrect to be
using the commit_params at this stage, doesn't it?

[clamy, 2014-09-23 21:13:27]

Thanks!
@creis: eventually we want to remove the FrameMsg_Navigate when we have
completely switched to browser side navigation. I think the other IPCs should
remain.

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

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/navigation_request_info.h:40: bool allow_download;
On 2014/09/19 23:12:31, Charlie Reis wrote:
> I still don't follow why this was moved from
FrameHostMsg_BeginNavigation_Params
> to here.  Is there a plan to move more of those params to here, or is this one
> different?

Ah it got lost in the moving of parameters. It is now part of CoreParams (and
provided by the browser). Ideally  I think FrameHostMsg_BeginNavigation_Params
should have as few parameters as is necessary, as it is coming from the
renderer, which may have been compromised.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/navigator_impl.cc:90: // Avoid downloading when in
view-source mode.
On 2014/09/19 23:12:31, Charlie Reis wrote:
> This comment sounds like it belongs above line 84.

Moved to the description of the parameter in CoreNavigationParameters.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/navigator_impl.cc:377: RequestNavigationParams
request_params;
On 2014/09/19 23:12:31, Charlie Reis wrote:
> Why are the first two scoped_ptrs and this one isn't?  Please explain in a
> comment.

Following changes in ownership of the *Params, they now all are non scoped_ptr.

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

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1225:
params.core_params.url = url;
On 2014/09/22 23:13:03, nasko wrote:
> nit: I'd put the core params before the commit ones.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1228:
params.commit_params.browser_navigation_start = base::TimeTicks::Now();
On 2014/09/22 23:13:03, nasko wrote:
> nit: move this up to the group of commit_* members

Moved it above the history parameters as history parameters are no longer part
of CommitNavigationParams.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_impl.cc:1339:
render_view_host_->SetState(RenderViewHostImpl::STATE_DEFAULT);
On 2014/09/19 23:12:31, Charlie Reis wrote:
> As mentioned, please either remove this or add a TODO to move it to DidCommit.

Done.

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

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:51: int
LoadFlagFromNavigationType(FrameMsg_Navigate_Type::Value navigation_type) {
On 2014/09/19 23:12:31, Charlie Reis wrote:
> Where is this code from?  (In other words, how does this work in the
> non-PlzNavigate case?)

This code is here to handle the case where we don't have a renderer. Currently
the caching policy is decided by blink in various places (in particular
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
and then translated to a load flag in WebURLLoaderImpl
(https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_...).
I have plans to unify it in the future. Added a TODO to reflect that.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:695:
*core_navigation_params_,
On 2014/09/19 23:12:31, Charlie Reis wrote:
> These are scoped_ptrs.  Shouldn't we be passing ownership of them and clearing
> them out?

They are now members of NavigationRequest.

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

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:545: // PlzNavigate:
Holds info that should be sent to the renderer when the
On 2014/09/19 23:12:31, Charlie Reis wrote:
> Do these have the same lifetime as navigation_request_?  Should they live
there
> instead?  (If not, please clarify how their lifetime differs in the comment.)

I moved them inside navigation_request_. They did not have the same lifetime
initially, but it is probably better if they do.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager.h:548:
scoped_ptr<CommitNavigationParams> commit_navigation_params_;
On 2014/09/19 23:12:31, Charlie Reis wrote:
> (I'm still a bit nervous about storing request-specific state in RFHM, with
the
> risk of it being overwritten by a different navigation request.  If this data
> lived with the request on the IO thread and was then passed back, then RFHM
> would be stateless with respect to ResourceRequests.  The thread hop seems
> problematic, though, and we probably don't want a map of requests indexed by
> navigation_request_id.  Perhaps storing it here is ok for the time being.)

I think storing them in the NavigationRequest makes it simpler, especially as
davidben@ has a CL that clarifies thread hops involving the NavigationRequest.

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

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:394:
FrameTreeNode* node) {
On 2014/09/19 23:12:32, Charlie Reis wrote:
> nit: Wrong indent.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1913:
BrowserSideNavigationReload) {
On 2014/09/19 23:12:32, Charlie Reis wrote:
> Fits on one line?

Done.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1935: 1;
On 2014/09/19 23:12:32, Charlie Reis wrote:
> Move 1 to previous line?  (Same below)

Done (git cl format is weird sometimes).

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

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/f...
content/common/frame_messages.h:254: // TODO(clamy): See if it possible to
define acommon struct between this IPC
On 2014/09/19 23:12:32, Charlie Reis wrote:
> nits: it is, a common

Done.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/f...
content/common/frame_messages.h:358: IPC_MESSAGE_ROUTED1(FrameMsg_Navigate ,
FrameMsg_Navigate_Params)
On 2014/09/22 23:13:03, nasko wrote:
> nit: extra space before the comma

Done.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/f...
content/common/frame_messages.h:459: // The renderer should request the url to
get access to the stream containing
On 2014/09/19 23:12:32, Charlie Reis wrote:
> should request |stream_url| to get

Done.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/f...
content/common/frame_messages.h:462: GURL,
On 2014/09/19 23:12:32, Charlie Reis wrote:
> Please name these params, as in the other messages (e.g., /* stream_url */).

Done.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/n...
File content/common/navigation_params.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/n...
content/common/navigation_params.h:29: // The URL to navigate to.
On 2014/09/22 23:13:03, nasko wrote:
> nit: empty line above the comment should make it a bit more readable.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/n...
content/common/navigation_params.h:43: // Whether or not we should allow the URL
to download.
On 2014/09/22 23:13:03, nasko wrote:
> nit: There was a long thread at some point about avoiding the word "we". I've
> tried to follow it as much as I can and it mostly results in better comments. 

Done.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/common/n...
content/common/navigation_params.h:52: bool is_post;
On 2014/09/22 23:13:03, nasko wrote:
> nit: emtpy line between the destructor and member and a missing comment.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/renderer...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/renderer...
content/renderer/render_frame_impl.cc:817: "url",
On 2014/09/19 23:12:32, Charlie Reis wrote:
> nit: These don't need to be wrapped one-per-line.  (That's only necessary for
> function declarations, not call sites.)

Done.

https://chromiumcodereview.appspot.com/483773002/diff/190001/content/renderer...
content/renderer/render_frame_impl.cc:861:
params.commit_params.pending_history_list_offset;
On 2014/09/22 23:13:04, nasko wrote:
> This method is starting a browser initiated navigation. It looks incorrect to
be
> using the commit_params at this stage, doesn't it?

History params are no longer part of CommitParams.

https://chromiumcodereview.appspot.com/483773002/diff/210001/content/common/n...
File content/common/navigation_params.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/210001/content/common/n...
content/common/navigation_params.h:86: ~CommitNavigationParams();
I removed the history parameters from CommitNavigationParams, because I am
hoping that we won't need them in the PlzNavigate architecture. If we find they
are still needed, we can reintroduce them later.

[nasko, 2014-09-24 22:42:15]

I really like the new structure where NavigationRequest owns all the parameters
and RFHM only holds onto a single NavigationRequest. Few more comments.

https://codereview.chromium.org/483773002/diff/210001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/483773002/diff/210001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:51: // TODO(clamy):
unify the code with what is happening on the browser side.
This is the browser side. Do you mean to unify with renderer side?

https://codereview.chromium.org/483773002/diff/210001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:617:
DCHECK(navigation_request_.get());
Is this valid DCHECK? In the case of renderer initiated navigation, this is the
entrypoint, so I'd expect us to not have generated the navigation_request_ just
yet.

https://codereview.chromium.org/483773002/diff/210001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:634: // use for the
navigation.
What happened with the code to cancel existing navigation?

https://codereview.chromium.org/483773002/diff/210001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:691:
frame_tree_node_->navigator()->CommitNavigation(
RFHM deals directly with RFHs and manages their lifetime/state. Why do we have
to go through the Navigator if we know which RFH we are about to commit to and
can directly call into it?

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

https://codereview.chromium.org/483773002/diff/210001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:396:
FrameTreeNode* node) {
nit: I'd put the node parameter first, as we logically want to "navigate a node
to a url". Same in the method below.

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

https://codereview.chromium.org/483773002/diff/210001/content/common/frame_me...
content/common/frame_messages.h:467:
IPC_MESSAGE_ROUTED3(FrameMsg_CommitNavigation,
How are we going to pass the response headers down to the renderer? Based on
David's proposed CL, they will not be included in the stream, which contains
only the body.

https://codereview.chromium.org/483773002/diff/210001/content/common/navigati...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/483773002/diff/210001/content/common/navigati...
content/common/navigation_params.cc:37: : is_post(is_post),
nit: wrong indent

https://codereview.chromium.org/483773002/diff/210001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/483773002/diff/210001/content/common/navigati...
content/common/navigation_params.h:86: ~CommitNavigationParams();
On 2014/09/23 21:13:26, clamy wrote:
> I removed the history parameters from CommitNavigationParams, because I am
> hoping that we won't need them in the PlzNavigate architecture. If we find
they
> are still needed, we can reintroduce them later.

How would history work in the PlzNavigate world then? History will move to the
browser, but unknown when. Do we expect to break it while developing
PlzNavigate? If yes, then let's call it out somewhere.

[clamy, 2014-09-26 17:22:32]

Thanks!

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

https://chromiumcodereview.appspot.com/483773002/diff/210001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:51: // TODO(clamy):
unify the code with what is happening on the browser side.
On 2014/09/24 22:42:14, nasko wrote:
> This is the browser side. Do you mean to unify with renderer side?

Done.

https://chromiumcodereview.appspot.com/483773002/diff/210001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:617:
DCHECK(navigation_request_.get());
On 2014/09/24 22:42:14, nasko wrote:
> Is this valid DCHECK? In the case of renderer initiated navigation, this is
the
> entrypoint, so I'd expect us to not have generated the navigation_request_
just
> yet.

I added a TODO to reflect that we should generate the navigation_request_ in
that case.

https://chromiumcodereview.appspot.com/483773002/diff/210001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:634: // use for the
navigation.
On 2014/09/24 22:42:14, nasko wrote:
> What happened with the code to cancel existing navigation?

It was moved to RequestNavigation which is where we now reset the
navigation_request_. David's patch makes it disappear entirely so the lifetime
should be even easier to track.

https://chromiumcodereview.appspot.com/483773002/diff/210001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:691:
frame_tree_node_->navigator()->CommitNavigation(
On 2014/09/24 22:42:14, nasko wrote:
> RFHM deals directly with RFHs and manages their lifetime/state. Why do we have
> to go through the Navigator if we know which RFH we are about to commit to and
> can directly call into it?

Because Navigator is making a security check to make sure that a WebUI is not
displayed in a non WebUI RFH. I initially had this security check as a method of
NavigatorImpl, but was asked for it to remain private and checked by the
navigator in a previous code review (I think by Charlie).

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

https://chromiumcodereview.appspot.com/483773002/diff/210001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:396:
FrameTreeNode* node) {
On 2014/09/24 22:42:15, nasko wrote:
> nit: I'd put the node parameter first, as we logically want to "navigate a
node
> to a url". Same in the method below.

Done.

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

https://chromiumcodereview.appspot.com/483773002/diff/210001/content/common/f...
content/common/frame_messages.h:467:
IPC_MESSAGE_ROUTED3(FrameMsg_CommitNavigation,
On 2014/09/24 22:42:15, nasko wrote:
> How are we going to pass the response headers down to the renderer? Based on
> David's proposed CL, they will not be included in the stream, which contains
> only the body.

I plan on adding another member to that IPC. If David's CL lands first then I'll
add it in this CL, otherwise in a follow up CL.

https://chromiumcodereview.appspot.com/483773002/diff/210001/content/common/n...
File content/common/navigation_params.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/210001/content/common/n...
content/common/navigation_params.cc:37: : is_post(is_post),
On 2014/09/24 22:42:15, nasko wrote:
> nit: wrong indent

Done.

https://chromiumcodereview.appspot.com/483773002/diff/210001/content/common/n...
File content/common/navigation_params.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/210001/content/common/n...
content/common/navigation_params.h:86: ~CommitNavigationParams();
On 2014/09/24 22:42:15, nasko wrote:
> On 2014/09/23 21:13:26, clamy wrote:
> > I removed the history parameters from CommitNavigationParams, because I am
> > hoping that we won't need them in the PlzNavigate architecture. If we find
> they
> > are still needed, we can reintroduce them later.
> 
> How would history work in the PlzNavigate world then? History will move to the
> browser, but unknown when. Do we expect to break it while developing
> PlzNavigate? If yes, then let's call it out somewhere.

Honestly I am not sure. It would be a lot better if it was in the browser. In
any case all PlzNavigate navigations should go through the browser which should
reduce the back/forward races we currently experience, right? I added a TODO to
maybe move history parameters to one of the structs.

[nasko, 2014-09-26 22:16:43]

This is almost ready to go, just a few more minor comments. One thing I didn't
see your take on is the usage of "Core" in the navigation params.

https://codereview.chromium.org/483773002/diff/210001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/483773002/diff/210001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:634: // use for the
navigation.
On 2014/09/26 17:22:32, clamy wrote:
> On 2014/09/24 22:42:14, nasko wrote:
> > What happened with the code to cancel existing navigation?
> 
> It was moved to RequestNavigation which is where we now reset the
> navigation_request_. David's patch makes it disappear entirely so the lifetime
> should be even easier to track.

Acknowledged.

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

https://codereview.chromium.org/483773002/diff/210001/content/common/frame_me...
content/common/frame_messages.h:467:
IPC_MESSAGE_ROUTED3(FrameMsg_CommitNavigation,
On 2014/09/26 17:22:32, clamy wrote:
> On 2014/09/24 22:42:15, nasko wrote:
> > How are we going to pass the response headers down to the renderer? Based on
> > David's proposed CL, they will not be included in the stream, which contains
> > only the body.
> 
> I plan on adding another member to that IPC. If David's CL lands first then
I'll
> add it in this CL, otherwise in a follow up CL.

This goes along with the comment I made about CommitNavigationParams being const
in NavigationRequest. We will have to add the headers to it at a later point in
time, so it doesn't seem like it will be const in the future.

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

https://codereview.chromium.org/483773002/diff/250001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:66: *info_,
Are we sure that this NavigationRequest will outlive NavigationRequestInfo? What
happens if just as we send this call to the IO thread, another navigation is
initiated on the UI thread and it resets the current NavigationRequest?

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

https://codereview.chromium.org/483773002/diff/250001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:58: const CommitNavigationParams
commit_params_;
nit: This seems to work, but looks strange. In my mind, commit parameters aren't
all known until commit time, yet they are declared const at the beginning of the
navigation.

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

https://codereview.chromium.org/483773002/diff/250001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1951:
main_test_rfh()->SendNavigate(page_id, kUrl);
nit: Is there anything we need to test after the navigation? If not, why not
exit right after the last EXPECT?

https://codereview.chromium.org/483773002/diff/250001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/483773002/diff/250001/content/common/navigati...
content/common/navigation_params.h:97: base::TimeTicks browser_navigation_start;
Just a note, no action needed: It seems that redirect chain belongs in this
commit message at a later point in time. Since ideally we won't be roundtripping
to the renderer for redirects. I just wanted to note it and maybe add a todo in
the future.

https://codereview.chromium.org/483773002/diff/250001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/483773002/diff/250001/content/renderer/render...
content/renderer/render_frame_impl.cc:1351: NOTREACHED();
nit: NOTIMPLEMENTED, indicates more correctly the intention.

https://codereview.chromium.org/483773002/diff/250001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/483773002/diff/250001/content/renderer/render...
content/renderer/render_frame_impl.h:545: 
nit: Overriding methods don't have spaces between them. Might be useful to move
this below the #ifdefs and keep all PlzNavigate methods together.

[clamy, 2014-09-29 20:45:31]

Thanks! I have renamed CoreNavigationParams to CommonNavigationParams in the
latest patch set.

https://chromiumcodereview.appspot.com/483773002/diff/250001/content/browser/...
File content/browser/frame_host/navigation_request.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/250001/content/browser/...
content/browser/frame_host/navigation_request.cc:66: *info_,
On 2014/09/26 22:16:43, nasko wrote:
> Are we sure that this NavigationRequest will outlive NavigationRequestInfo?
What
> happens if just as we send this call to the IO thread, another navigation is
> initiated on the UI thread and it resets the current NavigationRequest?

We are making a copy of info and passing it to the IO thread, so there should be
no lifetime issue. Actually we only keep info in NavigationRequest to be able to
query it for tests.

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

https://chromiumcodereview.appspot.com/483773002/diff/250001/content/browser/...
content/browser/frame_host/navigation_request.h:58: const CommitNavigationParams
commit_params_;
On 2014/09/26 22:16:43, nasko wrote:
> nit: This seems to work, but looks strange. In my mind, commit parameters
aren't
> all known until commit time, yet they are declared const at the beginning of
the
> navigation.

As of now, all parameters in CommitParams are known at the start of navigation.
If it changes, I will make commit_params_ non const. Is that okay with you?

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

https://chromiumcodereview.appspot.com/483773002/diff/250001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1951:
main_test_rfh()->SendNavigate(page_id, kUrl);
On 2014/09/26 22:16:43, nasko wrote:
> nit: Is there anything we need to test after the navigation? If not, why not
> exit right after the last EXPECT?

Done.

https://chromiumcodereview.appspot.com/483773002/diff/250001/content/common/n...
File content/common/navigation_params.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/250001/content/common/n...
content/common/navigation_params.h:97: base::TimeTicks browser_navigation_start;
On 2014/09/26 22:16:43, nasko wrote:
> Just a note, no action needed: It seems that redirect chain belongs in this
> commit message at a later point in time. Since ideally we won't be
roundtripping
> to the renderer for redirects. I just wanted to note it and maybe add a todo
in
> the future.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/250001/content/renderer...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/483773002/diff/250001/content/renderer...
content/renderer/render_frame_impl.cc:1351: NOTREACHED();
On 2014/09/26 22:16:43, nasko wrote:
> nit: NOTIMPLEMENTED, indicates more correctly the intention.

Done.

https://chromiumcodereview.appspot.com/483773002/diff/250001/content/renderer...
File content/renderer/render_frame_impl.h (right):

https://chromiumcodereview.appspot.com/483773002/diff/250001/content/renderer...
content/renderer/render_frame_impl.h:545: 
On 2014/09/26 22:16:43, nasko wrote:
> nit: Overriding methods don't have spaces between them. Might be useful to
move
> this below the #ifdefs and keep all PlzNavigate methods together.

Done.

[nasko, 2014-09-29 22:12:11]

Seems there are some compile failures on rename of core->common. Once those are
fixed and the nits below, LGTM!

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

https://codereview.chromium.org/483773002/diff/250001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:66: *info_,
On 2014/09/29 20:45:30, clamy wrote:
> On 2014/09/26 22:16:43, nasko wrote:
> > Are we sure that this NavigationRequest will outlive NavigationRequestInfo?
> What
> > happens if just as we send this call to the IO thread, another navigation is
> > initiated on the UI thread and it resets the current NavigationRequest?
> 
> We are making a copy of info and passing it to the IO thread, so there should
be
> no lifetime issue. Actually we only keep info in NavigationRequest to be able
to
> query it for tests.

Should it be prefixed/suffixed with test marker then?

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

https://codereview.chromium.org/483773002/diff/250001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:58: const CommitNavigationParams
commit_params_;
On 2014/09/29 20:45:30, clamy wrote:
> On 2014/09/26 22:16:43, nasko wrote:
> > nit: This seems to work, but looks strange. In my mind, commit parameters
> aren't
> > all known until commit time, yet they are declared const at the beginning of
> the
> > navigation.
> 
> As of now, all parameters in CommitParams are known at the start of
navigation.
> If it changes, I will make commit_params_ non const. Is that okay with you?

sgtm

[clamy, 2014-09-29 23:12:12]

Thanks for the review!
@creis: PTAL

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

https://codereview.chromium.org/483773002/diff/250001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:66: *info_,
On 2014/09/29 22:12:11, nasko wrote:
> On 2014/09/29 20:45:30, clamy wrote:
> > On 2014/09/26 22:16:43, nasko wrote:
> > > Are we sure that this NavigationRequest will outlive
NavigationRequestInfo?
> > What
> > > happens if just as we send this call to the IO thread, another navigation
is
> > > initiated on the UI thread and it resets the current NavigationRequest?
> > 
> > We are making a copy of info and passing it to the IO thread, so there
should
> be
> > no lifetime issue. Actually we only keep info in NavigationRequest to be
able
> to
> > query it for tests.
> 
> Should it be prefixed/suffixed with test marker then?

I have renamed the getter info_for_test(), which should make it clearer.

[Charlie Reis, 2014-09-29 23:49:53]

LGTM with one comment (mostly deferring to Nasko).  Sorry about the merge
conflicts-- they're likely from https://codereview.chromium.org/606113005/, but
hopefully not complicated to fix.

I think you all have discussed the idea of moving some of the navigation logic
from RFHM to Navigator in a future CL (since RFHM should primarily be about
swapping between RFHs).  That would be great from my perspective.

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

https://codereview.chromium.org/483773002/diff/290001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:58: const CommitNavigationParams
commit_params_;
Why is only one of these const?  Worth explaining in the comment.

[clamy, 2014-09-30 00:40:59]

Thanks! I think we all agree about moving the logic for navigation out of RFHM,
and I have another CL in the works for that.

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

https://codereview.chromium.org/483773002/diff/290001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:58: const CommitNavigationParams
commit_params_;
On 2014/09/29 23:49:53, Charlie Reis wrote:
> Why is only one of these const?  Worth explaining in the comment.

I have added a comment to reflect what may be modified in
CommonNavigationParams.

[clamy, 2014-09-30 00:41:08]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-09-30 00:42:04]

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

[commit-bot: I haz the power, 2014-09-30 01:11:11]

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

[commit-bot: I haz the power, 2014-09-30 01:11:13]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[clamy, 2014-09-30 18:19:28]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-09-30 18:20:47]

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

[commit-bot: I haz the power, 2014-09-30 18:34:33]

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

[commit-bot: I haz the power, 2014-09-30 18:34:35]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[clamy, 2014-09-30 18:37:05]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2014-09-30 18:38:34]

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

[commit-bot: I haz the power, 2014-09-30 20:50:57]

Committed patchset #13 (id:330001) as d8f8919a68a02c86e38f2d0c6c6b1d5c5895b2f8

[commit-bot: I haz the power, 2014-09-30 20:51:38]

Patchset 13 (id:??) landed as
https://crrev.com/9bfeef40d0a1597a6cbf5835f0bc2c2553818975
Cr-Commit-Position: refs/heads/master@{#297495}