Refactor NavigationState to use NavigationParams

Refactor NavigationState to use NavigationParams

This CL separates NavigationState into a public interface with a limited number
of functions exposed outside of content/, and an implementation. The
implementation now owns a set of NavigationParams. For browser-initiated
navigations they are passed by the RenderView, and for renderer-intiated
navigations they have default values. This also allows to reset the
pending_navigation_params of RenderView with their values when the provisional
load fails for browser-initiated navigations.

BUG=459033
Committed: https://crrev.com/5f34220fb49903614381e6efd516149174989b84
Cr-Commit-Position: refs/heads/master@{#321120}

[clamy, 2015-03-16 16:31:37]

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

[clamy, 2015-03-16 16:31:37]

@nasko: PTAL
@carlosk, fdegans: FYI
This should be the last refactoring state before the patch for error pages in
PlzNavigate. I think this one removes some of the bulk introduced by the last
refactoring. I removed many of the parameters exposed by NavigationState public
interface because they were not used outside of content. I also think that
avoiding to expose the internals of navigation outside of content is better.

[clamy, 2015-03-16 16:55:46]

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

[clamy, 2015-03-16 16:55:46]

@avi, creis: PTAL
This does interact with https://codereview.chromium.org/1002803002/, so I'd like
your opinion on the change. It should make keeping parameters passed by the
browser until commit time easier.

[Avi (use Gerrit), 2015-03-16 18:10:24]

I really like this cleanup. LGTM

https://codereview.chromium.org/1001043002/diff/20001/content/public/renderer...
File content/public/renderer/navigation_state.h (right):

https://codereview.chromium.org/1001043002/diff/20001/content/public/renderer...
content/public/renderer/navigation_state.h:27: virtual bool
GetWasWithinSamePage() const = 0;
I would prefer "WasWithinSamePage"; a "Get" prefix sounds weird for
boolean-returning functions.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
File content/renderer/navigation_state_impl.cc (right):

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
no (c)

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
File content/renderer/navigation_state_impl.h (right):

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
No (c).

[Charlie Reis, 2015-03-17 03:26:31]

I like this as well.  Please don't commit just yet, though-- it looks like there
might be a bug in the failed navigation case.

Mostly nits apart from that.

https://codereview.chromium.org/1001043002/diff/20001/content/public/renderer...
File content/public/renderer/navigation_state.h (right):

https://codereview.chromium.org/1001043002/diff/20001/content/public/renderer...
content/public/renderer/navigation_state.h:24: virtual ui::PageTransition
GetTransitionType() const = 0;
From the Content API guidelines:
http://www.chromium.org/developers/content-module/content-api

"don't add the const identifier to interfaces. For interfaces implemented by the
embedder, we can't make assumptions about what the embedder needs to implement
it. For interfaces implemented by content, the implementation details doesn't
have to be exposed."

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
File content/renderer/navigation_state_impl.h (right):

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:26: CommonNavigationParams&
common_params() { return common_params_; }
This needs to be const.  (No non-const refs allowed.)

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:36: ui::PageTransition
GetTransitionType() const override;
This needs a setter.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:48: // True if this navigation was not
initiated via WebFrame::LoadRequest.
nit: Blank line above.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:53: // Note: if is_content_initiated()
is false, whether this navigation should
nit: Blank line above.
nit: IsContentInitiated()

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:65:
DISALLOW_COPY_AND_ASSIGN(NavigationStateImpl);
nit: Blank line above.

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

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2449: error.unreachableURL, Referrer(),
I don't understand how the new code is equivalent in this case.  Where are you
handling the error.unreachableURL (etc)?

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

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2269: if
(ui::PageTransitionCoreTypeIs(navigation_state->common_params().transition,
Why not navigation_state->GetTransitionType()?

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2271:
navigation_state->common_params().transition =
Should use a setter for this.  Same below.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4035:
static_cast<NavigationStateImpl*>(document_state->navigation_state())
Why do you need this static cast?  IsContentInitiated is on the public
interface.

[clamy, 2015-03-17 16:08:18]

Thanks!

https://codereview.chromium.org/1001043002/diff/20001/content/public/renderer...
File content/public/renderer/navigation_state.h (right):

https://codereview.chromium.org/1001043002/diff/20001/content/public/renderer...
content/public/renderer/navigation_state.h:24: virtual ui::PageTransition
GetTransitionType() const = 0;
On 2015/03/17 03:26:30, Charlie Reis wrote:
> From the Content API guidelines:
> http://www.chromium.org/developers/content-module/content-api
> 
> "don't add the const identifier to interfaces. For interfaces implemented by
the
> embedder, we can't make assumptions about what the embedder needs to implement
> it. For interfaces implemented by content, the implementation details doesn't
> have to be exposed."

Done.

https://codereview.chromium.org/1001043002/diff/20001/content/public/renderer...
content/public/renderer/navigation_state.h:27: virtual bool
GetWasWithinSamePage() const = 0;
On 2015/03/16 18:10:24, Avi wrote:
> I would prefer "WasWithinSamePage"; a "Get" prefix sounds weird for
> boolean-returning functions.

Done.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
File content/renderer/navigation_state_impl.cc (right):

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2015/03/16 18:10:24, Avi wrote:
> no (c)

Done.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
File content/renderer/navigation_state_impl.h (right):

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/03/16 18:10:24, Avi wrote:
> No (c).

Done.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:26: CommonNavigationParams&
common_params() { return common_params_; }
On 2015/03/17 03:26:30, Charlie Reis wrote:
> This needs to be const.  (No non-const refs allowed.)

Done.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:36: ui::PageTransition
GetTransitionType() const override;
On 2015/03/17 03:26:30, Charlie Reis wrote:
> This needs a setter.

Done.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:48: // True if this navigation was not
initiated via WebFrame::LoadRequest.
On 2015/03/17 03:26:30, Charlie Reis wrote:
> nit: Blank line above.

Done.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:53: // Note: if is_content_initiated()
is false, whether this navigation should
On 2015/03/17 03:26:30, Charlie Reis wrote:
> nit: Blank line above.
> nit: IsContentInitiated()

Done.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:65:
DISALLOW_COPY_AND_ASSIGN(NavigationStateImpl);
On 2015/03/17 03:26:30, Charlie Reis wrote:
> nit: Blank line above.

Done.

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

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2449: error.unreachableURL, Referrer(),
On 2015/03/17 03:26:31, Charlie Reis wrote:
> I don't understand how the new code is equivalent in this case.  Where are you
> handling the error.unreachableURL (etc)?

Well until
https://codereview.chromium.org/971653004/diff/130001/content/renderer/render...
we were resetting the params with a default FrameMsg_Navigate_Params without
setting the url. I thought that based on the comment resetting it with
error.unreachableURL would be closer to the intention of having the error page
load be regarded as the same browser-initiated request then using an empty URL.
And now that we keep what we initially received from the browser in
NavigationState we can reset it with exactly what we received, hence why I'm not
setting the url to error.unreachableURL.

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

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2269: if
(ui::PageTransitionCoreTypeIs(navigation_state->common_params().transition,
On 2015/03/17 03:26:31, Charlie Reis wrote:
> Why not navigation_state->GetTransitionType()?

Done.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2271:
navigation_state->common_params().transition =
On 2015/03/17 03:26:31, Charlie Reis wrote:
> Should use a setter for this.  Same below.

Done.

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:4035:
static_cast<NavigationStateImpl*>(document_state->navigation_state())
On 2015/03/17 03:26:31, Charlie Reis wrote:
> Why do you need this static cast?  IsContentInitiated is on the public
> interface.

Done (it was not initially).

[Charlie Reis, 2015-03-17 16:29:06]

Thanks!  LGTM with nits.

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

https://codereview.chromium.org/1001043002/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2449: error.unreachableURL, Referrer(),
On 2015/03/17 16:08:18, clamy wrote:
> On 2015/03/17 03:26:31, Charlie Reis wrote:
> > I don't understand how the new code is equivalent in this case.  Where are
you
> > handling the error.unreachableURL (etc)?
> 
> Well until
>
https://codereview.chromium.org/971653004/diff/130001/content/renderer/render...
> we were resetting the params with a default FrameMsg_Navigate_Params without
> setting the url. I thought that based on the comment resetting it with
> error.unreachableURL would be closer to the intention of having the error page
> load be regarded as the same browser-initiated request then using an empty
URL.
> And now that we keep what we initially received from the browser in
> NavigationState we can reset it with exactly what we received, hence why I'm
not
> setting the url to error.unreachableURL.

Oh I see-- it's from a recent change.  Ok, I'll defer here since I don't have a
great understanding of the error page logic.

https://codereview.chromium.org/1001043002/diff/60001/content/renderer/naviga...
File content/renderer/navigation_state_impl.cc (right):

https://codereview.chromium.org/1001043002/diff/60001/content/renderer/naviga...
content/renderer/navigation_state_impl.cc:36:
NavigationStateImpl::~NavigationStateImpl() {
Style nit: Please list these in the same order as the .h file.

https://codereview.chromium.org/1001043002/diff/60001/content/renderer/naviga...
File content/renderer/navigation_state_impl.h (right):

https://codereview.chromium.org/1001043002/diff/60001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:35: void
SetTransitionType(ui::PageTransition transition) {
nit: set_transition_type
(Since it's a one liner that can be inlined.  It's ok for that to not match
GetTransitionType.)

https://codereview.chromium.org/1001043002/diff/60001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:39: // NavigationState implementation.
nit: Move these overrides up between CreateContentInitiated() and
common_params().

https://codereview.chromium.org/1001043002/diff/60001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:59: // Note: if is_content_initiated()
is false, whether this navigation should
nit: IsContentInitiated()

[clamy, 2015-03-17 17:19:55]

@sdefresne: PTAL at the changes in components/
@mattm: PTAL at the changes in chrome/renderer/safe_browsing
@torne: PTAL at the changes in android_webview/

@creis: Thanks!

https://codereview.chromium.org/1001043002/diff/60001/content/renderer/naviga...
File content/renderer/navigation_state_impl.cc (right):

https://codereview.chromium.org/1001043002/diff/60001/content/renderer/naviga...
content/renderer/navigation_state_impl.cc:36:
NavigationStateImpl::~NavigationStateImpl() {
On 2015/03/17 16:29:06, Charlie Reis wrote:
> Style nit: Please list these in the same order as the .h file.

Done.

https://codereview.chromium.org/1001043002/diff/60001/content/renderer/naviga...
File content/renderer/navigation_state_impl.h (right):

https://codereview.chromium.org/1001043002/diff/60001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:35: void
SetTransitionType(ui::PageTransition transition) {
On 2015/03/17 16:29:06, Charlie Reis wrote:
> nit: set_transition_type
> (Since it's a one liner that can be inlined.  It's ok for that to not match
> GetTransitionType.)

Done.

https://codereview.chromium.org/1001043002/diff/60001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:39: // NavigationState implementation.
On 2015/03/17 16:29:06, Charlie Reis wrote:
> nit: Move these overrides up between CreateContentInitiated() and
> common_params().

Done.

https://codereview.chromium.org/1001043002/diff/60001/content/renderer/naviga...
content/renderer/navigation_state_impl.h:59: // Note: if is_content_initiated()
is false, whether this navigation should
On 2015/03/17 16:29:06, Charlie Reis wrote:
> nit: IsContentInitiated()

Done.

[clamy, 2015-03-18 12:41:28]

clamy@chromium.org changed reviewers:
+ jochen@chromium.org, sdefresne@chromium.org, torne@chromium.org

[clamy, 2015-03-18 12:41:48]

@sdefresne: PTAL at the changes in components/
@jochen: PTAL at the changes in chrome/renderer/safe_browsing
@torne: PTAL at the changes in android_webview/

[Torne, 2015-03-18 13:00:35]

android_webview LGTM

[jochen (gone - plz use gerrit), 2015-03-18 13:01:18]

lgtm

[clamy, 2015-03-18 13:15:15]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-03-18 13:15:16]

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

[commit-bot: I haz the power, 2015-03-18 13:15:24]

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

[sdefresne, 2015-03-18 13:25:07]

lgtm

[commit-bot: I haz the power, 2015-03-18 13:48:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-18 13:48:58]

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