PlzNavigate: make content unit tests work with browser side navigation

PlzNavigate: make content unit tests work with browser side navigation

This CL fixes several WebContentsImplTests and RenderFrameHostManagerTests so
that they work with PlzNavigate enabled by making use of the helper functions
introduced in https://chromiumcodereview.appspot.com/761013003/.

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

[clamy, 2015-01-13 12:40:38]

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

[clamy, 2015-01-13 12:41:59]

@carlosk, nasko: PTAL

This makes a certain number of WebContentsImpl and RenderFrameHostManager tests
work with PlzNavigate.

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:267:
contents()->GetMainFrame()->PrepareForCommit(url);
It was necessary to move this above the part where we get the main and the
pending frame because when we are trying to navigate to a webui from a new tab,
a RenderFrameHost is created to display it that is committed immediately (since
it is a new tab). Therefore the main frame is replaced without a pending frame
being created, and we don't get the right values for the RFH to navigate: we try
to use the old one that has been deleted in the meantime.

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/test/test...
File content/test/test_web_contents.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/test/test...
content/test/test_web_contents.cc:153:
GetMainFrame()->PrepareForCommit(entry->GetURL());
There is exactly the same problem of the webui RFH being committed immediately
on creation as mentioned in render_frame_host_manager.cc, hence the reordering
of function calls.

[carlosk, 2015-01-15 12:32:10]

On 2015/01/13 12:41:59, clamy wrote:
> @carlosk, nasko: PTAL
> 
> This makes a certain number of WebContentsImpl and RenderFrameHostManager
tests
> work with PlzNavigate.
> 
>
https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
> File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
> content/browser/frame_host/render_frame_host_manager_unittest.cc:267:
> contents()->GetMainFrame()->PrepareForCommit(url);
> It was necessary to move this above the part where we get the main and the
> pending frame because when we are trying to navigate to a webui from a new
tab,
> a RenderFrameHost is created to display it that is committed immediately
(since
> it is a new tab). Therefore the main frame is replaced without a pending frame
> being created, and we don't get the right values for the RFH to navigate: we
try
> to use the old one that has been deleted in the meantime.
> 
>
https://chromiumcodereview.appspot.com/789643005/diff/80001/content/test/test...
> File content/test/test_web_contents.cc (right):
> 
>
https://chromiumcodereview.appspot.com/789643005/diff/80001/content/test/test...
> content/test/test_web_contents.cc:153:
> GetMainFrame()->PrepareForCommit(entry->GetURL());
> There is exactly the same problem of the webui RFH being committed immediately
> on creation as mentioned in render_frame_host_manager.cc, hence the reordering
> of function calls.

I reviewed all the code and found nothing to comment on. My only question is why
did you need to add so many calls to PrepareForCommit in WebContentsImplTest,
way more than the number of SendBeforeUnloadACK calls that existed before?

[nasko, 2015-01-17 00:22:01]

Looks good, mostly nits.

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
File content/browser/frame_host/navigator_impl.h (right):

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/navigator_impl.h:93: // event to execute in the
renderer.
nit: s/renderer/renderer process/.

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:884:
->IsWaitingForBeforeUnloadACK(frame_tree_node_);
Does it make sense to put this on the interface and avoid the cast here?

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:1416: // There's navigation
message params waiting to be sent. Now that we're not
nit: 'params' implies 'There're'

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:1417: // suspended anymore,
resume navigation by sending them. If we were swapped
We don't use suspending of navigations in PlzNavigate, do we? What are those
suspended params.

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:245: // renderer. Currently
this only happens in cross-site navigations.
nit: s/renderer/RenderFrame/

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:267:
contents()->GetMainFrame()->PrepareForCommit(url);
On 2015/01/13 12:41:59, clamy wrote:
> It was necessary to move this above the part where we get the main and the
> pending frame because when we are trying to navigate to a webui from a new
tab,
> a RenderFrameHost is created to display it that is committed immediately
(since
> it is a new tab). Therefore the main frame is replaced without a pending frame
> being created, and we don't get the right values for the RFH to navigate: we
try
> to use the old one that has been deleted in the meantime.

Can you put some of this explanation in the actual comment? It will be useful
detail for whoever is looking at this code later and not looking at this
particular CL.

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/test/test...
File content/test/test_web_contents.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/test/test...
content/test/test_web_contents.cc:153:
GetMainFrame()->PrepareForCommit(entry->GetURL());
On 2015/01/13 12:41:59, clamy wrote:
> There is exactly the same problem of the webui RFH being committed immediately
> on creation as mentioned in render_frame_host_manager.cc, hence the reordering
> of function calls.

Acknowledged.

[clamy, 2015-01-19 17:29:20]

