Add ink drop masking to TrayBackgroundView

Add ink drop masking to TrayBackgroundView

A virtual CreateInkDropMask() is added to InkDropHostView that
subclasses can override and provide their choice of mask for the ink
drop. TrayBackgroundView has implemented this to create a rounded
rectangle mask.

A few masks types are declared in ui/views/animation/ink_drop_mask.h
file.

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

Committed: https://crrev.com/932af7361a3588b36a1f15aa76b6199a4a66f646
Cr-Commit-Position: refs/heads/master@{#431972}

[mohsen, 2016-11-11 06:00:55]

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

[commit-bot: I haz the power, 2016-11-11 06:01:08]

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 06:01:11]

Description was changed from

==========
Added ink drop masking to TrayBackgroundView

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

to

==========
Add ink drop masking to TrayBackgroundView

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

[commit-bot: I haz the power, 2016-11-11 07:27:06]

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

[commit-bot: I haz the power, 2016-11-11 07:27:06]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-11-11 18:33:18]

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

[mohsen, 2016-11-11 18:33:18]

Please take a look... (This patch depends on https://crrev.com/2499453002)

[bruthig, 2016-11-11 20:16:43]

Left some comments.

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

https://codereview.chromium.org/2499523003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:382:
ink_drop_layer->SetBounds(local_bounds);
Capturing offline discussion:
Let's move the ownership of setting the bounds on the ink drop root layer into
the InkDropImpl. This will reduce the burden on AddInkDropLayer() overrides to
do the right thing.

We will have to add an InkDrop::BoundsUpdated() method so the host view can
notify the ink drop instance of the new bounds/size.

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

https://codereview.chromium.org/2499523003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.h:185:
std::unique_ptr<views::RoundRectInkDropMask> ink_drop_mask_;
Capturing offline discussion:
Let's move the ownership of the mask up to InkDropHostView and add an
override-able InkDropHost::CreateInkDropMask() method. If any subclass needs to
reference the mask by the correct type we can add an override-able
InkDropHost::OnInkDropMaskDestroyed() and allow the subclass to keep a reference
to the mask it returns from the Create function.

https://codereview.chromium.org/2499523003/diff/1/ui/views/animation/ink_drop...
File ui/views/animation/ink_drop_mask.h (right):

https://codereview.chromium.org/2499523003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_mask.h:8: #include "base/callback_forward.h"
Is this needed?

https://codereview.chromium.org/2499523003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_mask.h:30: void OnDelegatedFrameDamage(const
gfx::Rect& damage_rect_in_dip) override;
nit: Add '// ui::LayerDelegate:' or '// Overridden from ui::LayerDelegate:'

I think the later is the preferred syntax now.

https://codereview.chromium.org/2499523003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_mask.h:43: const gfx::Rect& bounds,
nit: |mask_bounds| would be more descriptive than just |bounds|.

https://codereview.chromium.org/2499523003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_mask.h:60: const gfx::Point& center,
nit: |mask_center|/|mask_radius| would be more descriptive than just
|center|/|radius|.

[mohsen, 2016-11-11 22:41:03]

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:41:46]

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 22:46:11]

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

[commit-bot: I haz the power, 2016-11-11 22:46:12]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-11-13 04:43:32]

Description was changed from

==========
Add ink drop masking to TrayBackgroundView

BUG=612573
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 04:52:09]

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 masking to TrayBackgroundView

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

[mohsen, 2016-11-13 23:22:44]

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

[mohsen, 2016-11-13 23:22:55]

The CQ bit was unchecked by mohsen@chromium.org

[commit-bot: I haz the power, 2016-11-13 23:22:55]

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-13 23:23:39]

The CQ bit was unchecked by mohsen@chromium.org

[mohsen, 2016-11-13 23:24:17]

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:24:26]

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-13 23:27:13]

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

[commit-bot: I haz the power, 2016-11-13 23:27:14]

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_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mohsen, 2016-11-13 23:31:29]

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

https://codereview.chromium.org/2499523003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:382:
ink_drop_layer->SetBounds(local_bounds);
On 2016/11/11 at 20:16:43, bruthig wrote:
> Capturing offline discussion:
> Let's move the ownership of setting the bounds on the ink drop root layer into
the InkDropImpl. This will reduce the burden on AddInkDropLayer() overrides to
do the right thing.
> 
> We will have to add an InkDrop::BoundsUpdated() method so the host view can
notify the ink drop instance of the new bounds/size.

