Make NavigationParams clearer

Make NavigationParams clearer

This CL follows https://codereview.chromium.org/971653004/ and clarifies the
use of the various NavigationParams. In particular it merges the
CommitNavigationParams and the HistoryNavigationParams since they are generated
and sent always by pair.

BUG=459033
Committed: https://crrev.com/57b4b8e10887992956e9f2f5906c1e44e5b3a628
Cr-Commit-Position: refs/heads/master@{#321991}

[clamy, 2015-03-19 17:12:30]

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

[clamy, 2015-03-19 17:12:31]

@creis, avi, nasko: PTAL

This is a followup on https://codereview.chromium.org/971653004/ following
Charlie's comments in this issue. The goal is to make the use of the various
*NavigationParams structs clearer.

https://codereview.chromium.org/1018383002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_entry_impl.cc (left):

https://codereview.chromium.org/1018383002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_entry_impl.cc:390: void
NavigationEntryImpl::ResetForCommit() {
The git diff is a bit weird here. What I did is move the Construct* methods
above ResetForCommit.

https://codereview.chromium.org/1018383002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.cc (left):

https://codereview.chromium.org/1018383002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:390: void
NavigationEntryImpl::ResetForCommit() {
The git diff is a bit weird in this file. What I'm doing is moving the
Construct* methods above ResetForCommit.

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

https://codereview.chromium.org/1018383002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:389: CommonNavigationParams
NavigationEntryImpl::ConstructCommonNavigationParams(
I've left the Construct methods here until we have a better understanding of
where to put them to interact properly with the FrameNavigationEntry.

[Avi (use Gerrit), 2015-03-19 18:21:40]

LGTM; cleaner.

Thanks for the heads-up.

[Charlie Reis, 2015-03-19 20:45:52]

Thanks!  This is much better overall, and combining Commit and History sounds
fine to me.

The last big issue from my perspective is using the word Commit for something
that starts a navigation.  Some thoughts and a few nits below.

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

https://codereview.chromium.org/1018383002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:389: CommonNavigationParams
NavigationEntryImpl::ConstructCommonNavigationParams(
On 2015/03/19 17:12:31, clamy wrote:
> I've left the Construct methods here until we have a better understanding of
> where to put them to interact properly with the FrameNavigationEntry.

Acknowledged.

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

https://codereview.chromium.org/1018383002/diff/20001/content/common/frame_me...
content/common/frame_messages.h:243:
IPC_STRUCT_TRAITS_BEGIN(content::BeginNavigationParams)
If we re-order navigation_params.h as I mention, we should move Begin above
Start here.  That would put these more in chronological order.

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

https://codereview.chromium.org/1018383002/diff/20001/content/common/navigati...
content/common/navigation_params.h:82: // These structs are sent by the browser
to the renderer to start (commit with
I don't know what "start (commit with PlzNavigate enabled)" means.  Can you
rephrase it?

https://codereview.chromium.org/1018383002/diff/20001/content/common/navigati...
content/common/navigation_params.h:88: // Used by FrameMsg_Navigate, holds the
parameters needed by the renderer to
nit: New sentence for "Holds..."

https://codereview.chromium.org/1018383002/diff/20001/content/common/navigati...
content/common/navigation_params.h:128: // Used by FrameMsg_Navigate, holds the
parameters needed by the renderer to
nit: New sentence for "Holds..."

https://codereview.chromium.org/1018383002/diff/20001/content/common/navigati...
content/common/navigation_params.h:129: // start a browser-initiated navigation
besides those in CommonNavigationParams.
This doesn't sound right.  Why are "commit" parameters needed to "start" a
navigation?

I've had trouble in the past with using the word "commit" in PlzNavigate for a
stage before the actual commit happens.  This is a much bigger problem now that
we're using these parameters in the current architecture for the start of a
navigation.  That happens well before anything is ready to commit, and in some
cases the commit won't ever happen (e.g., downloads, 204s).

I would really like to find a different name for this struct if we're going to
use it for starting a navigation.  Maybe something like RequestNavigationParams?
 (I'm open to suggestions, as long as they indicate that this is to make the
renderer process start a navigation attempt and not commit one.)

(Even in the PlzNavigate case, this struct is about getting the new renderer
process to issue a fake request for the stream that is ready to commit, right? 
The commit itself happens after that, asynchronously.  And if I understand
correctly, we hope to eliminate that fake request eventually, which might change
what's in here.)

https://codereview.chromium.org/1018383002/diff/20001/content/common/navigati...
content/common/navigation_params.h:194: // need to provide to the renderer.
nit: needs

https://codereview.chromium.org/1018383002/diff/20001/content/common/navigati...
content/common/navigation_params.h:206: // Provided by the renderer
----------------------------------------------------
What would you think about putting this before the "Provided by the browser"
section?  This happens earlier in the timeline, so I think it makes sense to
come before those.

[clamy, 2015-03-20 10:42:43]

Thanks!

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

https://codereview.chromium.org/1018383002/diff/20001/content/common/frame_me...
content/common/frame_messages.h:243:
IPC_STRUCT_TRAITS_BEGIN(content::BeginNavigationParams)
On 2015/03/19 20:45:51, Charlie Reis wrote:
> If we re-order navigation_params.h as I mention, we should move Begin above
> Start here.  That would put these more in chronological order.

Done.

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

https://codereview.chromium.org/1018383002/diff/20001/content/common/navigati...
content/common/navigation_params.h:82: // These structs are sent by the browser
to the renderer to start (commit with
On 2015/03/19 20:45:52, Charlie Reis wrote:
> I don't know what "start (commit with PlzNavigate enabled)" means.  Can you
> rephrase it?

Is it clearer now?

https://codereview.chromium.org/1018383002/diff/20001/content/common/navigati...
content/common/navigation_params.h:88: // Used by FrameMsg_Navigate, holds the
parameters needed by the renderer to
On 2015/03/19 20:45:51, Charlie Reis wrote:
> nit: New sentence for "Holds..."

Done.

https://codereview.chromium.org/1018383002/diff/20001/content/common/navigati...
content/common/navigation_params.h:128: // Used by FrameMsg_Navigate, holds the
parameters needed by the renderer to
On 2015/03/19 20:45:51, Charlie Reis wrote:
> nit: New sentence for "Holds..."

Done.

https://codereview.chromium.org/1018383002/diff/20001/content/common/navigati...
content/common/navigation_params.h:129: // start a browser-initiated navigation
besides those in CommonNavigationParams.
On 2015/03/19 20:45:51, Charlie Reis wrote:
> This doesn't sound right.  Why are "commit" parameters needed to "start" a
> navigation?
> 
> I've had trouble in the past with using the word "commit" in PlzNavigate for a
> stage before the actual commit happens.  This is a much bigger problem now
that
> we're using these parameters in the current architecture for the start of a
> navigation.  That happens well before anything is ready to commit, and in some
> cases the commit won't ever happen (e.g., downloads, 204s).
> 
> I would really like to find a different name for this struct if we're going to
> use it for starting a navigation.  Maybe something like
RequestNavigationParams?
>  (I'm open to suggestions, as long as they indicate that this is to make the
> renderer process start a navigation attempt and not commit one.)
> 
> (Even in the PlzNavigate case, this struct is about getting the new renderer
> process to issue a fake request for the stream that is ready to commit, right?

> The commit itself happens after that, asynchronously.  And if I understand
> correctly, we hope to eliminate that fake request eventually, which might
change
> what's in here.)

Done. They were initially called CommitNavigationParams, since there used to be
some RequestNavigationParams when we still had a RequestNavigation IPC. Now that
it is gone, they can be called RequestNavigationParams.

https://codereview.chromium.org/1018383002/diff/20001/content/common/navigati...
content/common/navigation_params.h:194: // need to provide to the renderer.
On 2015/03/19 20:45:51, Charlie Reis wrote:
> nit: needs

Done.

https://codereview.chromium.org/1018383002/diff/20001/content/common/navigati...
content/common/navigation_params.h:206: // Provided by the renderer
----------------------------------------------------
On 2015/03/19 20:45:51, Charlie Reis wrote:
> What would you think about putting this before the "Provided by the browser"
> section?  This happens earlier in the timeline, so I think it makes sense to
> come before those.

Done.

[Charlie Reis, 2015-03-20 21:07:34]

Thanks!  LGTM with nits.

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

https://codereview.chromium.org/1018383002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:434: int
pending_offset_to_send = pending_history_list_offset;
nit: Is there a reason to make copies of these?

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

https://codereview.chromium.org/1018383002/diff/40001/content/common/navigati...
content/common/navigation_params.h:161: // PlzNavigate: sent to the renderer to
commit a navigation.
Please change this last sentence to:
PlzNavigate: sent to the renderer to make it issue a stream request for a
navigation that is ready to commit.

[nasko, 2015-03-23 16:12:03]

IPC LGTM

[clamy, 2015-03-24 12:42:26]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-03-24 12:42:27]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
creis@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1018383002/#ps60001
(title: "Rebase + addressed comment")

[commit-bot: I haz the power, 2015-03-24 12:42:44]

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

[commit-bot: I haz the power, 2015-03-24 13:32:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-24 13:33:19]

Patchset 4 (id:??) landed as
https://crrev.com/57b4b8e10887992956e9f2f5906c1e44e5b3a628
Cr-Commit-Position: refs/heads/master@{#321991}

[clamy, 2015-03-24 13:33:38]

Thanks! (Forgot to send the mail before clicking on submit).

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

https://codereview.chromium.org/1018383002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.cc:434: int
pending_offset_to_send = pending_history_list_offset;
On 2015/03/20 21:07:34, Charlie Reis wrote:
> nit: Is there a reason to make copies of these?

I thought it would be weird and not very readable to modify the parameters of
the function directly. Also I did not want to call the constructor with ternary
operators as this would mean removing the comment, which is pretty useful.

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

https://codereview.chromium.org/1018383002/diff/40001/content/common/navigati...
content/common/navigation_params.h:161: // PlzNavigate: sent to the renderer to
commit a navigation.
On 2015/03/20 21:07:34, Charlie Reis wrote:
> Please change this last sentence to:
> PlzNavigate: sent to the renderer to make it issue a stream request for a
> navigation that is ready to commit.

Done.