RenderWidgetHost now stores both the WebKeyboardEvent and the native

RenderWidgetHost now only accepts a new NativeWebKeyboardEvent which owns a copy of the native os event.

Only the WebKeyboardEvent is sent over the IPC barrier, but the full structure passed to the unhandled key event handler.

BUG=4772

[Elliot Glaysher, 2009-03-03 23:57:10]

- Will delete native_message* when the code review tool returns to normal
- darin: Please review all the message stuff (and the mac stuff if you want to)
- avi: Please review all of this, including the mac stuff.

[Avi (use Gerrit), 2009-03-04 15:00:38]

LG. Good job; thanks for taking this on.

http://codereview.chromium.org/40065/diff/1001/1005
File base/native_event.h (right):

http://codereview.chromium.org/40065/diff/1001/1005#newcode36
Line 36: NativeEvent ReturnNullNativeEvent();
Should this be called "NullNativeEvent()"? x = ReturnNullNativeEvent() feels
weird, while x = NullNativeEvent() feels a lot more obvious as to the meaning.

http://codereview.chromium.org/40065/diff/1001/1010
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/40065/diff/1001/1010#newcode5
Line 5: #ifndef CHROME_BROWSER_RENDERER_HOST_RENDER_VIEW_HOST_H__
Any excuse to trim the underscores...

[Elliot Glaysher, 2009-03-05 00:44:52]

Darin, I've redid NativeEvent based on your feedback. Please take another look.

[Avi (use Gerrit), 2009-03-05 02:28:16]

It's a bit weird that you still have the |delete native_event| thing, but I
suppose it's lighter-weight than doing ref counting and passing an object
around. LBetterTM.

http://codereview.chromium.org/40065/diff/1029/56
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/40065/diff/1029/56#newcode1221
Line 1221: */
It's probably safe to just pull this block...

http://codereview.chromium.org/40065/diff/1029/70
File chrome/browser/tab_contents/web_contents_view_win.cc (right):

http://codereview.chromium.org/40065/diff/1029/70#newcode252
Line 252: if (!native_event || (!native_event->IsKeyDown() &&
!native_event->IsChar())) {
80 col

http://codereview.chromium.org/40065/diff/1029/75
File chrome/common/native_event.h (right):

http://codereview.chromium.org/40065/diff/1029/75#newcode59
Line 59: }
I can understand is_shift/control/alt, as that's state that isn't in MSG. But
it's weird to have member function that are one-platform, especially if it's
something that the class's user could do. Then again, convenience functions are
convenient...

http://codereview.chromium.org/40065/diff/1029/79
File webkit/glue/webinputevent.h (left):

http://codereview.chromium.org/40065/diff/1029/79#oldcode190
Line 190: #endif
:)

[Elliot Glaysher, 2009-03-05 18:18:58]

http://codereview.chromium.org/40065/diff/1029/56
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/40065/diff/1029/56#newcode1221
Line 1221: */
On 2009/03/05 02:28:16, Avi wrote:
> It's probably safe to just pull this block...

Eeek. How did I forget to pull that out?

http://codereview.chromium.org/40065/diff/1029/70
File chrome/browser/tab_contents/web_contents_view_win.cc (right):

http://codereview.chromium.org/40065/diff/1029/70#newcode252
Line 252: if (!native_event || (!native_event->IsKeyDown() &&
!native_event->IsChar())) {
On 2009/03/05 02:28:16, Avi wrote:
> 80 col

Done.

http://codereview.chromium.org/40065/diff/1029/75
File chrome/common/native_event.h (right):

http://codereview.chromium.org/40065/diff/1029/75#newcode59
Line 59: }
On 2009/03/05 02:28:16, Avi wrote:
> I can understand is_shift/control/alt, as that's state that isn't in MSG. But
> it's weird to have member function that are one-platform, especially if it's
> something that the class's user could do. Then again, convenience functions
are
> convenient...

I'd prefer to not take these out (or at the very least move these out of the
struct) since they enhance the readability of
WebContentsViewWin::HandleKeyboardEvent.

