Store NavigationEntryImpl data into NavigationRequest for later usage.

Store NavigationEntryImpl data into NavigationRequest for later usage.

So that default values for some parameters can be replaced by what they actually
should be in calls from RenderFrameHostManager. This should help with making
content_unittest pass with PlzNavigate enabled and eliminate some TODOs.

BUG=439423
Committed: https://crrev.com/0dc6c55b60d7593da4ad9dc832c6cf0594f1b3bf
Cr-Commit-Position: refs/heads/master@{#312368}

[carlosk, 2015-01-15 16:53:08]

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

[carlosk, 2015-01-15 16:53:08]

clamy@, nasko@, creis@: PTAL.

This CL fulfills 2 TODOs left from the speculative renderer and lowers the
number of failing content_unittests with PlzNavigate enabled:

* 24 to 22 failures:
 - NavigationControllerTest.RestoreNavigate
 - NavigationControllerTest.RestoreNavigateAfterFailure

* 34 to 33 crashes
 -  NavigationControllerTest.ClientRedirectAfterInPageNavigation

Questions:
* It doesn't seem like this parameters are important for renderer-initiated
navigations as they don't seem to have a NavigationEntryImpl associated with
them. Am I right?
* Should the ref-counted pointers to SiteInstance-s added to NavigationRequest
be cleared after the point in the navigation process when they are not necessary
anymore?

[carlosk, 2015-01-16 09:57:26]

On 2015/01/15 16:53:08, carlosk wrote:
> clamy@, nasko@, creis@: PTAL.
> 
> This CL fulfills 2 TODOs left from the speculative renderer and lowers the
> number of failing content_unittests with PlzNavigate enabled:
> 
> * 24 to 22 failures:
>  - NavigationControllerTest.RestoreNavigate
>  - NavigationControllerTest.RestoreNavigateAfterFailure
> 
> * 34 to 33 crashes
>  -  NavigationControllerTest.ClientRedirectAfterInPageNavigation
> 
> Questions:
> * It doesn't seem like this parameters are important for renderer-initiated
> navigations as they don't seem to have a NavigationEntryImpl associated with
> them. Am I right?
> * Should the ref-counted pointers to SiteInstance-s added to NavigationRequest
> be cleared after the point in the navigation process when they are not
necessary
> anymore?

I forgot to remove the mentioned TODOs. Already removed them locally and will
upload in the next patch.

[nasko, 2015-01-16 15:03:25]

Overall looks good, just some minor comments.

https://codereview.chromium.org/818853005/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/818853005/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.cc:54: is_restore_ =
nav_entry.restore_type() != NavigationEntryImpl::RESTORE_NONE;
Why not store the restore type directly?

https://codereview.chromium.org/818853005/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/818853005/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_request.h:68: void CopyDataFrom(const
NavigationEntryImpl& navitation_entry);
The name seems a bit strange, either CopyData and rely on the parameter type or
CopyDataFromNavEntry.

