Don't let swappedout:// page be visible when updating frame tree.

Don't let swappedout:// page be visible when updating frame tree.

BUG=143155
TEST=See bug, comment 19.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158025

[Charlie Reis, 2012-09-20 22:07:31]

Nasko, can you take a look?

[nasko, 2012-09-20 22:18:04]

Just a few small comments.

https://codereview.chromium.org/10952035/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/10952035/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1304: //
1) Open 2 tabs in the same process, with a frame in the opener.
nit: Should we say "iframe" or "subframe"? We got bitten by doing
createElement("frame") while testing :)

https://codereview.chromium.org/10952035/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1306: //  
 This creates a swapped out opener in the first tab.
nit: Technically in the "first tab's process."

https://codereview.chromium.org/10952035/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1310: //
just as it was navigating to a real page.
It might be worthwhile pointing out that the end result was navigating the
pending renderer to swappedout://, which then became the active page, since that
was the user visible problem.

https://codereview.chromium.org/10952035/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1357:
WaitForLoadStop(shell()->web_contents());
Isn't this redundant with the NavigateToURL waiting for load to complete?

https://codereview.chromium.org/10952035/diff/8001/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/10952035/diff/8001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:263: // TODO(nasko):
Don't send updates across BrowsingInstances.
nit: You can put in crbug.com/150855 for my TODO.

[Charlie Reis, 2012-09-20 23:25:38]

Thanks.  PTAL.

https://codereview.chromium.org/10952035/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/10952035/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1304: //
1) Open 2 tabs in the same process, with a frame in the opener.
On 2012/09/20 22:18:04, nasko wrote:
> nit: Should we say "iframe" or "subframe"? We got bitten by doing
> createElement("frame") while testing :)

Done.

https://codereview.chromium.org/10952035/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1306: //  
 This creates a swapped out opener in the first tab.
On 2012/09/20 22:18:04, nasko wrote:
> nit: Technically in the "first tab's process."

No, that's backwards.  The swapped out RenderView is in the first tab, but in
the second process.  Clarified.

https://codereview.chromium.org/10952035/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1310: //
just as it was navigating to a real page.
On 2012/09/20 22:18:04, nasko wrote:
> It might be worthwhile pointing out that the end result was navigating the
> pending renderer to swappedout://, which then became the active page, since
that
> was the user visible problem.

Done.

https://codereview.chromium.org/10952035/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_manager_browsertest.cc:1357:
WaitForLoadStop(shell()->web_contents());
On 2012/09/20 22:18:04, nasko wrote:
> Isn't this redundant with the NavigateToURL waiting for load to complete?

That's what I thought, but the test doesn't fail (in the absence of the fix)
without it.  I think the swappedout:// commit might have been separate, and this
was enough to see it.

https://codereview.chromium.org/10952035/diff/8001/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/10952035/diff/8001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:263: // TODO(nasko):
Don't send updates across BrowsingInstances.
On 2012/09/20 22:18:04, nasko wrote:
> nit: You can put in crbug.com/150855 for my TODO.

Done.

[nasko, 2012-09-21 13:37:30]

lgtm

[commit-bot: I haz the power, 2012-09-21 15:55:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/10952035/10004

[commit-bot: I haz the power, 2012-09-21 18:14:28]

Change committed as 158025