Supports Gtk keyboard themes....

Supports Gtk keyboard themes.

This CL fixes issue 11480: Support GTK keyboard themes (emacs keybindings).
A new class GtkKeyBindingsHandler has been added, which matches a key event against key bindings defined in current Gtk keyboard theme.
A new render message ViewMsg_SetEditCommandsForNextKeyEvent has been added for sending edit commands associated to a key event to renderer. This message shall be sent just before sending the key event. RenderView will handle this event and cache the edit commands until the key event is processed.
When processing the key event, EditClientImpl::handleKeyboardEvent() will eventually be called to handle the key event, if it's not handled by DOM and the focus is inside an input box. Then a newly added method WebViewDelegate::ExecuteEditCommandsForCurrentKeyEvent(), which is implemented in RenderView, will be called by EditClientImpl::handleKeyboardEvent() to execute edit commands previously sent from browser by ViewMsg_SetEditCommandsForNextKeyEvent message. If WebViewDelegate::ExecuteEditCommandsForCurrentKeyEvent() returns false, which means the key event doesn't have edit command associated, EditClientImpl will handle the key event with built-in logic, which may trigger a built-in key binding.
With this approach, system defined key bindings always have higher priority than built-in key bindings defined in editor_client_impl.cc.

Known issue:
If a key event matches not only a system defined key binding but also an accesskey of a DOM element, then both corresponding edit commands and accesskey action will be executed. Because accesskey is handled in WebViewImpl::CharEvent(), while edit commands are bound to RawKeyDown or KeyUp events.

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.


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

[James Su, 2009-08-11 07:33:53]

Hi,
  It's an updated version of CL http://codereview.chromium.org/159586. Now this
CL is fully functional. Key bindings defined in gtk keyboard theme are
prioritized over built-in editor key bindings.

Regards
James Su

[darin (slow to review), 2009-08-11 07:52:28]

Perhaps it would be better to compute the editor commands ahead of time and
include those along with the IPC that sends the WebKeyboardEvent.  That way
there does not have to be a round trip to the browser in order to gather those
commands.  That round trip could allow other input events to sneak in causing
the user's input to appear out-of-order with their typing.  The sequence A +
<key binding> + B might end up being processed as A + B + <key binding>, which
might not be what the user intended.

Also, instead of incorporating a flag into WebKeyboardEvent, the mere presence
of "editor commands" could be used to trigger this alternate behavior.  Maybe we
could just have the EditorClientImpl invoke a method on WebViewDelegate to
determine if the embedder wants to do its own custom 'default' handling for the
input event.

[James Su, 2009-08-11 09:38:55]

On 2009/08/11 07:52:28, darin wrote:
> Perhaps it would be better to compute the editor commands ahead of time and
> include those along with the IPC that sends the WebKeyboardEvent.  That way
> there does not have to be a round trip to the browser in order to gather those
> commands.  That round trip could allow other input events to sneak in causing
> the user's input to appear out-of-order with their typing.  The sequence A +
> <key binding> + B might end up being processed as A + B + <key binding>, which
> might not be what the user intended.
I considered this approach, but the problem is: the length of editor commands
are not fixed and there can be arbitrary number of editor commands associated to
a key binding, but WebKeyboardEvent can only contain POD members. Then we must
add a char array into WebKeyboardEvent that is large enough to hold the editor
commands, which will bloat the size of WebKeyboardEvent. Of course, if you think
the size of WebKeyboardEvent doesn't matter, I can modify the CL to follow this
approach.

For current approach, the event sequence might be a problem. But I'm wondering
how likely it'll happen.
I noticed that RenderWidgetHost::ForwardKeyboardEvent() records the size of
keyboard queue with following sentence:

HISTOGRAM_COUNTS_100("Renderer.KeyboardQueueSize", key_queue_.size());

As the event sequence will be out of order if this size is larger than 1. If we
can have this data, then we can know the probability of this case.

And actually, we can modify RenderWidgetHost to avoid the out of order issue by
sending the next event only when the ack message of previous event has been
received.

> 
> Also, instead of incorporating a flag into WebKeyboardEvent, the mere presence
> of "editor commands" could be used to trigger this alternate behavior.  Maybe
we
> could just have the EditorClientImpl invoke a method on WebViewDelegate to
> determine if the embedder wants to do its own custom 'default' handling for
the
> input event.

The approach to add a method to WebViewDelegate is feasible, however it requires
to modify more files, including render_widget.{cc,h} and render_view.{cc,h},
etc. Because input events are handled in render_widget, rather than render_view.

[darin (slow to review), 2009-08-11 16:24:19]

On Tue, Aug 11, 2009 at 2:38 AM, <suzhe@chromium.org> wrote:

> On 2009/08/11 07:52:28, darin wrote:
>
>> Perhaps it would be better to compute the editor commands ahead of
>>
> time and
>
>> include those along with the IPC that sends the WebKeyboardEvent.
>>
> That way
>
>> there does not have to be a round trip to the browser in order to
>>
> gather those
>
>> commands.  That round trip could allow other input events to sneak in
>>
> causing
>
>> the user's input to appear out-of-order with their typing.  The
>>
> sequence A +
>
>> <key binding> + B might end up being processed as A + B + <key
>>
> binding>, which
>
>> might not be what the user intended.
>>
> I considered this approach, but the problem is: the length of editor
> commands are not fixed and there can be arbitrary number of editor
> commands associated to a key binding, but WebKeyboardEvent can only
> contain POD members. Then we must add a char array into WebKeyboardEvent
> that is large enough to hold the editor commands, which will bloat the
> size of WebKeyboardEvent. Of course, if you think the size of
> WebKeyboardEvent doesn't matter, I can modify the CL to follow this
> approach.
>

I actually wasn't suggesting adding the editor commands to the
WebKeyboardEvent.  I agree that they don't make sense in that structure.  In
fact, my goal with this suggestion was to see if it is possible to avoid
making a change to WebKeyboardEvent.  Maybe doing so is still the cleanest
solution...



>
> For current approach, the event sequence might be a problem. But I'm
> wondering how likely it'll happen.
>

If the system is bogged down, the "unlikely things" will happen.  This is
the kind of subtle bug we should try hard to avoid.



> I noticed that RenderWidgetHost::ForwardKeyboardEvent() records the size
> of keyboard queue with following sentence:
>
> HISTOGRAM_COUNTS_100("Renderer.KeyboardQueueSize", key_queue_.size());
>
> As the event sequence will be out of order if this size is larger than
> 1. If we can have this data, then we can know the probability of this
> case.
>
> And actually, we can modify RenderWidgetHost to avoid the out of order
> issue by sending the next event only when the ack message of previous
> event has been received.
>

Yeah, this could work.  I don't know if we should do it for all input events
since it might interfere with smooth typing.



>
>
>  Also, instead of incorporating a flag into WebKeyboardEvent, the mere
>>
> presence
>
>> of "editor commands" could be used to trigger this alternate behavior.
>>
>  Maybe we
>
>> could just have the EditorClientImpl invoke a method on
>>
> WebViewDelegate to
>
>> determine if the embedder wants to do its own custom 'default'
>>
> handling for the
>
>> input event.
>>
>
> The approach to add a method to WebViewDelegate is feasible, however it
> requires to modify more files, including render_widget.{cc,h} and
> render_view.{cc,h}, etc. Because input events are handled in
> render_widget, rather than render_view.


This is true, but I don't necessarily see this as a deal breaker if it
yields a cleaner solution.

Where I'm coming from:  WebKeyboardEvent is supposed to be equivalent to
PlatformKeyboardEvent.  They should have the same fields, and it should be
possible to reconstruct a WebKeyboardEvent from a PlatformKeyboardEvent.

I realize that ViewMsg_HandleInputEvent does not give us much room for
flexibility.  However, maybe we could get by with a second IPC that we can
send before and after HandleInputEvent.  We could invent a
ViewMsg_SuppressDefaultKeyHandler that takes a bool parameter.  We could use
that to disable normal keyboard event handling for a single
ViewMsg_HandleInputEvent.

WDYT of that idea?
-Darin




