Make RenderFrameHostImpl::GetRenderWidgetHost() always return an object

Make RenderFrameHostImpl::GetRenderWidgetHost() always return an object

Using GetRenderWidgetHost as an accessor for the RFHI's data member is
error prone because it is easy to assume that a frame attached to the
frame tree will always have a widget to talk to, and also it is
inconsistent with RenderViewImpl::GetHost() which does not return
nullptr.

While it is generally preferable for features to interact with a
RenderWidgetHostView rather than a RenderWidgetHost for the purposes
of input and renderering interactions, where possible, RenderWidgetHost
availability is sometimes necessary and GetView() can unavoidably
return nullptr in some cases (for instance, if the renderer just
crashed).

This CL also does some related cleanup.

BUG=455245
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/0e8dc209f5e4a6140e43551de0e036324c68a383
Cr-Commit-Position: refs/heads/master@{#396751}

[kenrb, 2016-05-27 21:35:50]

Description was changed from

==========
Make RenderFrameHostImpl::GetRenderWidgetHost() always return an object

Using GetRenderWidgetHost as an accessor for the RFHI's data member is
error prone because it is easy to assume that a frame attached to the
frame tree will always have a widget to talk to, and also it is
inconsistent with RenderViewImpl::GetHost() which does not return
nullptr.

While it is generally preferable for features to interact with a
RenderWidgetHostView rather than a RenderWidgetHost for the purposes
of input and renderering interactions, where possible, RenderWidgetHost
availability is sometimes necessary and GetView() can unavoidably
return nullptr in some cases (for instance, if the renderer just
crashed).

This CL also does some related cleanup.

BUG=455245
==========

to

==========
Make RenderFrameHostImpl::GetRenderWidgetHost() always return an object

Using GetRenderWidgetHost as an accessor for the RFHI's data member is
error prone because it is easy to assume that a frame attached to the
frame tree will always have a widget to talk to, and also it is
inconsistent with RenderViewImpl::GetHost() which does not return
nullptr.

While it is generally preferable for features to interact with a
RenderWidgetHostView rather than a RenderWidgetHost for the purposes
of input and renderering interactions, where possible, RenderWidgetHost
availability is sometimes necessary and GetView() can unavoidably
return nullptr in some cases (for instance, if the renderer just
crashed).

This CL also does some related cleanup.

BUG=455245
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2016-05-27 22:01:15]

kenrb@chromium.org changed reviewers:
+ creis@chromium.org

[kenrb, 2016-05-27 22:01:16]

creis: PTAL?

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

Thanks!  LGTM with a few minor comments and suggestions.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/accessi...
File content/browser/accessibility/dump_accessibility_browsertest_base.cc
(right):

https://codereview.chromium.org/2023453003/diff/40001/content/browser/accessi...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:273:
static_cast<RenderWidgetHostViewBase*>(current_frame_host->GetView());
Sanity check: Before, this code skipped frames that weren't local frame roots,
since those had null widgets.  That seemed to work out well, since we would only
do the surface check below once per RWHV.  Now we'll do it once per frame, which
might be several times per RWHV.

That doesn't seem like a big problem to me (since the subsequent waits should
return right away), but maybe it's worth a comment in case someone later wants
to simplify this to iterate over the RWHVs instead of the frames.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:280: //
Out-of-process iframes nested underneath this one implicitly have their
This comment sounds like it could mean "subframes that share the same widget,"
but it actually means "local roots nested underneath this frame," right?  Can
you update the comment?

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:464: // unattached frame
and we could omit this check.
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.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:469: frame =
static_cast<RenderFrameHostImpl*>(frame->GetParent());
nit: Static cast is unnecessary, because RFHI::GetParent() returns a RFHI
(thanks to covariant return types).

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1190: frame =
static_cast<RenderFrameHostImpl*>(frame->GetParent());
nit: Static cast is unnecessary here as well.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:255: // Returns the
RenderWidgetHostImpl attached to this frame or the nearest
nit: owned by this frame or its nearest

(Is that accurate enough?  I realize the root widget is technically still owned
by the RVH, but it kind of seems like every frame is attached to a widget.)

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:259: //
GetRenderWidgetHost() will not return a nullptr.
nit: nullptr after a renderer crash.

(Helpful to give more context about when GetView is null.)

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:262: // frame tree.
nit: if this RenderFrameHost has no RenderWidgetHost in any of its ancestors,
which could happen if it has been detached from the frame tree.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:270: bool IsLocalRoot();
nit: Let's move this above GetRenderWidgetHost(), since it's a useful concept
for understanding the comment on GetRenderWidgetHost().

This can also be is_local_root() and inlined into the .h file, since it's
trivial and not virtual.

[kenrb, 2016-05-30 18:25:02]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2016-05-30 18:25:02]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2023453003/#ps60001
(title: "Addressing creis' comments")

