This CL fixes issue 11480: Support GTK keyboard themes (emacs keybindings)....

This CL fixes issue 11480: Support GTK keyboard themes (emacs keybindings).
A new virtual method UnhandledKeyboardEvent() has been added to RenderWidgetHostView interface, to give RenderWidgetHostView implementation a chance to handle key events which are not handled by renderer. This method will be called by RenderWidgetHost::OnMsgInputEventAck() for all unhandled keyboard events before calling RenderWidgetHost::UnhandledKeyboardEvent(). Mac and Windows implementations may also override RenderWidgetHostView::UnhandledKeyboardEvent() to support system defined key bindings.

This CL still has a problem, see following comment copied from the code:
// TODO(james.su@gmail.com): Check if current event flow is appropriate, which
// sends a key event to renderer(webkit) first, then calls this key bindings
// handler if the key event is not handled by renderer. So key bindings
// defined in our webkit glue have higher priority.
// However, some key bindings defined in our webkit glue may conflict with the
// key bindings defined in gtk keyboard theme, for example ctrl-a is
// bound to "SelectAll" in our webkit glue. But in gtk Emacs keyboard theme,
// it's bound to "MoveToBeginningOfParagraph".
// In such case, an Emacs user would prefer Emacs key bindings over our
// built-in key bindings.
// But it may also cause problem if we call this key bindings handler before
// sending key events to renderer, because there is no way to prevent webkit
// from interpreting it as a key binding. Then unexpected behavior may occur,
// if a key event matches a key binding defined in gtk keyboard theme as well as
// a (different) key binding defined in webkit glue.

We might need to change webkit glue implementation to let webkit work with system defined key bindings seamlessly.

BUG=11480
: Support GTK keyboard themes (emacs keybindings)
TEST=Switch to Emacs keyboard theme by changing the value of gconf key "/desktop/gnome/interface/gtk_key_theme" to "Emacs", then starts chrome and opens a webpage with a text input box. Input something into the text box, then press any of the Emacs key bindings defined in /usr/share/themes/Emacs/gtk-2.0-key/gtkrc, to see if it works as expected. For example, ctrl-p should move the cursor up one line, and ctrl-k should delete to the end of paragraph.

[james.su_gmail.com, 2009-07-29 13:11:12]

Hi,
 It's a CL to fix issue 11480, please help review it.

Regards
James Su

[Evan Stade, 2009-07-30 01:14:55]

I think the keyboard theme needs to have higher priority. Your ctrl-a example is
a good one: I don't think users will be happy if their keyboard theme is only
half-respected. This will require some work on the renderer (glue) side though,
because we only want to consult the keyboard theme if a text entry area has
focus.

But we can't ask the renderer and wait for its response every time we get a
keyboard event, and if we try to rely on message from webkit telling us when a
text view has gained or lost focus we will likely end up out-of-sync for short
periods of time.

Perhaps the best way to do this would be to change the keyboard event message to
include an editor command (if any). Then the renderer checks if a text view has
focus, if so it applies the command.

