OOPIF: Route keyboard events to focused frame in the browser process.

OOPIF: Route keyboard events to focused frame in the browser process.

Previously, keyboard events were always routed to the top frame's
RenderWidgetHost.  With OOPIF, the top frame then forwarded the event
to an appropriate subframe renderer if needed.  The extra hop is
inefficient and leaks information; moreover, the forwarding broke when
mouse event hit-testing in the browser process was introduced.

This CL adds logic to send keyboard events directly to the right
RenderWidgetHost in the browser process, using the currently focused
frame which is already tracked in each FrameTree.  This should be
supported for Aura, Mac, and Android.

BUG=530663, 339659
Committed: https://crrev.com/e2a1799925ea027bd71ea9367280ddfb4403987a
Cr-Commit-Position: refs/heads/master@{#357011}

[alexmos, 2015-10-23 22:41:50]

Description was changed from

==========
[DRAFT] One-hop keyboard routing

BUG=
==========

to

==========
OOPIF: Route keyboard events to focused frame in the browser process.

BUG=530663, 339659
==========

[alexmos, 2015-10-23 23:40:16]

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

[alexmos, 2015-10-23 23:40:16]

Ken, can you please take a first look at this?  Overall, this is very much along
the lines of Charlie's comment on
https://code.google.com/p/chromium/issues/detail?id=339659#c5.  Some
questions/comments below.

https://codereview.chromium.org/1405293002/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1405293002/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2762:
target_host->Send(new InputMsg_SetEditCommandsForNextKeyEvent(
These are currently handled on RenderView only, so this would currently not do
anything in the OOPIF case until we add support for WebFrameWidgets to handle
this.  Same for the Mac case (which sends quite a few such messages).

https://codereview.chromium.org/1405293002/diff/160001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1405293002/diff/160001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:104: void
RouteKeyboardEvent(WebContentsImpl* web_contents, base::char16 key) {
I wanted to just change SimulateKeyPress in browser_test_utils to route events
to the focused RWH, so that I could use that instead of this function, but that
would require making GetFocusedRenderWidgetHost public on WebContents.  Would
that be preferable to this approach?

https://codereview.chromium.org/1405293002/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1405293002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1593: RenderWidgetHostImpl*
WebContentsImpl::GetFocusedRenderWidgetHost() {
I wasn't sure whether this logic should go into RenderWidgetHostInputEventRouter
instead.  I kept it here for now, because RenderWidgetHostInputEventRouter
seemed more oriented toward mouse events and hittesting, and because it would've
required a frame_host DEPS exception, since it's in renderer_host but would need
to access FrameTree and FrameTreeNode for this code.

https://codereview.chromium.org/1405293002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1602: return target_rwh;
I thought about whether this should check AreCrossProcessFramesPossible and
return early if not, similarly to what's done for mouse events.  But the code
here should work regardless and just always get the root frame's RWH without
--site-per-process, so I thought that there isn't much benefit to adding the
check, other than getting to the root RWH a bit faster.

[kenrb, 2015-10-28 21:07:05]

Looks good, sorry it took me a while to get to this. A couple of comments but
nothing major.

https://codereview.chromium.org/1405293002/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1405293002/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1680: if
(host_) {
It's cleaner to invert this into an early return
if (!host_)
  return;

https://codereview.chromium.org/1405293002/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1405293002/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2762:
target_host->Send(new InputMsg_SetEditCommandsForNextKeyEvent(
On 2015/10/23 23:40:16, alexmos wrote:
> These are currently handled on RenderView only, so this would currently not do
> anything in the OOPIF case until we add support for WebFrameWidgets to handle
> this.  Same for the Mac case (which sends quite a few such messages).

Yeah I guess this needs some refactoring in the renderer.

https://codereview.chromium.org/1405293002/diff/160001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1405293002/diff/160001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:104: void
RouteKeyboardEvent(WebContentsImpl* web_contents, base::char16 key) {
On 2015/10/23 23:40:16, alexmos wrote:
> I wanted to just change SimulateKeyPress in browser_test_utils to route events
> to the focused RWH, so that I could use that instead of this function, but
that
> would require making GetFocusedRenderWidgetHost public on WebContents.  Would
> that be preferable to this approach?

I don't think you want it in the content API, so it shouldn't be on WebContents,
but doesn't browser_test_utils have access to WebContentsImpl?

https://codereview.chromium.org/1405293002/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1405293002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1593: RenderWidgetHostImpl*
WebContentsImpl::GetFocusedRenderWidgetHost() {
On 2015/10/23 23:40:16, alexmos wrote:
> I wasn't sure whether this logic should go into
RenderWidgetHostInputEventRouter
> instead.  I kept it here for now, because RenderWidgetHostInputEventRouter
> seemed more oriented toward mouse events and hittesting, and because it
would've
> required a frame_host DEPS exception, since it's in renderer_host but would
need
> to access FrameTree and FrameTreeNode for this code.

I was thinking about this also, but I can't think of a good reason to try to
bring them together. This is fine.

https://codereview.chromium.org/1405293002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1602: return target_rwh;
On 2015/10/23 23:40:16, alexmos wrote:
> I thought about whether this should check AreCrossProcessFramesPossible and
> return early if not, similarly to what's done for mouse events.  But the code
> here should work regardless and just always get the root frame's RWH without
> --site-per-process, so I thought that there isn't much benefit to adding the
> check, other than getting to the root RWH a bit faster.

Will this affect browser plugin? Currently that still does input event
forwarding, so if the code here short-circuits it then it might break something.
That was one of the reasons I disabled browser process hit testing for
non-site-per-process (the other being that I didn't want bugs to cause trunk
regressions).

[alexmos, 2015-10-29 05:26:29]

Thanks for reviewing!

https://codereview.chromium.org/1405293002/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/1405293002/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1680: if
(host_) {
On 2015/10/28 21:07:05, kenrb wrote:
> It's cleaner to invert this into an early return
> if (!host_)
>   return;

Done.

https://codereview.chromium.org/1405293002/diff/160001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1405293002/diff/160001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:104: void
RouteKeyboardEvent(WebContentsImpl* web_contents, base::char16 key) {
On 2015/10/28 21:07:05, kenrb wrote:
> On 2015/10/23 23:40:16, alexmos wrote:
> > I wanted to just change SimulateKeyPress in browser_test_utils to route
events
> > to the focused RWH, so that I could use that instead of this function, but
> that
> > would require making GetFocusedRenderWidgetHost public on WebContents. 
Would
> > that be preferable to this approach?
> 
> I don't think you want it in the content API, so it shouldn't be on
WebContents,
> but doesn't browser_test_utils have access to WebContentsImpl?

You're right, I got confused and thought that it didn't.  I modified
SimulateKeyPress to route events to the focused RWH and got rid of this helper.

https://codereview.chromium.org/1405293002/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1405293002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1602: return target_rwh;
On 2015/10/28 21:07:05, kenrb wrote:
> On 2015/10/23 23:40:16, alexmos wrote:
> > I thought about whether this should check AreCrossProcessFramesPossible and
> > return early if not, similarly to what's done for mouse events.  But the
code
> > here should work regardless and just always get the root frame's RWH without
> > --site-per-process, so I thought that there isn't much benefit to adding the
> > check, other than getting to the root RWH a bit faster.
> 
> Will this affect browser plugin? Currently that still does input event
> forwarding, so if the code here short-circuits it then it might break
something.
> That was one of the reasons I disabled browser process hit testing for
> non-site-per-process (the other being that I didn't want bugs to cause trunk
> regressions).

I've tried it on a sample app with <webview>, and keyboard events still worked,
both with and without --site-per-process.  I've dug into this a little, and so
far my understanding is that it shouldn't be affected.  The guest will not show
up in the FrameTree of the embedder WebContents, so we will still send the
keyboard event to the embedder's main frame RWH.  On the renderer side, the
embedder forwards the event for the guest back to browser process from
BrowserPlugin::handleInputEvent, which will reach
RenderWidgetHostViewGuest::OnHandleInputEvent.  Since I didn't modify any
keyboard forwarding there, the event should be forwarded directly to the guest's
RWH as before.

That said, I'm noticing that two WebViewTests related to interstitials are
failing on the site isolation bot with the latest patchset (they passed in
previous patchset).  I'm looking into those.

For the regular cases, my reasoning was that without
AreCrossProcessFramesPossible, calling GetView() on any frame in a FrameTree
will just walk up the parent chain to the main frame and return its RVHI/RWHI
(since it's the only frame that has a render_widget_host_).  I guess there's no
reason to do this walk without AreCrossProcessFramesPossible, so I added an
early escape to return the main frame's RWH.

[kenrb, 2015-10-29 14:40:57]

lgtm

[alexmos, 2015-10-29 17:19:00]

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

[alexmos, 2015-10-29 17:19:00]

Charlie, can you please do an owners review?

I think the two WebViewTest failures on linux_site_isolation are not related to
this, but instead caused by https://crbug.com/548912, which also affects the CL
this depends on (https://codereview.chromium.org/1406103005).  I'll be working
on that in parallel.

[Charlie Reis, 2015-10-29 18:14:02]

LGTM!

https://codereview.chromium.org/1405293002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1405293002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2761: //
RenderWidgetHosts for OOPIF.
Interesting-- is this related to the Edit stack work?  Looks like we don't have
a bug on file for that yet.  Can you file one and list it here?

https://codereview.chromium.org/1405293002/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1405293002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1602:
focused_frame->current_frame_host()->GetView()->GetRenderWidgetHost());
Ken, this is why I wanted RenderFrameHostImpl::GetRenderWidgetHost() to return
the widget associated with the current frame, and not nullptr in some cases.  We
end up with contortions like this, where we have to call
GetView()->GetRenderWidgetHost() and not GetRenderWidgetHost() directly.

I'll start a thread on site-isolation-dev@ to discuss this further.

[alexmos, 2015-10-29 22:22:08]

Thanks!

https://codereview.chromium.org/1405293002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1405293002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:2761: //
RenderWidgetHosts for OOPIF.
On 2015/10/29 18:14:02, Charlie Reis wrote:
> Interesting-- is this related to the Edit stack work?  Looks like we don't
have
> a bug on file for that yet.  Can you file one and list it here?

Yes, I filed https://crbug.com/549334 and referenced here.

https://codereview.chromium.org/1405293002/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1405293002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1602:
focused_frame->current_frame_host()->GetView()->GetRenderWidgetHost());
On 2015/10/29 18:14:02, Charlie Reis wrote:
> Ken, this is why I wanted RenderFrameHostImpl::GetRenderWidgetHost() to return
> the widget associated with the current frame, and not nullptr in some cases. 
We
> end up with contortions like this, where we have to call
> GetView()->GetRenderWidgetHost() and not GetRenderWidgetHost() directly.
> 
> I'll start a thread on site-isolation-dev@ to discuss this further.

+1 to having GetRenderWidgetHost() return the widget for the current frame if
that's possible.

[alexmos, 2015-10-29 23:43:44]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2015-10-29 23:43:45]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1405293002/#ps200001
(title: "Add edit stack bug reference.")

[commit-bot: I haz the power, 2015-10-29 23:44:13]

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

[kenrb, 2015-10-29 23:54:40]

https://codereview.chromium.org/1405293002/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1405293002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1602:
focused_frame->current_frame_host()->GetView()->GetRenderWidgetHost());
On 2015/10/29 22:22:08, alexmos wrote:
> On 2015/10/29 18:14:02, Charlie Reis wrote:
> > Ken, this is why I wanted RenderFrameHostImpl::GetRenderWidgetHost() to
return
> > the widget associated with the current frame, and not nullptr in some cases.

> We
> > end up with contortions like this, where we have to call
> > GetView()->GetRenderWidgetHost() and not GetRenderWidgetHost() directly.
> > 
> > I'll start a thread on site-isolation-dev@ to discuss this further.
> 
> +1 to having GetRenderWidgetHost() return the widget for the current frame if
> that's possible.

We had discussed removing RenderFrameHostImpl::GetRenderWidgetHost() before,
since its usage appeared to be mostly in tests, on the basis that code that
doesn't already have a pointer to a RenderWidgetHostImpl should probably be
talking to the RenderWidgetHostView. I see that a couple more
GetRenderWidgetHost() calls have appeared since we had that discussion.

In this case, GetFocusedRenderWidgetHost() is being called by a
RenderWidgetHostView that already has a pointer to a RenderWidgetHost, but wants
a different one. If we ever want to add ChildFrame-specific code for handling
keyboard events, this method will need to change to
GetFocusedRenderWidgetHostView(), which would be simpler here but would mean a
new method getting added to RenderWidgetHostViewBase (similar to how mouse event
routing works).

I didn't think of this when I was looking at the CL earlier. It is fine, but
thanks for pointing it out. It is something we might want to revisit for sure.

[commit-bot: I haz the power, 2015-10-30 00:40:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 00:40:44]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alexmos, 2015-10-30 00:43:31]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-10-30 00:44:48]

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

[commit-bot: I haz the power, 2015-10-30 01:27:02]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-10-30 01:28:00]

Patchset 11 (id:??) landed as
https://crrev.com/e2a1799925ea027bd71ea9367280ddfb4403987a
Cr-Commit-Position: refs/heads/master@{#357011}

[perkj_chrome, 2015-10-30 11:24:43]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/1407173005/ by perkj@chromium.org.

The reason for reverting is:
SitePerProcessBrowserTest.SubframeKeyboardEventRouting is flaky on Windows 7.


http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=...

http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%281%29/buil....