Browser Plugin: Simplify NewWindow code

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/web_contents/web_contents_impl.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
@@ skipping 1337 matching lines @@
     // For more info, see comment above class BrowserPluginGuest.
     int instance_id = GetBrowserPluginGuestManager()->get_next_instance_id();
     WebContentsImpl* new_contents_impl =
         static_cast<WebContentsImpl*>(new_contents);
     new_contents_impl->browser_plugin_guest_.reset(
         BrowserPluginGuest::Create(instance_id, new_contents_impl));
   }
   new_contents->Init(create_params);
 
   // 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();
+  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());
-
+      // 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);
     pending_contents_[route_id] = new_contents;
     registrar_.Add(this, NOTIFICATION_WEB_CONTENTS_DESTROYED,
                    Source<WebContents>(new_contents));
   }
 
   if (delegate_) {
     delegate_->WebContentsCreated(
@@ skipping 123 matching lines @@
   // in creating one, just return NULL.
   if (iter == pending_contents_.end()) {
     return NULL;
   }
 
   WebContentsImpl* new_contents = iter->second;
   pending_contents_.erase(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;
 
   // TODO(brettw): It seems bogus to reach into here and initialize the host.
   static_cast<RenderViewHostImpl*>(new_contents->GetRenderViewHost())->Init();
   return new_contents;
 }
 
 RenderWidgetHostView* WebContentsImpl::GetCreatedWidget(int route_id) {
@@ skipping 1914 matching lines @@
 }
 
 BrowserPluginGuestManager*
     WebContentsImpl::GetBrowserPluginGuestManager() const {
   return static_cast<BrowserPluginGuestManager*>(
       GetBrowserContext()->GetUserData(
           browser_plugin::kBrowserPluginGuestManagerKeyName));
 }
 
 }  // namespace content