mus: Fix handled status in UI event ack, add MessageLoop::NestingObserver

mus: Fix handled status in UI event ack, add MessageLoop::NestingObserver

mus applications receive their events from the window server, which runs
in another process.

Currently all input events to a mus application are immediately acked as
handled, because it's possible the event will start a nested message loop
(e.g. by spawning a context menu) and we don't want the event delivery
message to time out.

With this CL we observe for a message loop starting and if we see one we
immediately ack the event to avoid a timeout. Otherwise we process the
event normally and return a correct handled status.

BUG=604069
TEST=added to base_unittests and views_mus_unittests

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

[James Cook, 2016-04-15 23:15:06]

Description was changed from

==========
mus: Fix handled status of UI events, add MessageLoop::NestingObserver

Currently all input events to a mus application are immediately acked as
handled, because it's possible the event will start a nested message loop
(e.g. by spawning a context menu) and we don't want the event delivery
message to time out.

With this CL we observe for a message loop starting and if we see one we
immediately ack the event to avoid a timeout. Otherwise we process the
event normally and return a correct handled status.

BUG=
TEST=added to base_unittests
==========

to

==========
mus: Fix handled status in UI event ack, add MessageLoop::NestingObserver

Currently all input events to a mus application are immediately acked as
handled, because it's possible the event will start a nested message loop
(e.g. by spawning a context menu) and we don't want the event delivery
message to time out.

With this CL we observe for a message loop starting and if we see one we
immediately ack the event to avoid a timeout. Otherwise we process the
event normally and return a correct handled status.

BUG=604069
TEST=added to base_unittests
==========

[James Cook, 2016-04-15 23:16:36]

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

[James Cook, 2016-04-15 23:16:36]

sky, please take a look

(This CL is dependent on the EventResult enum,
https://codereview.chromium.org/1891413002/ )

[sky, 2016-04-16 16:28:33]

I didn't look at the base side of things as I'm not a base owner.

https://codereview.chromium.org/1891233006/diff/1/ui/views/mus/platform_windo...
File ui/views/mus/platform_window_mus.cc (right):

https://codereview.chromium.org/1891233006/diff/1/ui/views/mus/platform_windo...
ui/views/mus/platform_window_mus.cc:233: event_ack_callback_ =
std::move(*ack_callback);
I have a feeling you need to deal with multiple nestings (you only deal with one
here). I think a menu that triggers dnd may cause multiple nestings.

https://codereview.chromium.org/1891233006/diff/1/ui/views/mus/platform_windo...
ui/views/mus/platform_window_mus.cc:236: scoped_ptr<ui::Event> event =
ui::Event::Clone(event_in);
std::unique_ptr

https://codereview.chromium.org/1891233006/diff/1/ui/views/mus/platform_windo...
ui/views/mus/platform_window_mus.cc:241: if (event_ack_callback_) {
In addition to multiple nestings you should deal with this being deleted during
the dispatch.

[James Cook, 2016-04-27 15:28:57]

Description was changed from

==========
mus: Fix handled status in UI event ack, add MessageLoop::NestingObserver

Currently all input events to a mus application are immediately acked as
handled, because it's possible the event will start a nested message loop
(e.g. by spawning a context menu) and we don't want the event delivery
message to time out.

With this CL we observe for a message loop starting and if we see one we
immediately ack the event to avoid a timeout. Otherwise we process the
event normally and return a correct handled status.

BUG=604069
TEST=added to base_unittests
==========

to

==========
mus: Fix handled status in UI event ack, add MessageLoop::NestingObserver

mus applications receive their events from the window server, which runs
in another process.

Currently all input events to a mus application are immediately acked as
handled, because it's possible the event will start a nested message loop
(e.g. by spawning a context menu) and we don't want the event delivery
message to time out.

With this CL we observe for a message loop starting and if we see one we
immediately ack the event to avoid a timeout. Otherwise we process the
event normally and return a correct handled status.

BUG=604069
TEST=added to base_unittests
==========

[James Cook, 2016-04-27 17:07:34]

Description was changed from

==========
mus: Fix handled status in UI event ack, add MessageLoop::NestingObserver

mus applications receive their events from the window server, which runs
in another process.

Currently all input events to a mus application are immediately acked as
handled, because it's possible the event will start a nested message loop
(e.g. by spawning a context menu) and we don't want the event delivery
message to time out.

With this CL we observe for a message loop starting and if we see one we
immediately ack the event to avoid a timeout. Otherwise we process the
event normally and return a correct handled status.

BUG=604069
TEST=added to base_unittests
==========

to

==========
mus: Fix handled status in UI event ack, add MessageLoop::NestingObserver

mus applications receive their events from the window server, which runs
in another process.

Currently all input events to a mus application are immediately acked as
handled, because it's possible the event will start a nested message loop
(e.g. by spawning a context menu) and we don't want the event delivery
message to time out.

With this CL we observe for a message loop starting and if we see one we
immediately ack the event to avoid a timeout. Otherwise we process the
event normally and return a correct handled status.

BUG=604069
TEST=added to base_unittests and views_mus_unittests
==========

[James Cook, 2016-04-27 17:09:00]

sky, please take another look. I added some tests for event ack in
PlatformWindowMus, since the case that deletes |this| is a little tricky.

If you're OK with the ui/views/mus bits then I'll go talk to base owners.

[sky, 2016-04-27 17:49:48]

This approach looks fine by me.

https://codereview.chromium.org/1891233006/diff/60001/ui/views/mus/platform_w...
File ui/views/mus/platform_window_mus.cc (right):

https://codereview.chromium.org/1891233006/diff/60001/ui/views/mus/platform_w...
ui/views/mus/platform_window_mus.cc:32:
std::unique_ptr<base::Callback<void(EventResult)>>* ack_callback)
Did you consider making this take a std::unique_ptr, not a unique_ptr* and have
the destructor run the callback if it wasn't run already? Seems a bit simpler
and no pointers being passed around that way.

I would also be inclined to make this class add/remove itself from the
messageloop.

[James Cook, 2016-04-27 18:11:14]

sky, please take another look

https://codereview.chromium.org/1891233006/diff/60001/ui/views/mus/platform_w...
File ui/views/mus/platform_window_mus.cc (right):

https://codereview.chromium.org/1891233006/diff/60001/ui/views/mus/platform_w...
ui/views/mus/platform_window_mus.cc:32:
std::unique_ptr<base::Callback<void(EventResult)>>* ack_callback)
On 2016/04/27 17:49:48, sky wrote:
> Did you consider making this take a std::unique_ptr, not a unique_ptr* and
have
> the destructor run the callback if it wasn't run already? Seems a bit simpler
> and no pointers being passed around that way.
> 
> I would also be inclined to make this class add/remove itself from the
> messageloop.

