Refactor the keyboard events handling code related to RenderViewHostDelegate:...

Refactor the keyboard events handling code related to RenderViewHostDelegate::View and TabContentsDelegate interfaces.

Significant changes made by this CL:
1. The keyboard event handling code has been moved from TabContentsView implementation classes into BrowserWindow implementation classes.
Please refer to this discussion thread: http://groups.google.com/group/chromium-dev/browse_thread/thread/e6e0b5cc105659b7/9953c4308bb0000c
This change makes the keyboard event flow comply with the relationship between TabContents/TabContentsView and TabContentsDelegate/BrowserWindow.
Besides it, the code is also simplified a lot, for example, the keyboard event handling code in chrome/browser/views/tab_contents/tab_contents_view_{gtk,win}.cc are now merged into one copy and moved into chrome/browser/views/frame/browser_view.cc.

2. A pre-handle phrase has been added into the keyboard event handling flow. A keyboard event will be first sent to the browser for pre-handling before being sent to the renderer. Then if the event was not handled by the renderer, it'll be sent to the browser again for post-handling.

3. The keyboard accelerator handling code of Windows and Linux ports have been optimized to get rid off extra command lookup.

4. The keyboard event message flow between the browser and the renderer is changed back to full async mode, all complex logics introduced by revision 29857 are removed.


BUG=24479, 26054, 26131, 28839
TEST=See bug reports.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34234

[James Su, 2009-11-17 12:41:31]

Hi,
  This CL is just ready for review and test. It's a little complicated, please
help me review it and give me your opinion. If possible, please also test it on
as much platforms as possible. Especially ChromeOS and ChromeFrame. I'm not sure
if it works on all platforms.

Regards
James Su

[Elliot Glaysher, 2009-11-17 23:29:59]

This patch is so large and complex, I'm completely lost.

What's the flow control supposed to look like?

[James Su, 2009-11-18 00:42:57]

* Before this CL, the flow of a key event is:

(system) -> RWHV -> RWH -> renderer -> RWH -> RVH ->
RVHD::View(TabContentsView*).

The key event is finally handled in the TabContentsView* object. For Linux port,
TabContentsViewGtk just sends the key event to BrowserWindowGtk for final
handling.

In order to short circuit some reserved accelerators instead of sending them to
the renderer, IsReservedAccelerator() method is added into RVHD::View and
TabContentsDelegate interfaces, and finally implemented in the Browser class. In
RWH this method will be called first for each key event, to see if it should be
sent to the renderer.


* After this CL, the flow of a key event is:

1. pre-handle phrase:
(system) -> RWHV -> RWH -> RVH -> RVHD::View(TabContentsView) ->
TabContentsDelegate(Browser) -> BrowserWindow.

2. post-handle phrase:
RWH -> renderer -> RWH -> RVH -> RVHD::View(TabContentsView) ->
TabContentsDelegate(Browser) -> BrowserWindow.

The differences are:
a) Instead of calling RVHD::View::IsReservedAccelerator() to determine if a key
event should be sent to the renderer, the key event will first be dispatched
upwards with |pre_handle| == true for pre-handling. If the key event was handled
during the pre-handle phrase, then RWH will not send it to the renderer anymore.
Whether and how it'll be handled during the pre-handle phrase is determined by
the implementation of TabContentsDelegate and BrowserWindow, so that it can be
optimized rather than looking up the accelerator table each time (what the old
IsReservedAccelerator() does).
If a key event was not handled during the pre-handle phrase, it'll be dispatched
to the renderer as normal.

b) The real key event handling code is moved into BrowserWindow implementations
from TabContentsView* classes to comply with the relationship between
TabContents/TabContentsView and TabContentsDelegate. Because a key event will
only be handled by the implementations of TabContentsDelegate interface. For
normal browser, it'll be handled as an accelerator of the BrowserWindow object.
For other implementations, such as ExternalTabContainer, the key events may be
handled in different way.

Regards
James Su

On 2009/11/17 23:29:59, Elliot Glaysher wrote:
> This patch is so large and complex, I'm completely lost.
> 
> What's the flow control supposed to look like?

[Elliot Glaysher, 2009-11-18 23:20:06]

So let me make sure I have this straight:

