MacViews: Replace NativeWidget::GetEventHandler() with RepostNativeEvent(..)

MacViews: Replace NativeWidget::GetEventHandler() with RepostNativeEvent(..)

This removes an aura dependency in webview/web_dialog_view.cc and is a
step towards getting the views_examples target compiling on Mac.

NativeWidget::GetEventHandler isn't used elsewhere.

Pre-Aura Win would use ::DefWindowProc() in
WebDialogView::HandleKeyboardEvent(), so NativeWidget feels like the
right place for this.

BUG=366007
TEST=Just refactoring

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

[tapted, 2014-05-28 12:36:21]

Hi sadrul, please take a look

I've split this off from crrev/297143009 which has a bit more context - I tried
some other things there without success before arriving at this. For reference,
http://crrev.com/118343 is the revision where the code in web_dialog_view.cc
came into being.

https://codereview.chromium.org/307623002/diff/1/ui/views/controls/webview/we...
File ui/views/controls/webview/web_dialog_view.cc (left):

https://codereview.chromium.org/307623002/diff/1/ui/views/controls/webview/we...
ui/views/controls/webview/web_dialog_view.cc:283: ui::KeyEvent
aura_event(event.os_event->native_event(), false);
note that the native event is already copied in
native_web_keyboard_event_aura.cc, so removing this extra copy step should be
fine

[tapted, 2014-05-29 21:35:08]

sadrul: ping?

[sadrul, 2014-05-29 23:09:27]

https://codereview.chromium.org/307623002/diff/1/ui/views/controls/webview/we...
File ui/views/controls/webview/web_dialog_view.cc (left):

https://codereview.chromium.org/307623002/diff/1/ui/views/controls/webview/we...
ui/views/controls/webview/web_dialog_view.cc:283: ui::KeyEvent
aura_event(event.os_event->native_event(), false);
On 2014/05/28 12:36:21, tapted wrote:
> note that the native event is already copied in
> native_web_keyboard_event_aura.cc, so removing this extra copy step should be
> fine

Can you confirm that event.os_event->is_char() is always set to false for aura
here? Because if it's not, then the behaviour is going to be different. A
[D]CHECK would be a good idea.

https://codereview.chromium.org/307623002/diff/1/ui/views/widget/native_widget.h
File ui/views/widget/native_widget.h (right):

https://codereview.chromium.org/307623002/diff/1/ui/views/widget/native_widge...
ui/views/widget/native_widget.h:31: virtual void
RepostNativeEvent(gfx::NativeEvent os_event) = 0;
|os_event| may be confusing here, because in aura, gfx::NativeEvent is a
ui::Event. So call this |native_event| instead (here and in the overrides).

Can you move this into NativeWidgetPrivate?

[tapted, 2014-05-29 23:36:19]

will address other comments, but an initial note on one:

https://codereview.chromium.org/307623002/diff/1/ui/views/controls/webview/we...
File ui/views/controls/webview/web_dialog_view.cc (left):

https://codereview.chromium.org/307623002/diff/1/ui/views/controls/webview/we...
ui/views/controls/webview/web_dialog_view.cc:283: ui::KeyEvent
aura_event(event.os_event->native_event(), false);
On 2014/05/29 23:09:27, sadrul wrote:
> On 2014/05/28 12:36:21, tapted wrote:
> > note that the native event is already copied in
> > native_web_keyboard_event_aura.cc, so removing this extra copy step should
be
> > fine
> 
> Can you confirm that event.os_event->is_char() is always set to false for aura
> here? Because if it's not, then the behaviour is going to be different. A
> [D]CHECK would be a good idea.

Oooh - that's a good point. I'm not sure it's true in general here, and the
change might be an improvement..

is_char isn't too familiar to me, so I'm doing some research (still poking
around). However, my initial feeling is that the `false` that was here isn't
deliberate, and possibly a subtle bug.

This code was added so that `ESC` in a WebDialogView could close it, via
WebDialogView::AcceleratorPressed. AcceleratorFilter::OnKeyEvent will filter out
events where is_char is true. So the old code has the behaviour of turning
*translated* character events into accelerators, which doesn't seem right.