Done.

[sky, 2016-04-27 20:11:12]

LGTM

[James Cook, 2016-04-27 20:17:18]

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

[James Cook, 2016-04-27 20:17:19]

thestig, can you review src/base/* ?

[Lei Zhang, 2016-04-27 22:48:06]

lgtm

https://codereview.chromium.org/1891233006/diff/80001/ui/views/mus/platform_w...
File ui/views/mus/platform_window_mus.cc (right):

https://codereview.chromium.org/1891233006/diff/80001/ui/views/mus/platform_w...
ui/views/mus/platform_window_mus.cc:39: if (ack_callback_)
nit: should have braces.

[James Cook, 2016-04-27 23:31:18]

https://codereview.chromium.org/1891233006/diff/80001/ui/views/mus/platform_w...
File ui/views/mus/platform_window_mus.cc (right):

https://codereview.chromium.org/1891233006/diff/80001/ui/views/mus/platform_w...
ui/views/mus/platform_window_mus.cc:39: if (ack_callback_)
On 2016/04/27 22:48:06, Lei Zhang wrote:
> nit: should have braces.

Done.

[James Cook, 2016-04-27 23:32:38]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-04-27 23:32:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1891233006/#ps100001
(title: "review comments 3")

[commit-bot: I haz the power, 2016-04-27 23:33:09]

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

[commit-bot: I haz the power, 2016-04-28 00:50:20]

Description was changed from

==========
mus: Fix handled status in UI event ack, add MessageLoop::NestingObserver

mus applications receive their events from the window server, which runs
in another process.

Currently all input events to a mus application are immediately acked as
handled, because it's possible the event will start a nested message loop
(e.g. by spawning a context menu) and we don't want the event delivery
message to time out.

With this CL we observe for a message loop starting and if we see one we
immediately ack the event to avoid a timeout. Otherwise we process the
event normally and return a correct handled status.

BUG=604069
TEST=added to base_unittests and views_mus_unittests
==========

to

==========
mus: Fix handled status in UI event ack, add MessageLoop::NestingObserver

mus applications receive their events from the window server, which runs
in another process.

Currently all input events to a mus application are immediately acked as
handled, because it's possible the event will start a nested message loop
(e.g. by spawning a context menu) and we don't want the event delivery
message to time out.

With this CL we observe for a message loop starting and if we see one we
immediately ack the event to avoid a timeout. Otherwise we process the
event normally and return a correct handled status.

BUG=604069
TEST=added to base_unittests and views_mus_unittests
==========

[commit-bot: I haz the power, 2016-04-28 00:50:22]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-30 17:15:08]

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

[commit-bot: I haz the power, 2016-04-30 17:53:27]

Description was changed from

==========
mus: Fix handled status in UI event ack, add MessageLoop::NestingObserver

mus applications receive their events from the window server, which runs
in another process.

Currently all input events to a mus application are immediately acked as
handled, because it's possible the event will start a nested message loop
(e.g. by spawning a context menu) and we don't want the event delivery
message to time out.

With this CL we observe for a message loop starting and if we see one we
immediately ack the event to avoid a timeout. Otherwise we process the
event normally and return a correct handled status.

BUG=604069
TEST=added to base_unittests and views_mus_unittests
==========

to

==========
mus: Fix handled status in UI event ack, add MessageLoop::NestingObserver

mus applications receive their events from the window server, which runs
in another process.

Currently all input events to a mus application are immediately acked as
handled, because it's possible the event will start a nested message loop
(e.g. by spawning a context menu) and we don't want the event delivery
message to time out.

With this CL we observe for a message loop starting and if we see one we
immediately ack the event to avoid a timeout. Otherwise we process the
event normally and return a correct handled status.

BUG=604069
TEST=added to base_unittests and views_mus_unittests

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