Add ink drop ripple to overview mode button

Add ink drop ripple to overview mode button

TrayBackgroundView is updated to create appropriate ink drop ripple and
highlight instances when needed. This needed a way for ActionableView to
let its subclasses (i.e. TrayBackgroundView here) have control on ripple
state based on PerformAction() result.

Any subclass of TrayBackgroundView that needs ink drop can just enable
ink drop mode and everything should work in most cases.

BUG=612579
TEST=manually run TouchView Ash with --ash-md=experimental and try
     ripples on overview mode button.

Committed: https://crrev.com/4614a8ceb7ce5ba9eb6eedeb5c4683768c2b8b11
Cr-Commit-Position: refs/heads/master@{#431885}

[mohsen, 2016-11-10 23:16:48]

Description was changed from

==========
Add ink drop ripple to overview mode button

BUG=612579
TEST=manually run Ash with --ash-debug-shortcuts and use
     Ctrl+Alt+Shift+T to enable touch view mode. Then try the ripple on
     overview button.
==========

to

==========
Add ink drop ripple to overview mode button

BUG=612579
TEST=manually run TouchView Ash with --ash-md=experimental and try
     ripples on overview mode button.
==========

[mohsen, 2016-11-10 23:16:57]

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

[commit-bot: I haz the power, 2016-11-10 23:17:30]

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

[mohsen, 2016-11-10 23:21:53]

mohsen@chromium.org changed reviewers:
+ bruthig@chromium.org

[mohsen, 2016-11-10 23:21:54]

Please take a look...

[commit-bot: I haz the power, 2016-11-11 00:24:08]

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

[commit-bot: I haz the power, 2016-11-11 00:24:09]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-11-11 04:41:23]

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

[commit-bot: I haz the power, 2016-11-11 04:41:38]

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, 2016-11-11 05:35:03]

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

[commit-bot: I haz the power, 2016-11-11 05:35:04]

Dry run: This issue passed the CQ dry run.

[bruthig, 2016-11-11 17:30:40]

lgtm with a couple of nits and a request for a test.  Feel free to add other
OWNERs.

https://codereview.chromium.org/2499453002/diff/20001/ash/common/system/overv...
File ash/common/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/2499453002/diff/20001/ash/common/system/overv...
ash/common/system/overview/overview_button_tray.cc:76:
controller->ToggleOverview();
nit: Consider changing ToggleOverview() to return a bool to let the caller know
if a Toggle happened.

https://codereview.chromium.org/2499453002/diff/20001/ash/common/system/overv...
ash/common/system/overview/overview_button_tray.cc:78: return was_selecting !=
controller->IsSelecting();
nit: it would be useful to have a comment here as to when this can be false.

https://codereview.chromium.org/2499453002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/actionable_view.h (right):

https://codereview.chromium.org/2499453002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/actionable_view.h:58: // can override to change the
default behavior.
nit: Can you also note that this will not be called if this was destroyed.

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

https://codereview.chromium.org/2499453002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:274:
set_ink_drop_base_color(SK_ColorWHITE);
Can we move this color to a constant in tray_constants?

https://codereview.chromium.org/2499453002/diff/20001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.cc (right):

https://codereview.chromium.org/2499453002/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:711: if (!ink_drop_ripple_)
Can you add an InkDropImpl specific test for this?
Check out InkDropImplHideAutoHighlightTest.NoCrashDuringRippleTearDown.  You
will probably want to use the ZERO duration animation setting to force this use
case.

[mohsen, 2016-11-11 22:37:23]

mohsen@chromium.org changed reviewers:
+ tdanderson@chromium.org

[mohsen, 2016-11-11 22:37:24]

tdanderson@: Can you please take a look...

https://codereview.chromium.org/2499453002/diff/20001/ash/common/system/overv...
File ash/common/system/overview/overview_button_tray.cc (right):

https://codereview.chromium.org/2499453002/diff/20001/ash/common/system/overv...
ash/common/system/overview/overview_button_tray.cc:76:
controller->ToggleOverview();
On 2016/11/11 at 17:30:40, bruthig wrote:
> nit: Consider changing ToggleOverview() to return a bool to let the caller
know if a Toggle happened.

Done.

