Splits ToplevelWindowEventHandler into two

Splits ToplevelWindowEventHandler into two

The non event handler and non-aura parts end up in
WmToplevelWindowEventHandler with the rest remaining in
ToplevelWindowEventHandler.

WmToplevelWindowEventHandler is not actually an EventHandler, but I kept
the name to reinforce the expectation is that WmToplevelWindowEventHandler is called from an EventHandler.

BUG=603369
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/cf67ed5a0802c2d1d5b05a0d87cdd1612317555a
Cr-Commit-Position: refs/heads/master@{#395387}

[sky, 2016-05-20 21:40:18]

Description was changed from

==========
Splits ToplevelWindowEventHandler into two

The non event handler and non-aura parts end up in
WmToplevelWindowEventHandler with the rest remaining in
ToplevelWindowEventHandler.

WmToplevelWindowEventHandler is actually an EventHandler, but I kept
the name to reinfoce it's expected to be called for events.

BUG=603369
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Splits ToplevelWindowEventHandler into two

The non event handler and non-aura parts end up in
WmToplevelWindowEventHandler with the rest remaining in
ToplevelWindowEventHandler.

WmToplevelWindowEventHandler is not actually an EventHandler, but I kept
the name to reinforce the expectation is that WmToplevelWindowEventHandler is
called from an EventHandler.

BUG=603369
TEST=covered by tests
R=jamescook@chromium.org
==========

[James Cook, 2016-05-21 00:12:30]

I need to head out now, but I'll look at this tomorrow.

[James Cook, 2016-05-21 18:28:22]

LGTM with nits/kudos. (I'm trying to give more positive feedback in code
reviews.)

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_display_ob...
File ash/wm/common/wm_display_observer.h (right):

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_display_ob...
ash/wm/common/wm_display_observer.h:18: virtual void
OnDisplayConfigurationChanging() {}
It's pedantic, but I would comment above this one that it's called before the
display config change, and comment below that it's after. I really like
documentation on interface classes.

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_toplevel_w...
File ash/wm/common/wm_toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_toplevel_w...
ash/wm/common/wm_toplevel_window_event_handler.cc:364:
std::unique_ptr<WindowResizer> resizer(
Hooray for conversion to unique_ptr for clear ownership transfer!

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_toplevel_w...
File ash/wm/common/wm_toplevel_window_event_handler.h (right):

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_toplevel_w...
ash/wm/common/wm_toplevel_window_event_handler.h:36: // EventHandler.
Nice class comment.

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_toplevel_w...
ash/wm/common/wm_toplevel_window_event_handler.h:41: enum class EndReason {
I would call this DragEndReason or DragResult or something that helps tie it to
drags. My first reaction on seeing WmToplevelWindowEventHandler::EndReason in
other code is "what ended"?

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_toplevel_w...
ash/wm/common/wm_toplevel_window_event_handler.h:72: bool is_drag_in_progress()
const { return window_resizer_.get() != nullptr; }
I don't think this is used. (Man, I wish we had search-in-cl UI, like critique.)

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_toplevel_w...
ash/wm/common/wm_toplevel_window_event_handler.h:119: bool in_gesture_drag_ =
false;
optional nit: Consider initializing this in the .cc file, because doing it here
makes first_finger_hittest_ look maybe-uninitialized, but it's initialized in
the cc I presume to avoid the hit_test.h #include.

https://codereview.chromium.org/2001623003/diff/2/ash/wm/toplevel_window_even...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/2001623003/diff/2/ash/wm/toplevel_window_even...
ash/wm/toplevel_window_event_handler.cc:103:
wm_toplevel_window_event_handler_.RevertDrag();
I like having an explicit RevertDrag method.

https://codereview.chromium.org/2001623003/diff/2/ash/wm/toplevel_window_even...
File ash/wm/toplevel_window_event_handler.h (right):

https://codereview.chromium.org/2001623003/diff/2/ash/wm/toplevel_window_even...
ash/wm/toplevel_window_event_handler.h:60:
base::WeakPtrFactory<ToplevelWindowEventHandler> weak_factory_;
Just for my edification: We need the weak pointers because drag/resize can be
in-progress during ash::Shell teardown? Or is this just more generic assurance
for this object being deleted out from under WmToplevelWindowEventHandler?

[sky, 2016-05-23 15:48:52]

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_display_ob...
File ash/wm/common/wm_display_observer.h (right):

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_display_ob...
ash/wm/common/wm_display_observer.h:18: virtual void
OnDisplayConfigurationChanging() {}
On 2016/05/21 18:28:21, James Cook wrote:
> It's pedantic, but I would comment above this one that it's called before the
> display config change, and comment below that it's after. I really like
> documentation on interface classes.

Done.

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_toplevel_w...
File ash/wm/common/wm_toplevel_window_event_handler.h (right):

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_toplevel_w...
ash/wm/common/wm_toplevel_window_event_handler.h:41: enum class EndReason {
On 2016/05/21 18:28:21, James Cook wrote:
> I would call this DragEndReason or DragResult or something that helps tie it
to
> drags. My first reaction on seeing WmToplevelWindowEventHandler::EndReason in
> other code is "what ended"?

I went with DragResult.

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_toplevel_w...
ash/wm/common/wm_toplevel_window_event_handler.h:72: bool is_drag_in_progress()
const { return window_resizer_.get() != nullptr; }
On 2016/05/21 18:28:21, James Cook wrote:
> I don't think this is used. (Man, I wish we had search-in-cl UI, like
critique.)

I use it in a subsequent patch. But I'll add it there.

https://codereview.chromium.org/2001623003/diff/2/ash/wm/common/wm_toplevel_w...
ash/wm/common/wm_toplevel_window_event_handler.h:119: bool in_gesture_drag_ =
false;
On 2016/05/21 18:28:21, James Cook wrote:
> optional nit: Consider initializing this in the .cc file, because doing it
here
> makes first_finger_hittest_ look maybe-uninitialized, 

I agree with you, but I think it's just something we have to get use to with
c++11.

> but it's initialized in
> the cc I presume to avoid the hit_test.h #include.

Exactly.

https://codereview.chromium.org/2001623003/diff/2/ash/wm/toplevel_window_even...
File ash/wm/toplevel_window_event_handler.h (right):

https://codereview.chromium.org/2001623003/diff/2/ash/wm/toplevel_window_even...
ash/wm/toplevel_window_event_handler.h:60:
base::WeakPtrFactory<ToplevelWindowEventHandler> weak_factory_;
On 2016/05/21 18:28:21, James Cook wrote:
> Just for my edification: We need the weak pointers because drag/resize can be
> in-progress during ash::Shell teardown?

Yes, and in particular a nested message loop for drag and drop. Although I'm not
100% sure that we can actually hit that.

> Or is this just more generic assurance
> for this object being deleted out from under WmToplevelWindowEventHandler?

WeakPtrFactory is the less error prone version of the old destroyed_. I was
really just converting the old logic.

[sky, 2016-05-23 15:48:56]

The CQ bit was checked by sky@chromium.org

[sky, 2016-05-23 15:48:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2001623003/#ps50001
(title: "feedback")

[commit-bot: I haz the power, 2016-05-23 15:49:04]

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

[commit-bot: I haz the power, 2016-05-23 16:46:43]

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

[commit-bot: I haz the power, 2016-05-23 16:46:44]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sky, 2016-05-23 16:51:42]

The CQ bit was checked by sky@chromium.org

[sky, 2016-05-23 16:51:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2001623003/#ps70001
(title: "add events_base dep")

[commit-bot: I haz the power, 2016-05-23 16:52:03]

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

[commit-bot: I haz the power, 2016-05-23 17:51:07]

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

[commit-bot: I haz the power, 2016-05-23 17:51:08]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sky, 2016-05-23 18:31:58]

The CQ bit was checked by sky@chromium.org

[sky, 2016-05-23 18:31:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2001623003/#ps90001
(title: "fix gyp")

[commit-bot: I haz the power, 2016-05-23 18:32:27]

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

[commit-bot: I haz the power, 2016-05-23 19:38:53]

Description was changed from

==========
Splits ToplevelWindowEventHandler into two

The non event handler and non-aura parts end up in
WmToplevelWindowEventHandler with the rest remaining in
ToplevelWindowEventHandler.

WmToplevelWindowEventHandler is not actually an EventHandler, but I kept
the name to reinforce the expectation is that WmToplevelWindowEventHandler is
called from an EventHandler.

BUG=603369
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Splits ToplevelWindowEventHandler into two

The non event handler and non-aura parts end up in
WmToplevelWindowEventHandler with the rest remaining in
ToplevelWindowEventHandler.

WmToplevelWindowEventHandler is not actually an EventHandler, but I kept
the name to reinforce the expectation is that WmToplevelWindowEventHandler is
called from an EventHandler.

BUG=603369
TEST=covered by tests
R=jamescook@chromium.org
==========

[commit-bot: I haz the power, 2016-05-23 19:38:55]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-05-23 19:40:35]

Description was changed from

==========
Splits ToplevelWindowEventHandler into two

The non event handler and non-aura parts end up in
WmToplevelWindowEventHandler with the rest remaining in
ToplevelWindowEventHandler.

WmToplevelWindowEventHandler is not actually an EventHandler, but I kept
the name to reinforce the expectation is that WmToplevelWindowEventHandler is
called from an EventHandler.

BUG=603369
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Splits ToplevelWindowEventHandler into two

The non event handler and non-aura parts end up in
WmToplevelWindowEventHandler with the rest remaining in
ToplevelWindowEventHandler.

WmToplevelWindowEventHandler is not actually an EventHandler, but I kept
the name to reinforce the expectation is that WmToplevelWindowEventHandler is
called from an EventHandler.

BUG=603369
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/cf67ed5a0802c2d1d5b05a0d87cdd1612317555a
Cr-Commit-Position: refs/heads/master@{#395387}
==========

[commit-bot: I haz the power, 2016-05-23 19:40:37]

Patchset 6 (id:??) landed as
https://crrev.com/cf67ed5a0802c2d1d5b05a0d87cdd1612317555a
Cr-Commit-Position: refs/heads/master@{#395387}