Make RenderFrameHostImpl::GetRenderWidgetHost() always return an object

content/browser/frame_host/render_frame_host_impl.cc

 // Copyright 2013 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/frame_host/render_frame_host_impl.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 439 matching lines @@
   return render_view_host_;
 }
 
 ServiceRegistry* RenderFrameHostImpl::GetServiceRegistry() {
   return service_registry_.get();
 }
 
 blink::WebPageVisibilityState RenderFrameHostImpl::GetVisibilityState() {
   // Works around the crashes seen in https://crbug.com/501863, where the
   // active WebContents from a browser iterator may contain a render frame
-  // detached from the frame tree.
-  RenderWidgetHostView* view = RenderFrameHostImpl::GetView();
-  if (!view || !view->GetRenderWidgetHost())
+  // detached from the frame tree. This tries to find a RenderWidgetHost
+  // attached to an ancestor frame, and defaults to visibility hidden if
+  // it fails.
+  // TODO(yfriedman, peter): Ideally this would never be called on an
+  // unattached frame and we could omit this check.

[Charlie Reis, 2016/05/27 22:50:28]

Can you file a bug for removing this whole workaround and list it in the TODO? 
I agree with the comments on https://codereview.chromium.org/1355513003 that
this isn't something we should keep forever, especially now that it duplicates
the ancestor walk.

[kenrb, 2016/05/30 19:39:53]

Done.

+  RenderFrameHostImpl* frame = this;
+  while (frame) {
+    if (frame->render_widget_host_)
+      break;
+    frame = static_cast<RenderFrameHostImpl*>(frame->GetParent());

[Charlie Reis, 2016/05/27 22:50:28]

nit: Static cast is unnecessary, because RFHI::GetParent() returns a RFHI
(thanks to covariant return types).

[kenrb, 2016/05/30 19:39:53]

Done.

+  }
+  if (!frame)
     return blink::WebPageVisibilityStateHidden;
 
-  // TODO(mlamouri,kenrb): call GetRenderWidgetHost() directly when it stops
-  // returning nullptr in some cases. See https://crbug.com/455245.
   blink::WebPageVisibilityState visibility_state =
-      RenderWidgetHostImpl::From(view->GetRenderWidgetHost())->is_hidden()
-          ? blink::WebPageVisibilityStateHidden
-          : blink::WebPageVisibilityStateVisible;
+      GetRenderWidgetHost()->is_hidden() ? blink::WebPageVisibilityStateHidden
+                                         : blink::WebPageVisibilityStateVisible;
   GetContentClient()->browser()->OverridePageVisibilityState(this,
                                                              &visibility_state);
   return visibility_state;
 }
 
 bool RenderFrameHostImpl::Send(IPC::Message* message) {
   if (IPC_MESSAGE_ID_CLASS(message->type()) == InputMsgStart) {
     return render_view_host_->GetWidget()->input_router()->SendInput(
         base::WrapUnique(message));
   }
@@ skipping 689 matching lines @@
   if (!render_view_host_->CanAccessFilesOfPageState(state)) {
     bad_message::ReceivedBadMessage(
         GetProcess(), bad_message::RFH_CAN_ACCESS_FILES_OF_PAGE_STATE);
     return;
   }
 
   delegate_->UpdateStateForFrame(this, state);
 }
 
 RenderWidgetHostImpl* RenderFrameHostImpl::GetRenderWidgetHost() {
-  return render_widget_host_;
-}
-
-RenderWidgetHostView* RenderFrameHostImpl::GetView() {
   RenderFrameHostImpl* frame = this;
   while (frame) {
     if (frame->render_widget_host_)
-      return frame->render_widget_host_->GetView();
+      return frame->render_widget_host_;
     frame = static_cast<RenderFrameHostImpl*>(frame->GetParent());

[Charlie Reis, 2016/05/27 22:50:28]

nit: Static cast is unnecessary here as well.

[kenrb, 2016/05/30 19:39:54]

Done.

   }
 
   NOTREACHED();
   return nullptr;
 }
 
+bool RenderFrameHostImpl::IsLocalRoot() {
+  return !!render_widget_host_;
+}
+
+RenderWidgetHostView* RenderFrameHostImpl::GetView() {
+  return GetRenderWidgetHost()->GetView();
+}
+
 GlobalFrameRoutingId RenderFrameHostImpl::GetGlobalFrameRoutingId() {
   return GlobalFrameRoutingId(GetProcess()->GetID(), GetRoutingID());
 }
 
 int RenderFrameHostImpl::GetEnabledBindings() {
   return render_view_host_->GetEnabledBindings();
 }
 
 void RenderFrameHostImpl::SetNavigationHandle(
     std::unique_ptr<NavigationHandleImpl> navigation_handle) {
@@ skipping 1167 matching lines @@
   service_registry_android_.reset();
 #endif
 
   service_registry_.reset();
 
   // Disconnect with ImageDownloader Mojo service in RenderFrame.
   mojo_image_downloader_.reset();
 }
 
 bool RenderFrameHostImpl::IsFocused() {
-  // TODO(mlamouri,kenrb): call GetRenderWidgetHost() directly when it stops
-  // returning nullptr in some cases. See https://crbug.com/455245.
-  return RenderWidgetHostImpl::From(
-            GetView()->GetRenderWidgetHost())->is_focused() &&
+  return GetRenderWidgetHost()->is_focused() &&
          frame_tree_->GetFocusedFrame() &&
          (frame_tree_->GetFocusedFrame() == frame_tree_node() ||
           frame_tree_->GetFocusedFrame()->IsDescendantOf(frame_tree_node()));
 }
 
 bool RenderFrameHostImpl::UpdatePendingWebUI(const GURL& dest_url,
                                              int entry_bindings) {
   WebUI::TypeID new_web_ui_type =
       WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
           GetSiteInstance()->GetBrowserContext(), dest_url);
@@ skipping 453 matching lines @@
   // handler after it's destroyed so it can't run after the RFHI is destroyed.
   web_bluetooth_service_->SetClientConnectionErrorHandler(base::Bind(
       &RenderFrameHostImpl::DeleteWebBluetoothService, base::Unretained(this)));
 }
 
 void RenderFrameHostImpl::DeleteWebBluetoothService() {
   web_bluetooth_service_.reset();
 }
 
 }  // namespace content