Add window cycle completion and cancellation accelerators in mus+ash.

Add window cycle completion and cancellation accelerators in mus+ash.

Adds two accelerators to complete or cancel window cycle. Note that this
is just a temporary solution until we have proper solution for window
cycle event handling.

BUG=629191
Review-Url: https://codereview.chromium.org/2783613003
Cr-Commit-Position: refs/heads/master@{#460900}
Committed: https://chromium.googlesource.com/chromium/src/+/28527342321e416b8805e728dbbfa62d1c445b1c

[Hadi, 2017-03-28 16:00:30]

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

[commit-bot: I haz the power, 2017-03-28 16:01:06]

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

[Hadi, 2017-03-28 16:01:38]

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

[Hadi, 2017-03-28 16:01:39]

PTAL.

[Hadi, 2017-03-28 16:06:46]

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

[commit-bot: I haz the power, 2017-03-28 16:07:20]

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

[Hadi, 2017-03-28 16:08:37]

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

[commit-bot: I haz the power, 2017-03-28 16:09:10]

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

[sky, 2017-03-28 17:39:48]

https://codereview.chromium.org/2783613003/diff/40001/ash/mus/window_cycle_ev...
File ash/mus/window_cycle_event_filter_mus.cc (right):

https://codereview.chromium.org/2783613003/diff/40001/ash/mus/window_cycle_ev...
ash/mus/window_cycle_event_filter_mus.cc:17: const ui::Accelerator
kAccelerators[] = {
What phase of event processing do we end up processing these accelerators in? By
that I mean do we end up processing the accelerator in the pre or post phase
(see
https://chromium.googlesource.com/chromium/src/+/master/ash/mus/accelerators/...
).

[Hadi, 2017-03-28 17:57:52]

https://codereview.chromium.org/2783613003/diff/40001/ash/mus/window_cycle_ev...
File ash/mus/window_cycle_event_filter_mus.cc (right):

https://codereview.chromium.org/2783613003/diff/40001/ash/mus/window_cycle_ev...
ash/mus/window_cycle_event_filter_mus.cc:17: const ui::Accelerator
kAccelerators[] = {
On 2017/03/28 17:39:48, sky wrote:
> What phase of event processing do we end up processing these accelerators in?
By
> that I mean do we end up processing the accelerator in the pre or post phase
> (see
>
https://chromium.googlesource.com/chromium/src/+/master/ash/mus/accelerators/...
> ).

It is in pre phase in the few tries that I logged, and I think it should be in
the pre-phase. I think to make sure these are always in pre phase, we should
modify AcceleratorRouter::ShouldProcessAcceleratorNow() to always return true
for these accelerators. It currently returns true for alt+tab since it is in
kPreferredActions.

I think the change to return true for alt-release won't be very simple and I
first need to regenerate some cases that causes the problem, so can we do it in
another CL?

[commit-bot: I haz the power, 2017-03-28 18:04:45]

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

[commit-bot: I haz the power, 2017-03-28 18:04:46]

Dry run: This issue passed the CQ dry run.

[Hadi, 2017-03-28 19:36:03]

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

[commit-bot: I haz the power, 2017-03-28 19:36:27]

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

[Hadi, 2017-03-28 19:36:42]

https://codereview.chromium.org/2783613003/diff/40001/ash/mus/window_cycle_ev...
File ash/mus/window_cycle_event_filter_mus.cc (right):

https://codereview.chromium.org/2783613003/diff/40001/ash/mus/window_cycle_ev...
ash/mus/window_cycle_event_filter_mus.cc:17: const ui::Accelerator
kAccelerators[] = {
On 2017/03/28 17:39:48, sky wrote:
> What phase of event processing do we end up processing these accelerators in?
By
> that I mean do we end up processing the accelerator in the pre or post phase
> (see
>
https://chromium.googlesource.com/chromium/src/+/master/ash/mus/accelerators/...
> ).

I added a check to make sure these accelerators are tried in pre-target phase.

[sky, 2017-03-28 19:42:34]

It's a bit mysterious to have the isPretargetAccelerator separate from the
accelerators. How about moving the registration of this code into
the AcceleratorControllerRegistrar? In other words
no WindowCycleEventFilterMus.

On Tue, Mar 28, 2017 at 12:36 PM, <moshayedi@chromium.org> wrote:

>
> https://codereview.chromium.org/2783613003/diff/40001/ash/
> mus/window_cycle_event_filter_mus.cc
> File ash/mus/window_cycle_event_filter_mus.cc (right):
>
> https://codereview.chromium.org/2783613003/diff/40001/ash/
> mus/window_cycle_event_filter_mus.cc#newcode17
> ash/mus/window_cycle_event_filter_mus.cc:17: const ui::Accelerator
> kAccelerators[] = {
> On 2017/03/28 17:39:48, sky wrote:
> > What phase of event processing do we end up processing these
> accelerators in? By
> > that I mean do we end up processing the accelerator in the pre or post
> phase
> > (see
> >
> https://chromium.googlesource.com/chromium/src/+/master/ash/
> mus/accelerators/accelerator_controller_registrar.cc#75
> > ).
>
> I added a check to make sure these accelerators are tried in pre-target
> phase.
>
> https://codereview.chromium.org/2783613003/
>

-- 
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.

[commit-bot: I haz the power, 2017-03-28 19:56:09]

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

[commit-bot: I haz the power, 2017-03-28 19:56:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hadi, 2017-03-29 20:22:52]

Description was changed from

==========
Implement WindowCycleEventFilter for mus.

This CL implements WindowCycleEventFilterMus by registering accelerators
for alt release and alt-esc press.

BUG=629191
==========

to

==========
Add window cycle completion and cancellation accelerators in mus+ash.

Adds two accelerators to complete or cancel window cycle. Note that this
is just a temporary solution until we have proper solution for window
cycle event handling.

BUG=629191
==========

[Hadi, 2017-03-29 20:25:26]

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

[commit-bot: I haz the power, 2017-03-29 20:26:20]

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

[Hadi, 2017-03-29 20:26:49]

On 2017/03/28 19:42:34, sky wrote:
> It's a bit mysterious to have the isPretargetAccelerator separate from the
> accelerators. How about moving the registration of this code into
> the AcceleratorControllerRegistrar? In other words
> no WindowCycleEventFilterMus.

Sounds good. I moved these to AcceleratorControllerRegistrar.

[commit-bot: I haz the power, 2017-03-29 21:31:09]

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

[commit-bot: I haz the power, 2017-03-29 21:31:09]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-29 22:00:58]

Are there any tests that can be enabled now?

https://codereview.chromium.org/2783613003/diff/100001/ash/mus/accelerators/a...
File ash/mus/accelerators/accelerator_controller_registrar.cc (right):

https://codereview.chromium.org/2783613003/diff/100001/ash/mus/accelerators/a...
ash/mus/accelerators/accelerator_controller_registrar.cc:27: const
ui::Accelerator kWindowCycleCompleteAccelerator(
Style guide says statics (which include this) should only be POD (plain old
data). This is why we have structures like AcceleratorData. Make the two
accelerators members of AcceleratorControllerRegistrar.

Also, you shouldn't need to create an accelerator from a keyevent, create the
accelerator directly.

[Hadi, 2017-03-30 13:28:30]

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

[Hadi, 2017-03-30 13:28:34]

On 2017/03/29 22:00:58, sky wrote:
> Style guide says statics (which include this) should only be POD (plain old
> data). This is why we have structures like AcceleratorData. Make the two
> accelerators members of AcceleratorControllerRegistrar.

Done.

> Also, you shouldn't need to create an accelerator from a keyevent, create the
> accelerator directly.

Added key_state to constructor of ui::Accelerator so we don't need to use the
KeyEvent constructor for const release accelerators.

[commit-bot: I haz the power, 2017-03-30 13:28:57]

Dry run: 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, 2017-03-30 14:35:00]

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

[commit-bot: I haz the power, 2017-03-30 14:35:00]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-30 20:07:29]

LGTM with the following change

https://codereview.chromium.org/2783613003/diff/120001/ash/mus/accelerators/a...
File ash/mus/accelerators/accelerator_controller_registrar.h (right):

https://codereview.chromium.org/2783613003/diff/120001/ash/mus/accelerators/a...
ash/mus/accelerators/accelerator_controller_registrar.h:49: const
ui::Accelerator kWindowCycleCompleteAccelerator;
These should be unix_hacker_style. Use kFoo when "whose value is fixed for the
duration of the program", which isn't the case here.

[Hadi, 2017-03-30 20:39:56]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2017-03-30 20:39:57]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2783613003/#ps140001
(title: "rename constants.")

[commit-bot: I haz the power, 2017-03-30 20:40:36]

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, 2017-03-30 21:56:01]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490906396509150,
"parent_rev": "f9baf94fc70de8725932d3421fafa85db3d3cfbf", "commit_rev":
"28527342321e416b8805e728dbbfa62d1c445b1c"}

[commit-bot: I haz the power, 2017-03-30 21:56:48]

Description was changed from

==========
Add window cycle completion and cancellation accelerators in mus+ash.

Adds two accelerators to complete or cancel window cycle. Note that this
is just a temporary solution until we have proper solution for window
cycle event handling.

BUG=629191
==========

to

==========
Add window cycle completion and cancellation accelerators in mus+ash.

Adds two accelerators to complete or cancel window cycle. Note that this
is just a temporary solution until we have proper solution for window
cycle event handling.

BUG=629191

Review-Url: https://codereview.chromium.org/2783613003
Cr-Commit-Position: refs/heads/master@{#460900}
Committed:
https://chromium.googlesource.com/chromium/src/+/28527342321e416b8805e728dbbf...
==========

[commit-bot: I haz the power, 2017-03-30 21:56:50]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/28527342321e416b8805e728dbbf...