Handle alternate types in HandleMouseEvent.

Handle alternate types in HandleMouseEvent.

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

[girard, 2013-06-22 15:58:50]

https://src.chromium.org/viewvc/chrome?view=rev&revision=205831 introduced a
regression. NativeWidgetWin::HandleMouseEvent now handles subclasses of
ui::MouseEvent properly.

[sadrul, 2013-06-24 02:09:35]

A couple of drive-by comments.

https://codereview.chromium.org/16950030/diff/3/ui/base/events/event.h
File ui/base/events/event.h (right):

https://codereview.chromium.org/16950030/diff/3/ui/base/events/event.h#newcod...
ui/base/events/event.h:178: return type_ == ET_MOUSEWHEEL;
This is simple enough that I wouldn't add it.

https://codereview.chromium.org/16950030/diff/3/ui/views/widget/native_widget...
File ui/views/widget/native_widget_win.cc (right):

https://codereview.chromium.org/16950030/diff/3/ui/views/widget/native_widget...
ui/views/widget/native_widget_win.cc:751: ui::win::GetDeviceScaleFactor());
Most of the ui::Events are copyable. Looks like MouseWheelEvents aren't. It
probably makes more sense to just allow copying a MouseWheelEvent, so you can
just do:
  ui::MouseWheelEvent dpi_event(event);
(like for the MouseEvent case below)

https://codereview.chromium.org/16950030/diff/3/ui/views/widget/native_widget...
ui/views/widget/native_widget_win.cc:756: } else if (event.IsScrollEvent()) {
scroll events (i.e. ET_SCROLL*) are never generated on windows. So you can
simplify this:
  if (event.type() == ui::ET_MOUSEWHEEL) {
    ...
  } else {
    CHECK(!event.IsScrollEvent());  <-- not really necessary
    ...
  }

[girard, 2013-06-24 20:37:07]

Thanks for the review, Sadrul. I've refactored to use a transform, and
implemented MouseWheelEvent::UpdateForRootTransform.

https://codereview.chromium.org/16950030/diff/3/ui/views/widget/native_widget...
File ui/views/widget/native_widget_win.cc (right):

https://codereview.chromium.org/16950030/diff/3/ui/views/widget/native_widget...
ui/views/widget/native_widget_win.cc:751: ui::win::GetDeviceScaleFactor());
On 2013/06/24 02:09:35, sadrul wrote:
> Most of the ui::Events are copyable. Looks like MouseWheelEvents aren't. It
> probably makes more sense to just allow copying a MouseWheelEvent, so you can
> just do:
>   ui::MouseWheelEvent dpi_event(event);
> (like for the MouseEvent case below)

If we do this, we'd still need the static_cast in the call to the c'tor. (No big
deal). We would also need to expose a set_x_offset()/set_y_offset().

I think a cleaner approach would be to switch to a call to
UpdateForRootTransform() after constructing either event, and implement
MouseWheelEvent::UpdateForRootTransform.