https://codereview.chromium.org/2499453002/diff/20001/ash/common/system/overv...
ash/common/system/overview/overview_button_tray.cc:78: return was_selecting !=
controller->IsSelecting();
On 2016/11/11 at 17:30:40, bruthig wrote:
> nit: it would be useful to have a comment here as to when this can be false.

Done.

https://codereview.chromium.org/2499453002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/actionable_view.h (right):

https://codereview.chromium.org/2499453002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/actionable_view.h:58: // can override to change the
default behavior.
On 2016/11/11 at 17:30:40, bruthig wrote:
> nit: Can you also note that this will not be called if this was destroyed.

Done.

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

https://codereview.chromium.org/2499453002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:274:
set_ink_drop_base_color(SK_ColorWHITE);
On 2016/11/11 at 17:30:40, bruthig wrote:
> Can we move this color to a constant in tray_constants?

Actually, there is a constant for this color! Done.

https://codereview.chromium.org/2499453002/diff/20001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.cc (right):

https://codereview.chromium.org/2499453002/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:711: if (!ink_drop_ripple_)
On 2016/11/11 at 17:30:40, bruthig wrote:
> Can you add an InkDropImpl specific test for this?
> Check out InkDropImplHideAutoHighlightTest.NoCrashDuringRippleTearDown.  You
will probably want to use the ZERO duration animation setting to force this use
case.

Done.

[mohsen, 2016-11-11 22:53:37]

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

[commit-bot: I haz the power, 2016-11-11 22:54:02]

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

[mohsen, 2016-11-11 23:07:53]

mohsen@chromium.org changed reviewers:
+ skuhne@chromium.org, varkha@chromium.org

[mohsen, 2016-11-11 23:07:53]

+varkha@ for OWNERS in /ash/common/wm/ and /ash/wm/
+skuhne@ for OWNERS in /ash/system/ and /chrome/browser/ui/ash/multi_user/

[commit-bot: I haz the power, 2016-11-12 00:10:48]

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

[commit-bot: I haz the power, 2016-11-12 00:10:49]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2016-11-12 02:18:58]

ash/common/system LGTM with nits.

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right):

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:7: #include
"ash/common/accessibility_delegate.h"
nit: can you please include a CL description that gives a brief overview of the
changes made in the CL? It is helpful as an introduction for those reading the
code so they know what to look out for.

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/tray/...
File ash/common/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:187: // is active.
Can you please add to this comment to explain when/why you would want to not
draw an active background when the view is active? I find it a bit hard to
reason what the true/false arguments actually indicate at the call sites of
SetContentsBackground().

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:425: if (ink_drop_mode() ==
InkDropMode::OFF) {
double-checking: you mean for this to be == and not != ?

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/tray/...
File ash/common/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.h:115: // determines if the view
needs its active background drawn.
nit: I think a clearer wording would be "|draws_active| determines if the view's
background should be drawn as active when the view is in the active state" or
similar

https://codereview.chromium.org/2499453002/diff/40001/ash/common/wm/overview/...
File ash/common/wm/overview/window_selector_controller.h (right):

https://codereview.chromium.org/2499453002/diff/40001/ash/common/wm/overview/...
ash/common/wm/overview/window_selector_controller.h:35: // or touch rather than
keypresses. Returns true if the toggle was successful.
I think this documentation (which seems a bit out of date) can be trimmed down
to something like:

"Attempts to toggle overview mode and returns true if successful (the toggle
would be unsuccessful if there are no windows to show)."

[mohsen, 2016-11-13 04:51:44]

Description was changed from

==========
Add ink drop ripple to overview mode button

BUG=612579
TEST=manually run TouchView Ash with --ash-md=experimental and try
     ripples on overview mode button.
==========

to

==========
Add ink drop ripple to overview mode button

TrayBackgroundView is updated to create appropriate ink drop ripple and
highlight instances when needed. This needed a way for ActionableView to
let its subclasses (i.e. TrayBackgroundView here) have control on ripple
state based on PerformAction() result.

Any subclass of TrayBackgroundView that needs ink drop can just enable
ink drop mode and everything should work in most cases.

BUG=612579
TEST=manually run TouchView Ash with --ash-md=experimental and try
     ripples on overview mode button.
==========

[mohsen, 2016-11-13 05:10:04]

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right):

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:7: #include
"ash/common/accessibility_delegate.h"
On 2016/11/12 at 02:18:58, tdanderson wrote:
> nit: can you please include a CL description that gives a brief overview of
the changes made in the CL? It is helpful as an introduction for those reading
the code so they know what to look out for.

