RenderWidget: hoist synchronous IPC out of Init methods.

content/renderer/render_widget.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/renderer/render_widget.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 269 matching lines @@
   g_create_render_widget = create_render_widget;
   g_render_widget_initialized = render_widget_initialized;
 }
 
 // static
 RenderWidget* RenderWidget::Create(int32_t opener_id,
                                    CompositorDependencies* compositor_deps,
                                    blink::WebPopupType popup_type,
                                    const ScreenInfo& screen_info) {
   DCHECK(opener_id != MSG_ROUTING_NONE);
+  int32_t routing_id = MSG_ROUTING_NONE;
+
+  // Do a synchronous IPC to obtain a routing ID.
+  RenderThreadImpl::current_render_message_filter()->CreateNewWidget(
+      opener_id, popup_type, &routing_id);

[ncarter (slow), 2016/11/09 22:52:38]

This comes from the old RenderWidget::CreateWidget() function in the old code.

+
+  if (routing_id == MSG_ROUTING_NONE)

[lfg, 2016/11/09 23:27:30]

Is this for error handling when the IPC fails? The sync IPC should never return
MSG_ROUTING_NONE if it's executed.

[ncarter (slow), 2016/11/10 18:44:45]

I read up on this, and asked on chromium-mojo. Mojo Sync IPCs can fail, in the
same cases legacy IPCs did -- when the pipe is broken. In this case the method
returns false and doesn't touch the value of |routing_id|.

It looks like the preferred idiom is to check the return value of
CreateNewWidget, so I've switched to that.

+    return nullptr;
+
   scoped_refptr<RenderWidget> widget(new RenderWidget(
       compositor_deps, popup_type, screen_info, false, false, false));
-  if (widget->Init(opener_id)) {  // adds reference on success.

[ncarter (slow), 2016/11/09 22:52:38]

The one-arg Init() function has been inlined into RenderWidget::Create.

-    return widget.get();
-  }
-  return NULL;
+  widget->InitRoutingID(routing_id);
+  widget->Init(opener_id, RenderWidget::CreateWebWidget(widget.get()));
+  DCHECK(!widget->HasOneRef());  // RenderWidget::Init() adds a reference.
+  return widget.get();
 }
 
 // static
 RenderWidget* RenderWidget::CreateForFrame(
-    int routing_id,
+    int widget_routing_id,
     bool hidden,
     const ScreenInfo& screen_info,
     CompositorDependencies* compositor_deps,
     blink::WebLocalFrame* frame) {
-  CHECK_NE(routing_id, MSG_ROUTING_NONE);
+  CHECK_NE(widget_routing_id, MSG_ROUTING_NONE);
   // TODO(avi): Before RenderViewImpl has-a RenderWidget, the browser passes the
   // same routing ID for both the view routing ID and the main frame widget
   // routing ID. https://crbug.com/545684
-  RenderViewImpl* view = RenderViewImpl::FromRoutingID(routing_id);
+  RenderViewImpl* view = RenderViewImpl::FromRoutingID(widget_routing_id);
   if (view) {
     view->AttachWebFrameWidget(
         RenderWidget::CreateWebFrameWidget(view->GetWidget(), frame));
     return view->GetWidget();
   }
   scoped_refptr<RenderWidget> widget(
       g_create_render_widget
           ? g_create_render_widget(compositor_deps, blink::WebPopupTypeNone,
                                    screen_info, false, hidden, false)
           : new RenderWidget(compositor_deps, blink::WebPopupTypeNone,
                              screen_info, false, hidden, false));
-  widget->SetRoutingID(routing_id);
   widget->for_oopif_ = true;
-  // DoInit increments the reference count on |widget|, keeping it alive after
+  // Init increments the reference count on |widget|, keeping it alive after
   // this function returns.
-  if (widget->DoInit(MSG_ROUTING_NONE,
-                     RenderWidget::CreateWebFrameWidget(widget.get(), frame),
-                     CreateWidgetCallback())) {
-    if (g_render_widget_initialized)
-      g_render_widget_initialized(widget.get());
-    return widget.get();
-  }
-  return nullptr;
+  widget->InitRoutingID(widget_routing_id);
+  widget->Init(MSG_ROUTING_NONE,
+               RenderWidget::CreateWebFrameWidget(widget.get(), frame));
+
+  if (g_render_widget_initialized)
+    g_render_widget_initialized(widget.get());
+  return widget.get();
 }
 
 // static
 blink::WebFrameWidget* RenderWidget::CreateWebFrameWidget(
     RenderWidget* render_widget,
     blink::WebLocalFrame* frame) {
   if (!frame->parent()) {
     // TODO(dcheng): The main frame widget currently has a special case.
     // Eliminate this once WebView is no longer a WebWidget.
     return blink::WebFrameWidget::create(render_widget, frame->view(), frame);
@@ skipping 11 matching lines @@
     default:
       NOTREACHED();
   }
   return NULL;
 }
 
 void RenderWidget::CloseForFrame() {
   OnClose();
 }
 