Instead of just handling the key in ForwardKeyEvent() if the key event is one of
our reserved keys, we instead plumb from there to BrowserWindowXXX, which will
(immediately execute it if it's a something+tab command) or force the browser to
ignore commands and record what command the Browser thought was called and then
will try to dispatch that key. We now have the command ID. We need to do this
because the user could have remapped accelerators.

Then, for certain commands, we execute immediately, but for some we don't
because there might be emacs like keybindings and we store all this data in
BrowserWindowXXX as state. (Aside: How does this deal with multiple keys that
call pending commands really quickly, i.e if the renderer is blocked for a
while?)

Things then proceed as they have.

Comment: You are adding way too much global state to these objects. I think
there are too many assumptions that one key event will completely finish before
the next one starts?  Have you considered moving the state to something like
NativeWebKeyboardEvent which is already a little bundle of state passed around?
It should also aid testability?

http://codereview.chromium.org/400012/diff/5015/4048
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/400012/diff/5015/4048#newcode1192
Line 1192: // |browser_| object.
Is there any other way to achieve this? This seems really fragile.

[jam, 2009-11-19 01:07:26]

I'm not qualified to review this code.  You should definitely get Ben or Brett
to review this.

[James Su, 2009-11-19 04:12:52]

Hi,
  I just updated the CL to address the problem you mentioned below.

- James Su

On 2009/11/18 23:20:06, Elliot Glaysher wrote:
> So let me make sure I have this straight:
> 
> Instead of just handling the key in ForwardKeyEvent() if the key event is one
of
> our reserved keys, we instead plumb from there to BrowserWindowXXX, which will
> (immediately execute it if it's a something+tab command) or force the browser
to
> ignore commands and record what command the Browser thought was called and
then
> will try to dispatch that key. We now have the command ID. We need to do this
> because the user could have remapped accelerators.
> 
> Then, for certain commands, we execute immediately, but for some we don't
> because there might be emacs like keybindings and we store all this data in
> BrowserWindowXXX as state. (Aside: How does this deal with multiple keys that
> call pending commands really quickly, i.e if the renderer is blocked for a
> while?)
You are correct, it's a potential problem. I just removed the pending_* things
to avoid it.

> 
> Things then proceed as they have.
> 
> Comment: You are adding way too much global state to these objects. I think
> there are too many assumptions that one key event will completely finish
before
> the next one starts?  Have you considered moving the state to something like
> NativeWebKeyboardEvent which is already a little bundle of state passed
around?
> It should also aid testability?
> 
> http://codereview.chromium.org/400012/diff/5015/4048
> File chrome/browser/gtk/browser_window_gtk.cc (right):
> 
> http://codereview.chromium.org/400012/diff/5015/4048#newcode1192
> Line 1192: // |browser_| object.
> Is there any other way to achieve this? This seems really fragile.
It's the simplest approach I can think of to solve these two issues listed here.

[James Su, 2009-11-19 04:17:17]

Thanks. Just added them to Reviewers.

- James Su

On 2009/11/19 01:07:26, John Abd-El-Malek wrote:
> I'm not qualified to review this code.  You should definitely get Ben or Brett
> to review this.

[Elliot Glaysher, 2009-11-23 19:12:39]

ping brettw. I want your opinion on this patch.

[James Su, 2009-11-26 00:59:41]

ping, again.

Happy turkey day :-)

On 2009/11/23 19:12:39, Elliot Glaysher wrote:
> ping brettw. I want your opinion on this patch.

[James Su, 2009-11-26 01:26:43]

I may need to rework this CL after submitting CL 435002, to try to fix
http://crbug.com/28839 with this CL. Because they are all related. So please
hold on. Sorry for any inconvenience.

- James Su

On 2009/11/26 00:59:41, James Su wrote:
> ping, again.
> 
> Happy turkey day :-)
> 
> On 2009/11/23 19:12:39, Elliot Glaysher wrote:
> > ping brettw. I want your opinion on this patch.

[James Su, 2009-11-30 10:55:27]

Hi,
  This CL has been updated to implement the new approach described in
http://crbug.com/28839. Please help review it.
  If you think this CL is ok, I'd like to submit this CL directly and discard
http://codereview.chromium.org/435002, which has been depreciated by this CL.

- James Su

[darin (slow to review), 2009-12-02 08:01:47]

I wasn't able to finish reviewing all of this patch, but I didn't want to sit on
the comments I already made.  Here's a start.  I still need to review
browser/renderer_host, browser/tab_contents and browser/views.

http://codereview.chromium.org/400012/diff/19001/18014
File chrome/browser/blocked_popup_container_interactive_uitest.cc (right):

http://codereview.chromium.org/400012/diff/19001/18014#newcode286
chrome/browser/blocked_popup_container_interactive_uitest.cc:286: base::VKEY_F4,
views::Event::EF_CONTROL_DOWN));
what's the reason for switching this keyboard shortcut?

http://codereview.chromium.org/400012/diff/19001/18006
File chrome/browser/browser.h (right):

http://codereview.chromium.org/400012/diff/19001/18006#newcode474
chrome/browser/browser.h:474: bool IsReservedCommand(int id);
this name worksforme.

http://codereview.chromium.org/400012/diff/19001/18016
File chrome/browser/browser_window.h (right):

http://codereview.chromium.org/400012/diff/19001/18016#newcode271
chrome/browser/browser_window.h:271: // before sending it them to the renderer.
nit: spurious "them"?

http://codereview.chromium.org/400012/diff/19001/18016#newcode272
chrome/browser/browser_window.h:272: // This method is called from
Browser::PreHandleKeyboardEvent() method, which
in general, it is good to not write comments that just repeat what the code
does.  also, it is possible for Browser::PreHandleKeyboardEvent to be renamed or
relocated in the future, and this comment would likely not be updated if that
happened.

http://codereview.chromium.org/400012/diff/19001/18016#newcode282
chrome/browser/browser_window.h:282: // This method is called from
Browser::HandleKeyboardEvent() method, which
ditto

