wm: Allow double tap on overview button to quick switch.

wm: Allow double tap on overview button to quick switch.

Double-tapping the overview button will now fire up the most recently used app. For now I left the double tap logic inside the overview button since AFAIK there aren't much uses of double tap. But we can move the logic to CustomButton or something else if desired.

TEST=ash_unittests --gtest-filter="OverviewButtonTrayTest.*"
BUG=714869
Review-Url: https://codereview.chromium.org/2882073004
Cr-Commit-Position: refs/heads/master@{#473119}
Committed: https://chromium.googlesource.com/chromium/src/+/302f124fa2846ceb4912152efe5f22032bbda255

[sammiequon, 2017-05-15 19:45:25]

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

[commit-bot: I haz the power, 2017-05-15 19:46:03]

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-05-15 20:24:37]

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

[commit-bot: I haz the power, 2017-05-15 20:24:38]

Dry run: This issue passed the CQ dry run.

[sammiequon, 2017-05-16 22:55:15]

Patchset #1 (id:1) has been deleted

[sammiequon, 2017-05-16 22:58:24]

Description was changed from

==========
wm: Allow double tap on overview button to quick switch.

Double-tapping the overview button will now fire up the most recently used app.
The caveat is that single-tap events are now delayed (by as long as needed to
determine a double-tap).

TEST=ash_unittests --gtest-filter="OverviewButtonTrayTest.*"
BUG=714869
==========

to

==========
wm: Allow double tap on overview button to quick switch.

Double-tapping the overview button will now fire up the most recently used app.
For now I left the double tap logic inside the overview button since AFAIK there
aren't much uses of double tap. But we can move the logic to CustomButton or
something else if desired.

TEST=ash_unittests --gtest-filter="OverviewButtonTrayTest.*"
BUG=714869
==========

[sammiequon, 2017-05-16 22:58:24]

sammiequon@chromium.org changed reviewers:
+ derat@chromium.org

[sammiequon, 2017-05-16 22:58:43]

derat@ - Could you take a look. Thanks!

[sammiequon, 2017-05-16 23:01:54]

sammiequon@chromium.org changed reviewers:
+ jamescook@chromium.org
- derat@chromium.org

[sammiequon, 2017-05-16 23:02:40]

On 2017/05/16 22:58:43, sammiequon wrote:
> derat@ - Could you take a look. Thanks!

-derat@ +jamescook@

jamescook@ - Could you take a look. Thanks!

[James Cook, 2017-05-16 23:21:30]

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

[James Cook, 2017-05-16 23:21:31]

+sadrul

Is it possible to build this with ET_GESTURE_DOUBLE_TAP, rather than building
all the timing yourself? Or DoubleTapListener?

https://cs.chromium.org/chromium/src/ui/events/event_constants.h?dr&l=51

[sammiequon, 2017-05-16 23:40:51]

On 2017/05/16 23:21:31, James Cook wrote:
> +sadrul
> 
> Is it possible to build this with ET_GESTURE_DOUBLE_TAP, rather than building
> all the timing yourself? Or DoubleTapListener?
> 
> https://cs.chromium.org/chromium/src/ui/events/event_constants.h?dr&l=51

Yes it would be but I do mouse clicks count as gestures when the mouse is
plugged in in tablet mode?

[Daniel Erat, 2017-05-17 01:06:44]

