Separate RenderViewHost from RenderWidgetHost, part 4: delegate

content/browser/renderer_host/render_view_host_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/renderer_host/render_view_host_impl.h"
 
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 170 matching lines @@
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // RenderViewHostImpl, public:
 
 // static
 RenderViewHostImpl* RenderViewHostImpl::FromID(int render_process_id,
                                                int render_view_id) {
   RenderWidgetHost* widget =
       RenderWidgetHost::FromID(render_process_id, render_view_id);
-  if (!widget || !widget->IsRenderView())
-    return NULL;
-  return static_cast<RenderViewHostImpl*>(RenderWidgetHostImpl::From(widget));
+  if (!widget)
+    return nullptr;
+  return From(widget);
 }
 
 // static
 RenderViewHostImpl* RenderViewHostImpl::From(RenderWidgetHost* rwh) {
-  DCHECK(rwh->IsRenderView());
-  return static_cast<RenderViewHostImpl*>(RenderWidgetHostImpl::From(rwh));
+  DCHECK(rwh);
+  RenderWidgetHostOwnerDelegate* owner_delegate =
+      RenderWidgetHostImpl::From(rwh)->owner_delegate();
+  if (!owner_delegate)

[ncarter (slow), 2015/10/26 18:35:02]

Seems a little dirty for this code to assume it's the only owner delegate. I'm
okay with leaving it that way if we're pretty sure that
RenderWidgetHostOwnerDelegate will have a truly short life. But if we think in
the interim we might want to port other behaviors to this delegate (e.g.
RWHI::owned_by_render_frame_host_) then we should make it safer maybe?

Two options I'd be okay with:
 1. Add one of those lame-o methods
RenderWidgetHostOwnerDelegate::GetAsRenderViewHost()
 2. Or, alternatively just add here a DCHECK(result->GetWidget() == rwh) so that
we'll at least fail here if things do go south.

[Avi (use Gerrit), 2015/10/26 19:41:28]

Yes, this is the weakest part of the scheme.
BTW I want to rename that to a more general "is owned / should not delete self"
name.
That just adds the layering violation back in :(
That's fair. I do want to make that weird delegate go away quickly, FWIW.

+    return nullptr;
+  return static_cast<RenderViewHostImpl*>(owner_delegate);
 }
 
 RenderViewHostImpl::RenderViewHostImpl(
     SiteInstance* instance,
     RenderViewHostDelegate* delegate,
     RenderWidgetHostDelegate* widget_delegate,
     int32 routing_id,
     int32 main_frame_routing_id,
     bool swapped_out,
     bool hidden,
@@ skipping 17 matching lines @@
       sudden_termination_allowed_(false),
       render_view_termination_status_(base::TERMINATION_STATUS_STILL_RUNNING),
       virtual_keyboard_requested_(false),
       is_focused_element_editable_(false),
       updating_web_preferences_(false),
       render_view_ready_on_process_launch_(false),
       weak_factory_(this) {
   DCHECK(instance_.get());
   CHECK(delegate_);  // http://crbug.com/82827
 
+  GetWidget()->set_owner_delegate(this);
+
   GetProcess()->AddObserver(this);
   GetProcess()->EnableSendQueue();
 
   if (ResourceDispatcherHostImpl::Get()) {
     bool has_active_audio = false;
     if (has_initialized_audio_host) {
       scoped_refptr<AudioRendererHost> arh =
           static_cast<RenderProcessHostImpl*>(GetProcess())
               ->audio_renderer_host();
       if (arh.get())
@@ skipping 750 matching lines @@
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::Bind(&ResourceDispatcherHostImpl::OnRenderViewHostWasShown,
                    base::Unretained(ResourceDispatcherHostImpl::Get()),
                    GetProcess()->GetID(), GetRoutingID()));
   }
 
   RenderWidgetHostImpl::WasShown(latency_info);
 }
 
-bool RenderViewHostImpl::IsRenderView() const {
-  return true;
-}
-
 void RenderViewHostImpl::CreateNewWindow(
     int route_id,
     int main_frame_route_id,
     const ViewHostMsg_CreateWindow_Params& params,
     SessionStorageNamespace* session_storage_namespace) {
   ViewHostMsg_CreateWindow_Params validated_params(params);
   GetProcess()->FilterURL(false, &validated_params.target_url);
   GetProcess()->FilterURL(false, &validated_params.opener_url);
   GetProcess()->FilterURL(true, &validated_params.opener_security_origin);
 
@@ skipping 438 matching lines @@
   } else {
     render_view_ready_on_process_launch_ = true;
   }
 }
 
 void RenderViewHostImpl::RenderViewReady() {
   delegate_->RenderViewReady(this);
 }
 
 }  // namespace content