Done.

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

https://codereview.chromium.org/2499523003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.h:185:
std::unique_ptr<views::RoundRectInkDropMask> ink_drop_mask_;
On 2016/11/11 at 20:16:43, bruthig wrote:
> Capturing offline discussion:
> Let's move the ownership of the mask up to InkDropHostView and add an
override-able InkDropHost::CreateInkDropMask() method. If any subclass needs to
reference the mask by the correct type we can add an override-able
InkDropHost::OnInkDropMaskDestroyed() and allow the subclass to keep a reference
to the mask it returns from the Create function.

Done.

https://codereview.chromium.org/2499523003/diff/1/ui/views/animation/ink_drop...
File ui/views/animation/ink_drop_mask.h (right):

https://codereview.chromium.org/2499523003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_mask.h:8: #include "base/callback_forward.h"
On 2016/11/11 at 20:16:43, bruthig wrote:
> Is this needed?

Not sure what it's doing here! Removed.

https://codereview.chromium.org/2499523003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_mask.h:30: void OnDelegatedFrameDamage(const
gfx::Rect& damage_rect_in_dip) override;
On 2016/11/11 at 20:16:43, bruthig wrote:
> nit: Add '// ui::LayerDelegate:' or '// Overridden from ui::LayerDelegate:'
> 
> I think the later is the preferred syntax now.

Done.

https://codereview.chromium.org/2499523003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_mask.h:43: const gfx::Rect& bounds,
On 2016/11/11 at 20:16:43, bruthig wrote:
> nit: |mask_bounds| would be more descriptive than just |bounds|.

Done.

https://codereview.chromium.org/2499523003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_mask.h:60: const gfx::Point& center,
On 2016/11/11 at 20:16:43, bruthig wrote:
> nit: |mask_center|/|mask_radius| would be more descriptive than just
|center|/|radius|.

Done.

[mohsen, 2016-11-14 03:48: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 03:49:04]

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 03:51:13]

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

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

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mohsen, 2016-11-14 03:55:22]

Description was changed from

==========
Add ink drop masking to TrayBackgroundView

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

to

==========
Add ink drop masking to TrayBackgroundView

A virtual CreateInkDropMask() is added to InkDropHostView that
subclasses can override and provide their choice of mask for the ink
drop. TrayBackgroundView has implemented this to create a rounded
rectangle mask.

A few masks types are declared in ui/views/animation/ink_drop_mask.h
file.

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

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

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:01:14]

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 06:03:59]

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

[commit-bot: I haz the power, 2016-11-14 06:04:00]

Dry run: This issue passed the CQ dry run.

[bruthig, 2016-11-14 19:09:38]

lgtm with nits.  Feel free to add other OWNERs.

https://codereview.chromium.org/2499523003/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop.h (right):

https://codereview.chromium.org/2499523003/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop.h:28: virtual void UpdateLayerSize(const gfx::Size&
layer_size) = 0;
I think this would be better named as "HostSizeChanged()".  Client's shouldn't
need to be aware about the underlying Layer or it's size. And the docs should
tell clients when it needs to be called.

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

https://codereview.chromium.org/2499523003/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:626:
root_layer_->SetBounds(gfx::Rect(layer_size));
nit: A comment here saying that "we are updating |root_layer_|'s size because it
affects the clipping when a mask is applied" would be super helpful. It might be
worth mentioning that it only affects clipping when the masking layer is not
null.

https://codereview.chromium.org/2499523003/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.h (right):

https://codereview.chromium.org/2499523003/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.h:53: InkDropImpl(InkDropHost* ink_drop_host,
const gfx::Size& layer_size);
nit: Rename |layer_size| as |host_size|.  Clients don't need to know what the
size is being used for.

[mohsen, 2016-11-14 20:11:33]

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

[mohsen, 2016-11-14 20:12:21]

Patchset #5 (id:80001) has been deleted

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

Patchset #5 (id:100001) has been deleted

[mohsen, 2016-11-14 20:16:19]

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

[commit-bot: I haz the power, 2016-11-14 20:17:19]

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-14 20:19:38]