[darin (slow to review), 2009-03-05 21:40:00]

here's some of the review comments i had written down before we chatted in
person.  most of these are not relevant to the new design we talked about.

http://codereview.chromium.org/40065/diff/83/87
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/40065/diff/83/87#newcode315
Line 315: key_queue_.push(std::make_pair(input_event.type, native_event));
since we are only saving the NativeEvent in the case of a keyboard event,
perhaps this is where we could copy the keystate modifiers.

http://codereview.chromium.org/40065/diff/83/87#newcode317
Line 317: }
else we probably want to DCHECK native_event is null.

http://codereview.chromium.org/40065/diff/83/90
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/40065/diff/83/90#newcode80
Line 80: wke, new NativeEvent((GdkEvent*)event));
nit: use a C++ style cast.

http://codereview.chromium.org/40065/diff/83/92
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/40065/diff/83/92#newcode828
Line 828: new NativeEvent(m_hWnd, message, wparam, lparam));
it seems unfortunate to have a heap allocation here when we didn't need one
before.

http://codereview.chromium.org/40065/diff/83/103
File chrome/common/native_event.h (right):

http://codereview.chromium.org/40065/diff/83/103#newcode8
Line 8: #include "build/build_config.h"
no need for build_config.h here.  it comes in through basictypes.h

http://codereview.chromium.org/40065/diff/83/103#newcode27
Line 27: public:
nit: no need for public since it is a struct.