Done.

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/tray/...
File ash/common/system/tray/tray_background_view.cc (right):

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:187: // is active.
On 2016/11/12 at 02:18:58, tdanderson wrote:
> Can you please add to this comment to explain when/why you would want to not
draw an active background when the view is active? I find it a bit hard to
reason what the true/false arguments actually indicate at the call sites of
SetContentsBackground().

Done.

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:425: if (ink_drop_mode() ==
InkDropMode::OFF) {
On 2016/11/12 at 02:18:58, tdanderson wrote:
> double-checking: you mean for this to be == and not != ?

Yes, This is for the non-MD code to show "touch feedback" on the view. Not used
when there is an ink drop. Added a comment and TODO to remove after removing
non-MD code.

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/tray/...
File ash/common/system/tray/tray_background_view.h (right):

https://codereview.chromium.org/2499453002/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.h:115: // determines if the view
needs its active background drawn.
On 2016/11/12 at 02:18:58, tdanderson wrote:
> nit: I think a clearer wording would be "|draws_active| determines if the
view's background should be drawn as active when the view is in the active
state" or similar

Done.

https://codereview.chromium.org/2499453002/diff/40001/ash/common/wm/overview/...
File ash/common/wm/overview/window_selector_controller.h (right):

https://codereview.chromium.org/2499453002/diff/40001/ash/common/wm/overview/...
ash/common/wm/overview/window_selector_controller.h:35: // or touch rather than
keypresses. Returns true if the toggle was successful.
On 2016/11/12 at 02:18:58, tdanderson wrote:
> I think this documentation (which seems a bit out of date) can be trimmed down
to something like:
> 
> "Attempts to toggle overview mode and returns true if successful (the toggle
would be unsuccessful if there are no windows to show)."

Right. Done.

[mohsen, 2016-11-13 23:37:39]

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

[commit-bot: I haz the power, 2016-11-13 23:37:45]

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, 2016-11-14 00:26:38]

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

[commit-bot: I haz the power, 2016-11-14 00:26:39]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-11-14 04:01:51]

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

[commit-bot: I haz the power, 2016-11-14 04:02:05]

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, 2016-11-14 04:51:54]

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

[commit-bot: I haz the power, 2016-11-14 04:51:56]

Dry run: This issue passed the CQ dry run.

[Mr4D (OOO till 08-26), 2016-11-14 15:18:30]

c/b/u/a/multi_user lgtm

[Mr4D (OOO till 08-26), 2016-11-14 15:21:31]

Oh - also lgtm for the maximize_mode changes.

[varkha, 2016-11-14 17:54:29]

ash/common/wm/ LGTM.

[mohsen, 2016-11-14 18:20:38]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-11-14 18:20:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
bruthig@chromium.org, tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2499453002/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2016-11-14 18:21:21]

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-11-14 19:08:53]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-14 19:34:52]

Description was changed from

==========
Add ink drop ripple to overview mode button

TrayBackgroundView is updated to create appropriate ink drop ripple and
highlight instances when needed. This needed a way for ActionableView to
let its subclasses (i.e. TrayBackgroundView here) have control on ripple
state based on PerformAction() result.

Any subclass of TrayBackgroundView that needs ink drop can just enable
ink drop mode and everything should work in most cases.

BUG=612579
TEST=manually run TouchView Ash with --ash-md=experimental and try
     ripples on overview mode button.
==========

to

==========
Add ink drop ripple to overview mode button

TrayBackgroundView is updated to create appropriate ink drop ripple and
highlight instances when needed. This needed a way for ActionableView to
let its subclasses (i.e. TrayBackgroundView here) have control on ripple
state based on PerformAction() result.

Any subclass of TrayBackgroundView that needs ink drop can just enable
ink drop mode and everything should work in most cases.

BUG=612579
TEST=manually run TouchView Ash with --ash-md=experimental and try
     ripples on overview mode button.

Committed: https://crrev.com/4614a8ceb7ce5ba9eb6eedeb5c4683768c2b8b11
Cr-Commit-Position: refs/heads/master@{#431885}
==========

[commit-bot: I haz the power, 2016-11-14 19:34:54]

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