linux_aura: Port GtkKeybindingsHandler to Aura.

linux_aura: Port GtkKeybindingsHandler to Aura.

This is a quick hack to move the current GtkKeybindingsHandler code into
aura builds. This is necessary so that Linux users who have set custom
keybindings can use them in edit fields. This only works in the content
area; further patches are needed for this to work in views.

This redoes the interface so that instead of being hard coded into
content, we have a general interface set in ui/wm/. This should allow
views to also use this code in ui/views/, as well as its current usage
in content/.

BUG=319437, 358297
R=piman@chromium.org, sadrul@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260900

[Elliot Glaysher, 2014-03-26 21:42:41]

I am not happy with this patch.

The way the preexisting GtkKeybindingsHandler works is crazy; it creates a
GtkTextView subclass and overrides all the implementation of things like
DeleteWord to add a "DeleteWord" blink command, then sends those to the renderer
instead. That's really hacky. We certainly don't want this long term, but kareng
mentioned that the lack of emacs keybindings may be the bug cluster that blows
up in linux aura.

Things that still need to be done in this patch:

- Get the unit tests linking. I've gotten them to the point where they compile,
but I suspect there is no way to get them linking as part of the main unit_tests
binary due to the GTK dependency. This will require making a new binary, along
with all the bot junk that requires.

- Figure out what is intercepting C-w before they get to the renderer. C-w is
probably the most painful key to not have the emacs override for.

https://codereview.chromium.org/213283004/diff/1/chrome/browser/ui/libgtk2ui/...
File chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc (right):

https://codereview.chromium.org/213283004/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:105: // point?
piman, do you have any thoughts on what this is supposed to be?

[piman, 2014-03-26 23:44:25]

https://codereview.chromium.org/213283004/diff/1/chrome/browser/ui/libgtk2ui/...
File chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc (right):

https://codereview.chromium.org/213283004/diff/1/chrome/browser/ui/libgtk2ui/...
chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:105: // point?
On 2014/03/26 21:42:41, Elliot Glaysher wrote:
> piman, do you have any thoughts on what this is supposed to be?

My reading of it:
GDK installs in the core keymap some keysym on some modifier keys, to switch
between 2 GDK groups. There's code to find which modifier(s) that is if any, and
then sets this to 1 if the modifier is on.

Maybe it's ok to punt for now though.
1- Non-xkb servers must be rare
2- the likelihood of that modifier key to be relevant for the edit commands is
low

The intersection of 1 and 2 is likely to be extremely small!

https://codereview.chromium.org/213283004/diff/20001/content/public/browser/k...
File content/public/browser/keybinding_handler_aurax11.h (right):