I've checked for regressions (took me a while to find a dialog that still used
this!), and ESC still works to close the dialog.

(note: an example dialog is the 'Print using cloud print' dialog -- a link to
open it appears in the Ctrl+P dialog but only *after* selecting a cloud printer
as the destination.)

[tapted, 2014-05-30 01:59:44]

PTAL

https://codereview.chromium.org/307623002/diff/1/ui/views/controls/webview/we...
File ui/views/controls/webview/web_dialog_view.cc (left):

https://codereview.chromium.org/307623002/diff/1/ui/views/controls/webview/we...
ui/views/controls/webview/web_dialog_view.cc:283: ui::KeyEvent
aura_event(event.os_event->native_event(), false);
On 2014/05/29 23:36:19, tapted wrote:
> On 2014/05/29 23:09:27, sadrul wrote:
> > On 2014/05/28 12:36:21, tapted wrote:
> > > note that the native event is already copied in
> > > native_web_keyboard_event_aura.cc, so removing this extra copy step should
> be
> > > fine
> > 
> > Can you confirm that event.os_event->is_char() is always set to false for
aura
> > here? Because if it's not, then the behaviour is going to be different. A
> > [D]CHECK would be a good idea.
> 
> Oooh - that's a good point. I'm not sure it's true in general here, and the
> change might be an improvement..
> 
> is_char isn't too familiar to me, so I'm doing some research (still poking
> around). However, my initial feeling is that the `false` that was here isn't
> deliberate, and possibly a subtle bug.
> 
> This code was added so that `ESC` in a WebDialogView could close it, via
> WebDialogView::AcceleratorPressed. AcceleratorFilter::OnKeyEvent will filter
out
> events where is_char is true. So the old code has the behaviour of turning
> *translated* character events into accelerators, which doesn't seem right.
> 
> I've checked for regressions (took me a while to find a dialog that still used
> this!), and ESC still works to close the dialog.
> 
> (note: an example dialog is the 'Print using cloud print' dialog -- a link to
> open it appears in the Ctrl+P dialog but only *after* selecting a cloud
printer
> as the destination.)

Also note the non-aura version on Windows would just DefWindowProc without
changes to the event (see e.g. http://crrev.com/118343 ), which also suggests to
me that the unconditional `is_char = false` was unintentional.

If you prefer I can check it here. However, it would mean bringing back an #if
defined(USE_AURA) which I'm also hoping to avoid :)

https://codereview.chromium.org/307623002/diff/1/ui/views/widget/native_widget.h
File ui/views/widget/native_widget.h (right):

https://codereview.chromium.org/307623002/diff/1/ui/views/widget/native_widge...
ui/views/widget/native_widget.h:31: virtual void
RepostNativeEvent(gfx::NativeEvent os_event) = 0;
On 2014/05/29 23:09:27, sadrul wrote:
> |os_event| may be confusing here, because in aura, gfx::NativeEvent is a
> ui::Event. So call this |native_event| instead (here and in the overrides).

Done.

> Can you move this into NativeWidgetPrivate?

Done.

[sadrul, 2014-05-30 02:26:53]

LGTM

https://codereview.chromium.org/307623002/diff/40001/ui/views/widget/native_w...
File ui/views/widget/native_widget.h (right):

https://codereview.chromium.org/307623002/diff/40001/ui/views/widget/native_w...
ui/views/widget/native_widget.h:8: #include "ui/gfx/native_widget_types.h"
You probably don't need this.

[tapted, 2014-05-30 03:03:26]

https://codereview.chromium.org/307623002/diff/40001/ui/views/widget/native_w...
File ui/views/widget/native_widget.h (right):

https://codereview.chromium.org/307623002/diff/40001/ui/views/widget/native_w...
ui/views/widget/native_widget.h:8: #include "ui/gfx/native_widget_types.h"
On 2014/05/30 02:26:55, sadrul wrote:
> You probably don't need this.

Done. (oops)

[tapted, 2014-05-30 03:05:00]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-05-30 03:06:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tapted@chromium.org/307623002/60001

[commit-bot: I haz the power, 2014-05-30 07:18:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-30 10:35:42]

Change committed as 273793