Use active state on touch start for tray icons

Adds touch feedback to tray icons in touchview when --ash-enable-touchview-touch-feedback flag is passed.

TEST=WebNotificationTrayTest.MAYBE_NoTouchFeedback, WebNotificationTrayTest.MAYBE_MaximizeModeTouchFeedback, WebNotificationTrayTest.MAYBE_TouchFeedbackCancellation
BUG=398398
Committed: https://crrev.com/367a1bee843f70e3cf6e3ea439e6b1711d80f059
Cr-Commit-Position: refs/heads/master@{#293608}

[jonross, 2014-08-27 15:22:36]

jonross@chromium.org changed reviewers:
+ flackr@chromium.org

[jonross, 2014-08-27 15:22:36]

Hey Rob,

This review adds a flag for all of the upcoming TouchView touch feedback feature
work.

[flackr, 2014-08-27 18:35:31]

Can we land this as part of the first patch to land some of this behavior? We
typically don't add the flag until it has some effect.

https://codereview.chromium.org/515573002/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/515573002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:5949: +          Enter TouchView mode in
order to receive additional visual feedback for touch input.
How about something like "Enables additional touch feedback on UI controls when
in TouchView mode".

[jonross, 2014-08-28 21:41:40]

I have added an implementation of touch feedback to TrayBackgroundView. This
will allow the trays to respond to touch presses.

[flackr, 2014-08-29 15:34:10]

Can you update the description to be about the touch feedback? The addition of
the flag is really secondary to the changes being made. I.e. "Adds touch
feedback to tray icons in touchview when --ash-enable-touchview-touch-feedback
flag is passed.

Also, the title can be something which explicitly calls out the added feature
like "Use active state on touch start on tray icons."

https://codereview.chromium.org/515573002/diff/20001/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/515573002/diff/20001/ash/ash_switches.h#newco...
ash/ash_switches.h:36: #if defined(OS_CHROMEOS)
why OS_CHROMEOS only?

https://codereview.chromium.org/515573002/diff/20001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/515573002/diff/20001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:444: switch (event->type()) {
For so few cases an if / else if is probably cleaner.

https://codereview.chromium.org/515573002/diff/20001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:450: case ui::ET_GESTURE_TAP_CANCEL: {
Do you need ET_GESTURE_TAP as well to restore the inactive background when
complete?

https://codereview.chromium.org/515573002/diff/20001/ash/system/web_notificat...
File ash/system/web_notification/web_notification_tray_unittest.cc (right):

https://codereview.chromium.org/515573002/diff/20001/ash/system/web_notificat...
ash/system/web_notification/web_notification_tray_unittest.cc:490: #define
MAYBE_TouchFeedbackCancellation TouchFeedbackCancellation
Assuming we do want this CHROMEOS only I'd just put the tests in the #if
defined(OS_CHROMEOS) since they're not relevant on other platforms. Usually
platform specific disables are because of a bug rather than choosing not to do
the thing on that platform.

[jonross, 2014-08-29 17:20:21]

https://codereview.chromium.org/515573002/diff/20001/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/515573002/diff/20001/ash/ash_switches.h#newco...
ash/ash_switches.h:36: #if defined(OS_CHROMEOS)
On 2014/08/29 15:34:10, flackr wrote:
> why OS_CHROMEOS only?

Offline I was told to keep this to chrome os only. However if the flag was in
other ash builds there would be no functional difference for non-chrome os.

https://codereview.chromium.org/515573002/diff/20001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/515573002/diff/20001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:444: switch (event->type()) {
On 2014/08/29 15:34:10, flackr wrote:
> For so few cases an if / else if is probably cleaner.

Done.

https://codereview.chromium.org/515573002/diff/20001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:450: case ui::ET_GESTURE_TAP_CANCEL: {
On 2014/08/29 15:34:10, flackr wrote:
> Do you need ET_GESTURE_TAP as well to restore the inactive background when
> complete?

Currently when you touch these icons they are only active while their bubbles
are visible. When the bubble dismisses the active state is removed. I'm
currently not responding to the tap gesture to maintain this look.

So they become active on the touch down, and stay active until either a
cancelling gesture is received, or the bubble is dismissed.

https://codereview.chromium.org/515573002/diff/20001/ash/system/web_notificat...
File ash/system/web_notification/web_notification_tray_unittest.cc (right):

https://codereview.chromium.org/515573002/diff/20001/ash/system/web_notificat...
ash/system/web_notification/web_notification_tray_unittest.cc:490: #define
MAYBE_TouchFeedbackCancellation TouchFeedbackCancellation
On 2014/08/29 15:34:10, flackr wrote:
> Assuming we do want this CHROMEOS only I'd just put the tests in the #if
> defined(OS_CHROMEOS) since they're not relevant on other platforms. Usually
> platform specific disables are because of a bug rather than choosing not to do
> the thing on that platform.

Done.

[jonross, 2014-09-02 14:25:28]

Updated to reflect not restricting touch feedback to maximize mode.

[flackr, 2014-09-03 13:42:07]

https://codereview.chromium.org/515573002/diff/20001/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/515573002/diff/20001/ash/ash_switches.h#newco...
ash/ash_switches.h:36: #if defined(OS_CHROMEOS)
On 2014/08/29 17:20:21, jonross wrote:
> On 2014/08/29 15:34:10, flackr wrote:
> > why OS_CHROMEOS only?
> 
> Offline I was told to keep this to chrome os only. However if the flag was in
> other ash builds there would be no functional difference for non-chrome os.

There's no need to have the flag only built on chromeos. I think the intention
was not to list it in about_flags.cc on non-chromeos.

https://codereview.chromium.org/515573002/diff/60001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/515573002/diff/60001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:334: touch_feedback_enabled_ = true;
It would be better / cleaner to just check the flag when you're making a
decision based on this.

https://codereview.chromium.org/515573002/diff/60001/ash/system/web_notificat...
File ash/system/web_notification/web_notification_tray_unittest.cc (right):

https://codereview.chromium.org/515573002/diff/60001/ash/system/web_notificat...
ash/system/web_notification/web_notification_tray_unittest.cc:488:
TEST_F(WebNotificationTrayTest, MaximizeModeTouchFeedback) {
Just TouchFeedback, the feature doesn't need to be tied to maximize mode.

https://codereview.chromium.org/515573002/diff/60001/ash/system/web_notificat...
ash/system/web_notification/web_notification_tray_unittest.cc:505:
press.time_stamp()+base::TimeDelta::FromMilliseconds(50));
nit: spaces around operator.

https://codereview.chromium.org/515573002/diff/60001/ash/system/web_notificat...
ash/system/web_notification/web_notification_tray_unittest.cc:539:
generator.Dispatch(&move);
It should be inactive before the touch release right? Can we verify at this
time?

[jonross, 2014-09-03 15:48:21]

https://codereview.chromium.org/515573002/diff/20001/ash/ash_switches.h
File ash/ash_switches.h (right):

https://codereview.chromium.org/515573002/diff/20001/ash/ash_switches.h#newco...
ash/ash_switches.h:36: #if defined(OS_CHROMEOS)
On 2014/09/03 13:42:07, flackr wrote:
> On 2014/08/29 17:20:21, jonross wrote:
> > On 2014/08/29 15:34:10, flackr wrote:
> > > why OS_CHROMEOS only?
> > 
> > Offline I was told to keep this to chrome os only. However if the flag was
in
> > other ash builds there would be no functional difference for non-chrome os.
> 
> There's no need to have the flag only built on chromeos. I think the intention
> was not to list it in about_flags.cc on non-chromeos.

Done.

https://codereview.chromium.org/515573002/diff/60001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/515573002/diff/60001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:334: touch_feedback_enabled_ = true;
On 2014/09/03 13:42:07, flackr wrote:
> It would be better / cleaner to just check the flag when you're making a
> decision based on this.

Done.

https://codereview.chromium.org/515573002/diff/60001/ash/system/web_notificat...
File ash/system/web_notification/web_notification_tray_unittest.cc (right):

https://codereview.chromium.org/515573002/diff/60001/ash/system/web_notificat...
ash/system/web_notification/web_notification_tray_unittest.cc:488:
TEST_F(WebNotificationTrayTest, MaximizeModeTouchFeedback) {
On 2014/09/03 13:42:07, flackr wrote:
> Just TouchFeedback, the feature doesn't need to be tied to maximize mode.

Missed that.

https://codereview.chromium.org/515573002/diff/60001/ash/system/web_notificat...
ash/system/web_notification/web_notification_tray_unittest.cc:488:
TEST_F(WebNotificationTrayTest, MaximizeModeTouchFeedback) {
On 2014/09/03 13:42:07, flackr wrote:
> Just TouchFeedback, the feature doesn't need to be tied to maximize mode.

Missed that. Done.

https://codereview.chromium.org/515573002/diff/60001/ash/system/web_notificat...
ash/system/web_notification/web_notification_tray_unittest.cc:505:
press.time_stamp()+base::TimeDelta::FromMilliseconds(50));
On 2014/09/03 13:42:07, flackr wrote:
> nit: spaces around operator.

Done.

https://codereview.chromium.org/515573002/diff/60001/ash/system/web_notificat...
ash/system/web_notification/web_notification_tray_unittest.cc:539:
generator.Dispatch(&move);
On 2014/09/03 13:42:07, flackr wrote:
> It should be inactive before the touch release right? Can we verify at this
> time?

Done.

[flackr, 2014-09-03 16:41:44]

https://codereview.chromium.org/515573002/diff/80001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/515573002/diff/80001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:433:
HasSwitch(switches::kAshEnableTouchViewTouchFeedback)) {
Curious, does this introduce any perf hit? If so, we should check the event type
first since SCROLL_UPDATE for example is fired frequently but we only get one
TAP_DOWN, SCROLL_BEGIN and TAP_CANCEL.

https://codereview.chromium.org/515573002/diff/80001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/515573002/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:5916: +          Enable touch feedback in
TouchView Mode.
Update strings - maybe "Enable additional touch feedback on UI components"?

[jonross, 2014-09-03 18:51:44]

https://codereview.chromium.org/515573002/diff/80001/ash/system/tray/tray_bac...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/515573002/diff/80001/ash/system/tray/tray_bac...
ash/system/tray/tray_background_view.cc:433:
HasSwitch(switches::kAshEnableTouchViewTouchFeedback)) {
On 2014/09/03 16:41:44, flackr wrote:
> Curious, does this introduce any perf hit? If so, we should check the event
type
> first since SCROLL_UPDATE for example is fired frequently but we only get one
> TAP_DOWN, SCROLL_BEGIN and TAP_CANCEL.

Done.

https://codereview.chromium.org/515573002/diff/80001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/515573002/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:5916: +          Enable touch feedback in
TouchView Mode.
On 2014/09/03 16:41:44, flackr wrote:
> Update strings - maybe "Enable additional touch feedback on UI components"?

Done.

[flackr, 2014-09-03 21:44:35]

LGTM with nits

https://codereview.chromium.org/515573002/diff/100001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/515573002/diff/100001/ash/ash_switches.cc#new...
ash/ash_switches.cc:69: // Enables additional visual feedback to touch input
while the device is in
nit: it's always enabled, not just while in touchview.

https://codereview.chromium.org/515573002/diff/100001/ash/system/web_notifica...
File ash/system/web_notification/web_notification_tray_unittest.cc (right):

https://codereview.chromium.org/515573002/diff/100001/ash/system/web_notifica...
ash/system/web_notification/web_notification_tray_unittest.cc:98: #if
defined(OS_CHROMEOS)
nit: probably don't need these anymore, since the flag should work on all
platforms.

https://codereview.chromium.org/515573002/diff/100001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/515573002/diff/100001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5915: +        <message
name="IDS_FLAGS_ASH_ENABLE_TOUCH_VIEW_TOUCH_FEEDBACK_NAME" desc="Title for the
flag to enable additional visual feedback for touch while in TouchView mode.">
nit: Update desc= as well, remove "while in TouchView mode".

[jonross, 2014-09-04 15:17:02]

jonross@chromium.org changed reviewers:
+ oshima@chromium.org

[jonross, 2014-09-04 15:17:03]

Hi Oshima,

I've begun adding visual feedback for touch events. In this review I modify the
effects of the trays that appear in the shelf. Could you provide an owners
review for the files under ash/system?

https://codereview.chromium.org/515573002/diff/100001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/515573002/diff/100001/ash/ash_switches.cc#new...
ash/ash_switches.cc:69: // Enables additional visual feedback to touch input
while the device is in
On 2014/09/03 21:44:35, flackr wrote:
> nit: it's always enabled, not just while in touchview.

Done.

https://codereview.chromium.org/515573002/diff/100001/ash/system/web_notifica...
File ash/system/web_notification/web_notification_tray_unittest.cc (right):

https://codereview.chromium.org/515573002/diff/100001/ash/system/web_notifica...
ash/system/web_notification/web_notification_tray_unittest.cc:98: #if
defined(OS_CHROMEOS)
On 2014/09/03 21:44:35, flackr wrote:
> nit: probably don't need these anymore, since the flag should work on all
> platforms.

Done.

https://codereview.chromium.org/515573002/diff/100001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/515573002/diff/100001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5915: +        <message
name="IDS_FLAGS_ASH_ENABLE_TOUCH_VIEW_TOUCH_FEEDBACK_NAME" desc="Title for the
flag to enable additional visual feedback for touch while in TouchView mode.">
On 2014/09/03 21:44:35, flackr wrote:
> nit: Update desc= as well, remove "while in TouchView mode".

Done.

[oshima, 2014-09-04 18:35:54]

On 2014/09/04 15:17:03, jonross wrote:
> Hi Oshima,
> 
> I've begun adding visual feedback for touch events. In this review I modify
the
> effects of the trays that appear in the shelf. Could you provide an owners
> review for the files under ash/system?

It's better to ask one of ash/system/OWNERS.

> 
> https://codereview.chromium.org/515573002/diff/100001/ash/ash_switches.cc
> File ash/ash_switches.cc (right):
> 
>
https://codereview.chromium.org/515573002/diff/100001/ash/ash_switches.cc#new...
> ash/ash_switches.cc:69: // Enables additional visual feedback to touch input
> while the device is in
> On 2014/09/03 21:44:35, flackr wrote:
> > nit: it's always enabled, not just while in touchview.
> 
> Done.
> 
>
https://codereview.chromium.org/515573002/diff/100001/ash/system/web_notifica...
> File ash/system/web_notification/web_notification_tray_unittest.cc (right):
> 
>
https://codereview.chromium.org/515573002/diff/100001/ash/system/web_notifica...
> ash/system/web_notification/web_notification_tray_unittest.cc:98: #if
> defined(OS_CHROMEOS)
> On 2014/09/03 21:44:35, flackr wrote:
> > nit: probably don't need these anymore, since the flag should work on all
> > platforms.
> 
> Done.
> 
>
https://codereview.chromium.org/515573002/diff/100001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/515573002/diff/100001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:5915: +        <message
> name="IDS_FLAGS_ASH_ENABLE_TOUCH_VIEW_TOUCH_FEEDBACK_NAME" desc="Title for the
> flag to enable additional visual feedback for touch while in TouchView mode.">
> On 2014/09/03 21:44:35, flackr wrote:
> > nit: Update desc= as well, remove "while in TouchView mode".
> 
> Done.

[jonross, 2014-09-04 18:40:12]

jonross@chromium.org changed reviewers:
+ skuhne@chromium.org
- oshima@chromium.org

[jonross, 2014-09-04 18:40:13]

Hi Stefan,

Oshima has asked that I get an owner from ash/system for this review. Could you
provide the owner review? I've begun adding visual feedback for touch events. In
this review I modify the effects of the trays that appear in the shelf.

[Mr4D (OOO till 08-26), 2014-09-04 19:58:35]

Please see comments!

https://codereview.chromium.org/515573002/diff/120001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/515573002/diff/120001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:60:
SetDrawBackgroundAsActive(controller->IsSelecting());
Shouldn't this command also be dependent on the commandline option?

https://codereview.chromium.org/515573002/diff/120001/ash/system/tray/tray_ba...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/515573002/diff/120001/ash/system/tray/tray_ba...
ash/system/tray/tray_background_view.cc:439:
HasSwitch(switches::kAshEnableTouchViewTouchFeedback)) {
CommandLine::ForCurrentProcess()->
    HasSwitch(switches::kAshEnableTouchViewTouchFeedback)

is being used twice here. Since it is in both conditions you might want to check
this only once like

if (commandline..) {
   if (..) {}
   else if {}
}

Furthermore - this is a fixed value throughout the session and touch functions
can get called a lot, so you might consider some pre-parsing

https://codereview.chromium.org/515573002/diff/120001/ash/system/web_notifica...
File ash/system/web_notification/web_notification_tray_unittest.cc (right):

https://codereview.chromium.org/515573002/diff/120001/ash/system/web_notifica...
ash/system/web_notification/web_notification_tray_unittest.cc:537:
generator.Dispatch(&move);
Shouldn't you here wait too?

[jonross, 2014-09-05 16:31:49]

https://codereview.chromium.org/515573002/diff/120001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/515573002/diff/120001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:60:
SetDrawBackgroundAsActive(controller->IsSelecting());
On 2014/09/04 19:58:35, Mr4D wrote:
> Shouldn't this command also be dependent on the commandline option?

This I added because currently the overview button does not paint an active
state while overview mode is on. Meanwhile the other trays do paint as active,
regardless of the touch feedback experiment.

However I could make this dependent on the command line.

https://codereview.chromium.org/515573002/diff/120001/ash/system/tray/tray_ba...
File ash/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/515573002/diff/120001/ash/system/tray/tray_ba...
ash/system/tray/tray_background_view.cc:439:
HasSwitch(switches::kAshEnableTouchViewTouchFeedback)) {
On 2014/09/04 19:58:35, Mr4D wrote:
> CommandLine::ForCurrentProcess()->
>     HasSwitch(switches::kAshEnableTouchViewTouchFeedback)
> 
> is being used twice here. Since it is in both conditions you might want to
check
> this only once like
> 
> if (commandline..) {
>    if (..) {}
>    else if {}
> }
> 
> Furthermore - this is a fixed value throughout the session and touch functions
> can get called a lot, so you might consider some pre-parsing

Done.

https://codereview.chromium.org/515573002/diff/120001/ash/system/web_notifica...
File ash/system/web_notification/web_notification_tray_unittest.cc (right):

https://codereview.chromium.org/515573002/diff/120001/ash/system/web_notifica...
ash/system/web_notification/web_notification_tray_unittest.cc:537:
generator.Dispatch(&move);
On 2014/09/04 19:58:35, Mr4D wrote:
> Shouldn't you here wait too?

Done.

[Mr4D (OOO till 08-26), 2014-09-05 16:38:58]

lgtm

https://codereview.chromium.org/515573002/diff/120001/ash/system/overview/ove...
File ash/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/515573002/diff/120001/ash/system/overview/ove...
ash/system/overview/overview_button_tray.cc:60:
SetDrawBackgroundAsActive(controller->IsSelecting());
I think that should be fine.

[jonross, 2014-09-05 16:41:55]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-09-05 16:43:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jonross@chromium.org/515573002/140001

[commit-bot: I haz the power, 2014-09-05 18:16:53]

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

[commit-bot: I haz the power, 2014-09-05 18:16:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jonross, 2014-09-05 19:54:03]

jonross@chromium.org changed reviewers:
+ asvitkine@chromium.org

[jonross, 2014-09-05 19:54:04]

Hi Alexei,

I am adding a new startup flag for testing touch feedback in ChromeOS.

Could you provide an owner review to tools/metrics/histograms/histograms.xml
which I have updated in order for AboutFlagsHistoframTest to pass.

[Alexei Svitkine (slow), 2014-09-05 19:57:32]

lgtm

[jonross, 2014-09-05 21:06:57]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-09-05 21:08:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jonross@chromium.org/515573002/160001

[commit-bot: I haz the power, 2014-09-06 03:10:34]

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

[commit-bot: I haz the power, 2014-09-06 03:10:35]

Exceeded time limit waiting for builds to trigger.

[jonross, 2014-09-06 12:07:19]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-09-06 12:08:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jonross@chromium.org/515573002/160001

[commit-bot: I haz the power, 2014-09-06 12:38:01]

Committed patchset #9 (id:160001) as d333dbc26a2566cfa3c82972a24047e828e9d09b

[commit-bot: I haz the power, 2014-09-10 03:42:19]

Patchset 9 (id:??) landed as
https://crrev.com/367a1bee843f70e3cf6e3ea439e6b1711d80f059
Cr-Commit-Position: refs/heads/master@{#293608}