http://codereview.chromium.org/159586/diff/8/1008
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/159586/diff/8/1008#newcode70
Line 70: class RenderWidgetHostViewGtkKeyBindings {
ok this file is beginning to get absurdly long. Can we split this out into
another file if it's not too much trouble?

http://codereview.chromium.org/159586/diff/8/1008#newcode156
Line 156: host_view_->GetRenderWidgetHost()->ForwardEditCommand(name, value);
It looks like ForwardEditCommand does nothing. Is that why you have the
DLOG(INFO) showing you what commands are being forwarded?

[Evan Stade, 2009-07-30 01:15:15]

also, would be nice to hear Evan's take on what I just said

[james.su_gmail.com, 2009-07-30 06:24:10]

Changing keyboard event message would be a feasible solution. The only issue is:
one key event may generate multiple editor commands, how to store them in
WebKeyboardEvent effectively?

http://codereview.chromium.org/159586/diff/8/1008
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/159586/diff/8/1008#newcode70
Line 70: class RenderWidgetHostViewGtkKeyBindings {
On 2009/07/30 01:14:56, Evan Stade wrote:
> ok this file is beginning to get absurdly long. Can we split this out into
> another file if it's not too much trouble?

I'd like to split this file. Any suggestion to the new file name? Is it
necessary to split RenderWidgetHostViewGtkKeyBindings and
RenderWidgetHostViewGtkIMContext into two separated files?

http://codereview.chromium.org/159586/diff/8/1008#newcode156
Line 156: host_view_->GetRenderWidgetHost()->ForwardEditCommand(name, value);
On 2009/07/30 01:14:56, Evan Stade wrote:
> It looks like ForwardEditCommand does nothing. Is that why you have the
> DLOG(INFO) showing you what commands are being forwarded?

this DLOG is just for debugging, will be removed afterwards. ForwardEditCommand
is implemented in RenderViewHost.

[james.su_gmail.com, 2009-07-30 06:26:01]

I'm setting up my chromium account. Will send this CL again with my new account
asap.

[James Su, 2009-07-30 10:54:58]

Hi,
  I just went through the code path of keyboard event handling. When a key event
is generated in a text input box(eg. when user press Right), following code path
will be executed:

In browser:
 RenderWidgetHostViewGtkWidget::KeyPressReleaseEvent() ->
 RenderWidgetHost::ForwardKeyboardEvent() ->
In renderer:
 RenderWidget::OnHandleInputEvent() ->
In webkit glue:
 WebViewImpl::handleInputEvent() ->
 WebViewImpl::KeyEvent() ->
In webkit:
 EventHandler::keyEvent() ->
 Node::dispatchEvent() ->
 EventHandler::defaultKeyboardEventHandler() ->
 Editor::handleKeyboardEvent() ->
In webkit glue:
 EditorClientImpl::handleKeyboardEvent() ->
 (execute editor command "MoveRight" against the target node of the event)
In render:
 Send back ACK message indicating the event has been processed.

You can see that editor key bindings are handled in EditorClientImpl's
handleKeyboardEvent(). If we want to handle system defined key bindings at the
beginning of this process and carry the editor command information along with
the keyboard event, we must make sure the information can be carried into
EditorClientImpl::handleKeyboardEvent(), so that system defined and built-in key
bindings can both be handled here.

In order to achieve this goal, we may modify WebKit::WebKeyboardEvent,
WebCore::PlatformKeyboardEvent and related classes, which would be a huge
change. I doubt if it's feasible for us.

Regards
James Su

[Evan Stade, 2009-07-30 19:05:33]

I could be mistaken but I don't think it would require all that. In our renderer
or glue layer can't you figure out if an editor has focus and at that point
either dispatch the edit command *or* continue with normal keypress handling?

[Evan Stade, 2009-07-30 19:15:54]

oh, and for the names of the files, you can just use the name of the class (for
class ClassNameGoesLikeThis, filename should be
class_name_goes_like_this.{h,cc})

[tony, 2009-07-30 20:08:08]

This all seems really tricky.  Can't we sent a message to renderers about which
mode we're in and just update the keyDownEntries in editor_client_impl.cc?  I'm
worried we're going to break something like javascript trying to get key events.

[Hironori Bono, 2009-07-31 03:37:33]

+1 for tony.
It is better to design a customization scheme that sends an IPC message about
the shortcut keys so that our EditorClientImpl class can use them. Customizing
keyboard shortcuts of our EditorClientImpl class is a long-lasting issue since
last October (Issue 896) and it is better for us to provide a
platform-independent solution to users and extension APIs for extension
programmers, respectively. :)

Regards,

Hironori Bono

[James Su, 2009-07-31 03:38:37]

On 2009/07/30 19:05:33, Evan Stade wrote:
> I could be mistaken but I don't think it would require all that. In our
renderer
> or glue layer can't you figure out if an editor has focus and at that point
> either dispatch the edit command *or* continue with normal keypress handling?

A key event must be delivered down to HTML DOM before handling as a editor key
binding. If the event is processed or cancelled in DOM, it shouldn't be handled
anymore.
So we can't simply intercept the key event process flow in renderer or our glue
code.

(from now on, I'll be using my chromium account)

Regards
James Su

[James Su, 2009-07-31 04:31:16]

On 2009/07/31 03:37:33, hbono wrote:
> +1 for tony.
> It is better to design a customization scheme that sends an IPC message about
> the shortcut keys so that our EditorClientImpl class can use them. Customizing
> keyboard shortcuts of our EditorClientImpl class is a long-lasting issue since
> last October (Issue 896) and it is better for us to provide a
> platform-independent solution to users and extension APIs for extension
> programmers, respectively. :)
> 
> Regards,
> 
> Hironori Bono

I'm not sure if EditorClientImpl is the correct place to do it. We might not be
able to retrieve system pre-defined key bindings statically, taking this gtk
implementation as an example. In such case, we must send key binding matching
result down to webkit for each key event.

And IMHO, customizing key bindings is a feature belonging to browser, just same
as extension. So I think it would be more reasonable to move key binding
handling up to browser. And we may provide a mechanism to manage all
customizable key bindings in an unified way, not only the editor key bindings,
but also accelerator keys of browser window.

Regards
James Su