Thanks! Please find my answers below. However, following the landing of the
speculative RFH I will have to rework a new version of this patch, as far as the
specific unit tests are concerned. Their failures with the speculative RFH are
slightly different from without.

@carlosk: as explained offline, with PlzNavigate, PrepareForCommit needs to be
called on every navigation. With the current code path, SendBeforeUnloadACK only
needs to be called for browser-initiated cross-site navigations.

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
File content/browser/frame_host/navigator_impl.h (right):

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/navigator_impl.h:93: // event to execute in the
renderer.
On 2015/01/17 00:22:01, nasko wrote:
> nit: s/renderer/renderer process/.

Done (note that the comment was moved to Navigator).

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:884:
->IsWaitingForBeforeUnloadACK(frame_tree_node_);
On 2015/01/17 00:22:01, nasko wrote:
> Does it make sense to put this on the interface and avoid the cast here?

Tried it in the latest patch set, it may be slightly better. I'm not sure though
that I understand the precise difference between Navigator and NavigatorImpl.
Considering they are both in content/browser/frame_host and therefore not
public, why do we have both?

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:1416: // There's navigation
message params waiting to be sent. Now that we're not
On 2015/01/17 00:22:01, nasko wrote:
> nit: 'params' implies 'There're'

The whole block was removed since it landed in the speculative RFH patch.

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.cc:1417: // suspended anymore,
resume navigation by sending them. If we were swapped
On 2015/01/17 00:22:01, nasko wrote:
> We don't use suspending of navigations in PlzNavigate, do we? What are those
> suspended params.

Same here.

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/render_frame_host_impl.h:245: // renderer. Currently
this only happens in cross-site navigations.
On 2015/01/17 00:22:01, nasko wrote:
> nit: s/renderer/RenderFrame/

Done.

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/80001/content/browser/f...
content/browser/frame_host/render_frame_host_manager_unittest.cc:267:
contents()->GetMainFrame()->PrepareForCommit(url);
On 2015/01/17 00:22:01, nasko wrote:
> On 2015/01/13 12:41:59, clamy wrote:
> > It was necessary to move this above the part where we get the main and the
> > pending frame because when we are trying to navigate to a webui from a new
> tab,
> > a RenderFrameHost is created to display it that is committed immediately
> (since
> > it is a new tab). Therefore the main frame is replaced without a pending
frame
> > being created, and we don't get the right values for the RFH to navigate: we
> try
> > to use the old one that has been deleted in the meantime.
> 
> Can you put some of this explanation in the actual comment? It will be useful
> detail for whoever is looking at this code later and not looking at this
> particular CL.

Done.

[clamy, 2015-01-20 14:18:48]

@nasko, carlosk: PTAL at the latest patchset which also handles a few unit tests
failures that were made apparent with the speculative RFH patch.