-void RenderWidget::SetRoutingID(int32_t routing_id) {
-  routing_id_ = routing_id;
-  input_handler_.reset(new RenderWidgetInputHandler(
-      GetRenderWidgetInputHandlerDelegate(this), this));
-}
-
 void RenderWidget::SetSwappedOut(bool is_swapped_out) {
   // We should only toggle between states.
   DCHECK(is_swapped_out_ != is_swapped_out);
   is_swapped_out_ = is_swapped_out;
 
   // If we are swapping out, we will call ReleaseProcess, allowing the process
   // to exit if all of its RenderViews are swapped out.  We wait until the
   // WasSwappedOut call to do this, to allow the unload handler to finish.
   // If we are swapping in, we call AddRefProcess to prevent the process from
   // exiting.
   if (!is_swapped_out_)
     RenderProcess::current()->AddRefProcess();
 }
 
-bool RenderWidget::CreateWidget(int32_t opener_id,
-                                blink::WebPopupType popup_type,
-                                int32_t* routing_id) {
-  RenderThreadImpl::current_render_message_filter()->CreateNewWidget(
-      opener_id, popup_type, routing_id);
-  return true;
+void RenderWidget::InitRoutingID(int32_t routing_id) {
+  DCHECK_EQ(routing_id_, MSG_ROUTING_NONE);
+  routing_id_ = routing_id;
+  input_handler_.reset(new RenderWidgetInputHandler(
+      GetRenderWidgetInputHandlerDelegate(this), this));
 }
 
-bool RenderWidget::Init(int32_t opener_id) {
-  bool success = DoInit(opener_id, RenderWidget::CreateWebWidget(this),
-      base::Bind(&RenderWidget::CreateWidget, base::Unretained(this),
-           opener_id, popup_type_, &routing_id_));
-  if (success) {
-    SetRoutingID(routing_id_);
-    return true;
-  }
-  return false;
-}
-
-bool RenderWidget::DoInit(int32_t opener_id,
-                          WebWidget* web_widget,
-                          CreateWidgetCallback create_widget_callback) {
+void RenderWidget::Init(int32_t opener_id, WebWidget* web_widget) {
   DCHECK(!webwidget_internal_);
+  DCHECK_NE(routing_id_, MSG_ROUTING_NONE);
 
   if (opener_id != MSG_ROUTING_NONE)
     opener_id_ = opener_id;
 
   webwidget_internal_ = web_widget;
   webwidget_mouse_lock_target_.reset(
       new WebWidgetLockTarget(webwidget_internal_));
   mouse_lock_dispatcher_.reset(new RenderWidgetMouseLockDispatcher(this));
 
-  bool result = true;
-  if (!create_widget_callback.is_null())
-    result = std::move(create_widget_callback).Run();
-
-  if (result) {
-    RenderThread::Get()->AddRoute(routing_id_, this);
-    // Take a reference on behalf of the RenderThread.  This will be balanced
-    // when we receive ViewMsg_Close.
-    AddRef();
-    if (RenderThreadImpl::current()) {
-      RenderThreadImpl::current()->WidgetCreated();
-      if (is_hidden_)
-        RenderThreadImpl::current()->WidgetHidden();
-    }
-
-    return true;
-  } else {
-    // The above Send can fail when the tab is closing.
-    return false;
+  RenderThread::Get()->AddRoute(routing_id_, this);
+  // Take a reference on behalf of the RenderThread.  This will be balanced
+  // when we receive ViewMsg_Close.
+  AddRef();
+  if (RenderThreadImpl::current()) {
+    RenderThreadImpl::current()->WidgetCreated();
+    if (is_hidden_)
+      RenderThreadImpl::current()->WidgetHidden();
   }
 }
 
 void RenderWidget::WasSwappedOut() {
   // If we have been swapped out and no one else is using this process,
   // it's safe to exit now.
   CHECK(is_swapped_out_);
   RenderProcess::current()->ReleaseProcess();
 }
 
@@ skipping 1622 matching lines @@
 bool RenderWidget::isPointerLocked() {
   return mouse_lock_dispatcher_->IsMouseLockedTo(
       webwidget_mouse_lock_target_.get());
 }
 
 blink::WebWidget* RenderWidget::GetWebWidget() const {
   return webwidget_internal_;
 }
 
 }  // namespace content