PlzNavigate: unify OnCommitNavigation with OnNavigate in RenderFrameImpl

PlzNavigate: unify OnCommitNavigation with OnNavigate in RenderFrameImpl

This CL creates a NavigateInternal method in RenderFrameImpl that is used both
by OnCommitNavigation and OnNavigate, which creates a unified codepath for
navigations in RenderFrameImpl.

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

[clamy, 2015-04-28 10:24:45]

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

[clamy, 2015-04-28 10:26:41]

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

I was working on making history navigation work with PlzNavigate, and realized
that the code in OnCommitNavigation was starting to get extremely close to the
code in OnNavigate, therefore I'm trying to unify both in an internal function
before proceeding with the history navigation support.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4376: if (is_reload &&
!browser_side_navigation) {
Currently reloads are not properly supported by PlzNavigate, hence why they are
treated as normal navigations (and the TODO).

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4389: } else if (is_history_navigation &&
!browser_side_navigation) {
Same for history navigations.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4415: if
(!start_params.extra_headers.empty() && !browser_side_navigation) {
This could maybe be extracted to a helper function, wdyt?

[Avi (use Gerrit), 2015-04-28 15:39:08]

I'm happy with this, as it's basically a move of code. I have a comment about a
conflict with a reverted CL.

LGTM

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1105: }
Note that this block is in conflict with your patch; it was reverted in
https://codereview.chromium.org/1108863002. You will have to play very nicely
with Nate when you're moving a block of code that he hasn't solidly committed.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4355:
ui::PAGE_TRANSITION_MANUAL_SUBFRAME)) {
See above about this block.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4415: if
(!start_params.extra_headers.empty() && !browser_side_navigation) {
On 2015/04/28 10:26:41, clamy wrote:
> This could maybe be extracted to a helper function, wdyt?

Merged with what other code?

[nasko, 2015-04-28 17:23:43]

I'm super excited to see both codepaths starting to merge. Just a couple of
nits.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4378: // override should be given to the
generated request,
nit: period/full stop instead of comma after "request".

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4391: // override should be given to the
generated request,
nit: Another ',' -> '.'

[Charlie Reis, 2015-04-28 23:01:03]

Nice!  LGTM with nits, and be aware of the merge conflict Avi mentioned with
https://codereview.chromium.org/1108863002.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4376: if (is_reload &&
!browser_side_navigation) {
On 2015/04/28 10:26:40, clamy wrote:
> Currently reloads are not properly supported by PlzNavigate, hence why they
are
> treated as normal navigations (and the TODO).

Acknowledged.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4389: } else if (is_history_navigation &&
!browser_side_navigation) {
On 2015/04/28 10:26:40, clamy wrote:
> Same for history navigations.

Acknowledged.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4415: if
(!start_params.extra_headers.empty() && !browser_side_navigation) {
On 2015/04/28 15:39:07, Avi wrote:
> On 2015/04/28 10:26:41, clamy wrote:
> > This could maybe be extracted to a helper function, wdyt?
> 
> Merged with what other code?

Just a AddExtraHeaders(start_params.extra_headers, request)?  I suppose it would
cut this long function down by a few lines, but I'm not sure it's needed.  Let's
not do it while moving the function, at any rate.  Could be nice for a followup
CL.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4431: const char* data = NULL;
nullptr (while we're here and tweaking small things anyway)

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4444: // PlzNavigate: Make sure that blink
loader will not try to use browser side
s/blink/Blink's/

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4449: // Record this before starting the
load, we need a lower bound of this time
nit: Period rather than comma (or "since we").

[clamy, 2015-04-29 09:36:57]

Thanks!

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:1105: }
On 2015/04/28 15:39:07, Avi wrote:
> Note that this block is in conflict with your patch; it was reverted in
> https://codereview.chromium.org/1108863002. You will have to play very nicely
> with Nate when you're moving a block of code that he hasn't solidly committed.

Acknowledged.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4355:
ui::PAGE_TRANSITION_MANUAL_SUBFRAME)) {
On 2015/04/28 15:39:08, Avi wrote:
> See above about this block.

Done. It was removed following a rebase on
https://codereview.chromium.org/1112513002/.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4378: // override should be given to the
generated request,
On 2015/04/28 17:23:43, nasko wrote:
> nit: period/full stop instead of comma after "request".

Done.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4391: // override should be given to the
generated request,
On 2015/04/28 17:23:43, nasko wrote:
> nit: Another ',' -> '.'

Done.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4415: if
(!start_params.extra_headers.empty() && !browser_side_navigation) {
On 2015/04/28 23:01:03, Charlie Reis wrote:
> On 2015/04/28 15:39:07, Avi wrote:
> > On 2015/04/28 10:26:41, clamy wrote:
> > > This could maybe be extracted to a helper function, wdyt?
> > 
> > Merged with what other code?
> 
> Just a AddExtraHeaders(start_params.extra_headers, request)?  I suppose it
would
> cut this long function down by a few lines, but I'm not sure it's needed. 
Let's
> not do it while moving the function, at any rate.  Could be nice for a
followup
> CL.

Acknowledged.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4431: const char* data = NULL;
On 2015/04/28 23:01:03, Charlie Reis wrote:
> nullptr (while we're here and tweaking small things anyway)

Done.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4444: // PlzNavigate: Make sure that blink
loader will not try to use browser side
On 2015/04/28 23:01:03, Charlie Reis wrote:
> s/blink/Blink's/

Done.

https://codereview.chromium.org/1112513002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:4449: // Record this before starting the
load, we need a lower bound of this time
On 2015/04/28 23:01:03, Charlie Reis wrote:
> nit: Period rather than comma (or "since we").

Done.

[nasko, 2015-04-29 14:09:55]

LGTM

[clamy, 2015-04-29 14:10:44]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-04-29 14:10:44]

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/1112513002/#ps2 (title:
"Addressed comments")

[commit-bot: I haz the power, 2015-04-29 14:10:56]

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

[commit-bot: I haz the power, 2015-04-29 14:48:25]

Committed patchset #3 (id:2)

[commit-bot: I haz the power, 2015-04-29 14:49:29]

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