Don't route a non-frame widget's keyboard event to main frame widget.

Don't route a non-frame widget's keyboard event to main frame widget.

After keyboard event routing was updated for out-of-process iframes
(https://codereview.chromium.org/1405293002/), we started routing all
events to the main frame when site isolation is disabled.  This works
fine for things like popup menu widgets, since Blink contains a
codepath for forwarding input events to an active popup widget, if it
exists (see m_pagePopup check in WebViewImpl::handleKeyEvent()) -- so
both before and after, we reach the handling in
WebPagePopupImpl::handleKeyEvent.  Unfortunately, that forwarding
doesn't get hit for fullscreen widgets create for Pepper Flash, so
those widgets fail to receive keyboard events properly.

This CL changes keyboard event routing so that it won't try to look up
focused frames when keyboard events arrive at anything other than the
main frame widget.  I.e., all non-frame widgets like popup menus will
handle key events directly, as was the case before
https://codereview.chromium.org/1405293002/.

BUG=555524
Committed: https://crrev.com/c4cbacbea47af5c139bb8ed7a51e8d22ff2a97fd
Cr-Commit-Position: refs/heads/master@{#360980}

[alexmos, 2015-11-20 02:39:14]

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

[alexmos, 2015-11-20 02:39:15]

Charlie and Ken, what do you think about this fix?

Basically, I realize that I can't default to using the main frame's RWH in
GetFocusedRenderWidgetHost() -- see  https://crbug.com/555524#c4.  Popup menu
widgets still work with this, but fullscreen Flash widgets don't.  So, I'm
falling back to only looking up the focused frame's widget if the event arrived
at the main frame's widget.  To do that, I passed in the event's source RWH, and
renamed the keyboard routing function, since it wasn't really getting the
focused frame's RWH anymore.  

Alternatively, I could leave GetFocusedRenderWidgetHost alone (by returning
nullptr for the problematic cases, so that call sites don't use the returned
RWH), but I didn't like it as much, since the function's intent became
confusing, plus it wouldn't fix the issue for sending popup menu events directly
to their widgets.

Do you think I can get away without a test for now, to land this sooner?  Rather
than build a fullscreen Flash test (which looks complicated), I wanted to have a
more generic test for non-frame widgets, but I can't do that until we fix
https://crbug.com/549728.   I'm hoping I can add a good test as part of tackling
that bug (I actually would like to take it on soon if no one else gets to it).

https://codereview.chromium.org/1463153002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/1463153002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1602: return
GetMainFrame()->GetRenderWidgetHost();
This was the problematic case in the bug.

https://codereview.chromium.org/1463153002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1463153002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1608: return rwh;
This should ensure that the bug is fixed with --site-per-process as well.  This
also restores the old codepath that was used for routing keyboard events popup
menu widgets (see explanation in CL description).

[Charlie Reis, 2015-11-20 19:24:12]

Thanks!  LGTM with some naming questions.

https://codereview.chromium.org/1463153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1463153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_delegate.h:121: // When a
RenderWidgetHost |rwh| receives a keyboard event, this function
I'm trying to follow what it means for a RWH to receive a keyboard event.  I'm
not very familiar with event routing, but so I'd assumed the keyboard events are
received by the RWHV, which has to decide which RWH should consume them.  Are
they actually received by the RWH?

I think it might also help to have a more explicit parameter name than "rhw,"
like "receiving_widget" or something to that effect?

https://codereview.chromium.org/1463153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_delegate.h:127: virtual
RenderWidgetHostImpl* GetRenderWidgetHostForKeyboardEvent(
I kind of liked that the previous name wasn't specific to keyboard events, since
it seemed like it could be useful for other cases that depend on focused frame. 
But maybe there aren't other cases that will need this?

(It seems like GetFocusedRenderWidgetHost(receiving_widget) might still be an
acceptable name, if it is meant to return the focused widget associated with the
receiving widget.  I'll defer to Ken here, though.)

https://codereview.chromium.org/1463153002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/1463153002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1602: return
GetMainFrame()->GetRenderWidgetHost();
On 2015/11/20 02:39:15, alexmos wrote:
> This was the problematic case in the bug.

Acknowledged.

https://codereview.chromium.org/1463153002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1463153002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:1608: return rwh;
On 2015/11/20 02:39:15, alexmos wrote:
> This should ensure that the bug is fixed with --site-per-process as well. 
This
> also restores the old codepath that was used for routing keyboard events popup
> menu widgets (see explanation in CL description).

Acknowledged.

[alexmos, 2015-11-20 23:27:41]

Thanks!

https://codereview.chromium.org/1463153002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1463153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_delegate.h:121: // When a
RenderWidgetHost |rwh| receives a keyboard event, this function
On 2015/11/20 19:24:11, Charlie Reis wrote:
> I'm trying to follow what it means for a RWH to receive a keyboard event.  I'm
> not very familiar with event routing, but so I'd assumed the keyboard events
are
> received by the RWHV, which has to decide which RWH should consume them.  Are
> they actually received by the RWH?
> 
> I think it might also help to have a more explicit parameter name than "rhw,"
> like "receiving_widget" or something to that effect?

Yes, my understanding is also that RWHV receives the keyboard input event. 
Before my changes, it always passed it along to its RWH, but now it uses this
function to decide which RWH should ultimately receive it.  I rewrote this
comment - PTAL.

I like renaming the parameter to receiving_widget -- done.

https://codereview.chromium.org/1463153002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_delegate.h:127: virtual
RenderWidgetHostImpl* GetRenderWidgetHostForKeyboardEvent(
On 2015/11/20 19:24:12, Charlie Reis wrote:
> I kind of liked that the previous name wasn't specific to keyboard events,
since
> it seemed like it could be useful for other cases that depend on focused
frame. 
> But maybe there aren't other cases that will need this?

> (It seems like GetFocusedRenderWidgetHost(receiving_widget) might still be an
> acceptable name, if it is meant to return the focused widget associated with
the
> receiving widget.  I'll defer to Ken here, though.)

I liked the more generic name too.  I changed it back, I also think it's
acceptable now that the parameter is named as receiving_widget.

[Charlie Reis, 2015-11-20 23:31:38]

I like it.  LGTM.

[alexmos, 2015-11-21 00:12:40]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2015-11-21 00:13:24]

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

[commit-bot: I haz the power, 2015-11-21 01:29:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-11-21 01:30:45]

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