Make RenderFrameHostImpl::GetRenderWidgetHost() always return an object

content/browser/frame_host/render_frame_host_impl.cc

Index: content/browser/frame_host/render_frame_host_impl.cc
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index 6bb8df43f18c52bd7d062cfe36f7465a7172d4f8..70687b539f64930603e077a192aab22fe4fcb21d 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -457,17 +457,23 @@ ServiceRegistry* RenderFrameHostImpl::GetServiceRegistry() {
 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;
@@ -1177,14 +1183,10 @@ void RenderFrameHostImpl::OnUpdateState(const PageState& 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.

   }
 
@@ -1192,6 +1194,14 @@ RenderWidgetHostView* RenderFrameHostImpl::GetView() {
   return nullptr;
 }
 
+bool RenderFrameHostImpl::IsLocalRoot() {
+  return !!render_widget_host_;
+}
+
+RenderWidgetHostView* RenderFrameHostImpl::GetView() {
+  return GetRenderWidgetHost()->GetView();
+}
+
 GlobalFrameRoutingId RenderFrameHostImpl::GetGlobalFrameRoutingId() {
   return GlobalFrameRoutingId(GetProcess()->GetID(), GetRoutingID());
 }
@@ -2379,10 +2389,7 @@ void RenderFrameHostImpl::InvalidateMojoConnection() {
 }
 
 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()));