https://codereview.chromium.org/213283004/diff/20001/content/public/browser/k...
content/public/browser/keybinding_handler_aurax11.h:17: class CONTENT_EXPORT
KeybindingHandlerAuraX11 {
Could it make sense to have this and the other classes at a lower level, e.g.
aura?
1- to not expand the content API unnecessarily
2- so that this could be reused in other places, say the omnibox

[Elliot Glaysher, 2014-03-28 22:36:25]

msw: fyi.

OK, so this is now feature complete in the content area, but doesn't work at all
in views yet. This moves the interface down to the ui/wm/ layer so that the code
can be reused with views though.

This patch still hasn't gotten the unit tests linking. That will be Monday's
task.

[piman, 2014-03-28 22:40:23]

LGTM for content/ and the overall approach.

[Elliot Glaysher, 2014-03-31 20:07:12]

sky: views review

[sky, 2014-03-31 20:44:04]

https://codereview.chromium.org/213283004/diff/100001/ui/wm/public/text_edit_...
File ui/wm/public/text_edit_command_x11.cc (right):

https://codereview.chromium.org/213283004/diff/100001/ui/wm/public/text_edit_...
ui/wm/public/text_edit_command_x11.cc:114: default:
Can't you remove this to get a compile error if a new command is added without
updating this?

https://codereview.chromium.org/213283004/diff/100001/ui/wm/public/text_edit_...
File ui/wm/public/text_edit_command_x11.h (right):

https://codereview.chromium.org/213283004/diff/100001/ui/wm/public/text_edit_...
ui/wm/public/text_edit_command_x11.h:5: #ifndef
UI_WM_PUBLIC_TEXT_EDIT_COMMAND_X11_H_
I tend to feel these classes are not particularly window manager like. Is there
a reason they need to live here and not some where else in ui? Maybe ui/base or
event ui/events? (ping Sadrul if you like the later).

https://codereview.chromium.org/213283004/diff/100001/ui/wm/public/text_edit_...
ui/wm/public/text_edit_command_x11.h:16: class WM_CORE_EXPORT TextEditCommandX11
{
Document copy and assignment explicitly allowed.

https://codereview.chromium.org/213283004/diff/100001/ui/wm/public/text_edit_...
ui/wm/public/text_edit_command_x11.h:63: std::string argument() const { return
argument_; }
const std::string&

https://codereview.chromium.org/213283004/diff/100001/ui/wm/public/text_edit_...
ui/wm/public/text_edit_command_x11.h:72: CommandId command_id_;
const (same for 74).

[Elliot Glaysher, 2014-03-31 21:40:09]

Per sky@, redirecting to sadrul@ now that the interface has been moved from
ui/wm/ to ui/events/.

https://codereview.chromium.org/213283004/diff/100001/ui/wm/public/text_edit_...
File ui/wm/public/text_edit_command_x11.h (right):

https://codereview.chromium.org/213283004/diff/100001/ui/wm/public/text_edit_...
ui/wm/public/text_edit_command_x11.h:72: CommandId command_id_;
On 2014/03/31 20:44:05, sky wrote:
> const (same for 74).

You can't have const members for classes which can be copy constructed.

[sadrul, 2014-03-31 22:58:55]

I am not familiar with the GTK+ bindings at this level. So my review for
libgtk2ui may be lacking. But I looked through the docs, and in general, looks
good. I have a couple of questions about the X display used, and the interaction
with the IME.

ui/events/ views lgtm

https://codereview.chromium.org/213283004/diff/120001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc (right):

https://codereview.chromium.org/213283004/diff/120001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:40: if
(XkbQueryExtension(gfx::GetXDisplay(), &opcode, &event, &error,
has_xkb_ = XkbQueryExtension(....

https://codereview.chromium.org/213283004/diff/120001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:53: if
(!event.IsKeyEvent())
Looks like this is only called from places that should always send key-events
here. Should this be a CHECK?

https://codereview.chromium.org/213283004/diff/120001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:57: if
(key_event.is_char() || !key_event.native_event())
Is the native-event preserved once it goes through IME?

https://codereview.chromium.org/213283004/diff/120001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:102: GdkKeymap *keymap
= gdk_keymap_get_for_display(gdk_display_get_default());
We create our own connection to the display in the message-pump, and generally
use that everywhere, except for the GTK windows we still use (file-selection
dialog, etc.). I can't think of something that would break because of using
gdk_display_get_default() here, but wanted to point it out anyway (in case we do
see some breakage, this would be one place to look at).

https://codereview.chromium.org/213283004/diff/120001/ui/events/x/text_edit_c...
File ui/events/x/text_edit_command_x11.h (right):

https://codereview.chromium.org/213283004/diff/120001/ui/events/x/text_edit_c...
ui/events/x/text_edit_command_x11.h:14: // Represents a command that performs a
specific transformation on text.
s/transformation/operation/? (since not everything the following list is a
transformation)

[Elliot Glaysher, 2014-03-31 23:16:24]

https://codereview.chromium.org/213283004/diff/120001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc (right):

https://codereview.chromium.org/213283004/diff/120001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:53: if
(!event.IsKeyEvent())
On 2014/03/31 22:58:56, sadrul wrote:
> Looks like this is only called from places that should always send key-events
> here. Should this be a CHECK?

Done.

https://codereview.chromium.org/213283004/diff/120001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:57: if
(key_event.is_char() || !key_event.native_event())
On 2014/03/31 22:58:56, sadrul wrote:
> Is the native-event preserved once it goes through IME?

It should be; this is copied straight from the GTK+ version of this class. In
the GTK version of this class which lives in content, the event objects here
maintain the native event.

https://codereview.chromium.org/213283004/diff/120001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:102: GdkKeymap *keymap
= gdk_keymap_get_for_display(gdk_display_get_default());
On 2014/03/31 22:58:56, sadrul wrote:
> We create our own connection to the display in the message-pump, and generally
> use that everywhere, except for the GTK windows we still use (file-selection
> dialog, etc.). I can't think of something that would break because of using
> gdk_display_get_default() here, but wanted to point it out anyway (in case we
do
> see some breakage, this would be one place to look at).

I haven't seen any problems in local testing, or on the trybots. I'm not sure
what I'd replace this call with. This is one of the only places I know about
where we have to hack XEvents into synthetic GdkEvents, and we do need a
GdkKeymap here.

[Elliot Glaysher, 2014-03-31 23:16:55]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-03-31 23:17:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/213283004/140001

[piman, 2014-03-31 23:29:40]

On Mon, Mar 31, 2014 at 4:16 PM, <erg@chromium.org> wrote:

>
> https://codereview.chromium.org/213283004/diff/120001/
> chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc
> File chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc (right):
>
> https://codereview.chromium.org/213283004/diff/120001/
> chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc#newcode53
> chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:53: if
> (!event.IsKeyEvent())
> On 2014/03/31 22:58:56, sadrul wrote:
>
>> Looks like this is only called from places that should always send
>>
> key-events
>
>> here. Should this be a CHECK?
>>
>
> Done.
>
>
> https://codereview.chromium.org/213283004/diff/120001/
> chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc#newcode57
> chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:57: if
> (key_event.is_char() || !key_event.native_event())
> On 2014/03/31 22:58:56, sadrul wrote:
>
>> Is the native-event preserved once it goes through IME?
>>
>
> It should be; this is copied straight from the GTK+ version of this
> class. In the GTK version of this class which lives in content, the
> event objects here maintain the native event.
>
>
> https://codereview.chromium.org/213283004/diff/120001/
> chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc#newcode102
> chrome/browser/ui/libgtk2ui/gtk2_key_bindings_handler.cc:102: GdkKeymap
> *keymap = gdk_keymap_get_for_display(gdk_display_get_default());
> On 2014/03/31 22:58:56, sadrul wrote:
>
>> We create our own connection to the display in the message-pump, and
>>
> generally
>
>> use that everywhere, except for the GTK windows we still use
>>
> (file-selection
>
>> dialog, etc.). I can't think of something that would break because of
>>
> using
>
>> gdk_display_get_default() here, but wanted to point it out anyway (in
>>
> case we do
>
>> see some breakage, this would be one place to look at).
>>
>
> I haven't seen any problems in local testing, or on the trybots. I'm not
> sure what I'd replace this call with. This is one of the only places I
> know about where we have to hack XEvents into synthetic GdkEvents, and
> we do need a GdkKeymap here.
>

Using 2 displays can cause problems if you expect ordering between commands
on one vs on the other, but in this case I don't think we have such
assumption anyway?


>
> https://codereview.chromium.org/213283004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Elliot Glaysher, 2014-03-31 23:31:08]

On 2014/03/31 23:29:40, piman wrote:
> On Mon, Mar 31, 2014 at 4:16 PM, <mailto:erg@chromium.org> wrote:
> > I haven't seen any problems in local testing, or on the trybots. I'm not
> > sure what I'd replace this call with. This is one of the only places I
> > know about where we have to hack XEvents into synthetic GdkEvents, and
> > we do need a GdkKeymap here.
> 
> Using 2 displays can cause problems if you expect ordering between commands
> on one vs on the other, but in this case I don't think we have such
> assumption anyway?

I don't believe we do. If there were two displays, I'd be more worried about
them having different keymaps.

[commit-bot: I haz the power, 2014-03-31 23:55:03]

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

[commit-bot: I haz the power, 2014-03-31 23:55:03]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Elliot Glaysher, 2014-04-01 00:17:06]

sky: requesting owners stamp for chrome/browser/ui/browser_command_controller.cc

[sky, 2014-04-01 00:24:14]

Said file LGTM

[Elliot Glaysher, 2014-04-01 00:26:56]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-04-01 00:27:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/213283004/140001

[commit-bot: I haz the power, 2014-04-01 03:57:02]

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

[commit-bot: I haz the power, 2014-04-01 03:57:03]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Elliot Glaysher, 2014-04-01 17:32:41]

Committed patchset #8 manually as r260900 (presubmit successful).