http://codereview.chromium.org/400012/diff/19001/18038
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/400012/diff/19001/18038#newcode483
chrome/browser/cocoa/browser_window_cocoa.mm:483:
(ChromeEventProcessingWindow*)window_;
nit: use C++ style casts?

http://codereview.chromium.org/400012/diff/19001/18003
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/400012/diff/19001/18003#newcode510
chrome/browser/extensions/extension_host.cc:510: event.windowsKeyCode ==
base::VKEY_ESCAPE && is_keyboard_shortcut)
why is it necessary to null check is_keyboard_shortcut?  why not just require
the caller to pass a non-null is_keyboard_shortcut?

http://codereview.chromium.org/400012/diff/19001/18042
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/400012/diff/19001/18042#newcode320
chrome/renderer/render_widget.cc:320: // it's not processed by the webkit, then
we need to suppress the upcoming
nit: "not processed by webkit"

[James Su, 2009-12-02 09:08:12]

Thanks a lot for your review. CL has been updated accordingly.

- James Su

http://codereview.chromium.org/400012/diff/19001/18014
File chrome/browser/blocked_popup_container_interactive_uitest.cc (right):

http://codereview.chromium.org/400012/diff/19001/18014#newcode286
chrome/browser/blocked_popup_container_interactive_uitest.cc:286: base::VKEY_F4,
views::Event::EF_CONTROL_DOWN));
On 2009/12/02 08:01:48, darin wrote:
> what's the reason for switching this keyboard shortcut?

With this CL, ctrl+w will be sent to the renderer on Linux. Because it might
conflict with emacs key binding. See http://crbug.com/26131. So ctrl+f4 is
tested instead.

http://codereview.chromium.org/400012/diff/19001/18006
File chrome/browser/browser.h (right):

http://codereview.chromium.org/400012/diff/19001/18006#newcode474
chrome/browser/browser.h:474: bool IsReservedCommand(int id);
On 2009/12/02 08:01:48, darin wrote:
> this name worksforme.

Done.

http://codereview.chromium.org/400012/diff/19001/18016
File chrome/browser/browser_window.h (right):

http://codereview.chromium.org/400012/diff/19001/18016#newcode271
chrome/browser/browser_window.h:271: // before sending it them to the renderer.
On 2009/12/02 08:01:48, darin wrote:
> nit: spurious "them"?

Done.

http://codereview.chromium.org/400012/diff/19001/18016#newcode272
chrome/browser/browser_window.h:272: // This method is called from
Browser::PreHandleKeyboardEvent() method, which
On 2009/12/02 08:01:48, darin wrote:
> in general, it is good to not write comments that just repeat what the code
> does.  also, it is possible for Browser::PreHandleKeyboardEvent to be renamed
or
> relocated in the future, and this comment would likely not be updated if that
> happened.

Done.

http://codereview.chromium.org/400012/diff/19001/18016#newcode282
chrome/browser/browser_window.h:282: // This method is called from
Browser::HandleKeyboardEvent() method, which
On 2009/12/02 08:01:48, darin wrote:
> ditto

Done.

http://codereview.chromium.org/400012/diff/19001/18038
File chrome/browser/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/400012/diff/19001/18038#newcode483
chrome/browser/cocoa/browser_window_cocoa.mm:483:
(ChromeEventProcessingWindow*)window_;
On 2009/12/02 08:01:48, darin wrote:
> nit: use C++ style casts?

Done.

http://codereview.chromium.org/400012/diff/19001/18003
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/400012/diff/19001/18003#newcode510
chrome/browser/extensions/extension_host.cc:510: event.windowsKeyCode ==
base::VKEY_ESCAPE && is_keyboard_shortcut)
On 2009/12/02 08:01:48, darin wrote:
> why is it necessary to null check is_keyboard_shortcut?  why not just require
> the caller to pass a non-null is_keyboard_shortcut?

Done.

http://codereview.chromium.org/400012/diff/19001/18042
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/400012/diff/19001/18042#newcode320
chrome/renderer/render_widget.cc:320: // it's not processed by the webkit, then
we need to suppress the upcoming
On 2009/12/02 08:01:48, darin wrote:
> nit: "not processed by webkit"

Done.

[James Su, 2009-12-07 09:24:15]

Ping.

[James Su, 2009-12-09 09:15:18]

Ping darin. This CL has been pending for 3 weeks :-(

On 2009/12/07 09:24:15, James Su wrote:
> Ping.

[darin (slow to review), 2009-12-09 17:48:06]

LGTM

I'm very sorry for the delays in reviewing your patch.  Please if you see days
slipping by, send me a direct email.  The pings on this CL were buried in a pile
of other review related emails :(

http://codereview.chromium.org/400012/diff/30001/24040
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/400012/diff/30001/24040#newcode156
chrome/common/render_messages_internal.h:156: // 2. An optional boolean value
indicating if a RawKeyDown event is associated
i think it would be cleaner if we instead always prefixed the payload with a
fixed event preamble struct.  that way if we need to add more fields like this,
it won't become a mess.  this kind of thing can be changed in a follow-up CL. 
there's enough code change in this CL already :)