http://codereview.chromium.org/40065/diff/83/103#newcode50
Line 50: bool IsKeyDown() const {
nit: is_key_down and is_char since these are such simple functions?

http://codereview.chromium.org/40065/diff/83/103#newcode62
Line 62: DISALLOW_COPY_AND_ASSIGN(NativeEvent);
oh cool... i thought you might have situations where you needed to support
copying these around.

http://codereview.chromium.org/40065/diff/83/106
File chrome/common/native_event_win.cc (right):

http://codereview.chromium.org/40065/diff/83/106#newcode16
Line 16: is_alt = (GetKeyState(VK_MENU) & 0x8000) != 0;
are you sure it is wise to query key state here?   i noticed that we sometimes
do fairly non-trivial work to query the key state.  see for example
WebMouseWheelEvent.  maybe instead you should make key state be a parameter to
the constructor?

[Elliot Glaysher, 2009-03-06 01:30:03]

Patch rewritten with a subclass of WebKeyboardEvent to hold native structures
(which aren't sent over the wire), as suggested by darin.

[darin (slow to review), 2009-03-06 04:45:35]

looks much better, thanks.  just some nit picks:

http://codereview.chromium.org/40065/diff/1052/130
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/40065/diff/1052/130#newcode302
Line 302: if (WebInputEvent::IsKeyboardEventType(key_event.type)) {
previously, we would have only added to the queue if process_->channel() was
non-null.  that may be worth preserving.

http://codereview.chromium.org/40065/diff/1052/146
File chrome/common/native_web_keyboard_event.h (right):

http://codereview.chromium.org/40065/diff/1052/146#newcode6
Line 6: #define BASE_GFX_NATIVE_EVENT_H_
nit: include guard is out-of-date

http://codereview.chromium.org/40065/diff/1052/146#newcode8
Line 8: #include "build/build_config.h"
nit: no need to include this since you are including basictypes.h (probably can
exclude that too since webinputevent.h includes it)

http://codereview.chromium.org/40065/diff/1052/146#newcode28
Line 28: public:
nit: no need for public here since it is a struct

http://codereview.chromium.org/40065/diff/1052/146#newcode44
Line 44: #if defined(OS_WIN)
nit: maybe all of these member variables could have the same name:  os_event
perhaps?

http://codereview.chromium.org/40065/diff/1052/149
File chrome/common/native_web_keyboard_event_win.cc (right):

http://codereview.chromium.org/40065/diff/1052/149#newcode8
Line 8: actual_message.hwnd = NULL;
memset(&actual_message, 0, sizeof(actual_message));

however, the base class default constructor does nothing, so perhaps this one
should too.  in fact, i'd be surprised if this constructor is ever called.  is
it?  i bet you could make it private forcing people to use the other
constructor.

http://codereview.chromium.org/40065/diff/1052/149#newcode23
Line 23: NativeWebKeyboardEvent::NativeWebKeyboardEvent(
i don't think there is any reason to implement copy constructors or assignment
operators for OS_WIN.  the defaults should work properly.

[Avi (use Gerrit), 2009-03-06 18:01:34]

lgtm

http://codereview.chromium.org/40065/diff/1052/148
File chrome/common/native_web_keyboard_event_mac.mm (right):

http://codereview.chromium.org/40065/diff/1052/148#newcode15
Line 15: event([event retain]) {
Not that it matters, but the semantics for gtk and win are copying of the event
while here you have joint ownership.

[Elliot Glaysher, 2009-03-06 20:51:24]

http://codereview.chromium.org/40065/diff/1052/130
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/40065/diff/1052/130#newcode302
Line 302: if (WebInputEvent::IsKeyboardEventType(key_event.type)) {
On 2009/03/06 04:45:35, darin wrote:
> previously, we would have only added to the queue if process_->channel() was
> non-null.  that may be worth preserving.

Done.

http://codereview.chromium.org/40065/diff/1052/146
File chrome/common/native_web_keyboard_event.h (right):

http://codereview.chromium.org/40065/diff/1052/146#newcode6
Line 6: #define BASE_GFX_NATIVE_EVENT_H_
On 2009/03/06 04:45:35, darin wrote:
> nit: include guard is out-of-date

Done.

http://codereview.chromium.org/40065/diff/1052/146#newcode8
Line 8: #include "build/build_config.h"
On 2009/03/06 04:45:35, darin wrote:
> nit: no need to include this since you are including basictypes.h (probably
can
> exclude that too since webinputevent.h includes it)

Done.

http://codereview.chromium.org/40065/diff/1052/146#newcode28
Line 28: public:
On 2009/03/06 04:45:35, darin wrote:
> nit: no need for public here since it is a struct

Done.

http://codereview.chromium.org/40065/diff/1052/146#newcode44
Line 44: #if defined(OS_WIN)
On 2009/03/06 04:45:35, darin wrote:
> nit: maybe all of these member variables could have the same name:  os_event
> perhaps?

Done.

http://codereview.chromium.org/40065/diff/1052/148
File chrome/common/native_web_keyboard_event_mac.mm (right):

http://codereview.chromium.org/40065/diff/1052/148#newcode15
Line 15: event([event retain]) {
On 2009/03/06 18:01:34, Avi wrote:
> Not that it matters, but the semantics for gtk and win are copying of the
event
> while here you have joint ownership.

Yeah. There's no refcounting on GdkEvents (since they don't derive from
GObject), otherwise I'd do this in linux, too.

http://codereview.chromium.org/40065/diff/1052/149
File chrome/common/native_web_keyboard_event_win.cc (right):

http://codereview.chromium.org/40065/diff/1052/149#newcode8
Line 8: actual_message.hwnd = NULL;
On 2009/03/06 04:45:35, darin wrote:
> memset(&actual_message, 0, sizeof(actual_message));
> 
> however, the base class default constructor does nothing, so perhaps this one
> should too.  in fact, i'd be surprised if this constructor is ever called.  is
> it?  i bet you could make it private forcing people to use the other
> constructor.

I can't make this private because I use the default constructor for unit
testing, but I can cut out the work.

http://codereview.chromium.org/40065/diff/1052/149#newcode23
Line 23: NativeWebKeyboardEvent::NativeWebKeyboardEvent(
On 2009/03/06 04:45:35, darin wrote:
> i don't think there is any reason to implement copy constructors or assignment
> operators for OS_WIN.  the defaults should work properly.

I feel this is the lesser of two evils compared to the ifdef statements
necessary in the header to supply declare copy constructors/operator=s for some
(but not all) platforms.