[carlosk, 2015-01-20 15:38:02]

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:377:
RenderFrameHostImpl* GetFrameHostForNavigation(
Oh, I see what you did here! Initially I didn't like the name choice but I
realized that in the future, once PlzNavigate is done, we'll directly invoke the
likely named method from the |manager|. Nice!

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:390:
RenderFrameHostImpl* GetPendingFrameHost(
Thinking about the future again, maybe here we should move away from the
"pending" name?

I understand that once the currently existing "pending" stuff is gone we could
rename speculative to that. But I think that will be somewhat confusing.

WDYT?

[nasko, 2015-01-21 00:41:32]

Just a couple of questions.

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:435:
cross_navigation_pending_ = false;
Shouldn't this flag be cleared in CommitPending itself?

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:743:
cross_navigation_pending_ = navigation_rfh != render_frame_host_.get();
Are we sure the returned RFH will actually start off a navigation?

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:269: // webui
from a new tab, a RenderFrameHost is created to display it that is
nit: s/webui/WebUI/

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:270: //
committed immediately (since it is a new tab). Therefore the main frame
Is this only the case for new tab with WebUI? What about window.open?

[clamy, 2015-01-21 13:22:11]

Thanks! Please find my answers below.

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:435:
cross_navigation_pending_ = false;
On 2015/01/21 00:41:32, nasko wrote:
> Shouldn't this flag be cleared in CommitPending itself?

Done.

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:743:
cross_navigation_pending_ = navigation_rfh != render_frame_host_.get();
On 2015/01/21 00:41:32, nasko wrote:
> Are we sure the returned RFH will actually start off a navigation?

No, this is also called by BeginNavigation, so at this point we may not be using
it. However the cross_navigation_pending_ flag is only useful for tests, so I
think it should be ok to set it here.

If we want to make sure that it is only set when a navigation will take place in
the speculative renderer we could have a boolean in the function arguments to
distinguish between when the function is called at BeginNavigation or commit
time. Or have a setter for cross__navigation_pending_ called by
NavigatorImpl::CommitNavigation if needed.

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:269: // webui
from a new tab, a RenderFrameHost is created to display it that is
On 2015/01/21 00:41:32, nasko wrote:
> nit: s/webui/WebUI/

Done.

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:270: //
committed immediately (since it is a new tab). Therefore the main frame
On 2015/01/21 00:41:32, nasko wrote:
> Is this only the case for new tab with WebUI? What about window.open?

For this to happen, you need the current renderer to not be live, and to not be
reused for the navigation. So in the case of the WebUI I think it does not have
the right bindings, but for window.open, wouldn't we use the current RFH if it
is not live?

https://chromiumcodereview.appspot.com/789643005/diff/120001/content/browser/...
content/browser/frame_host/render_frame_host_manager_unittest.cc:390:
RenderFrameHostImpl* GetPendingFrameHost(
On 2015/01/20 15:38:02, carlosk wrote:
> Thinking about the future again, maybe here we should move away from the
> "pending" name?
> 
> I understand that once the currently existing "pending" stuff is gone we could
> rename speculative to that. But I think that will be somewhat confusing.
> 
> WDYT?

I was trying to match what is done with the
TestWebContents::GetPendingMainFrame(). I think it is better to keep a similar
name here, for consistency.

[clamy, 2015-01-21 16:31:03]

@nasko, carlosk: following the landing of
https://chromiumcodereview.appspot.com/818853005/, I have uploaded a new
patchset.

[carlosk, 2015-01-21 17:12:28]

On 2015/01/21 16:31:03, clamy wrote:
> @nasko, carlosk: following the landing of
> https://chromiumcodereview.appspot.com/818853005/, I have uploaded a new
> patchset.

lgtm

[nasko, 2015-01-21 17:21:33]

LGTM with a few nits

https://chromiumcodereview.appspot.com/789643005/diff/200001/content/browser/...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/200001/content/browser/...
content/browser/frame_host/navigator_impl.cc:841:
navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
nit: indent two more spaces

https://chromiumcodereview.appspot.com/789643005/diff/200001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/200001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:739:
cross_navigation_pending_ = navigation_rfh != render_frame_host_.get();
nit: If cross_navigation_pending_ is not used for PlzNavigate, why is it set
here then? This method is only used in PlzNavigate, right?

https://chromiumcodereview.appspot.com/789643005/diff/200001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1464:
cross_navigation_pending_ = false;
nit: I'd move this down to where we actually swap the RFHs. It would avoid a
case where a method call between here and the actual swap consults this value,
yet we are in the middle of committing.

[clamy, 2015-01-21 19:39:41]

Thanks!

https://chromiumcodereview.appspot.com/789643005/diff/200001/content/browser/...
File content/browser/frame_host/navigator_impl.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/200001/content/browser/...
content/browser/frame_host/navigator_impl.cc:841:
navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
On 2015/01/21 17:21:33, nasko wrote:
> nit: indent two more spaces

Done.

https://chromiumcodereview.appspot.com/789643005/diff/200001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/200001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:739:
cross_navigation_pending_ = navigation_rfh != render_frame_host_.get();
On 2015/01/21 17:21:33, nasko wrote:
> nit: If cross_navigation_pending_ is not used for PlzNavigate, why is it set
> here then? This method is only used in PlzNavigate, right?

Yes, but it is checked by a certain number of tests that fail if we don't
properly set it.

https://chromiumcodereview.appspot.com/789643005/diff/200001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:1464:
cross_navigation_pending_ = false;
On 2015/01/21 17:21:33, nasko wrote:
> nit: I'd move this down to where we actually swap the RFHs. It would avoid a
> case where a method call between here and the actual swap consults this value,
> yet we are in the middle of committing.

Done.

[clamy, 2015-01-21 19:39:46]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2015-01-21 19:42:24]

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

[nasko, 2015-01-21 19:47:07]

https://chromiumcodereview.appspot.com/789643005/diff/200001/content/browser/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://chromiumcodereview.appspot.com/789643005/diff/200001/content/browser/...
content/browser/frame_host/render_frame_host_manager.cc:739:
cross_navigation_pending_ = navigation_rfh != render_frame_host_.get();
On 2015/01/21 19:39:41, clamy wrote:
> On 2015/01/21 17:21:33, nasko wrote:
> > nit: If cross_navigation_pending_ is not used for PlzNavigate, why is it set
> > here then? This method is only used in PlzNavigate, right?
> 
> Yes, but it is checked by a certain number of tests that fail if we don't
> properly set it.

Then the comment that it isn't used in PlzNavigate is incorrect. Let's fix that
at some point.

[commit-bot: I haz the power, 2015-01-21 20:38:55]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-01-21 20:40:31]

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