Browser Plugin: Simplify NewWindow code

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index a4ff3e39f1f5582bb0efccdde877ccaa6b7159ec..93d29a502e240e1d22305e2a1bd2a446398c7b18 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -1357,13 +1357,14 @@ void WebContentsImpl::CreateNewWindow(
   // Save the window for later if we're not suppressing the opener (since it
   // will be shown immediately) and if it's not a guest (since we separately

[Charlie Reis, 2013/04/04 21:48:46]

This comment looks stale now.  For guests, we are now saving the window for
later?

[Fady Samuel, 2013/04/04 22:35:15]

We save the window until WebContentsImpl::ShowCreatedWindow, yes. Updated.

   // track when to show guests).
-  if (!params.opener_suppressed && !is_guest) {
-    WebContentsViewPort* new_view = new_contents->view_.get();
-
-    // TODO(brettw): It seems bogus that we have to call this function on the
-    // newly created object and give it one of its own member variables.
-    new_view->CreateViewForWidget(new_contents->GetRenderViewHost());
+  if (!params.opener_suppressed) {
+    if (!is_guest) {
+      WebContentsViewPort* new_view = new_contents->view_.get();
 
+      // TODO(brettw): It seems bogus that we have to call this function on the
+      // newly created object and give it one of its own member variables.
+      new_view->CreateViewForWidget(new_contents->GetRenderViewHost());
+    }
     // Save the created window associated with the route so we can show it
     // later.
     DCHECK_NE(MSG_ROUTING_NONE, route_id);
@@ -1507,6 +1508,10 @@ WebContentsImpl* WebContentsImpl::GetCreatedWindow(int route_id) {
   registrar_.Remove(this, NOTIFICATION_WEB_CONTENTS_DESTROYED,
                     Source<WebContents>(new_contents));
 
+  // Don't initialize the guest WebContents immediately.
+  if (new_contents->GetRenderProcessHost()->IsGuest())

[Charlie Reis, 2013/04/04 21:48:46]

What if the process died and didn't have a connection?  We would have returned
NULL below, but now we return the new_contents.  Should this be below that
check?

[Fady Samuel, 2013/04/04 22:35:15]

If the process crashed then we'll do cleanup elsewhere: in BrowserPluginGuest.

The new contents won't have a RenderWidgetHostView yet for guests, we install
that on attachment to a BrowserPlugin.

+    return new_contents;
+
   if (!new_contents->GetRenderProcessHost()->HasConnection() ||
       !new_contents->GetRenderViewHost()->GetView())
     return NULL;