https://codereview.chromium.org/16950030/diff/3/ui/views/widget/native_widget...
ui/views/widget/native_widget_win.cc:756: } else if (event.IsScrollEvent()) {
On 2013/06/24 02:09:35, sadrul wrote:
> scroll events (i.e. ET_SCROLL*) are never generated on windows. So you can
> simplify this:
>   if (event.type() == ui::ET_MOUSEWHEEL) {
>     ...
>   } else {
>     CHECK(!event.IsScrollEvent());  <-- not really necessary
>     ...
>   }

Thanks - cleaned this up. (I was trying to be more conservative/paranoid on the
types supported.)

[dewittj, 2013-06-24 21:26:52]

Is there any reason we can't just do the scale factor calculations in the
MouseWheelEvent constructor?  Then the const-ref-ness would not be a problem.

[tapted, 2013-06-25 04:18:03]

drive-by

https://codereview.chromium.org/16950030/diff/13001/ui/base/events/event.h
File ui/base/events/event.h (left):

https://codereview.chromium.org/16950030/diff/13001/ui/base/events/event.h#ol...
ui/base/events/event.h:437: DISALLOW_COPY_AND_ASSIGN(MouseWheelEvent);
I wonder... instead of removing this. What about adding it to ui::MouseEvent or
giving MouseEvent an explicit, _protected_ copy constructor? This could help
prevent these kinds of slicing issues in future. Removing this here (and the
current lack in MouseEvent) seems unsafe when code is using static_cast based on
EventType.

Of course, you'll need a different fix for the bug, such as Justin's suggestion.

[girard, 2013-06-25 12:37:55]

On 2013/06/24 21:26:52, dewittj wrote:
> Is there any reason we can't just do the scale factor calculations in the
> MouseWheelEvent constructor?  Then the const-ref-ness would not be a problem.

In general, hwnd_message_handler deals in pixel coordinates, and requires
clients to perform translations. It's used by NativeWidgetWin (win/views), and
it's also used by desktop_root_window_host_win (for win/aura). The clients have
done all the mappings from pixel to dip. Most of the mappings were fairly
simple, with this being the exception.

[girard, 2013-06-25 17:17:22]

I've cleaned up the code as per Sadrul's review. 

I've also removed an unused variable - MouseEvent::root_location_ - this private
var isn't directly used, and hid the LocatedEvent::root_location_.

https://codereview.chromium.org/16950030/diff/13001/ui/base/events/event.h
File ui/base/events/event.h (left):

https://codereview.chromium.org/16950030/diff/13001/ui/base/events/event.h#ol...
ui/base/events/event.h:437: DISALLOW_COPY_AND_ASSIGN(MouseWheelEvent);
On 2013/06/25 04:18:03, tapted wrote:
> I wonder... instead of removing this. What about adding it to ui::MouseEvent
or
> giving MouseEvent an explicit, _protected_ copy constructor? This could help
> prevent these kinds of slicing issues in future. Removing this here (and the
> current lack in MouseEvent) seems unsafe when code is using static_cast based
on
> EventType.
> 
> Of course, you'll need a different fix for the bug, such as Justin's
suggestion.

The client code needs to copy both MouseEvent's and MouseWheelEvent's, so moving
the macro won't get us anything.

[Ben Goodger (Google), 2013-06-26 16:37:38]

https://codereview.chromium.org/16950030/diff/13001/ui/base/events/event.h
File ui/base/events/event.h (right):

https://codereview.chromium.org/16950030/diff/13001/ui/base/events/event.h#ne...
ui/base/events/event.h:431: // Overridden from LocatedEvent.
can/should this private?

at the very least put it after the other accessors since it is not a method
introduced by this class.

[girard, 2013-06-26 17:59:05]

https://codereview.chromium.org/16950030/diff/13001/ui/base/events/event.h
File ui/base/events/event.h (right):

https://codereview.chromium.org/16950030/diff/13001/ui/base/events/event.h#ne...
ui/base/events/event.h:431: // Overridden from LocatedEvent.
On 2013/06/26 16:37:38, Ben Goodger (Google) wrote:
> can/should this private?
> 
> at the very least put it after the other accessors since it is not a method
> introduced by this class.

Moved.

Making it private would mean that myEvent->UpdateForRootTransform() would work,
but myMouseWheelEvent->UpdateForRootTransform() wouldn't (although it would with
a static cast...)
cf:
http://stackoverflow.com/questions/1828009/can-you-hide-a-virtual-method-in-c

[Ben Goodger (Google), 2013-06-26 22:01:29]

lgtm


On Wed, Jun 26, 2013 at 10:59 AM, <girard@chromium.org> wrote:

>
> https://codereview.chromium.**org/16950030/diff/13001/ui/**
>
base/events/event.h<https://codereview.chromium.org/16950030/diff/13001/ui/base/events/event.h>
> File ui/base/events/event.h (right):
>
> https://codereview.chromium.**org/16950030/diff/13001/ui/**
>
base/events/event.h#newcode431<https://codereview.chromium.org/16950030/diff/13001/ui/base/events/event.h#newcode431>
> ui/base/events/event.h:431: // Overridden from LocatedEvent.
> On 2013/06/26 16:37:38, Ben Goodger (Google) wrote:
>
>> can/should this private?
>>
>
>  at the very least put it after the other accessors since it is not a
>>
> method
>
>> introduced by this class.
>>
>
> Moved.
>
> Making it private would mean that myEvent->**UpdateForRootTransform()
> would work, but myMouseWheelEvent->**UpdateForRootTransform() wouldn't
> (although it would with a static cast...)
> cf:
> http://stackoverflow.com/**questions/1828009/can-you-**
>
hide-a-virtual-method-in-c<http://stackoverflow.com/questions/1828009/can-you-hide-a-virtual-method-in-c>
>
>
https://codereview.chromium.**org/16950030/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-06-26 22:38:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/girard@chromium.org/16950030/20001

[commit-bot: I haz the power, 2013-06-27 09:08:09]

Change committed as 208866