Adds ability for pre-target accelerators to not consume events

Adds ability for pre-target accelerators to not consume events

Prior to this change if you registered an accelerator for a key event
then only the accelerator got the event. After this change the tree
that registered the accelerator (wm) need not consume the event. If
the wm doesn't consume the event it'll be processed normally.

BUG=612331
TEST=covered by tests
R=sadrul@chromium.org, dcheng@chromium.org

Committed: https://crrev.com/76d46665b53e76c0d8c2e6a642ae686ef2b44048
Cr-Commit-Position: refs/heads/master@{#404450}

[dcheng, 2016-07-07 04:04:21]

https://codereview.chromium.org/2125883003/diff/1/services/ui/ws/window_tree.cc
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2125883003/diff/1/services/ui/ws/window_tree....
services/ui/ws/window_tree.cc:970: event_ack_id_ = 0x1000000 | (rand() &
0xffffff);
Out of curiosity, are there/should there be any requirements around this not
being guessable, since it's passed back from a WindowTreeClient to the
WindowTree?

https://codereview.chromium.org/2125883003/diff/1/ui/events/mojo/event_consta...
File ui/events/mojo/event_constants.mojom (right):

https://codereview.chromium.org/2125883003/diff/1/ui/events/mojo/event_consta...
ui/events/mojo/event_constants.mojom:9: // . EventMatcher registered for
PRE_TARGET. If the EventMatcher does not
I was wondering this while reviewing the dragging CL, but can a renderer somehow
use SetEventObserver to capture events it shouldn't be privy too? I'm assuming
the WindowTree that the embedded renderer sees is scoped to just the stuff under
that renderer?

[sky, 2016-07-07 13:38:38]

https://codereview.chromium.org/2125883003/diff/1/services/ui/ws/window_tree.cc
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2125883003/diff/1/services/ui/ws/window_tree....
services/ui/ws/window_tree.cc:970: event_ack_id_ = 0x1000000 | (rand() &
0xffffff);
On 2016/07/07 04:04:21, dcheng wrote:
> Out of curiosity, are there/should there be any requirements around this not
> being guessable, since it's passed back from a WindowTreeClient to the
> WindowTree?

I don't think it's been given much thought. If the client starts calling
OnWindowInputEventAck randomly we'll hit a NOTIMPLEMENTED on 1305. We need to
decide what the right handling is.

https://codereview.chromium.org/2125883003/diff/1/ui/events/mojo/event_consta...
File ui/events/mojo/event_constants.mojom (right):

https://codereview.chromium.org/2125883003/diff/1/ui/events/mojo/event_consta...
ui/events/mojo/event_constants.mojom:9: // . EventMatcher registered for
PRE_TARGET. If the EventMatcher does not
On 2016/07/07 04:04:21, dcheng wrote:
> I was wondering this while reviewing the dragging CL, but can a renderer
somehow
> use SetEventObserver to capture events it shouldn't be privy too? I'm assuming
> the WindowTree that the embedded renderer sees is scoped to just the stuff
under
> that renderer?

I've moved these comments to clarify they are for accelerators, which only the
windowmanager can register. Other clients can register an eventobserver which
can see events they normally wouldn't, but event observers can't consume events.

[sadrul, 2016-07-07 20:04:42]

I think we should allow this only for key-events, because for mouse/touch-event
based accelerators, we shouldn't wait for async responses from other clients
before we send an event to the target window. Even then, it would still be
possible for touch-events to be waiting on ack of a just-sent keyboard
accelerator from the window manager, before the touch-event can be dispatched to
the target client. Is this correct (because this feels a bit unfortunate)? What
are the use-cases of this behaviour?

https://codereview.chromium.org/2125883003/diff/60001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2125883003/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:36: //   processing stops. If
the EventMatcher does not consume the event processing
Do you mean 'If the accelerator handler does not consume the event ...'?

https://codereview.chromium.org/2125883003/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:39: //   matching event
observers and notified at the same time. The target is only
s/and/are

https://codereview.chromium.org/2125883003/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:41: //   target consumes the
event, processing stops.
What happens if there is a non-accelerator event for the target, but the
accelerator is yet to be acked by the window manager?

[sky, 2016-07-07 21:51:09]

On Thu, Jul 7, 2016 at 1:04 PM, <sadrul@chromium.org> wrote:
>
> I think we should allow this only for key-events, because for
mouse/touch-event
> based accelerators, we shouldn't wait for async responses from other clients
> before we send an event to the target window.

I think there are other use cases where we will need to wait for the
windowmanager before sending to the client. For example, maximized
mode has an eventfilter that exits (and stops propagation) for touchs
occurring in a special region.

> Even then, it would still be
> possible for touch-events to be waiting on ack of a just-sent keyboard
> accelerator from the window manager, before the touch-event can be dispatched
to
> the target client. Is this correct (because this feels a bit unfortunate)?

Touch events are already blocked on keyevent delivery to the focused
app anyway, right? This only adds an extra hop if the windowmanager
has registered a matching accelerator and it doesn't consume it.

> What
> are the use-cases of this behaviour?

I want to bring up ash/accelerators. That code may or may not need to
consume the event. When I get that code working I should be able to
remove AcceleratorRegistrar.

>
https://codereview.chromium.org/2125883003/diff/60001/services/ui/public/inte...
> File services/ui/public/interfaces/window_tree.mojom (right):
>
>
https://codereview.chromium.org/2125883003/diff/60001/services/ui/public/inte...
> services/ui/public/interfaces/window_tree.mojom:36: // processing
> stops. If the EventMatcher does not consume the event processing
> Do you mean 'If the accelerator handler does not consume the event ...'?
>
>
https://codereview.chromium.org/2125883003/diff/60001/services/ui/public/inte...
> services/ui/public/interfaces/window_tree.mojom:39: // matching event
> observers and notified at the same time. The target is only
> s/and/are
>
>
https://codereview.chromium.org/2125883003/diff/60001/services/ui/public/inte...
> services/ui/public/interfaces/window_tree.mojom:41: // target consumes
> the event, processing stops.
> What happens if there is a non-accelerator event for the target, but the
> accelerator is yet to be acked by the window manager?
>
> https://codereview.chromium.org/2125883003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2016-07-07 21:55:43]

https://codereview.chromium.org/2125883003/diff/60001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2125883003/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:36: //   processing stops. If
the EventMatcher does not consume the event processing
On 2016/07/07 20:04:42, sadrul wrote:
> Do you mean 'If the accelerator handler does not consume the event ...'?

I changed it to client as that is what I said on the previous line.

https://codereview.chromium.org/2125883003/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:39: //   matching event
observers and notified at the same time. The target is only
On 2016/07/07 20:04:42, sadrul wrote:
> s/and/are

Done.

https://codereview.chromium.org/2125883003/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:41: //   target consumes the
event, processing stops.
On 2016/07/07 20:04:41, sadrul wrote:
> What happens if there is a non-accelerator event for the target, but the
> accelerator is yet to be acked by the window manager?

Not sure what you mean. If there is no-accelerator than we aren't blocked
waiting for the wm. OTOH if there is an accelerator then step 1 waits until we
get the ack from the wm. Maybe I'm misunderstanding your question?

[sadrul, 2016-07-08 15:40:45]

Mostly nits. Left some comments about using ui::EventResult and ui::EventPhase
where possible. Otherwise, lgtm

https://codereview.chromium.org/2125883003/diff/80001/services/ui/public/cpp/...
File services/ui/public/cpp/window_manager_delegate.h (right):

https://codereview.chromium.org/2125883003/diff/80001/services/ui/public/cpp/...
services/ui/public/cpp/window_manager_delegate.h:109: virtual mojom::EventResult
OnAccelerator(uint32_t id, const ui::Event& event);
ui::EventResult instead?

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/event_di...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/event_di...
services/ui/ws/event_dispatcher.cc:258: if (event.IsKeyEvent()) {
Should we add some debug check here to verify that we are sending the right
event with the expected phase (e.g. of match_phase is POST_ONLY, then the
previous |match_phase| was ANY and |event| was the same)?

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/event_di...
File services/ui/ws/event_dispatcher.h (right):

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/event_di...
services/ui/ws/event_dispatcher.h:49: 
Would it make sense to set Event::phase_ on the event instead?

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.h (right):

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.h:100: 
Perhaps use ui::EventPhase here too?

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:440:
window_manager_internal_->OnAccelerator(ack_id, accelerator_id,
Use event_ack_id_ instead?

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1307: DVLOG(1) << "OnAcceleratorAck supplied
unexpected event_id";
This is the wrong place?

[sky, 2016-07-08 16:23:51]

https://codereview.chromium.org/2125883003/diff/80001/services/ui/public/cpp/...
File services/ui/public/cpp/window_manager_delegate.h (right):

https://codereview.chromium.org/2125883003/diff/80001/services/ui/public/cpp/...
services/ui/public/cpp/window_manager_delegate.h:109: virtual mojom::EventResult
OnAccelerator(uint32_t id, const ui::Event& event);
On 2016/07/08 15:40:43, sadrul wrote:
> ui::EventResult instead?

This code only cares about two results. ui::EventResult defines four values, two
of which don't make sense in this context. I think it best to stick with
mojom::EventResult.

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/event_di...
File services/ui/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/event_di...
services/ui/ws/event_dispatcher.cc:258: if (event.IsKeyEvent()) {
On 2016/07/08 15:40:43, sadrul wrote:
> Should we add some debug check here to verify that we are sending the right
> event with the expected phase (e.g. of match_phase is POST_ONLY, then the
> previous |match_phase| was ANY and |event| was the same)?

Done.

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/event_di...
File services/ui/ws/event_dispatcher.h (right):

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/event_di...
services/ui/ws/event_dispatcher.h:49: 
On 2016/07/08 15:40:44, sadrul wrote:
> Would it make sense to set Event::phase_ on the event instead?

Similar comment to the other type, ui::EventPhase doesn't really match the
phases we have now, so I'm inclined to stick with just this.

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/window_m...
File services/ui/ws/window_manager_state.h (right):

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/window_m...
services/ui/ws/window_manager_state.h:100: 
On 2016/07/08 15:40:44, sadrul wrote:
> Perhaps use ui::EventPhase here too?

Similar comment about ui::EventPhase containing extraneous states.

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:440:
window_manager_internal_->OnAccelerator(ack_id, accelerator_id,
On 2016/07/08 15:40:44, sadrul wrote:
> Use event_ack_id_ instead?

Done.

https://codereview.chromium.org/2125883003/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1307: DVLOG(1) << "OnAcceleratorAck supplied
unexpected event_id";
On 2016/07/08 15:40:44, sadrul wrote:
> This is the wrong place?

Done.

[dcheng, 2016-07-08 17:12:13]

LGTM. Thanks for writing up some docs!

As discussed offline:
- in the future, consider preventing embeds from using event observer
- i'm a bit iffy about this use of rand(). rand() is fairly guessable, and I'm
not sure it's even properly seeded in Chrome. But that already exists today. But
we probably want to revisit this: either we don't care about guessability (and
it should just be a straight sequence number) or we do care about guessability
and we should make it truly random.

[sky, 2016-07-08 17:28:01]

The CQ bit was checked by sky@chromium.org

[sky, 2016-07-08 17:28:02]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2125883003/#ps120001
(title: "merge")

[commit-bot: I haz the power, 2016-07-08 17:28:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 18:43:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-08 18:45:04]

Description was changed from

==========
Adds ability for pre-target accelerators to not consume events

Prior to this change if you registered an accelerator for a key event
then only the accelerator got the event. After this change the tree
that registered the accelerator (wm) need not consume the event. If
the wm doesn't consume the event it'll be processed normally.

BUG=612331
TEST=covered by tests
R=sadrul@chromium.org, dcheng@chromium.org
==========

to

==========
Adds ability for pre-target accelerators to not consume events

Prior to this change if you registered an accelerator for a key event
then only the accelerator got the event. After this change the tree
that registered the accelerator (wm) need not consume the event. If
the wm doesn't consume the event it'll be processed normally.

BUG=612331
TEST=covered by tests
R=sadrul@chromium.org, dcheng@chromium.org

Committed: https://crrev.com/76d46665b53e76c0d8c2e6a642ae686ef2b44048
Cr-Commit-Position: refs/heads/master@{#404450}
==========

[commit-bot: I haz the power, 2016-07-08 18:45:05]

Patchset 7 (id:??) landed as
https://crrev.com/76d46665b53e76c0d8c2e6a642ae686ef2b44048
Cr-Commit-Position: refs/heads/master@{#404450}