mohsen@chromium.org changed reviewers:
+ sadrul@chromium.org, skuhne@chromium.org, tdanderson@chromium.org

[mohsen, 2016-11-14 20:19:39]

+tdanderson@ for changes in /ash/common/system/tray/
+skuhne@ for changes in /ash/shelf/
+sadrul@ for changes in /ui/views/BUILD.gn and /ui/views/controls/button/

https://codereview.chromium.org/2499523003/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop.h (right):

https://codereview.chromium.org/2499523003/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop.h:28: virtual void UpdateLayerSize(const gfx::Size&
layer_size) = 0;
On 2016/11/14 at 19:09:38, bruthig wrote:
> I think this would be better named as "HostSizeChanged()".  Client's shouldn't
need to be aware about the underlying Layer or it's size. And the docs should
tell clients when it needs to be called.

Sure. Done.

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

https://codereview.chromium.org/2499523003/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.cc:626:
root_layer_->SetBounds(gfx::Rect(layer_size));
On 2016/11/14 at 19:09:38, bruthig wrote:
> nit: A comment here saying that "we are updating |root_layer_|'s size because
it affects the clipping when a mask is applied" would be super helpful. It might
be worth mentioning that it only affects clipping when the masking layer is not
null.

Done.

https://codereview.chromium.org/2499523003/diff/60001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_impl.h (right):

https://codereview.chromium.org/2499523003/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_impl.h:53: InkDropImpl(InkDropHost* ink_drop_host,
const gfx::Size& layer_size);
On 2016/11/14 at 19:09:38, bruthig wrote:
> nit: Rename |layer_size| as |host_size|.  Clients don't need to know what the
size is being used for.

Done.

[tdanderson, 2016-11-14 20:21:35]

ash/common/system LGTM

[bruthig, 2016-11-14 21:07:41]

https://codereview.chromium.org/2499523003/diff/120001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2499523003/diff/120001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.cc:222:
ink_drop_->HostSizeChanged(size());
Should we be re-creating/applying the mask here too, or maybe in an
InkDropHostView::OnMaskChanged() event that clients/subclasses would have to
call?

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

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

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

Dry run: This issue passed the CQ dry run.

[bruthig, 2016-11-14 21:45:42]

https://codereview.chromium.org/2499523003/diff/120001/ui/views/animation/ink...
File ui/views/animation/ink_drop_host_view.cc (right):

https://codereview.chromium.org/2499523003/diff/120001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.cc:222:
ink_drop_->HostSizeChanged(size());
On 2016/11/14 21:07:41, bruthig wrote:
> Should we be re-creating/applying the mask here too, or maybe in an
> InkDropHostView::OnMaskChanged() event that clients/subclasses would have to
> call?

Discussed offline, my request is not needed until we are supporting
InkDropHostView's that are dynamically resized.

[sadrul, 2016-11-14 22:11:37]

stamp lgtm

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

shelf_view_unittest lgtm

[mohsen, 2016-11-14 23:22:06]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-11-14 23:22:07]

The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org
Link to the patchset: https://codereview.chromium.org/2499523003/#ps120001
(title: "Addressed review comments")

[commit-bot: I haz the power, 2016-11-14 23:24:20]

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 23:36:52]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-11-14 23:43:42]

Description was changed from

==========
Add ink drop masking to TrayBackgroundView

A virtual CreateInkDropMask() is added to InkDropHostView that
subclasses can override and provide their choice of mask for the ink
drop. TrayBackgroundView has implemented this to create a rounded
rectangle mask.

A few masks types are declared in ui/views/animation/ink_drop_mask.h
file.

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

to

==========
Add ink drop masking to TrayBackgroundView

A virtual CreateInkDropMask() is added to InkDropHostView that
subclasses can override and provide their choice of mask for the ink
drop. TrayBackgroundView has implemented this to create a rounded
rectangle mask.

A few masks types are declared in ui/views/animation/ink_drop_mask.h
file.

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

Committed: https://crrev.com/932af7361a3588b36a1f15aa76b6199a4a66f646
Cr-Commit-Position: refs/heads/master@{#431972}
==========

[commit-bot: I haz the power, 2016-11-14 23:43:43]

Patchset 5 (id:??) landed as
https://crrev.com/932af7361a3588b36a1f15aa76b6199a4a66f646
Cr-Commit-Position: refs/heads/master@{#431972}