Add a new constructor to KeyEvent.

Adds the ability to construct a KeyEvent from a NativeEvent[2], and converts some code to use it.

Removes some of the Windows-specific stuff from KeyEvent.

BUG=72040
TEST=none


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

[Ben Goodger (Google), 2011-02-10 20:27:36]

Sending this for comment and review.

sky - everything
rjkroege/sadrul/backer - GDK/X bits.

[Ben Goodger (Google), 2011-02-10 20:39:44]

Note also that this doesn't quite compile yet. I am figuring on the trybots to
help me here with Gtk, and the TOUCH_UI folk to help me with X :-)

[sadrul-g, 2011-02-10 20:51:24]

On Thu, Feb 10, 2011 at 3:39 PM,  <ben@chromium.org> wrote:
> Note also that this doesn't quite compile yet. I am figuring on the trybots to
> help me here with Gtk, and the TOUCH_UI folk to help me with X :-)
>
> http://codereview.chromium.org/6487002/

I think at the moment, we have XEvent version of KeyEvent, MouseEvent
and TouchEvent. So GDK version of these event constructors shouldn't
be necessary anymore.

Sadrul

[Ben Goodger (Google), 2011-02-10 20:57:24]

We will need them at least for ChromeOS, which is still using Gtk.

Once we get the new windowing system built (current project), we should be
able to remove that dependency.

-Ben

On Thu, Feb 10, 2011 at 12:50 PM, Sadrul Chowdhury <sadrul@google.com>wrote:

> On Thu, Feb 10, 2011 at 3:39 PM,  <ben@chromium.org> wrote:
> > Note also that this doesn't quite compile yet. I am figuring on the
> trybots to
> > help me here with Gtk, and the TOUCH_UI folk to help me with X :-)
> >
> > http://codereview.chromium.org/6487002/
>
> I think at the moment, we have XEvent version of KeyEvent, MouseEvent
> and TouchEvent. So GDK version of these event constructors shouldn't
> be necessary anymore.
>
> Sadrul
>

[sky, 2011-02-10 20:59:35]

http://codereview.chromium.org/6487002/diff/7111/views/events/event.cc
File views/events/event.cc (right):

http://codereview.chromium.org/6487002/diff/7111/views/events/event.cc#newcode12
views/events/event.cc:12: : type_(type),
Initialize native_event_ and native_event_2_ here and in all constructors.

http://codereview.chromium.org/6487002/diff/7111/views/focus/accelerator_hand...
File views/focus/accelerator_handler.h (right):

http://codereview.chromium.org/6487002/diff/7111/views/focus/accelerator_hand...
views/focus/accelerator_handler.h:63: static std::vector<MSG> current_messages_;
statics can only be plain old data (POD).

[rjkroege, 2011-02-10 21:15:26]

I've added Bryan who knows the keyboard event injection code.

http://codereview.chromium.org/6487002/diff/7111/views/events/event.h
File views/events/event.h (right):

http://codereview.chromium.org/6487002/diff/7111/views/events/event.h#newcode97
views/events/event.h:97: // of woe.
I'd a TODO here or in native_types.h that was a little bit more explicit in case
we've forgotten what needs to happen later.

[Ben Goodger (Google), 2011-02-10 21:22:37]

http://codereview.chromium.org/6487002/diff/7111/views/events/event.h
File views/events/event.h (right):

http://codereview.chromium.org/6487002/diff/7111/views/events/event.h#newcode97
views/events/event.h:97: // of woe.
I rewrote the comment in native_types.h as:


// Once we replace usage of Gtk/Gdk types with Xlib types across the board in
// views, we can remove NativeEvent2 and typedef XEvent* to NativeEvent. The
// world will then be beautiful(ish).

[Ben Goodger (Google), 2011-02-10 21:30:47]

OK as of this latest version this change now builds on every platform covered by
our trybots. Scott, I've addressed the items you pointed out. Specifically
looking for Touch/X feedback at this time.

[sky, 2011-02-10 21:50:31]

LGTM

[sadrul, 2011-02-10 21:52:01]

I also needed the following changes:


diff --git a/views/widget/gtk_views_window.cc b/views/widget/gtk_views_window.cc
index b94c2cd..7da1490 100644
--- a/views/widget/gtk_views_window.cc
+++ b/views/widget/gtk_views_window.cc
@@ -18,7 +18,7 @@ static void gtk_views_window_move_focus(GtkWindow* window,
   if (focus_manager) {
     GdkEvent* key = gtk_get_current_event();
     if (key && key->type == GDK_KEY_PRESS) {
-      views::KeyEvent key_event(reinterpret_cast<GdkEventKey*>(key));
+      views::KeyEvent key_event(key);
       focus_manager->OnKeyEvent(key_event);
     }
   } else if (GTK_WINDOW_CLASS(gtk_views_window_parent_class)->move_focus) {
diff --git a/views/widget/widget_gtk.cc b/views/widget/widget_gtk.cc
index d40ec45..fc3e616 100644
--- a/views/widget/widget_gtk.cc
+++ b/views/widget/widget_gtk.cc
@@ -882,7 +882,7 @@ bool WidgetGtk::HandleKeyboardEvent(GdkEventKey* event) {
   if (!focus_manager_)
     return false;
 
-  KeyEvent key(event);
+  KeyEvent key(reinterpret_cast<NativeEvent>(event));
   int key_code = key.key_code();
   bool handled = false;
 
@@ -1168,7 +1168,7 @@ gboolean WidgetGtk::OnFocusOut(GtkWidget* widget,
GdkEventFocus* event) {
 }
 
 gboolean WidgetGtk::OnKeyEvent(GtkWidget* widget, GdkEventKey* event) {
-  KeyEvent key(event);
+  KeyEvent key(reinterpret_cast<NativeEvent>(event));
 
   // Always reset |should_handle_menu_key_release_| unless we are handling a
   // VKEY_MENU key release event. It ensures that VKEY_MENU accelerator can
only

diff --git a/views/controls/textfield/native_textfield_gtk.cc
b/views/controls/textfield/native_textfield_gtk.cc
index 0b7cba1..be5b713 100644
--- a/views/controls/textfield/native_textfield_gtk.cc
+++ b/views/controls/textfield/native_textfield_gtk.cc
@@ -384,7 +384,7 @@ gboolean NativeTextfieldGtk::OnKeyPressEventHandler(
 gboolean NativeTextfieldGtk::OnKeyPressEvent(GdkEventKey* event) {
   Textfield::Controller* controller = textfield_->GetController();
   if (controller) {
-    KeyEvent key_event(event);
+    KeyEvent key_event(reinterpret_cast<NativeEvent>(event));
     return controller->HandleKeyEvent(textfield_, key_event);
   }
   return false;
@@ -407,7 +407,7 @@ gboolean NativeTextfieldGtk::OnActivate() {
 
   Textfield::Controller* controller = textfield_->GetController();
   if (controller) {
-    KeyEvent views_key_event(key_event);
+    KeyEvent views_key_event(event);
     handled = controller->HandleKeyEvent(textfield_, views_key_event);
   }

http://codereview.chromium.org/6487002/diff/3067/views/events/event_gtk.cc
File views/events/event_gtk.cc (right):

http://codereview.chromium.org/6487002/diff/3067/views/events/event_gtk.cc#ne...
views/events/event_gtk.cc:65: native_event_2 = NULL;
native_event_2_

http://codereview.chromium.org/6487002/diff/4074/views/events/event_x.cc
File views/events/event_x.cc (right):

http://codereview.chromium.org/6487002/diff/4074/views/events/event_x.cc#newc...
views/events/event_x.cc:233: native_event_2 = NULL;
native_event_2_

[bryeung, 2011-02-11 02:38:16]

LGTM (wrt the keyboard event injection code)

[sadrul, 2011-02-11 16:12:47]

http://codereview.chromium.org/6487002/diff/1095/views/events/event_gtk.cc
File views/events/event_gtk.cc (right):

http://codereview.chromium.org/6487002/diff/1095/views/events/event_gtk.cc#ne...
views/events/event_gtk.cc:63: void Event::Init() {
Event::Init and Event::InitWithNativeEvent are also in event_x.cc. So these can
be removed from here (or wrapped inside #if !defined(TOUCH_UI)

http://codereview.chromium.org/6487002/diff/1095/views/events/event_x.cc
File views/events/event_x.cc (right):

http://codereview.chromium.org/6487002/diff/1095/views/events/event_x.cc#newc...
views/events/event_x.cc:233: native_event_2 = NULL;
native_event_2_

[sky, 2011-02-11 16:42:41]

LGTM