Move provisional navigation parameters to RenderFrame, and use the HistoryController to distribute …

Move pending navigation parameters to RenderFrame, and use the HistoryController to distribute them to affected frames.

BUG=369661
TEST=existing tests

Committed: https://crrev.com/0392dbf51928648a36cd40128b4ac8c9fd574b87
Cr-Commit-Position: refs/heads/master@{#322176}

[Avi (use Gerrit), 2015-03-20 23:13:26]

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

[Avi (use Gerrit), 2015-03-20 23:13:27]

Charlie:

This is some changes in the HistoryController, but mostly code movement from
RenderView to RenderFrame. Plus a TODO for you.

[Charlie Reis, 2015-03-23 22:23:02]

Very nice.  Bug fix (pending_nav_params_ for multiple subframes), moving code
from RV to RF, and dead code removal-- good on several fronts.

A few thoughts below.  Can you also mention the multiple subframe case in the CL
description to give a bit more context for the HistoryController change?

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/histor...
File content/renderer/history_controller.h (right):

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/histor...
content/renderer/history_controller.h:52: struct NavigationParams;
Style nit: Classes before structs.

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/histor...
content/renderer/history_controller.h:147: // |GoToEntry|.
Let's mention that this is passed to each RenderFrame that does a load from the
provisional_entry_, and why that's needed.

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

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2510: bool is_new_navigation = commit_type
== blink::WebStandardCommit;
Why remove this if we need it below?

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

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2633: } else if (commit_type ==
blink::WebBackForwardCommit) {
What would you think about doing this part of the CL separately from the rest?

If I understand correctly, the rest is a bug fix and some cleanup necessary so
that you can make this change to the classification logic.  It seems like it
might be worthwhile to land them separately so we can tell which part is to
blame if any regressions crop up.

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

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.h:790: scoped_ptr<NavigationParams>
provisional_navigation_params_;
I've always considered "provisional" as the Blink term for content's "pending." 
I'd like to keep it as pending_navigation_params_ as before, unless there's a
reason to switch it.

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/render...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:2178: RenderViewObserver, observers_,
DidCreateDataSource(frame, ds));
Nice!  Happy to see dead code going away, since no one listens to this.

[Avi (use Gerrit), 2015-03-24 20:33:46]

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/histor...
File content/renderer/history_controller.h (right):

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/histor...
content/renderer/history_controller.h:52: struct NavigationParams;
On 2015/03/23 22:23:01, Charlie Reis wrote:
> Style nit: Classes before structs.

I've seen it both ways, and written it both ways, but I don't really care.

Done.

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/histor...
content/renderer/history_controller.h:147: // |GoToEntry|.
On 2015/03/23 22:23:02, Charlie Reis wrote:
> Let's mention that this is passed to each RenderFrame that does a load from
the
> provisional_entry_, and why that's needed.

That' actually not the case; by the time GetItemForNewChildFrame is called,
there is no provisional_entry_ any more, and info is pulled from current_entry_.
Rephrasing in any case.

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

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2510: bool is_new_navigation = commit_type
== blink::WebStandardCommit;
On 2015/03/23 22:23:02, Charlie Reis wrote:
> Why remove this if we need it below?

To keep the if() branches parallel:

if (commit_type = x)
else if (commit_type = y)

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

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2633: } else if (commit_type ==
blink::WebBackForwardCommit) {
On 2015/03/23 22:23:02, Charlie Reis wrote:
> What would you think about doing this part of the CL separately from the rest?

Hmm... In the original change, the !navigation_state->request_committed() is a
no-op, and it stays so for this change. Let me remove that one line, but keep
the rest the same.

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

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.h:790: scoped_ptr<NavigationParams>
provisional_navigation_params_;
On 2015/03/23 22:23:02, Charlie Reis wrote:
> I've always considered "provisional" as the Blink term for content's
"pending." 
> I'd like to keep it as pending_navigation_params_ as before, unless there's a
> reason to switch it.

That's OK with me.

I called it "pending" because it used to be called "pending" in the
HistoryController to match the pending variable there.

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/render...
File content/renderer/render_view_impl.cc (left):

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:2178: RenderViewObserver, observers_,
DidCreateDataSource(frame, ds));
On 2015/03/23 22:23:02, Charlie Reis wrote:
> Nice!  Happy to see dead code going away, since no one listens to this.

Acknowledged.

[Charlie Reis, 2015-03-24 22:04:08]

LGTM with one nit.  Also, it looks like you might have a minor conflict with
@clamy's recent CL.

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/histor...
File content/renderer/history_controller.h (right):

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/histor...
content/renderer/history_controller.h:147: // |GoToEntry|.
On 2015/03/24 20:33:46, Avi wrote:
> On 2015/03/23 22:23:02, Charlie Reis wrote:
> > Let's mention that this is passed to each RenderFrame that does a load from
> the
> > provisional_entry_, and why that's needed.
> 
> That' actually not the case; by the time GetItemForNewChildFrame is called,
> there is no provisional_entry_ any more, and info is pulled from
current_entry_.
> Rephrasing in any case. 

Acknowledged.

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

https://codereview.chromium.org/1027863002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2633: } else if (commit_type ==
blink::WebBackForwardCommit) {
On 2015/03/24 20:33:46, Avi wrote:
> On 2015/03/23 22:23:02, Charlie Reis wrote:
> > What would you think about doing this part of the CL separately from the
rest?
> 
> Hmm... In the original change, the !navigation_state->request_committed() is a
> no-op, and it stays so for this change. Let me remove that one line, but keep
> the rest the same.

Acknowledged.

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

https://codereview.chromium.org/1027863002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.h:549: void SetProvisionalNavigationParams(
SetPendingNavigationParams, now that we've changed the member name.

[Avi (use Gerrit), 2015-03-25 15:57:12]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2015-03-25 15:57:12]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1027863002/#ps80001
(title: "fixes")

[Avi (use Gerrit), 2015-03-25 15:57:24]

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

https://codereview.chromium.org/1027863002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.h:549: void SetProvisionalNavigationParams(
On 2015/03/24 22:04:08, Charlie Reis wrote:
> SetPendingNavigationParams, now that we've changed the member name.

Done.

[commit-bot: I haz the power, 2015-03-25 15:57:39]

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

[commit-bot: I haz the power, 2015-03-25 16:55:53]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-25 16:56:31]

Patchset 5 (id:??) landed as
https://crrev.com/0392dbf51928648a36cd40128b4ac8c9fd574b87
Cr-Commit-Position: refs/heads/master@{#322176}