Remove the FrameMsg_Navigate_Params

Remove the FrameMsg_Navigate_Params

This CL removes the FrameMsg_Navigate_Params in favor of using the various
*NavigationParams struct. This allows a better unification of the current
navigation logic and the one being developped for browser-side navigation (aka
PlzNavigate).

In particular, the pending navigation parameters in RenderViewImpl are now composed of *NavigationParams struct which will allow to properly set them in PlzNavigate. It should also allow us to have the NavigationState be created from the NavigationParams. This is needed for calling the logic of RenderFrameImpl::didCommitProvisionalLoad without a data source in PlzNavigate.

BUG=459033
Committed: https://crrev.com/34e1278e145473eeecf118e63f3530dfd3620f57
Cr-Commit-Position: refs/heads/master@{#320263}

[clamy, 2015-03-09 14:34:45]

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

[clamy, 2015-03-09 14:34:47]

@nasko: PTAL
@carlosk, fdegans: FYI

This CL follows https://codereview.chromium.org/979443002/ and removes the
FrameMsg_Navigate_Params in favor of the various *NavigationParams struct. This
should allow for easier sharing of logic on the renderer-side (since OnNavigate
and OnCommitNavigation will use the same structs as a base).

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

https://codereview.chromium.org/971653004/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:113: commit_params.request_time
= base::Time::Now();
This setting of the time needs to be done in every CommitNavigationParams,
otherwise we hit a DCHECK in RenderFrameImpl::UpdateFrameNavigationTiming. I'm
considering having it in the default creator for CommitNavigationParams, wdyt?

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

https://codereview.chromium.org/971653004/diff/100001/content/common/navigati...
content/common/navigation_params.h:175: struct CONTENT_EXPORT
StartNavigationParams {
I was not quite sure what to name those, so I'd be interested in better naming
suggestions :).

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

https://codereview.chromium.org/971653004/diff/100001/content/renderer/render...
content/renderer/render_frame_impl.cc:1073:
render_view_->pending_common_params_.reset(
We probably need to do something similar in OnCommitNavigation, which we don't
actually, but I decided to leave this for another patch since I'm trying to keep
this one as small as possible.

https://codereview.chromium.org/971653004/diff/100001/content/renderer/render...
content/renderer/render_frame_impl.cc:2444: if
(!navigation_state->is_content_initiated()) {
This is quite bulky, and my intention is to refactor NavigationState next to
store the right parameters so that we can pass them from RenderView and then
back when we need them. This should also help making sure that the
didCommitProvisionalLoad logic, which is currently heavily dependent on a
NavigationState, can be called by PlzNavigate in the error case, without a data
source having been created in blink.

https://codereview.chromium.org/971653004/diff/100001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/971653004/diff/100001/content/renderer/render...
content/renderer/render_view_impl.cc:2277: navigation_state =
NavigationState::CreateBrowserInitiated(
As explained in render_frame_impl.cc, the next goal is to have NavigationState
take ownership of the pending HistoryNavigationParams and StartNavigationParams
here, which should make this cleaner.

https://codereview.chromium.org/971653004/diff/100001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/971653004/diff/100001/content/renderer/render...
content/renderer/render_view_impl.h:845: scoped_ptr<CommonNavigationParams>
pending_common_params_;
Unlike in RenderFrameHostImpl, I did not bundle these parameters together in a
struct, as the StartNavigationParameters is not used in PlzNavigate. Now I could
also define the struct in navigation_params.h and use it both in
RenderFrameHostImpl and here (with a default value for the StartNavigationParams
in PlzNavigate).

[nasko, 2015-03-09 23:24:24]

The CL description is lacking a BUG and some comments.

Overall, splitting the one params struct into multiples makes code a bit more
"bulky". I do see where we want to go in the future, so I think this is fine as
long as we have some task at the end to step back and look at what makes sense
to combine together again.

https://codereview.chromium.org/971653004/diff/100001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/971653004/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:237: CommonNavigationParams
MakeCommonNavigationParams(
minor nit: s/make/construct/. It is a bit longer, so open to alternatives or not
doing it at all.

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

https://codereview.chromium.org/971653004/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:113: commit_params.request_time
= base::Time::Now();
On 2015/03/09 14:34:46, clamy wrote:
> This setting of the time needs to be done in every CommitNavigationParams,
> otherwise we hit a DCHECK in RenderFrameImpl::UpdateFrameNavigationTiming. I'm
> considering having it in the default creator for CommitNavigationParams, wdyt?

Let's set those in the default creator and then we can overwrite them if needed
in specific places.

https://codereview.chromium.org/971653004/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/971653004/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:636: struct NavigateParams {
Let's name this so that it is obvious it is used only for suspended navigations.
Let's say SuspendedNavigationParams?

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

https://codereview.chromium.org/971653004/diff/100001/content/common/navigati...
content/common/navigation_params.h:173: // Parameters needed at the start of a
navigation. Used by FrameMsg_navigate.
nit: FrameMsg_Navigate

https://codereview.chromium.org/971653004/diff/100001/content/common/navigati...
content/common/navigation_params.h:185: // Whether the navigation is a POST
request (as opposed to a GET).
nit: Empty line above the comment.

https://codereview.chromium.org/971653004/diff/100001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/971653004/diff/100001/content/renderer/render...
content/renderer/render_view_impl.h:845: scoped_ptr<CommonNavigationParams>
pending_common_params_;
On 2015/03/09 14:34:47, clamy wrote:
> Unlike in RenderFrameHostImpl, I did not bundle these parameters together in a
> struct, as the StartNavigationParameters is not used in PlzNavigate. Now I
could
> also define the struct in navigation_params.h and use it both in
> RenderFrameHostImpl and here (with a default value for the
StartNavigationParams
> in PlzNavigate).

Unifying those together will be nice, as we don't have a chance of having
different lifetimes.

[clamy, 2015-03-10 16:45:45]

Thanks! I now have a struct bundling the other structs, hopefully it should make
the code a bit less bulky.

https://codereview.chromium.org/971653004/diff/100001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/971653004/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:237: CommonNavigationParams
MakeCommonNavigationParams(
On 2015/03/09 23:24:23, nasko wrote:
> minor nit: s/make/construct/. It is a bit longer, so open to alternatives or
not
> doing it at all.

Done.

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

https://codereview.chromium.org/971653004/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:113: commit_params.request_time
= base::Time::Now();
On 2015/03/09 23:24:23, nasko wrote:
> On 2015/03/09 14:34:46, clamy wrote:
> > This setting of the time needs to be done in every CommitNavigationParams,
> > otherwise we hit a DCHECK in RenderFrameImpl::UpdateFrameNavigationTiming.
I'm
> > considering having it in the default creator for CommitNavigationParams,
wdyt?
> 
> Let's set those in the default creator and then we can overwrite them if
needed
> in specific places.

Done.

https://codereview.chromium.org/971653004/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/971653004/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:636: struct NavigateParams {
On 2015/03/09 23:24:23, nasko wrote:
> Let's name this so that it is obvious it is used only for suspended
navigations.
> Let's say SuspendedNavigationParams?

I've moved it to navigation_params.h to reuse in RenderViewImpl.

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

https://codereview.chromium.org/971653004/diff/100001/content/common/navigati...
content/common/navigation_params.h:173: // Parameters needed at the start of a
navigation. Used by FrameMsg_navigate.
On 2015/03/09 23:24:23, nasko wrote:
> nit: FrameMsg_Navigate

Done.

https://codereview.chromium.org/971653004/diff/100001/content/common/navigati...
content/common/navigation_params.h:185: // Whether the navigation is a POST
request (as opposed to a GET).
On 2015/03/09 23:24:23, nasko wrote:
> nit: Empty line above the comment.

Done.

https://codereview.chromium.org/971653004/diff/100001/content/renderer/render...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/971653004/diff/100001/content/renderer/render...
content/renderer/render_view_impl.h:845: scoped_ptr<CommonNavigationParams>
pending_common_params_;
On 2015/03/09 23:24:23, nasko wrote:
> On 2015/03/09 14:34:47, clamy wrote:
> > Unlike in RenderFrameHostImpl, I did not bundle these parameters together in
a
> > struct, as the StartNavigationParameters is not used in PlzNavigate. Now I
> could
> > also define the struct in navigation_params.h and use it both in
> > RenderFrameHostImpl and here (with a default value for the
> StartNavigationParams
> > in PlzNavigate).
> 
> Unifying those together will be nice, as we don't have a chance of having
> different lifetimes.

Done.

[nasko, 2015-03-11 13:59:00]

Packing things in one struct does seem a bit cleaner from code readability
perspective. 

I have one clarification question, though LGTM.

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

https://codereview.chromium.org/971653004/diff/130001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:112: frame_tree_node,
common_params, begin_params, CommitNavigationParams(),
This looks much nicer this way!

https://codereview.chromium.org/971653004/diff/130001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/971653004/diff/130001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:299: void Navigate(const
CommonNavigationParams& common_params,
Is there a specific reason this doesn't take the NavigationParams struct?

[clamy, 2015-03-11 16:50:07]

Thanks!

https://codereview.chromium.org/971653004/diff/130001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/971653004/diff/130001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:299: void Navigate(const
CommonNavigationParams& common_params,
On 2015/03/11 13:59:00, nasko wrote:
> Is there a specific reason this doesn't take the NavigationParams struct?

No, however this makes the logic in the function a bit less readable, since
instead of commit_params.url we have to call
navigation_params.commit_params.url.

[clamy, 2015-03-12 10:16:52]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2015-03-12 10:17:00]

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

[commit-bot: I haz the power, 2015-03-12 11:26:22]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-03-12 11:27:16]

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

[nasko, 2015-03-13 20:59:21]

On 2015/03/12 11:27:16, I haz the power (commit-bot) wrote:
> Patchset 8 (id:??) landed as
> https://crrev.com/34e1278e145473eeecf118e63f3530dfd3620f57
> Cr-Commit-Position: refs/heads/master@{#320263}

Looking at codesearch, there are 4 references to FrameMsg_Navigate_Params, we
should remove them. They are all forward declarations.

[Charlie Reis, 2015-03-16 19:09:43]

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

[Charlie Reis, 2015-03-16 19:09:43]

Hmm, please include me on CLs that change NavigationEntry in the future.  This
is looking very difficult to merge with the session history changes I'm making
for --site-per-process, since it doesn't work well with navigations to
FrameNavigationEntries.  Also, NavigationEntry hasn't needed to depend on
NavigationController in the past, which feels like a mistake to me.

I'll try to review this later today to see how we can reconcile it with where
we're headed.

[Charlie Reis, 2015-03-17 04:01:04]

Some ex post facto comments below.  Would you mind addressing them in a
follow-up CL?  Thanks!

In general, I continue to like the idea of tracking these parameters in a more
central way.  I still feel like it's hard to understand the breakdown-- I don't
know why there's 4 structs, and I don't know which params to expect in each
struct.  It's important to give people a mental model in the comments of
navigation_params.h, or else the APIs that take all 4 structs will look
arbitrarily verbose.

https://codereview.chromium.org/971653004/diff/130001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (right):

https://codereview.chromium.org/971653004/diff/130001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:372: GetURL(),
GetReferrer(), GetTransitionType(), navigation_type,
This is where things get awkward for --site-per-process session history.  Things
like the URL and Referrer live on FrameNavigationEntry, not NavigationEntry.  I
can pass in a FrameNavigationEntry, but that does make me wonder whether this is
the right class for these Construct methods.

I suppose we can leave them for now and see if there's a more natural place for
them as more of the session history changes land.

https://codereview.chromium.org/971653004/diff/130001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/971653004/diff/130001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:237: CommonNavigationParams
ConstructCommonNavigationParams(
These belong much higher up in the class, after the public interface (before
ResetForCommit).

https://codereview.chromium.org/971653004/diff/130001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:242:
NavigationControllerImpl* controller) const;
NavigationEntryImpl should not depend on NavigationControllerImpl.  Can you pass
in the offsets and length instead?

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

https://codereview.chromium.org/971653004/diff/130001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:27: // A UI thread object that
owns a navigation request until it commits. It
Something tangential to put on your radar: I'm hoping to remove the fields of
NavigationEntryImpl that are cleared in ResetForCommit and move them elsewhere,
such as this class or the params objects held by it.  Does that seem doable? 
(We shouldn't be relying on NavigationEntry to keep existing between start and
commit.)

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

https://codereview.chromium.org/971653004/diff/130001/content/common/navigati...
content/common/navigation_params.h:138: // PlzNavigate: sent to the renderer
when the navigation is ready to commit.
Why are there different structs for Commit params and History params?  They both
have the same comment description.  Can you update the comments for them to
explain why they are separate?

https://codereview.chromium.org/971653004/diff/130001/content/common/navigati...
content/common/navigation_params.h:175: struct CONTENT_EXPORT
StartNavigationParams {
Why is Start listed last?  This should be declared in the same order as it's
used in NavigationParams (i.e., after Common).

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

https://codereview.chromium.org/971653004/diff/130001/content/renderer/render...
content/renderer/render_frame_impl.cc:3924: // TODO(clamy): reset the pending
navigation parameters in RenderView.
Technically those parameters should be moved to RenderFrame, but I think you're
saying that pending_navigation_params_ needs to be reset, whichever class it
lives in.

[clamy, 2015-03-17 17:02:53]

I will work on another CL to address your comments. I think some of the structs
could be merged/renamed to provide a better understanding of what they are
about. They essentially breakdown into 4:
- parameters used everywhere (CommonNavigationParams)
- parameters sent to the browser by the renderer during a renderer-initiated
navigation in PlzNavigate (BeginNavigationParams)
- parameters the browser should send to the renderer to start(current
code)/commit(PlzNavigate) a navigation, used currently and in PlzNavigate
(CommitNavigationParams and HistoryNavigationParams which could be merged)
- parameters the browser shoudl send to the renderer to start a navigation in
the current code (StartNavigationParams)

The names could probably be improved to better convey the intention behind each
of these structs.

[Charlie Reis, 2015-03-17 17:08:13]

On 2015/03/17 17:02:53, clamy wrote:
> I will work on another CL to address your comments. I think some of the
structs
> could be merged/renamed to provide a better understanding of what they are
> about. They essentially breakdown into 4:
> - parameters used everywhere (CommonNavigationParams)
> - parameters sent to the browser by the renderer during a renderer-initiated
> navigation in PlzNavigate (BeginNavigationParams)
> - parameters the browser should send to the renderer to start(current
> code)/commit(PlzNavigate) a navigation, used currently and in PlzNavigate
> (CommitNavigationParams and HistoryNavigationParams which could be merged)
> - parameters the browser shoudl send to the renderer to start a navigation in
> the current code (StartNavigationParams)
> 
> The names could probably be improved to better convey the intention behind
each
> of these structs.

Thanks, this helps!  (For example, I missed that Start vs Begin are for current
vs PlzNavigate.)  Let's try to convey these ideas more clearly in the .h file.