(looks like you found other reviewers; i'm unfamiliar with this code)

[sammiequon, 2017-05-17 01:27:12]

On 2017/05/17 01:06:44, Daniel Erat wrote:
> (looks like you found other reviewers; i'm unfamiliar with this code)

Yeah, I thought you were OOO! Maybe I was looking at the wrong week.

[sadrul, 2017-05-17 04:22:13]

On 2017/05/16 23:40:51, sammiequon wrote:
> On 2017/05/16 23:21:31, James Cook wrote:
> > +sadrul
> > 
> > Is it possible to build this with ET_GESTURE_DOUBLE_TAP, rather than
building
> > all the timing yourself? Or DoubleTapListener?
> > 
> > https://cs.chromium.org/chromium/src/ui/events/event_constants.h?dr&l=51
> 
> Yes it would be but I do mouse clicks count as gestures when the mouse is
> plugged in in tablet mode?

Mouse clicks should not be treated as gestures, no. For mouse-clicks, there is
EF_IS_DOUBLE_CLICK [1] that you can use if you want.

[1]
https://cs.chromium.org/chromium/src/ui/events/event_constants.h?type=cs&l=132

[sammiequon, 2017-05-17 16:30:40]

On 2017/05/17 04:22:13, sadrul wrote:
> On 2017/05/16 23:40:51, sammiequon wrote:
> > On 2017/05/16 23:21:31, James Cook wrote:
> > > +sadrul
> > > 
> > > Is it possible to build this with ET_GESTURE_DOUBLE_TAP, rather than
> building
> > > all the timing yourself? Or DoubleTapListener?
> > > 
> > > https://cs.chromium.org/chromium/src/ui/events/event_constants.h?dr&l=51
> > 
> > Yes it would be but I do mouse clicks count as gestures when the mouse is
> > plugged in in tablet mode?
> 
> Mouse clicks should not be treated as gestures, no. For mouse-clicks, there is
> EF_IS_DOUBLE_CLICK [1] that you can use if you want.
> 
> [1]
> https://cs.chromium.org/chromium/src/ui/events/event_constants.h?type=cs&l=132

Hi Sadrul|James,

Quick question, I added OnGestureEvent(..) into overview_button_tray but I am
not seeing any ET_GESTURE_DOUBLE_TAP events coming through, though they are sent
out at [1]. Is there anything else I have to enable in addition to [2] (or
should I have not done this). Also, would using ET_GESTURE_TAP and tap_count()
be good enough? Seems like that the more common way from my searches.

Thanks!

[1]
https://cs.chromium.org/chromium/src/ui/events/gesture_detection/gesture_prov...
[2]
https://cs.chromium.org/chromium/src/ui/events/gestures/gesture_provider_aura...

[James Cook, 2017-05-17 16:34:29]

Sadrul's probably the best person to answer.

[sammiequon, 2017-05-18 01:20:35]

sadrul@ - I went ahead and used tap_count() because I did not see much uses of
ET_GESTURE_DOUBLE_TAP. Please take a look (or let me know if
ET_GESTURE_DOUBLE_TAP is the way to go). Thanks!

[sadrul, 2017-05-18 13:41:39]

lgtm

https://codereview.chromium.org/2882073004/diff/40001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray_unittest.cc (right):

https://codereview.chromium.org/2882073004/diff/40001/ash/system/overview/ove...
ash/system/overview/overview_button_tray_unittest.cc:122: // two single taps and
open then close overview mode.
I don't think this case is actually tested. Can you add this case, or remove the
comment?

[James Cook, 2017-05-18 15:10:54]

LGTM after sadrul's comment addressed.

https://codereview.chromium.org/2882073004/diff/40001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray_unittest.cc (right):

https://codereview.chromium.org/2882073004/diff/40001/ash/system/overview/ove...
ash/system/overview/overview_button_tray_unittest.cc:125:
wm::ActivateWindow(window1.get());
Do you have to activate this one?

https://codereview.chromium.org/2882073004/diff/40001/ash/system/overview/ove...
ash/system/overview/overview_button_tray_unittest.cc:150:
EXPECT_TRUE(Shell::Get()->window_selector_controller()->IsSelecting());
Nice test - easy to read. I like the style of blocks of code with comments about
what you're doing.

[sammiequon, 2017-05-18 16:19:06]

Thanks!

https://codereview.chromium.org/2882073004/diff/40001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray_unittest.cc (right):

https://codereview.chromium.org/2882073004/diff/40001/ash/system/overview/ove...
ash/system/overview/overview_button_tray_unittest.cc:122: // two single taps and
open then close overview mode.
On 2017/05/18 13:41:38, sadrul wrote:
> I don't think this case is actually tested. Can you add this case, or remove
the
> comment?

Done.

https://codereview.chromium.org/2882073004/diff/40001/ash/system/overview/ove...
ash/system/overview/overview_button_tray_unittest.cc:125:
wm::ActivateWindow(window1.get());
On 2017/05/18 15:10:54, James Cook wrote:
> Do you have to activate this one?

Nope. Removed.

https://codereview.chromium.org/2882073004/diff/40001/ash/system/overview/ove...
ash/system/overview/overview_button_tray_unittest.cc:150:
EXPECT_TRUE(Shell::Get()->window_selector_controller()->IsSelecting());
On 2017/05/18 15:10:54, James Cook wrote:
> Nice test - easy to read. I like the style of blocks of code with comments
about
> what you're doing.

Thanks :-).

[sammiequon, 2017-05-18 16:20:49]

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

[commit-bot: I haz the power, 2017-05-18 16:22:01]

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-05-18 17:08:12]

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

