PlzNavigate: send history params at commit time to the renderer

PlzNavigate: send history params at commit time to the renderer

This CL introduces a new struct HistoryNavigationParams used in
FrameMsg_Navigate and FrameMsg_CommitNavigation. This allows to check for stale
backforward navigations when browser-side navigation is enabled.

BUG=376091
TBR=nasko@chromium.org

Committed: https://crrev.com/cbe86a0c3963fa295521e10f94367e5a45378fd5
Cr-Commit-Position: refs/heads/master@{#319276}

[clamy, 2015-03-03 17:50:10]

Patchset #1 (id:1) has been deleted

[clamy, 2015-03-03 17:51:37]

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

[clamy, 2015-03-03 18:02:08]

@ nasko, avi: PTAL
@ carlosk, fdegans: FYI

This CL properly transmits the history parameters to the renderer, which allows
us to check for stale back forward navigations. This is also a first step in a
refactoring to allow properly calling the logic in DidCommitProvisionalLoad when
we want to show error pages in PlzNavigate.

https://codereview.chromium.org/979443002/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/979443002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1043: render_view_->history_list_offset_ =
params.current_history_list_offset;
This was moved to PrepareRenderViewForNavigation, so that it is executed by
PlzNavigate as well.

[Avi (use Gerrit), 2015-03-04 21:23:13]

I think I have the mental model of the flow in my head, and this seems right.
Even with PlzNavigate we still run the risk of a stale navigation with
renderer-initiated navigation? :(

Charlie, wdyt? (lgtm from me)

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

https://codereview.chromium.org/979443002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:4291: bool is_history_navigation,
This parameter is no longer needed, as you're passing in |true| for the
PlzNavigate code path.

https://codereview.chromium.org/979443002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:4305: // TODO(clamy): remove
check_for_stale_navigation
Do this TODO (and remove it).

[Charlie Reis, 2015-03-05 05:01:03]

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

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

Yes, it looks like this should work, apart from one bug below.  I'm a bit
concerned that we don't have a test that caught the bug-- would it be possible
to add one?

Otherwise LGTM with nits.

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

https://codereview.chromium.org/979443002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2408:
render_view_->pending_navigation_params_->history_params.page_id =
nit: This is getting a bit unwieldy.  Can you save
render_view_->pending_navigation_params_ in a local to make it easier to read?

https://codereview.chromium.org/979443002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:4291: bool is_history_navigation,
On 2015/03/04 21:23:12, Avi wrote:
> This parameter is no longer needed, as you're passing in |true| for the
> PlzNavigate code path.

You meant check_for_stale_navigation, right?

https://codereview.chromium.org/979443002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:4337: render_view_->history_list_offset_ =
Putting it here isn't equivalent to where it was before, since we might return
early before getting here.  In fact, we usually return early before getting
here.  (This last block is only about swapped out main frames.)

[Avi (use Gerrit), 2015-03-05 05:05:55]

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

https://codereview.chromium.org/979443002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:4291: bool is_history_navigation,
On 2015/03/05 05:01:04, Charlie Reis wrote:
> On 2015/03/04 21:23:12, Avi wrote:
> > This parameter is no longer needed, as you're passing in |true| for the
> > PlzNavigate code path.
> 
> You meant check_for_stale_navigation, right?

D'oh. Yes, I meant to tag the previous line.

[clamy, 2015-03-05 13:15:00]

Thanks! I added a RenderViewTest that fails if the history list update of the
RenderView is not done properly. Since both of you are fine with the patch, I'm
TBRing nasko@ for the review of frame_messages.h.

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

https://codereview.chromium.org/979443002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2408:
render_view_->pending_navigation_params_->history_params.page_id =
On 2015/03/05 05:01:04, Charlie Reis wrote:
> nit: This is getting a bit unwieldy.  Can you save
> render_view_->pending_navigation_params_ in a local to make it easier to read?

Done.

https://codereview.chromium.org/979443002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:4291: bool is_history_navigation,
On 2015/03/05 05:05:55, Avi wrote:
> On 2015/03/05 05:01:04, Charlie Reis wrote:
> > On 2015/03/04 21:23:12, Avi wrote:
> > > This parameter is no longer needed, as you're passing in |true| for the
> > > PlzNavigate code path.
> > 
> > You meant check_for_stale_navigation, right?
> 
> D'oh. Yes, I meant to tag the previous line.

Done.

https://codereview.chromium.org/979443002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:4305: // TODO(clamy): remove
check_for_stale_navigation
On 2015/03/04 21:23:12, Avi wrote:
> Do this TODO (and remove it).

Done.

https://codereview.chromium.org/979443002/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:4337: render_view_->history_list_offset_ =
On 2015/03/05 05:01:04, Charlie Reis wrote:
> Putting it here isn't equivalent to where it was before, since we might return
> early before getting here.  In fact, we usually return early before getting
> here.  (This last block is only about swapped out main frames.)

I moved it below the last return to false and added a test that would fail if an
equivalent bug was reintroduced.

[clamy, 2015-03-05 13:15:38]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-03-05 13:15:39]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/979443002/#ps60001 (title:
"Addressed review comments")

[commit-bot: I haz the power, 2015-03-05 13:16:36]

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

[nasko, 2015-03-05 14:33:44]

IPC LGTM

[commit-bot: I haz the power, 2015-03-05 16:46:51]

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

[commit-bot: I haz the power, 2015-03-05 16:46:53]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[clamy, 2015-03-05 17:04:44]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2015-03-05 17:05:53]

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

[commit-bot: I haz the power, 2015-03-05 17:40:10]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-03-05 17:41:30]

Patchset 3 (id:??) landed as
https://crrev.com/cbe86a0c3963fa295521e10f94367e5a45378fd5
Cr-Commit-Position: refs/heads/master@{#319276}