https://codereview.chromium.org/818853005/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/818853005/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:654: void
RenderFrameHostManager::BeginNavigation(NavigationRequest* request) {
It doesn't seem we modify the request at all here, why not pass it as const
reference?

https://codereview.chromium.org/818853005/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:668: NavigationRequest*
request) {
Same comment about const reference.

[clamy, 2015-01-16 16:39:02]

Thanks! Just a few comments on my part.

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
content/browser/frame_host/navigator_impl.cc:897:
navigation_request->CopyDataFrom(entry);
Considering that we only call CopyDataFrom once, just below the constructor, how
about making NavigationEntry a parameter of the constructor and doing the work
there?

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.cc:654: void
RenderFrameHostManager::BeginNavigation(NavigationRequest* request) {
On 2015/01/16 15:03:24, nasko wrote:
> It doesn't seem we modify the request at all here, why not pass it as const
> reference?

+1. It'd be good if we avoided passing a pointer to the RenderFrameHostManager,
to make it clear that the only class modifying the NavigationRequest is
Navigator.

[carlosk, 2015-01-19 15:02:52]

Thanks for all comments.

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
File content/browser/frame_host/navigation_request.cc (right):

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
content/browser/frame_host/navigation_request.cc:54: is_restore_ =
nav_entry.restore_type() != NavigationEntryImpl::RESTORE_NONE;
On 2015/01/16 15:03:24, nasko wrote:
> Why not store the restore type directly?

The reason was that we only need to know if it is a restore or not, and not the
kind of restore.

But I changed to storing the actual type after discussing with clamy@ that it
might be needed in the future.

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
File content/browser/frame_host/navigation_request.h (right):

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
content/browser/frame_host/navigation_request.h:68: void CopyDataFrom(const
NavigationEntryImpl& navitation_entry);
On 2015/01/16 15:03:24, nasko wrote:
> The name seems a bit strange, either CopyData and rely on the parameter type
or
> CopyDataFromNavEntry.

The method was removed in favor of an optional constructor parameter.

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
content/browser/frame_host/navigator_impl.cc:897:
navigation_request->CopyDataFrom(entry);
On 2015/01/16 16:39:02, clamy wrote:
> Considering that we only call CopyDataFrom once, just below the constructor,
how
> about making NavigationEntry a parameter of the constructor and doing the work
> there?

Done.

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.cc:654: void
RenderFrameHostManager::BeginNavigation(NavigationRequest* request) {
On 2015/01/16 16:39:02, clamy wrote:
> On 2015/01/16 15:03:24, nasko wrote:
> > It doesn't seem we modify the request at all here, why not pass it as const
> > reference?
> 
> +1. It'd be good if we avoided passing a pointer to the
RenderFrameHostManager,
> to make it clear that the only class modifying the NavigationRequest is
> Navigator.

Yes, my bad on this. It's a const reference now.

https://chromiumcodereview.appspot.com/818853005/diff/20001/content/browser/f...
content/browser/frame_host/render_frame_host_manager.cc:668: NavigationRequest*
request) {
On 2015/01/16 15:03:24, nasko wrote:
> Same comment about const reference.

Done.

[clamy, 2015-01-19 15:53:47]

Thanks! A few comments for myself.

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
File content/browser/frame_host/navigation_request.h (right):

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
content/browser/frame_host/navigation_request.h:11: #include
"content/browser/frame_host/navigation_entry_impl.h"
Why did you remove the forward declaration in favor of an include?

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
content/browser/frame_host/navigator_impl.cc:894: &entry));
Please correct the indentation here.

[carlosk, 2015-01-19 16:08:52]

Thanks!

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
File content/browser/frame_host/navigation_request.h (right):

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
content/browser/frame_host/navigation_request.h:11: #include
"content/browser/frame_host/navigation_entry_impl.h"
On 2015/01/19 15:53:46, clamy wrote:
> Why did you remove the forward declaration in favor of an include?

It was required now because of the restore_type() getter returning a
NavigationEntryImpl::RestoreType (incomplete type error otherwise).

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
content/browser/frame_host/navigator_impl.cc:894: &entry));
On 2015/01/19 15:53:46, clamy wrote:
> Please correct the indentation here.

This is apparently correct... I did run git cl format.

[clamy, 2015-01-19 16:11:10]

Thanks! Lgtm.

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
File content/browser/frame_host/navigation_request.h (right):

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
content/browser/frame_host/navigation_request.h:11: #include
"content/browser/frame_host/navigation_entry_impl.h"
On 2015/01/19 16:08:52, carlosk wrote:
> On 2015/01/19 15:53:46, clamy wrote:
> > Why did you remove the forward declaration in favor of an include?
> 
> It was required now because of the restore_type() getter returning a
> NavigationEntryImpl::RestoreType (incomplete type error otherwise).

Acknowledged.

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
content/browser/frame_host/navigator_impl.cc:894: &entry));
On 2015/01/19 16:08:52, carlosk wrote:
> On 2015/01/19 15:53:46, clamy wrote:
> > Please correct the indentation here.
> 
> This is apparently correct... I did run git cl format.
No you're right I read to quickly and though this was one of the parameters for
the CommitNavigationparams. My bad.

[nasko, 2015-01-21 00:29:55]

LGTM with a nit

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
File content/browser/frame_host/navigation_request.cc (right):

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
content/browser/frame_host/navigation_request.cc:31: if (entry != nullptr) {
nit: if (entry)

[carlosk, 2015-01-21 13:53:54]

Thanks. Done, uploaded and committing...

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
File content/browser/frame_host/navigation_request.cc (right):

https://chromiumcodereview.appspot.com/818853005/diff/40001/content/browser/f...
content/browser/frame_host/navigation_request.cc:31: if (entry != nullptr) {
On 2015/01/21 00:29:55, nasko wrote:
> nit: if (entry)

Done.

[carlosk, 2015-01-21 13:54:34]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2015-01-21 13:57:57]

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

[commit-bot: I haz the power, 2015-01-21 14:31:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-21 14:32:34]

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