mash: Fix DCHECK when clicking on window close box

mash: Fix DCHECK when clicking on window close box

There is no pointer event equivalent for mouse event type MOUSE_CAPTURE_CHANGED
and MoveEventHandler doesn't need to deal with this event type.

https://codereview.chromium.org/1814533002/ caused a change in mouse event
handling that caused this DCHECK to be hit.

BUG=598330
TEST=mojo_runner mojo:mash_session and click the close box for any window

Committed: https://crrev.com/5e8057486c37f23bfdbd1febb5fa24d36e48f19d
Cr-Commit-Position: refs/heads/master@{#383858}

[James Cook, 2016-03-28 21:30:06]

jamescook@chromium.org changed reviewers:
+ moshayedi@chromium.org

[James Cook, 2016-03-28 21:30:07]

Hadi, please take a look.

[James Cook, 2016-03-28 21:31:23]

On 2016/03/28 21:30:07, James Cook wrote:
> Hadi, please take a look.

Also, I didn't add a test for this case because I couldn't figure out how to
initialize an aura::Window and aura::RootWindow in mash_unittests. I think the
test suite needs some changes to support doing this. I filed crbug.com/598421
for this.

[James Cook, 2016-03-28 21:31:37]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-28 21:31:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1843433002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1843433002/1

[commit-bot: I haz the power, 2016-03-28 22:10:24]

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

[commit-bot: I haz the power, 2016-03-28 22:10:25]

Dry run: This issue passed the CQ dry run.

[Hadi, 2016-03-29 14:44:23]

lgtm

[James Cook, 2016-03-29 15:35:37]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org

[James Cook, 2016-03-29 15:35:37]

msw, can I get OWNERS?

[msw, 2016-03-29 15:47:40]

lgtm if needed for a temp workaround, but we need a broader fix, afaict.

https://codereview.chromium.org/1843433002/diff/1/mash/wm/frame/move_event_ha...
File mash/wm/frame/move_event_handler.cc (right):

https://codereview.chromium.org/1843433002/diff/1/mash/wm/frame/move_event_ha...
mash/wm/frame/move_event_handler.cc:64: if (event->type() ==
ui::ET_MOUSE_CAPTURE_CHANGED)
The PointerEvent ctors will NOTREACHED on other mouse and touch event types that
aren't in those switch statements. Should we guard more broadly against these?
Perhaps we should add a pointer event util/factory function that checks if it
can convert, then return false/null if not?
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/event.cc...

[James Cook, 2016-03-29 17:38:06]

jamescook@chromium.org changed reviewers:
+ sadrul@chromium.org

[James Cook, 2016-03-29 17:38:07]

Mike, please take another look.

Sadrul, does this look like what you had in mind?

https://codereview.chromium.org/1843433002/diff/1/mash/wm/frame/move_event_ha...
File mash/wm/frame/move_event_handler.cc (right):

https://codereview.chromium.org/1843433002/diff/1/mash/wm/frame/move_event_ha...
mash/wm/frame/move_event_handler.cc:64: if (event->type() ==
ui::ET_MOUSE_CAPTURE_CHANGED)
On 2016/03/29 15:47:40, msw wrote:
> The PointerEvent ctors will NOTREACHED on other mouse and touch event types
that
> aren't in those switch statements. Should we guard more broadly against these?
> Perhaps we should add a pointer event util/factory function that checks if it
> can convert, then return false/null if not?
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/event.cc...

Yeah, good point.  I talked to Sadrul and we decided to add a util function
PointerEvent::CanConvertFrom(const Event&). I looked at doing a factory
function, but almost all callers of the PointerEvent converters have the event
on the stack, so returning nullptr is awkward, and we don't have an
event-representing-null I can use either.

[msw, 2016-03-29 17:50:27]

lgtm with nits

https://codereview.chromium.org/1843433002/diff/20001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/1843433002/diff/20001/ui/events/event.cc#newc...
ui/events/event.cc:751: switch (mouse_event.type()) {
nit: DCHECK CanConvertFrom here?

https://codereview.chromium.org/1843433002/diff/20001/ui/events/event.cc#newc...
ui/events/event.cc:782: switch (touch_event.type()) {
nit: DCHECK CanConvertFrom here?

https://codereview.chromium.org/1843433002/diff/20001/ui/events/event_unittes...
File ui/events/event_unittest.cc (right):

https://codereview.chromium.org/1843433002/diff/20001/ui/events/event_unittes...
ui/events/event_unittest.cc:729: const gfx::Point origin(0, 0);
nit: use Point default ctor

https://codereview.chromium.org/1843433002/diff/20001/ui/events/event_unittes...
ui/events/event_unittest.cc:766: MouseWheelEvent(gfx::Vector2d(0, 0), origin,
origin, time, 0, 0)));
nit: use Vector2d default ctor

[James Cook, 2016-03-29 18:23:27]

Mike, Sadrul, PTAL

https://codereview.chromium.org/1843433002/diff/20001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/1843433002/diff/20001/ui/events/event.cc#newc...
ui/events/event.cc:751: switch (mouse_event.type()) {
On 2016/03/29 17:50:27, msw wrote:
> nit: DCHECK CanConvertFrom here?

Done.

https://codereview.chromium.org/1843433002/diff/20001/ui/events/event.cc#newc...
ui/events/event.cc:782: switch (touch_event.type()) {
On 2016/03/29 17:50:27, msw wrote:
> nit: DCHECK CanConvertFrom here?

Done.

https://codereview.chromium.org/1843433002/diff/20001/ui/events/event_unittes...
File ui/events/event_unittest.cc (right):

https://codereview.chromium.org/1843433002/diff/20001/ui/events/event_unittes...
ui/events/event_unittest.cc:729: const gfx::Point origin(0, 0);
On 2016/03/29 17:50:27, msw wrote:
> nit: use Point default ctor

Done and renamed to 'point' because the location isn't important.

https://codereview.chromium.org/1843433002/diff/20001/ui/events/event_unittes...
ui/events/event_unittest.cc:766: MouseWheelEvent(gfx::Vector2d(0, 0), origin,
origin, time, 0, 0)));
On 2016/03/29 17:50:27, msw wrote:
> nit: use Vector2d default ctor

Done.

[msw, 2016-03-29 18:24:59]

still lgtm

[sadrul, 2016-03-29 19:29:22]

lgtm

[James Cook, 2016-03-29 20:55:32]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-03-29 20:55:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
moshayedi@chromium.org
Link to the patchset: https://codereview.chromium.org/1843433002/#ps40001
(title: "review comments 2")

[commit-bot: I haz the power, 2016-03-29 20:56:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1843433002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1843433002/40001

[commit-bot: I haz the power, 2016-03-29 23:17:08]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-29 23:18:17]

Description was changed from

==========
mash: Fix DCHECK when clicking on window close box

There is no pointer event equivalent for mouse event type MOUSE_CAPTURE_CHANGED
and MoveEventHandler doesn't need to deal with this event type.

https://codereview.chromium.org/1814533002/ caused a change in mouse event
handling that caused this DCHECK to be hit.

BUG=598330
TEST=mojo_runner mojo:mash_session and click the close box for any window
==========

to

==========
mash: Fix DCHECK when clicking on window close box

There is no pointer event equivalent for mouse event type MOUSE_CAPTURE_CHANGED
and MoveEventHandler doesn't need to deal with this event type.

https://codereview.chromium.org/1814533002/ caused a change in mouse event
handling that caused this DCHECK to be hit.

BUG=598330
TEST=mojo_runner mojo:mash_session and click the close box for any window

Committed: https://crrev.com/5e8057486c37f23bfdbd1febb5fa24d36e48f19d
Cr-Commit-Position: refs/heads/master@{#383858}
==========

[commit-bot: I haz the power, 2016-03-29 23:18:18]

Patchset 3 (id:??) landed as
https://crrev.com/5e8057486c37f23bfdbd1febb5fa24d36e48f19d
Cr-Commit-Position: refs/heads/master@{#383858}