>
>
> http://codereview.chromium.org/165293
>

[tony, 2009-08-11 17:02:33]

This all seems really complex to me.  Why don't we just enumerate all the
keybindings at start up
(http://library.gnome.org/devel/gtk/unstable/gtk-Bindings.html) and pass them
down to the renderer when the renderer is created.  This data could be used to
populate the table in editor_client_impl.cc.

If we get a signal when the bindings change, we could then send an updated table
to the renderers, but it doesn't look like other GTK+ apps support that (at
least, I tried to change it while pidgin was running and nothing seemed to
happen).

[darin (slow to review), 2009-08-11 18:14:24]

Maybe we could have a WebViewDelegate method that is called to see if the
"embedder" wants to override the default handling for a key event.  Then,
the WebViewDelegate could call WebFrame::executeCommand from that method and
return a flag to indicate that the normal default handling should be
suppressed.
-Darin



On Tue, Aug 11, 2009 at 10:02 AM, <tony@chromium.org> wrote:

> This all seems really complex to me.  Why don't we just enumerate all
> the keybindings at start up
> (http://library.gnome.org/devel/gtk/unstable/gtk-Bindings.html) and pass
> them down to the renderer when the renderer is created.  This data could
> be used to populate the table in editor_client_impl.cc.
>
> If we get a signal when the bindings change, we could then send an
> updated table to the renderers, but it doesn't look like other GTK+ apps
> support that (at least, I tried to change it while pidgin was running
> and nothing seemed to happen).
>
>
>
> http://codereview.chromium.org/165293
>

[Matt Tolton, 2009-08-11 20:05:44]

Looking at this from the OS X side of things: it's difficult to get a list of
all of the key bindings as tony suggested, so it would be hard to reuse the same
mechanism for both linux and mac.

On 2009/08/11 18:14:24, darin wrote:
> Maybe we could have a WebViewDelegate method that is called to see if the
> "embedder" wants to override the default handling for a key event.  Then,
> the WebViewDelegate could call WebFrame::executeCommand from that method and
> return a flag to indicate that the normal default handling should be
> suppressed.
> -Darin
> 
> 
> 
> On Tue, Aug 11, 2009 at 10:02 AM, <tony@chromium.org> wrote:
> 
> > This all seems really complex to me.  Why don't we just enumerate all
> > the keybindings at start up
> > (http://library.gnome.org/devel/gtk/unstable/gtk-Bindings.html) and pass
> > them down to the renderer when the renderer is created.  This data could
> > be used to populate the table in editor_client_impl.cc.
> >
> > If we get a signal when the bindings change, we could then send an
> > updated table to the renderers, but it doesn't look like other GTK+ apps
> > support that (at least, I tried to change it while pidgin was running
> > and nothing seemed to happen).
> >
> >
> >
> > http://codereview.chromium.org/165293
> >
>

[Hironori Bono, 2009-08-12 02:51:37]

Thank you for your hard work.
Even though I realize you work very hard for fixing input-related issues of
Linux Chrome, I'm wondering if it is a good idea to submit such a huge change
without any automated tests now.
Even though we currently have some RenderViewTests which tests input-related
issues, it works only on Windows. To make this matter worse, we don't have any
automated tests which test IME-related issues of our front-ends.
For what it's worth, we don't have any methods (but manual tests) that verify an
input-related change can work without any regressions. (I realize this is
definitely my fault and I have to apologize to all of you for the lack of test
frameworks for i18n issues.)
So, I think it may be better for us to add more input-related tests (or set up
another automated test framework if needed) before adding new features.

Regards,

Hironori Bono

[Evan Stade, 2009-08-12 02:56:00]

On Tue, Aug 11, 2009 at 7:51 PM, <hbono@chromium.org> wrote:
> Thank you for your hard work.
> Even though I realize you work very hard for fixing input-related issues
> of Linux Chrome, I'm wondering if it is a good idea to submit such a
> huge change without any automated tests now.
> Even though we currently have some RenderViewTests which tests
> input-related issues, it works only on Windows. To make this matter
> worse, we don't have any automated tests which test IME-related issues
> of our front-ends.
> For what it's worth, we don't have any methods (but manual tests) that
> verify an input-related change can work without any regressions. (I
> realize this is definitely my fault and I have to apologize to all of
> you for the lack of test frameworks for i18n issues.)
> So, I think it may be better for us to add more input-related tests (or
> set up another automated test framework if needed) before adding new
> features.
>
> Regards,
>
> Hironori Bono
>
> http://codereview.chromium.org/165293
>

FWIW I'm working on getting automated tests for linux.

[James Su, 2009-08-12 03:58:00]

On 2009/08/11 17:02:33, tony wrote:
> This all seems really complex to me.  Why don't we just enumerate all the
> keybindings at start up
> (http://library.gnome.org/devel/gtk/unstable/gtk-Bindings.html) and pass them
> down to the renderer when the renderer is created.  This data could be used to
> populate the table in editor_client_impl.cc.
Gtk doesn't provide a method to enumerate all the predefined key bindings. The
only method we can use is gtk_bindings_activate(), which accepts a key event and
activates corresponding signal if it matches a predefined key binding. And the
key binding setting can be modified on-the-fly through gtk settings interface,
see:
http://library.gnome.org/devel/gtk/unstable/GtkSettings.html#GtkSettings--gtk....
The corresponding gconf key is /desktop/gnome/interface/gtk_key_theme.

> 
> If we get a signal when the bindings change, we could then send an updated
table
> to the renderers, but it doesn't look like other GTK+ apps support that (at
> least, I tried to change it while pidgin was running and nothing seemed to
> happen).

[James Su, 2009-08-12 04:10:52]

On 2009/08/11 16:24:19, darin wrote:
> On Tue, Aug 11, 2009 at 2:38 AM, <suzhe@chromium.org> wrote:
> 
> > On 2009/08/11 07:52:28, darin wrote:
> >
> >> Perhaps it would be better to compute the editor commands ahead of
> >>
> > time and
> >
> >> include those along with the IPC that sends the WebKeyboardEvent.
> >>
> > That way
> >
> >> there does not have to be a round trip to the browser in order to
> >>
> > gather those
> >
> >> commands.  That round trip could allow other input events to sneak in
> >>
> > causing
> >
> >> the user's input to appear out-of-order with their typing.  The
> >>
> > sequence A +
> >
> >> <key binding> + B might end up being processed as A + B + <key
> >>
> > binding>, which
> >
> >> might not be what the user intended.
> >>
> > I considered this approach, but the problem is: the length of editor
> > commands are not fixed and there can be arbitrary number of editor
> > commands associated to a key binding, but WebKeyboardEvent can only
> > contain POD members. Then we must add a char array into WebKeyboardEvent
> > that is large enough to hold the editor commands, which will bloat the
> > size of WebKeyboardEvent. Of course, if you think the size of
> > WebKeyboardEvent doesn't matter, I can modify the CL to follow this
> > approach.
> >
> 
> I actually wasn't suggesting adding the editor commands to the
> WebKeyboardEvent.  I agree that they don't make sense in that structure.  In
> fact, my goal with this suggestion was to see if it is possible to avoid
> making a change to WebKeyboardEvent.  Maybe doing so is still the cleanest
> solution...
> 
> 
> 
> >
> > For current approach, the event sequence might be a problem. But I'm
> > wondering how likely it'll happen.
> >
> 
> If the system is bogged down, the "unlikely things" will happen.  This is
> the kind of subtle bug we should try hard to avoid.
> 
> 
> 
> > I noticed that RenderWidgetHost::ForwardKeyboardEvent() records the size
> > of keyboard queue with following sentence:
> >
> > HISTOGRAM_COUNTS_100("Renderer.KeyboardQueueSize", key_queue_.size());
> >
> > As the event sequence will be out of order if this size is larger than
> > 1. If we can have this data, then we can know the probability of this
> > case.
> >
> > And actually, we can modify RenderWidgetHost to avoid the out of order
> > issue by sending the next event only when the ack message of previous
> > event has been received.
> >
> 
> Yeah, this could work.  I don't know if we should do it for all input events
> since it might interfere with smooth typing.
I just tried it and felt no difference than before. Will upload the change for
review. I did it only for keyboard events.

> 
> 
> 
> >
> >
> >  Also, instead of incorporating a flag into WebKeyboardEvent, the mere
> >>
> > presence
> >
> >> of "editor commands" could be used to trigger this alternate behavior.
> >>
> >  Maybe we
> >
> >> could just have the EditorClientImpl invoke a method on
> >>
> > WebViewDelegate to
> >
> >> determine if the embedder wants to do its own custom 'default'
> >>
> > handling for the
> >
> >> input event.
> >>
> >
> > The approach to add a method to WebViewDelegate is feasible, however it
> > requires to modify more files, including render_widget.{cc,h} and
> > render_view.{cc,h}, etc. Because input events are handled in
> > render_widget, rather than render_view.
> 
> 
> This is true, but I don't necessarily see this as a deal breaker if it
> yields a cleaner solution.
> 
> Where I'm coming from:  WebKeyboardEvent is supposed to be equivalent to
> PlatformKeyboardEvent.  They should have the same fields, and it should be
> possible to reconstruct a WebKeyboardEvent from a PlatformKeyboardEvent.
Agree. We should avoid touching WebKeyboardEvent as much as possible.

> 
> I realize that ViewMsg_HandleInputEvent does not give us much room for
> flexibility.  However, maybe we could get by with a second IPC that we can
> send before and after HandleInputEvent.  We could invent a
> ViewMsg_SuppressDefaultKeyHandler that takes a bool parameter.  We could use
> that to disable normal keyboard event handling for a single
> ViewMsg_HandleInputEvent.
I think it's feasible. I'll investigate this approach.

> 
> WDYT of that idea?
> -Darin
> 
> 
> 
> 
> >
> >
> > http://codereview.chromium.org/165293
> >
>

[James Su, 2009-08-12 04:25:49]

How does it work on OSX? Is it similar than what current CL does?

On 2009/08/11 20:05:44, Matt Tolton wrote:
> Looking at this from the OS X side of things: it's difficult to get a list of
> all of the key bindings as tony suggested, so it would be hard to reuse the
same
> mechanism for both linux and mac.
> 
> On 2009/08/11 18:14:24, darin wrote:
> > Maybe we could have a WebViewDelegate method that is called to see if the
> > "embedder" wants to override the default handling for a key event.  Then,
> > the WebViewDelegate could call WebFrame::executeCommand from that method and
> > return a flag to indicate that the normal default handling should be
> > suppressed.
> > -Darin
> > 
> > 
> > 
> > On Tue, Aug 11, 2009 at 10:02 AM, <tony@chromium.org> wrote:
> > 
> > > This all seems really complex to me.  Why don't we just enumerate all
> > > the keybindings at start up
> > > (http://library.gnome.org/devel/gtk/unstable/gtk-Bindings.html) and pass
> > > them down to the renderer when the renderer is created.  This data could
> > > be used to populate the table in editor_client_impl.cc.
> > >
> > > If we get a signal when the bindings change, we could then send an
> > > updated table to the renderers, but it doesn't look like other GTK+ apps
> > > support that (at least, I tried to change it while pidgin was running
> > > and nothing seemed to happen).
> > >
> > >
> > >
> > > http://codereview.chromium.org/165293
> > >
> >

[darin (slow to review), 2009-08-12 04:26:50]

On Tue, Aug 11, 2009 at 9:10 PM, <suzhe@chromium.org> wrote:

> On 2009/08/11 16:24:19, darin wrote:
>
>> On Tue, Aug 11, 2009 at 2:38 AM, <suzhe@chromium.org> wrote:
>>
>
>  > On 2009/08/11 07:52:28, darin wrote:
>> >
>> >> Perhaps it would be better to compute the editor commands ahead of
>> >>
>> > time and
>> >
>> >> include those along with the IPC that sends the WebKeyboardEvent.
>> >>
>> > That way
>> >
>> >> there does not have to be a round trip to the browser in order to
>> >>
>> > gather those
>> >
>> >> commands.  That round trip could allow other input events to sneak
>>
> in
>
>> >>
>> > causing
>> >
>> >> the user's input to appear out-of-order with their typing.  The
>> >>
>> > sequence A +
>> >
>> >> <key binding> + B might end up being processed as A + B + <key
>> >>
>> > binding>, which
>> >
>> >> might not be what the user intended.
>> >>
>> > I considered this approach, but the problem is: the length of editor
>> > commands are not fixed and there can be arbitrary number of editor
>> > commands associated to a key binding, but WebKeyboardEvent can only
>> > contain POD members. Then we must add a char array into
>>
> WebKeyboardEvent
>
>> > that is large enough to hold the editor commands, which will bloat
>>
> the
>
>> > size of WebKeyboardEvent. Of course, if you think the size of
>> > WebKeyboardEvent doesn't matter, I can modify the CL to follow this
>> > approach.
>> >
>>
>
>  I actually wasn't suggesting adding the editor commands to the
>> WebKeyboardEvent.  I agree that they don't make sense in that
>>
> structure.  In
>
>> fact, my goal with this suggestion was to see if it is possible to
>>
> avoid
>
>> making a change to WebKeyboardEvent.  Maybe doing so is still the
>>
> cleanest
>
>> solution...
>>
>
>
>
>  >
>> > For current approach, the event sequence might be a problem. But I'm
>> > wondering how likely it'll happen.
>> >
>>
>
>  If the system is bogged down, the "unlikely things" will happen.  This
>>
> is
>
>> the kind of subtle bug we should try hard to avoid.
>>
>
>
>
>  > I noticed that RenderWidgetHost::ForwardKeyboardEvent() records the
>>
> size
>
>> > of keyboard queue with following sentence:
>> >
>> > HISTOGRAM_COUNTS_100("Renderer.KeyboardQueueSize",
>>
> key_queue_.size());
>
>> >
>> > As the event sequence will be out of order if this size is larger
>>
> than
>
>> > 1. If we can have this data, then we can know the probability of
>>
> this
>
>> > case.
>> >
>> > And actually, we can modify RenderWidgetHost to avoid the out of
>>
> order
>
>> > issue by sending the next event only when the ack message of
>>
> previous
>
>> > event has been received.
>> >
>>
>
>  Yeah, this could work.  I don't know if we should do it for all input
>>
> events
>
>> since it might interfere with smooth typing.
>>
> I just tried it and felt no difference than before. Will upload the
> change for review. I did it only for keyboard events.
>


Keep in mind that sometimes you don't feel the slowdown until the system is
busy.  It is hard to know that the problem won't exhibit itself in real life
on real hardware.




>
>
>
>
>  >
>> >
>> >  Also, instead of incorporating a flag into WebKeyboardEvent, the
>>
> mere
>
>> >>
>> > presence
>> >
>> >> of "editor commands" could be used to trigger this alternate
>>
> behavior.
>
>> >>
>> >  Maybe we
>> >
>> >> could just have the EditorClientImpl invoke a method on
>> >>
>> > WebViewDelegate to
>> >
>> >> determine if the embedder wants to do its own custom 'default'
>> >>
>> > handling for the
>> >
>> >> input event.
>> >>
>> >
>> > The approach to add a method to WebViewDelegate is feasible, however
>>
> it
>
>> > requires to modify more files, including render_widget.{cc,h} and
>> > render_view.{cc,h}, etc. Because input events are handled in
>> > render_widget, rather than render_view.
>>
>
>
>  This is true, but I don't necessarily see this as a deal breaker if it
>> yields a cleaner solution.
>>
>
>  Where I'm coming from:  WebKeyboardEvent is supposed to be equivalent
>>
> to
>
>> PlatformKeyboardEvent.  They should have the same fields, and it
>>
> should be
>
>> possible to reconstruct a WebKeyboardEvent from a
>>
> PlatformKeyboardEvent.
> Agree. We should avoid touching WebKeyboardEvent as much as possible.
>
>
>  I realize that ViewMsg_HandleInputEvent does not give us much room for
>> flexibility.  However, maybe we could get by with a second IPC that we
>>
> can
>
>> send before and after HandleInputEvent.  We could invent a
>> ViewMsg_SuppressDefaultKeyHandler that takes a bool parameter.  We
>>
> could use
>
>> that to disable normal keyboard event handling for a single
>> ViewMsg_HandleInputEvent.
>>
> I think it's feasible. I'll investigate this approach.
>
>
>  WDYT of that idea?
>> -Darin
>>
>
>
>
>
>  >
>> >
>> > http://codereview.chromium.org/165293
>> >
>>
>
>
>
>
> http://codereview.chromium.org/165293
>

[James Su, 2009-08-12 04:40:16]

I agree that automated tests would be very important for us. Testing pure
keyboard input automatically would not be very hard, because we can always get
predictable result for a keyboard event. But when involving IME, the situation
would become much more complicated, because the result might be unpredicatable
at all.
It would be a long term task to setup such an automated test framework,
especially if we want to support all three platforms. So I'd prefer to fix input
related issue in parallel with setting up this test framework to save our time.
Meanwhile, we can enforce manual tests and code review to avoid regressions.

Regards
James Su

On 2009/08/12 02:51:37, hbono wrote:
> Thank you for your hard work.
> Even though I realize you work very hard for fixing input-related issues of
> Linux Chrome, I'm wondering if it is a good idea to submit such a huge change
> without any automated tests now.
> Even though we currently have some RenderViewTests which tests input-related
> issues, it works only on Windows. To make this matter worse, we don't have any
> automated tests which test IME-related issues of our front-ends.
> For what it's worth, we don't have any methods (but manual tests) that verify
an
> input-related change can work without any regressions. (I realize this is
> definitely my fault and I have to apologize to all of you for the lack of test
> frameworks for i18n issues.)
> So, I think it may be better for us to add more input-related tests (or set up
> another automated test framework if needed) before adding new features.
> 
> Regards,
> 
> Hironori Bono

[James Su, 2009-08-12 04:41:50]

On 2009/08/12 02:56:00, Evan Stade wrote:
> On Tue, Aug 11, 2009 at 7:51 PM, <hbono@chromium.org> wrote:
> > Thank you for your hard work.
> > Even though I realize you work very hard for fixing input-related issues
> > of Linux Chrome, I'm wondering if it is a good idea to submit such a
> > huge change without any automated tests now.
> > Even though we currently have some RenderViewTests which tests
> > input-related issues, it works only on Windows. To make this matter
> > worse, we don't have any automated tests which test IME-related issues
> > of our front-ends.
> > For what it's worth, we don't have any methods (but manual tests) that
> > verify an input-related change can work without any regressions. (I
> > realize this is definitely my fault and I have to apologize to all of
> > you for the lack of test frameworks for i18n issues.)
> > So, I think it may be better for us to add more input-related tests (or
> > set up another automated test framework if needed) before adding new
> > features.
> >
> > Regards,
> >
> > Hironori Bono
> >
> > http://codereview.chromium.org/165293
> >
> 
> FWIW I'm working on getting automated tests for linux.

Just curious what kind of techniques you are going to use?

[brettw, 2009-08-12 05:08:26]

On 2009/08/12 04:10:52, James Su wrote:
> I just tried it and felt no difference than before. Will upload the change for
> review. I did it only for keyboard events.

It's important to realize how this is implemented. When you send a message from
the renderer to the browser, it gets passed from the WebKit thread to the
Renderer I/O thread, then to the browser process I/O thread, then to the UI
thread. Sending the message the other way is the opposite. By doing a round-trip
like this, it has to go through 6 message queues and 6 context switches (two of
them being process ones).

Normally, the OS tries to make context switching as fast as possible, there is
little else for the CPU to do, and there are few messages in each queue. The
operation will be very fast.

When the system is loaded with different threads wanting attention, each context
switch will be a chance for the OS to switch away to do something else (it's
unlikely we will use up our timeslice). When the system has many messages or is
doing a lot of I/O such that the named pipes are slow, doing this round trip
magnifies the effect by 6 times. This type of thing can lead to noticeable lag
in unpredictable ways.

So we should really not do round tripes if at all possible.

[James Su, 2009-08-12 05:13:29]

With the solution suggested by Darin, this can be avoided.

Regards
James Su

On 2009/08/12 05:08:26, brettw wrote:
> On 2009/08/12 04:10:52, James Su wrote:
> > I just tried it and felt no difference than before. Will upload the change
for
> > review. I did it only for keyboard events.
> 
> It's important to realize how this is implemented. When you send a message
from
> the renderer to the browser, it gets passed from the WebKit thread to the
> Renderer I/O thread, then to the browser process I/O thread, then to the UI
> thread. Sending the message the other way is the opposite. By doing a
round-trip
> like this, it has to go through 6 message queues and 6 context switches (two
of
> them being process ones).
> 
> Normally, the OS tries to make context switching as fast as possible, there is
> little else for the CPU to do, and there are few messages in each queue. The
> operation will be very fast.
> 
> When the system is loaded with different threads wanting attention, each
context
> switch will be a chance for the OS to switch away to do something else (it's
> unlikely we will use up our timeslice). When the system has many messages or
is
> doing a lot of I/O such that the named pipes are slow, doing this round trip
> magnifies the effect by 6 times. This type of thing can lead to noticeable lag
> in unpredictable ways.
> 
> So we should really not do round tripes if at all possible.

[James Su, 2009-08-12 05:21:57]

Thanks for your suggestion. I'll look into it asap.

Regards
James Su

2009/8/12 Darin Fisher <darin@chromium.org>

> James, I thought about this some more, and I think the current solution is
> too complicated.  I still think something like my earlier suggestion would
> be best.  That is, we should pre-compute the "editor command" for a
> WebKeyboardEvent and arrange for that to be sent along with the
> WebKeyboardEvent (somehow) so that we do not need to do any back-n-forth
> IPCs or suppression of further input events.  I think this may mean
> re-working ViewMsg_HandleInputEvent or perhaps adding an alternative IPC.
> Assuming the RenderView receives such an IPC, it can store the editor
> command in a member variable and then call WebView::handleInputEvent.  Then,
> in EditorClientImpl, we can call WebViewDelegate to ask it if it wishes to
> override the default action for the current key event.  At that time, we can
> call WebFrame::executeCommand using the stored editor command.
>
> I think there are other variations of this approach that could work, but
> the main property is that we still only have a single IPC.
>
> -Darin
>
>
>
> On Tue, Aug 11, 2009 at 9:41 PM, <suzhe@chromium.org> wrote:
>
>> On 2009/08/12 02:56:00, Evan Stade wrote:
>>
>>> On Tue, Aug 11, 2009 at 7:51 PM, <hbono@chromium.org> wrote:
>>> > Thank you for your hard work.
>>> > Even though I realize you work very hard for fixing input-related
>>>
>> issues
>>
>>> > of Linux Chrome, I'm wondering if it is a good idea to submit such a
>>> > huge change without any automated tests now.
>>> > Even though we currently have some RenderViewTests which tests
>>> > input-related issues, it works only on Windows. To make this matter
>>> > worse, we don't have any automated tests which test IME-related
>>>
>> issues
>>
>>> > of our front-ends.
>>> > For what it's worth, we don't have any methods (but manual tests)
>>>
>> that
>>
>>> > verify an input-related change can work without any regressions. (I
>>> > realize this is definitely my fault and I have to apologize to all
>>>
>> of
>>
>>> > you for the lack of test frameworks for i18n issues.)
>>> > So, I think it may be better for us to add more input-related tests
>>>
>> (or
>>
>>> > set up another automated test framework if needed) before adding new
>>> > features.
>>> >
>>> > Regards,
>>> >
>>> > Hironori Bono
>>> >
>>> > http://codereview.chromium.org/165293
>>> >
>>>
>>
>>  FWIW I'm working on getting automated tests for linux.
>>>
>>
>> Just curious what kind of techniques you are going to use?
>>
>>
>> http://codereview.chromium.org/165293
>>
>
>

[darin (slow to review), 2009-08-12 05:27:21]

James, I thought about this some more, and I think the current solution is
too complicated.  I still think something like my earlier suggestion would
be best.  That is, we should pre-compute the "editor command" for a
WebKeyboardEvent and arrange for that to be sent along with the
WebKeyboardEvent (somehow) so that we do not need to do any back-n-forth
IPCs or suppression of further input events.  I think this may mean
re-working ViewMsg_HandleInputEvent or perhaps adding an alternative IPC.
Assuming the RenderView receives such an IPC, it can store the editor
command in a member variable and then call WebView::handleInputEvent.  Then,
in EditorClientImpl, we can call WebViewDelegate to ask it if it wishes to
override the default action for the current key event.  At that time, we can
call WebFrame::executeCommand using the stored editor command.

I think there are other variations of this approach that could work, but the
main property is that we still only have a single IPC.

-Darin



On Tue, Aug 11, 2009 at 9:41 PM, <suzhe@chromium.org> wrote:

> On 2009/08/12 02:56:00, Evan Stade wrote:
>
>> On Tue, Aug 11, 2009 at 7:51 PM, <hbono@chromium.org> wrote:
>> > Thank you for your hard work.
>> > Even though I realize you work very hard for fixing input-related
>>
> issues
>
>> > of Linux Chrome, I'm wondering if it is a good idea to submit such a
>> > huge change without any automated tests now.
>> > Even though we currently have some RenderViewTests which tests
>> > input-related issues, it works only on Windows. To make this matter
>> > worse, we don't have any automated tests which test IME-related
>>
> issues
>
>> > of our front-ends.
>> > For what it's worth, we don't have any methods (but manual tests)
>>
> that
>
>> > verify an input-related change can work without any regressions. (I
>> > realize this is definitely my fault and I have to apologize to all
>>
> of
>
>> > you for the lack of test frameworks for i18n issues.)
>> > So, I think it may be better for us to add more input-related tests
>>
> (or
>
>> > set up another automated test framework if needed) before adding new
>> > features.
>> >
>> > Regards,
>> >
>> > Hironori Bono
>> >
>> > http://codereview.chromium.org/165293
>> >
>>
>
>  FWIW I'm working on getting automated tests for linux.
>>
>
> Just curious what kind of techniques you are going to use?
>
>
> http://codereview.chromium.org/165293
>

[James Su, 2009-08-18 09:05:52]

Hi,
  I updated the CL according your comments. Please help review again. And do you
know why EventHandler::handleAccessKey() is called against Char events instead
of RawKeyDown events?

Regards
James Su

[James Su, 2009-08-20 01:30:02]

Ping.

On 2009/08/18 09:05:52, James Su wrote:
> Hi,
>   I updated the CL according your comments. Please help review again. And do
you
> know why EventHandler::handleAccessKey() is called against Char events instead
> of RawKeyDown events?
> 
> Regards
> James Su

[James Su, 2009-08-22 00:30:32]

Can anybody help me review this CL?

Thanks
James Su

On 2009/08/20 01:30:02, James Su wrote:
> Ping.
> 
> On 2009/08/18 09:05:52, James Su wrote:
> > Hi,
> >   I updated the CL according your comments. Please help review again. And do
> you
> > know why EventHandler::handleAccessKey() is called against Char events
instead
> > of RawKeyDown events?
> > 
> > Regards
> > James Su

[Evan Stade, 2009-08-22 01:23:38]

Darin?

[Hironori Bono, 2009-08-24 12:25:42]

James,

I personally prefer this option proposed by tony.
When test_shell can also load its own key-bindings at its start-up, it can fix
some layout tests caused by key-bindings (e.g.
'LayoutTests/editing/input/emacs-ctrl-o.html',
'LayoutTests/editing/pasteboard/emacs-cntl-y-001.html', etc.) On the other hand,
this solution cannot fix such layout tests, right?

Also, this change is too monolithic (and complicated) for me to verify it works
without any regressions. I'm very wondering why you didn't split this change
into so simple changes that you can add unit tests to each one. (I personally
believe if it is hard to split a change into small ones, it is better to
re-design it.) It is definitely better to provide clean and simple code that
many engineers can read it, understand it, and change it without our help.

Finally, I recommend to work for fixing existing layout bugs, memory bugs, and
test flakiness *BEFORE* adding such features. As written by an e-mail from
pkasting (*1), we still need a lot of engineers for fixing such bugs even though
many engineers have been working very hard for them. So, to move forward without
fearing such regressions or flakiness, it is definitely better for you to fix
such issues as well.

(*1)
http://groups.google.com/group/chromium-dev/browse_thread/thread/5d7ec37b8c2a...

Regards,

Hironori Bono

On 2009/08/11 17:02:33, tony wrote:
> This all seems really complex to me.  Why don't we just enumerate all the
> keybindings at start up
> (http://library.gnome.org/devel/gtk/unstable/gtk-Bindings.html) and pass them
> down to the renderer when the renderer is created.  This data could be used to
> populate the table in editor_client_impl.cc.
> 
> If we get a signal when the bindings change, we could then send an updated
table
> to the renderers, but it doesn't look like other GTK+ apps support that (at
> least, I tried to change it while pidgin was running and nothing seemed to
> happen).

[James Su, 2009-08-24 14:24:59]

I have no idea on how to implement the solution suggested by Tony. I'd very
appreciate if you could give me some points about it.

For the unittests, I probably can add some unittests for
gtk_key_bindings_handler.cc. And I'd like to port RenderViewTest to Linux (and
probably Mac) before adding unittests for the changes to renderer.

Splitting this CL into multiples by module boundaries is straightforward, but I
wonder if it's better than one CL. Because you can't verify if a part of this CL
actually work without applying the whole CL. Even if we have unittests for each
part.

And feel free to assign relevant bugs to me, I'll try my best to fix them.

Regards
James Su

On 2009/08/24 12:25:42, hbono wrote:
> James,
> 
> I personally prefer this option proposed by tony.
> When test_shell can also load its own key-bindings at its start-up, it can fix
> some layout tests caused by key-bindings (e.g.
> 'LayoutTests/editing/input/emacs-ctrl-o.html',
> 'LayoutTests/editing/pasteboard/emacs-cntl-y-001.html', etc.) On the other
hand,
> this solution cannot fix such layout tests, right?
> 
> Also, this change is too monolithic (and complicated) for me to verify it
works
> without any regressions. I'm very wondering why you didn't split this change
> into so simple changes that you can add unit tests to each one. (I personally
> believe if it is hard to split a change into small ones, it is better to
> re-design it.) It is definitely better to provide clean and simple code that
> many engineers can read it, understand it, and change it without our help.
> 
> Finally, I recommend to work for fixing existing layout bugs, memory bugs, and
> test flakiness *BEFORE* adding such features. As written by an e-mail from
> pkasting (*1), we still need a lot of engineers for fixing such bugs even
though
> many engineers have been working very hard for them. So, to move forward
without
> fearing such regressions or flakiness, it is definitely better for you to fix
> such issues as well.
> 
> (*1)
>
http://groups.google.com/group/chromium-dev/browse_thread/thread/5d7ec37b8c2a...
> 
> Regards,
> 
> Hironori Bono
> 
> On 2009/08/11 17:02:33, tony wrote:
> > This all seems really complex to me.  Why don't we just enumerate all the
> > keybindings at start up
> > (http://library.gnome.org/devel/gtk/unstable/gtk-Bindings.html) and pass
them
> > down to the renderer when the renderer is created.  This data could be used
to
> > populate the table in editor_client_impl.cc.
> > 
> > If we get a signal when the bindings change, we could then send an updated
> table
> > to the renderers, but it doesn't look like other GTK+ apps support that (at
> > least, I tried to change it while pidgin was running and nothing seemed to
> > happen).

[Matt Tolton, 2009-08-24 16:56:55]

hbono,

Out of curiosity, why is it ok for you to submit a change like
http://codereview.chromium.org/150206 apparently without a single line
of test code, but it's not ok for James to do so?

On Mon, Aug 24, 2009 at 5:25 AM, <hbono@chromium.org> wrote:
> James,
>
> I personally prefer this option proposed by tony.
> When test_shell can also load its own key-bindings at its start-up, it
> can fix some layout tests caused by key-bindings (e.g.
> 'LayoutTests/editing/input/emacs-ctrl-o.html',
> 'LayoutTests/editing/pasteboard/emacs-cntl-y-001.html', etc.) On the
> other hand, this solution cannot fix such layout tests, right?
>
> Also, this change is too monolithic (and complicated) for me to verify
> it works without any regressions. I'm very wondering why you didn't
> split this change into so simple changes that you can add unit tests to
> each one. (I personally believe if it is hard to split a change into
> small ones, it is better to re-design it.) It is definitely better to
> provide clean and simple code that many engineers can read it,
> understand it, and change it without our help.
>
> Finally, I recommend to work for fixing existing layout bugs, memory
> bugs, and test flakiness *BEFORE* adding such features. As written by an
> e-mail from pkasting (*1), we still need a lot of engineers for fixing
> such bugs even though many engineers have been working very hard for
> them. So, to move forward without fearing such regressions or flakiness,
> it is definitely better for you to fix such issues as well.
>
> (*1)
> http://groups.google.com/group/chromium-dev/browse_thread/thread/5d7ec37b=
8c2a3974#
>
> Regards,
>
> Hironori Bono
>
> On 2009/08/11 17:02:33, tony wrote:
>>
>> This all seems really complex to me. =A0Why don't we just enumerate all
>
> the
>>
>> keybindings at start up
>> (http://library.gnome.org/devel/gtk/unstable/gtk-Bindings.html) and
>
> pass them
>>
>> down to the renderer when the renderer is created. =A0This data could be
>
> used to
>>
>> populate the table in editor_client_impl.cc.
>
>> If we get a signal when the bindings change, we could then send an
>
> updated table
>>
>> to the renderers, but it doesn't look like other GTK+ apps support
>
> that (at
>>
>> least, I tried to change it while pidgin was running and nothing
>
> seemed to
>>
>> happen).
>
>
>
> http://codereview.chromium.org/165293
>

[tony, 2009-08-24 17:10:49]

Since it's not possible to get keybindings on startup, I think this is the right
approach.  That said, I defer the actual review to Darin.

http://codereview.chromium.org/165293/diff/2001/3001
File chrome/browser/renderer_host/gtk_key_bindings_handler.cc (right):

http://codereview.chromium.org/165293/diff/2001/3001#newcode84
Line 84: edit_commands_.push_back(name + ":" + value);
It seems simpler and safer to pass a pair of strings (or a struct of 2 strings)
than to use a string delimiter.

http://codereview.chromium.org/165293/diff/2001/3014
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/165293/diff/2001/3014#newcode3365
Line 3365: frame->executeCommand(WebString::fromUTF8(name),
Should we only continue if the last command succeeded?  Maybe we should DCHECK
that executeCommand returns true?

[James Su, 2009-08-25 06:13:19]

CL updated according to Tony's comment.

http://codereview.chromium.org/165293/diff/2001/3001
File chrome/browser/renderer_host/gtk_key_bindings_handler.cc (right):

http://codereview.chromium.org/165293/diff/2001/3001#newcode84
Line 84: edit_commands_.push_back(name + ":" + value);
On 2009/08/24 17:10:50, tony wrote:
> It seems simpler and safer to pass a pair of strings (or a struct of 2
strings)
> than to use a string delimiter.

I changed to use std::vector<std::pair<std::string, std::string> >, do you think
it's ok?

http://codereview.chromium.org/165293/diff/2001/3014
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/165293/diff/2001/3014#newcode3365
Line 3365: frame->executeCommand(WebString::fromUTF8(name),
On 2009/08/24 17:10:50, tony wrote:
> Should we only continue if the last command succeeded?  Maybe we should DCHECK
> that executeCommand returns true?

The return value shall be check, but I don't think DCHECK is required here, as
returning false here is valid.

[James Su, 2009-08-28 08:32:54]

An unittest has been added for gtk_key_bindings_handler.cc.

Regards
James Su

[James Su, 2009-09-01 06:45:27]

Ping.

On 2009/08/28 08:32:54, James Su wrote:
> An unittest has been added for gtk_key_bindings_handler.cc.
> 
> Regards
> James Su

[Nico, 2009-09-04 02:21:43]

Darin, did you have a chance to look at this? This CL will have an influence on
how we handle the same problem on OS X, and we want to tackle this soonish on
the OS X side.

[darin (slow to review), 2009-09-04 06:47:15]

Sorry for the huge delay in getting this feedback to you.

http://codereview.chromium.org/165293/diff/6028/7032
File chrome/browser/renderer_host/gtk_key_bindings_handler.h (right):

http://codereview.chromium.org/165293/diff/6028/7032#newcode6
Line 6: #define CHROME_BROWSER_RENDERER_HOST_GTK_KEY_BINDINGS_HANDLER_H_
i'm not a good reviewer for this file.

http://codereview.chromium.org/165293/diff/6028/7032#newcode127
Line 127: std::vector<std::pair<std::string, std::string> > edit_commands_;
use the EditCommands typedef?  perhaps that typedef should not be defined in
renderer_messages.h since including that file is expensive (it includes a lot of
other headers).

http://codereview.chromium.org/165293/diff/6028/7035
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/165293/diff/6028/7035#newcode109
Line 109: DLOG(INFO) << "Key" << (event->type == GDK_KEY_PRESS ? "Press" :
"Release")
is this important for all developers to see all of the time?

http://codereview.chromium.org/165293/diff/6028/7043
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/165293/diff/6028/7043#newcode144
Line 144: // * edit_commands (std::vector<std::pair<std::string, std::string> >)
nit: looks like the comment doesn't match the code.

i would just avoid mentioning the type in the comment since it is not necessary
to repeat it here.

http://codereview.chromium.org/165293/diff/6028/7046
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/165293/diff/6028/7046#newcode3357
Line 3357: const std::vector<std::pair<std::string, std::string> >&
edit_commands) {
did you mean to use the EditCommands typedef?

http://codereview.chromium.org/165293/diff/6028/7047
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/165293/diff/6028/7047#newcode321
Line 321: OnKeyEventHandled();
In this context, the "On" prefix is reserved for IPC message handlers.  Please
consider naming this DidHandleKeyEvent instead.

http://codereview.chromium.org/165293/diff/6028/7049
File webkit/glue/editor_client_impl.cc (right):

http://codereview.chromium.org/165293/diff/6028/7049#newcode686
Line 686: if ((d && d->ExecuteEditCommandsForCurrentKeyEvent()) ||
I don't think it is necessary to be specific about running edit commands at this
layer.  All you want to do here is give the WebViewDelegate a chance to suppress
the default key event handler.

How about naming the WebViewDelegate method something more like
"AllowDefaultKeyboardEventHandler"?

Then the WebViewDelegate can suppress the default keyboard event handler for
whatever reason it likes.

http://codereview.chromium.org/165293/diff/6028/7050
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/165293/diff/6028/7050#newcode622
Line 622: if (handler->keyEvent(evt)) {
nice cleanup, thanks!

[James Su, 2009-09-04 07:41:27]

Thanks a lot for your feedback. I'll update this CL accordingly as soon as
possible.

Regards
James Su

http://codereview.chromium.org/165293/diff/6028/7032
File chrome/browser/renderer_host/gtk_key_bindings_handler.h (right):

http://codereview.chromium.org/165293/diff/6028/7032#newcode127
Line 127: std::vector<std::pair<std::string, std::string> > edit_commands_;
On 2009/09/04 06:47:15, darin wrote:
> use the EditCommands typedef?  perhaps that typedef should not be defined in
> renderer_messages.h since including that file is expensive (it includes a lot
of
> other headers).

Any better place for this typedef?

[darin (slow to review), 2009-09-04 20:04:08]

On Fri, Sep 4, 2009 at 12:41 AM, <suzhe@chromium.org> wrote:

> Thanks a lot for your feedback. I'll update this CL accordingly as soon
> as possible.
>
> Regards
> James Su
>
>
> http://codereview.chromium.org/165293/diff/6028/7032
> File chrome/browser/renderer_host/gtk_key_bindings_handler.h (right):
>
> http://codereview.chromium.org/165293/diff/6028/7032#newcode127
> Line 127: std::vector<std::pair<std::string, std::string> >
> edit_commands_;
> On 2009/09/04 06:47:15, darin wrote:
>
>> use the EditCommands typedef?  perhaps that typedef should not be
>>
> defined in
>
>> renderer_messages.h since including that file is expensive (it
>>
> includes a lot of
>
>> other headers).
>>
>
> Any better place for this typedef?
>
>
I recommend just creating a new header file for it.
-Darin



>
> http://codereview.chromium.org/165293
>

[darin (slow to review), 2009-09-04 20:17:47]

On Fri, Sep 4, 2009 at 12:40 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Fri, Sep 4, 2009 at 12:41 AM, <suzhe@chromium.org> wrote:
>
>> Thanks a lot for your feedback. I'll update this CL accordingly as soon
>> as possible.
>>
>> Regards
>> James Su
>>
>>
>> http://codereview.chromium.org/165293/diff/6028/7032
>> File chrome/browser/renderer_host/gtk_key_bindings_handler.h (right):
>>
>> http://codereview.chromium.org/165293/diff/6028/7032#newcode127
>> Line 127: std::vector<std::pair<std::string, std::string> >
>> edit_commands_;
>> On 2009/09/04 06:47:15, darin wrote:
>>
>>> use the EditCommands typedef?  perhaps that typedef should not be
>>>
>> defined in
>>
>>> renderer_messages.h since including that file is expensive (it
>>>
>> includes a lot of
>>
>>> other headers).
>>>
>>
>> Any better place for this typedef?
>>
>>
> I recommend just creating a new header file for it.
> -Darin
>
>
You could go one step further and make typedefs for both the pair as well as
the vector of pairs.

EditCommand and EditCommands perhaps?

-Darin



>
>
>>
>> http://codereview.chromium.org/165293
>>
>
>

[James Su, 2009-09-06 06:04:28]

Hi,
  CL has been updated, please help review again.

Regards
James Su

http://codereview.chromium.org/165293/diff/6028/7032
File chrome/browser/renderer_host/gtk_key_bindings_handler.h (right):

http://codereview.chromium.org/165293/diff/6028/7032#newcode127
Line 127: std::vector<std::pair<std::string, std::string> > edit_commands_;

On 2009/09/04 07:41:27, James Su wrote:
> On 2009/09/04 06:47:15, darin wrote:
> > use the EditCommands typedef?  perhaps that typedef should not be defined in
> > renderer_messages.h since including that file is expensive (it includes a
lot
> of
> > other headers).
> 
> Any better place for this typedef?

I added chrome/common/edit_command_types.h to hold these typedefs. Is it ok?

http://codereview.chromium.org/165293/diff/6028/7035
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/165293/diff/6028/7035#newcode109
Line 109: DLOG(INFO) << "Key" << (event->type == GDK_KEY_PRESS ? "Press" :
"Release")
On 2009/09/04 06:47:15, darin wrote:
> is this important for all developers to see all of the time?

Deleted.

http://codereview.chromium.org/165293/diff/6028/7043
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/165293/diff/6028/7043#newcode144
Line 144: // * edit_commands (std::vector<std::pair<std::string, std::string> >)
On 2009/09/04 06:47:15, darin wrote:
> nit: looks like the comment doesn't match the code.
> 
> i would just avoid mentioning the type in the comment since it is not
necessary
> to repeat it here.

Done.

http://codereview.chromium.org/165293/diff/6028/7046
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/165293/diff/6028/7046#newcode3357
Line 3357: const std::vector<std::pair<std::string, std::string> >&
edit_commands) {
On 2009/09/04 06:47:15, darin wrote:
> did you mean to use the EditCommands typedef?

Done.

http://codereview.chromium.org/165293/diff/6028/7047
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/165293/diff/6028/7047#newcode321
Line 321: OnKeyEventHandled();
On 2009/09/04 06:47:15, darin wrote:
> In this context, the "On" prefix is reserved for IPC message handlers.  Please
> consider naming this DidHandleKeyEvent instead.

Done.

http://codereview.chromium.org/165293/diff/6028/7049
File webkit/glue/editor_client_impl.cc (right):

http://codereview.chromium.org/165293/diff/6028/7049#newcode686
Line 686: if ((d && d->ExecuteEditCommandsForCurrentKeyEvent()) ||
On 2009/09/04 06:47:15, darin wrote:
> I don't think it is necessary to be specific about running edit commands at
this
> layer.  All you want to do here is give the WebViewDelegate a chance to
suppress
> the default key event handler.
> 
> How about naming the WebViewDelegate method something more like
> "AllowDefaultKeyboardEventHandler"?
> 
> Then the WebViewDelegate can suppress the default keyboard event handler for
> whatever reason it likes.

How about HandleCurrentKeyboardEvent()? This method is actually not only for
suppressing the default key event handler, but also for handling the key event,
so that evt->setDefaultHandled() can be called properly if the event was
handled.

[James Su, 2009-09-09 01:00:49]

Ping.

On 2009/09/06 06:04:28, James Su wrote:
> Hi,
>   CL has been updated, please help review again.
> 
> Regards
> James Su
> 
> http://codereview.chromium.org/165293/diff/6028/7032
> File chrome/browser/renderer_host/gtk_key_bindings_handler.h (right):
> 
> http://codereview.chromium.org/165293/diff/6028/7032#newcode127
> Line 127: std::vector<std::pair<std::string, std::string> > edit_commands_;
> 
> On 2009/09/04 07:41:27, James Su wrote:
> > On 2009/09/04 06:47:15, darin wrote:
> > > use the EditCommands typedef?  perhaps that typedef should not be defined
in
> > > renderer_messages.h since including that file is expensive (it includes a
> lot
> > of
> > > other headers).
> > 
> > Any better place for this typedef?
> 
> I added chrome/common/edit_command_types.h to hold these typedefs. Is it ok?
> 
> http://codereview.chromium.org/165293/diff/6028/7035
> File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):
> 
> http://codereview.chromium.org/165293/diff/6028/7035#newcode109
> Line 109: DLOG(INFO) << "Key" << (event->type == GDK_KEY_PRESS ? "Press" :
> "Release")
> On 2009/09/04 06:47:15, darin wrote:
> > is this important for all developers to see all of the time?
> 
> Deleted.
> 
> http://codereview.chromium.org/165293/diff/6028/7043
> File chrome/common/render_messages_internal.h (right):
> 
> http://codereview.chromium.org/165293/diff/6028/7043#newcode144
> Line 144: // * edit_commands (std::vector<std::pair<std::string, std::string>
>)
> On 2009/09/04 06:47:15, darin wrote:
> > nit: looks like the comment doesn't match the code.
> > 
> > i would just avoid mentioning the type in the comment since it is not
> necessary
> > to repeat it here.
> 
> Done.
> 
> http://codereview.chromium.org/165293/diff/6028/7046
> File chrome/renderer/render_view.cc (right):
> 
> http://codereview.chromium.org/165293/diff/6028/7046#newcode3357
> Line 3357: const std::vector<std::pair<std::string, std::string> >&
> edit_commands) {
> On 2009/09/04 06:47:15, darin wrote:
> > did you mean to use the EditCommands typedef?
> 
> Done.
> 
> http://codereview.chromium.org/165293/diff/6028/7047
> File chrome/renderer/render_widget.cc (right):
> 
> http://codereview.chromium.org/165293/diff/6028/7047#newcode321
> Line 321: OnKeyEventHandled();
> On 2009/09/04 06:47:15, darin wrote:
> > In this context, the "On" prefix is reserved for IPC message handlers. 
Please
> > consider naming this DidHandleKeyEvent instead.
> 
> Done.
> 
> http://codereview.chromium.org/165293/diff/6028/7049
> File webkit/glue/editor_client_impl.cc (right):
> 
> http://codereview.chromium.org/165293/diff/6028/7049#newcode686
> Line 686: if ((d && d->ExecuteEditCommandsForCurrentKeyEvent()) ||
> On 2009/09/04 06:47:15, darin wrote:
> > I don't think it is necessary to be specific about running edit commands at
> this
> > layer.  All you want to do here is give the WebViewDelegate a chance to
> suppress
> > the default key event handler.
> > 
> > How about naming the WebViewDelegate method something more like
> > "AllowDefaultKeyboardEventHandler"?
> > 
> > Then the WebViewDelegate can suppress the default keyboard event handler for
> > whatever reason it likes.
> 
> How about HandleCurrentKeyboardEvent()? This method is actually not only for
> suppressing the default key event handler, but also for handling the key
event,
> so that evt->setDefaultHandled() can be called properly if the event was
> handled.

[darin (slow to review), 2009-09-09 07:32:31]

looking good...

http://codereview.chromium.org/165293/diff/15008/14027
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/165293/diff/15008/14027#newcode10
Line 10: #include <utility>
nit: i don't think you need to include this anymore

http://codereview.chromium.org/165293/diff/15008/14030
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/165293/diff/15008/14030#newcode64
Line 64: #include "webkit/api/public/WebInputEvent.h"
nit: is this change necessary?

http://codereview.chromium.org/165293/diff/15008/14030#newcode118
Line 118: using WebKit::WebInputEvent;
nit: is this change necessary?

http://codereview.chromium.org/165293/diff/15008/14030#newcode3399
Line 3399: if (edit_commands_.empty() || !webview())
if you get called, then the webview() must be valid.  the RenderView owns the
webview, and we only reach this method if we called handleInputEvent.

http://codereview.chromium.org/165293/diff/15008/14029
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/165293/diff/15008/14029#newcode24
Line 24: #include "chrome/common/edit_command_types.h"
nit: the _types.h suffix is a little uncommon in chromium code.  how about just
calling this edit_command.h?

it might also be nice if the EditCommand were a struct with fields named "name"
and "value".  that would probably help improve readability a bit in places where
it is used.

http://codereview.chromium.org/165293/diff/15008/14035
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/165293/diff/15008/14035#newcode753
Line 753: virtual bool HandleCurrentKeyboardEvent() {
> How about HandleCurrentKeyboardEvent()? This method is
> actually not only for suppressing the default key event
> handler, but also for handling the key event, so that
> evt->setDefaultHandled() can be called properly if the
> event was handled.

ah, i missed the fact that this needs to return a boolean.  good point!  so a
name like this makes more sense.

i think it would be good to modify the comment to spell out the fact that the
"CurrentKeyboardEvent" refers to the keyboard event passed to handleInputEvent. 
perhaps you could say that HandleCurrentKeyboardEvent is called in response to
handleInputEvent when the default action for the keyboard event is not
suppressed by the page.

[James Su, 2009-09-09 10:45:23]

Thanks for your feedback. CL has been updated, please help review again.

Regards
James Su

http://codereview.chromium.org/165293/diff/15008/14027
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/165293/diff/15008/14027#newcode10
Line 10: #include <utility>
On 2009/09/09 07:32:31, darin wrote:
> nit: i don't think you need to include this anymore

Done.

http://codereview.chromium.org/165293/diff/15008/14030
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/165293/diff/15008/14030#newcode64
Line 64: #include "webkit/api/public/WebInputEvent.h"
On 2009/09/09 07:32:31, darin wrote:
> nit: is this change necessary?

Done.

http://codereview.chromium.org/165293/diff/15008/14030#newcode118
Line 118: using WebKit::WebInputEvent;
On 2009/09/09 07:32:31, darin wrote:
> nit: is this change necessary?

Done.

http://codereview.chromium.org/165293/diff/15008/14030#newcode3399
Line 3399: if (edit_commands_.empty() || !webview())
On 2009/09/09 07:32:31, darin wrote:
> if you get called, then the webview() must be valid.  the RenderView owns the
> webview, and we only reach this method if we called handleInputEvent.

Done.

http://codereview.chromium.org/165293/diff/15008/14029
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/165293/diff/15008/14029#newcode24
Line 24: #include "chrome/common/edit_command_types.h"
On 2009/09/09 07:32:31, darin wrote:
> nit: the _types.h suffix is a little uncommon in chromium code.  how about
just
> calling this edit_command.h?
> 
> it might also be nice if the EditCommand were a struct with fields named
"name"
> and "value".  that would probably help improve readability a bit in places
where
> it is used.

Done.

http://codereview.chromium.org/165293/diff/15008/14035
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/165293/diff/15008/14035#newcode753
Line 753: virtual bool HandleCurrentKeyboardEvent() {
On 2009/09/09 07:32:31, darin wrote:
> > How about HandleCurrentKeyboardEvent()? This method is
> > actually not only for suppressing the default key event
> > handler, but also for handling the key event, so that
> > evt->setDefaultHandled() can be called properly if the
> > event was handled.
> 
> ah, i missed the fact that this needs to return a boolean.  good point!  so a
> name like this makes more sense.
> 
> i think it would be good to modify the comment to spell out the fact that the
> "CurrentKeyboardEvent" refers to the keyboard event passed to
handleInputEvent. 
> perhaps you could say that HandleCurrentKeyboardEvent is called in response to
> handleInputEvent when the default action for the keyboard event is not
> suppressed by the page.

Done.

[darin (slow to review), 2009-09-09 17:10:30]

LGTM w/ comment nit:

http://codereview.chromium.org/165293/diff/16010/16032
File webkit/glue/webview_delegate.h (right):

http://codereview.chromium.org/165293/diff/16010/16032#newcode751
Line 751: // WebViewImpl::handleInputEvent().
nit: please don't mention WebViewImpl here.  that name is an implementation
detail.  how about writing "passed to the WebView's handleInputEvent method."

that is better than WebView::handleInputEvent() since handleInputEvent is not a
method of WebView :-)

[James Su, 2009-09-09 17:19:57]

Thanks for your review. I just updated the comment. Shall I submit this CL right
now or wait for others' feedback?

Regards
James Su

On 2009/09/09 17:10:30, darin wrote:
> LGTM w/ comment nit:
> 
> http://codereview.chromium.org/165293/diff/16010/16032
> File webkit/glue/webview_delegate.h (right):
> 
> http://codereview.chromium.org/165293/diff/16010/16032#newcode751
> Line 751: // WebViewImpl::handleInputEvent().
> nit: please don't mention WebViewImpl here.  that name is an implementation
> detail.  how about writing "passed to the WebView's handleInputEvent method."
> 
> that is better than WebView::handleInputEvent() since handleInputEvent is not
a
> method of WebView :-)

[Evan Martin, 2009-09-09 17:26:12]

I read through this and it looks awesome.  Please double-check that these still
work in web content: plain text, CJK text, arrow keys, home, end, delete, and
then submit.  :)

[Hironori Bono, 2009-09-10 06:24:22]

Sorry for my stupid question in advance.
Even though I don't have any objections against this change, I'm wondering what
DOM events we should send when we send an EditCommand to a renderer. We still
need to send keyboard events?

Regards,

Hironori Bono

[James Su, 2009-09-10 08:34:08]

Good question.

Edit commands associated to a keyboard event can be treated as the default
action of the event. According to http://www.w3.org/TR/DOM-Level-3-Events/, the
keyboard event shall be dispatched to DOM first and the default action shall
only be triggered if the keyboard event was not handled nor cancelled.

AFAIK, edit command is actually a part of HTML5 spec, see:
http://www.w3.org/TR/html5/editing.html. Regarding to the DOM event associated
to an edit command, the spec has following statement:

All of the actions defined above, whether triggered by the user or
programmatically (e.g. by execCommand() commands), must fire mutation events as
appropriate.

For mutation events, please refer to:
http://www.w3.org/TR/DOM-Level-3-Events/#events-mutationevents

Regards
James Su

On 2009/09/10 06:24:22, hbono wrote:
> Sorry for my stupid question in advance.
> Even though I don't have any objections against this change, I'm wondering
what
> DOM events we should send when we send an EditCommand to a renderer. We still
> need to send keyboard events?
> 
> Regards,
> 
> Hironori Bono