[commit-bot: I haz the power, 2017-05-18 17:08:13]

Dry run: This issue passed the CQ dry run.

[sammiequon, 2017-05-18 19:28:02]

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

[commit-bot: I haz the power, 2017-05-18 19:29:12]

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-05-18 20:14:59]

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

[commit-bot: I haz the power, 2017-05-18 20:15:00]

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

[sammiequon, 2017-05-18 20:41:47]

Patchset #4 (id:80001) has been deleted

[sammiequon, 2017-05-18 20:47:44]

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

[commit-bot: I haz the power, 2017-05-18 20:49:02]

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-05-18 21:55:47]

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

[commit-bot: I haz the power, 2017-05-18 21:55:48]

Dry run: This issue passed the CQ dry run.

[sammiequon, 2017-05-18 22:05:55]

jamescook@ - I had to make a few changes to make sure things work as expected.
Could you check if it is still OK? Thanks!

[James Cook, 2017-05-19 02:37:31]

still lgtm

[sammiequon, 2017-05-19 07:31:23]

On 2017/05/19 02:37:31, James Cook wrote:
> still lgtm

Thanks!

[sammiequon, 2017-05-19 07:31:31]

The CQ bit was checked by sammiequon@chromium.org

[sammiequon, 2017-05-19 07:31:32]

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/2882073004/#ps100001
(title: "Slight fix.")

[commit-bot: I haz the power, 2017-05-19 07:32:16]

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-05-19 07:51:32]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495179091848090,
"parent_rev": "4eec0f46bf71277f9de364ea8f4fb2f41d894b16", "commit_rev":
"302f124fa2846ceb4912152efe5f22032bbda255"}

[commit-bot: I haz the power, 2017-05-19 07:51:50]

Description was changed from

==========
wm: Allow double tap on overview button to quick switch.

Double-tapping the overview button will now fire up the most recently used app.
For now I left the double tap logic inside the overview button since AFAIK there
aren't much uses of double tap. But we can move the logic to CustomButton or
something else if desired.

TEST=ash_unittests --gtest-filter="OverviewButtonTrayTest.*"
BUG=714869
==========

to

==========
wm: Allow double tap on overview button to quick switch.

Double-tapping the overview button will now fire up the most recently used app.
For now I left the double tap logic inside the overview button since AFAIK there
aren't much uses of double tap. But we can move the logic to CustomButton or
something else if desired.

TEST=ash_unittests --gtest-filter="OverviewButtonTrayTest.*"
BUG=714869

Review-Url: https://codereview.chromium.org/2882073004
Cr-Commit-Position: refs/heads/master@{#473119}
Committed:
https://chromium.googlesource.com/chromium/src/+/302f124fa2846ceb4912152efe5f...
==========

[commit-bot: I haz the power, 2017-05-19 07:51:51]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/302f124fa2846ceb4912152efe5f...