[commit-bot: I haz the power, 2016-05-30 18:25:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2023453003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2023453003/60001

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

https://codereview.chromium.org/2023453003/diff/40001/content/browser/accessi...
File content/browser/accessibility/dump_accessibility_browsertest_base.cc
(right):

https://codereview.chromium.org/2023453003/diff/40001/content/browser/accessi...
content/browser/accessibility/dump_accessibility_browsertest_base.cc:273:
static_cast<RenderWidgetHostViewBase*>(current_frame_host->GetView());
On 2016/05/27 22:50:28, Charlie Reis wrote:
> Sanity check: Before, this code skipped frames that weren't local frame roots,
> since those had null widgets.  That seemed to work out well, since we would
only
> do the surface check below once per RWHV.  Now we'll do it once per frame,
which
> might be several times per RWHV.
> 
> That doesn't seem like a big problem to me (since the subsequent waits should
> return right away), but maybe it's worth a comment in case someone later wants
> to simplify this to iterate over the RWHVs instead of the frames.

Good point, fixed it to match the old behavior.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.cc:280: //
Out-of-process iframes nested underneath this one implicitly have their
On 2016/05/27 22:50:28, Charlie Reis wrote:
> This comment sounds like it could mean "subframes that share the same widget,"
> but it actually means "local roots nested underneath this frame," right?  Can
> you update the comment?

Done.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:464: // unattached frame
and we could omit this check.
On 2016/05/27 22:50:28, Charlie Reis wrote:
> 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.

Done.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:469: frame =
static_cast<RenderFrameHostImpl*>(frame->GetParent());
On 2016/05/27 22:50:28, Charlie Reis wrote:
> nit: Static cast is unnecessary, because RFHI::GetParent() returns a RFHI
> (thanks to covariant return types).

Done.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1190: frame =
static_cast<RenderFrameHostImpl*>(frame->GetParent());
On 2016/05/27 22:50:28, Charlie Reis wrote:
> nit: Static cast is unnecessary here as well.

Done.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:255: // Returns the
RenderWidgetHostImpl attached to this frame or the nearest
On 2016/05/27 22:50:28, Charlie Reis wrote:
> nit: owned by this frame or its nearest
> 
> (Is that accurate enough?  I realize the root widget is technically still
owned
> by the RVH, but it kind of seems like every frame is attached to a widget.)

I think it's accurate enough. Even though technically the root frame doesn't
(yet) own its widget, it still has its render_widget_host_ pointer set as if it
does.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:259: //
GetRenderWidgetHost() will not return a nullptr.
On 2016/05/27 22:50:28, Charlie Reis wrote:
> nit: nullptr after a renderer crash.
> 
> (Helpful to give more context about when GetView is null.)

Done.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:262: // frame tree.
On 2016/05/27 22:50:28, Charlie Reis wrote:
> nit: if this RenderFrameHost has no RenderWidgetHost in any of its ancestors,
> which could happen if it has been detached from the frame tree.

Done.

https://codereview.chromium.org/2023453003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:270: bool IsLocalRoot();
On 2016/05/27 22:50:28, Charlie Reis wrote:
> nit: Let's move this above GetRenderWidgetHost(), since it's a useful concept
> for understanding the comment on GetRenderWidgetHost().
> 
> This can also be is_local_root() and inlined into the .h file, since it's
> trivial and not virtual.

Done.

[commit-bot: I haz the power, 2016-05-30 19:51:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-30 19:53:13]

Description was changed from

==========
Make RenderFrameHostImpl::GetRenderWidgetHost() always return an object

Using GetRenderWidgetHost as an accessor for the RFHI's data member is
error prone because it is easy to assume that a frame attached to the
frame tree will always have a widget to talk to, and also it is
inconsistent with RenderViewImpl::GetHost() which does not return
nullptr.

While it is generally preferable for features to interact with a
RenderWidgetHostView rather than a RenderWidgetHost for the purposes
of input and renderering interactions, where possible, RenderWidgetHost
availability is sometimes necessary and GetView() can unavoidably
return nullptr in some cases (for instance, if the renderer just
crashed).

This CL also does some related cleanup.

BUG=455245
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make RenderFrameHostImpl::GetRenderWidgetHost() always return an object

Using GetRenderWidgetHost as an accessor for the RFHI's data member is
error prone because it is easy to assume that a frame attached to the
frame tree will always have a widget to talk to, and also it is
inconsistent with RenderViewImpl::GetHost() which does not return
nullptr.

While it is generally preferable for features to interact with a
RenderWidgetHostView rather than a RenderWidgetHost for the purposes
of input and renderering interactions, where possible, RenderWidgetHost
availability is sometimes necessary and GetView() can unavoidably
return nullptr in some cases (for instance, if the renderer just
crashed).

This CL also does some related cleanup.

BUG=455245
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/0e8dc209f5e4a6140e43551de0e036324c68a383
Cr-Commit-Position: refs/heads/master@{#396751}
==========

[commit-bot: I haz the power, 2016-05-30 19:53:15]

Patchset 4 (id:??) landed as
https://crrev.com/0e8dc209f5e4a6140e43551de0e036324c68a383
Cr-Commit-Position: refs/heads/master@{#396751}