Add ink drop ripple to status tray

Add ink drop ripple to status tray

This is still not handling view resizes that happen when an ink drop is
active (e.g., in status tray). A workaround is implemented, but proper
fix would need a follow-up CL.

This CL also changes round rect mask to take insets instead of bounds so
that there would be no need to update mask bounds when size of host view
is updated.

BUG=612544
TEST=manual

Committed: https://crrev.com/c109436f07a06f711d50c982e2284f1460abd8a9
Cr-Commit-Position: refs/heads/master@{#433113}

[mohsen, 2016-11-17 05:23:11]

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

[commit-bot: I haz the power, 2016-11-17 05:23: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-17 05:27:33]

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

[mohsen, 2016-11-17 05:27:33]

+bruthig@ for changes in /ui/views/animations/.
+tdanderson@ for changes in /ash/common/system/.

PTAL. This CL depends on https://crrev.com/2508063004.

[bruthig, 2016-11-17 06:31:21]

See comments inline.

https://codereview.chromium.org/2510603003/diff/1/ash/common/system/tray/syst...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2510603003/diff/1/ash/common/system/tray/syst...
ash/common/system/tray/system_tray.cc:872: gfx::Insets
SystemTray::GetBackgroundInsets() const {
Double check with Sebastien but I imagine we want the ink drop to be behind the
avatar.  I think you would be able to just make TrayUser::avatar_ paint to it's
own layer.  This would also solve the rounded corners on the ink drop on the
right side too.

https://codereview.chromium.org/2510603003/diff/1/ash/common/system/tray/syst...
File ash/common/system/tray/system_tray.h (right):

https://codereview.chromium.org/2510603003/diff/1/ash/common/system/tray/syst...
ash/common/system/tray/system_tray.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Where is the ink drop placed in RTL mode?  Does that apply to all UI elements
that have an ink drop and center it on the click point?

https://codereview.chromium.org/2510603003/diff/1/ash/common/system/tray/syst...
ash/common/system/tray/system_tray.h:271: TrayUser* tray_user_active_;
Consider renaming as |active_tray_user_|.

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

https://codereview.chromium.org/2510603003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:592: gfx::Rect
TrayBackgroundView::GetBackgroundBounds() const {
GetBackgroundBounds() doesn't seem accurate here.  Should this be named
something more like GetInkDropBounds()?

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

https://codereview.chromium.org/2510603003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_mask.h:20: class VIEWS_EXPORT InkDropMask : public
ui::LayerDelegate {
Somewhere we should document that the mask size should be the same as the masked
layer's size.  This might be a good spot for that.

https://codereview.chromium.org/2510603003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_mask.h:24: void UpdateLayerSize(const gfx::Size&
new_layer_size);
A comment here saying why/when this should be called would be helpful.

[commit-bot: I haz the power, 2016-11-17 08:25:44]

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

[commit-bot: I haz the power, 2016-11-17 08:25:44]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-11-17 19:52:30]

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

[commit-bot: I haz the power, 2016-11-17 19:53:32]

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-17 19:58:41]

Patchset #2 (id:20001) has been deleted

[mohsen, 2016-11-17 20:00:03]

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

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

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-17 20:04:25]

Description was changed from

==========
Add ink drop ripple to status tray

This is the first version of ink drop ripple for status tray. There are
still a few issues that would be fixed in follow-up CLs:
 - Status tray can resize when a ripple is active on it. This is no
   handled properly here. Instead, a workaround is implemented that
   would work most of the times;
 - Ink drop is not placed correctly in RTL mode;
 - When an avatar is present, the right edge of the ink drop should not
   have round corners.

BUG=612544
TEST=manual
==========

to

==========
Add ink drop ripple to status tray

This is still not handling view resizes that happen when an ink drop is
active (e.g. in status tray). A workaround is implemented, but proper
fix would need a follow-up CL.

BUG=612544
TEST=manual
==========

[mohsen, 2016-11-17 20:09:21]

Description was changed from

==========
Add ink drop ripple to status tray

This is still not handling view resizes that happen when an ink drop is
active (e.g. in status tray). A workaround is implemented, but proper
fix would need a follow-up CL.

BUG=612544
TEST=manual
==========

to

==========
Add ink drop ripple to status tray

This is still not handling view resizes that happen when an ink drop is
active (e.g., in status tray). A workaround is implemented, but proper
fix would need a follow-up CL.

This CL also changes round rect mask to take insets instead of bounds so
that there would be no need to update mask bounds when size of host view
is updated.

BUG=612544
TEST=manual
==========

[mohsen, 2016-11-17 20:10:13]

bruthig@: please take another look...

https://codereview.chromium.org/2510603003/diff/1/ash/common/system/tray/syst...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2510603003/diff/1/ash/common/system/tray/syst...
ash/common/system/tray/system_tray.cc:872: gfx::Insets
SystemTray::GetBackgroundInsets() const {
On 2016/11/17 at 06:31:20, bruthig wrote:
> Double check with Sebastien but I imagine we want the ink drop to be behind
the avatar.  I think you would be able to just make TrayUser::avatar_ paint to
it's own layer.  This would also solve the rounded corners on the ink drop on
the right side too.

Sure. Done.

https://codereview.chromium.org/2510603003/diff/1/ash/common/system/tray/syst...
File ash/common/system/tray/system_tray.h (right):

https://codereview.chromium.org/2510603003/diff/1/ash/common/system/tray/syst...
ash/common/system/tray/system_tray.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2016/11/17 at 06:31:20, bruthig wrote:
> Where is the ink drop placed in RTL mode?  Does that apply to all UI elements
that have an ink drop and center it on the click point?

It's not related to the click point. It's related to different left and right
insets we might have for the ink drop. BTW, the issue is now fixed.

https://codereview.chromium.org/2510603003/diff/1/ash/common/system/tray/syst...
ash/common/system/tray/system_tray.h:271: TrayUser* tray_user_active_;
On 2016/11/17 at 06:31:20, bruthig wrote:
> Consider renaming as |active_tray_user_|.

Not needed anymore. Removed.

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

https://codereview.chromium.org/2510603003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:592: gfx::Rect
TrayBackgroundView::GetBackgroundBounds() const {
On 2016/11/17 at 06:31:20, bruthig wrote:
> GetBackgroundBounds() doesn't seem accurate here.  Should this be named
something more like GetInkDropBounds()?

With the new changes maybe it is more accurate now?

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

https://codereview.chromium.org/2510603003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_mask.h:20: class VIEWS_EXPORT InkDropMask : public
ui::LayerDelegate {
On 2016/11/17 at 06:31:21, bruthig wrote:
> Somewhere we should document that the mask size should be the same as the
masked layer's size.  This might be a good spot for that.

Done.

https://codereview.chromium.org/2510603003/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_mask.h:24: void UpdateLayerSize(const gfx::Size&
new_layer_size);
On 2016/11/17 at 06:31:21, bruthig wrote:
> A comment here saying why/when this should be called would be helpful.

Done.

[commit-bot: I haz the power, 2016-11-17 20:36:42]

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

[commit-bot: I haz the power, 2016-11-17 20:36:43]

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

[bruthig, 2016-11-17 21:48:02]

lgtm with nits

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

https://codereview.chromium.org/2510603003/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.h:167: gfx::Rect
GetBackgroundBounds() const;
With the name 'GetBackgroundBounds()' I would expect this to be used by
PaintMaterial() and for the creation of the InkDropMask, but
'GetBackgroundBounds()' is adding some extra padding to work around dynamically
resizing hosts and shouldn't be used in these places. So I still think this
would be better named as GetInkDropBounds()

[mohsen, 2016-11-17 23:02:41]

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

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

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-17 23:08:11]

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

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

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

[tdanderson, 2016-11-17 23:55:22]

LGTM with nits

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

https://codereview.chromium.org/2510603003/diff/80001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:61: // Switches left and right
insets if RTL mode is active.
Thank you for making sure this works well in RTL!

https://codereview.chromium.org/2510603003/diff/80001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:604: // coordinates.
Please adjust whitespace as discussed.

How about "|insets| are relative to contents bounds. Change them to be relative
to local bounds."

https://codereview.chromium.org/2510603003/diff/80001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:621: //
https://crbug.com/666175).
Thanks for the informative comment, TODO, and bug.

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

https://codereview.chromium.org/2510603003/diff/80001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.h:162: // Helper function that
returns background insets relative to local bounds.
nit: perhaps "calculates" instead of "returns" ?

https://codereview.chromium.org/2510603003/diff/80001/ash/common/system/tray/...
File ash/common/system/tray/tray_popup_utils.h (right):

https://codereview.chromium.org/2510603003/diff/80001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.h:165: // Returns the effective ink drop
bounds for |host| according to the
nit: this comment references "bounds" but the function returns gfx::Insets. Can
you please update the comment accordingly?

[mohsen, 2016-11-18 00:03:10]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-11-18 00:03:10]

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/2510603003/#ps90001
(title: "Addressed review comments")

[commit-bot: I haz the power, 2016-11-18 00:05:55]

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-18 04:46:17]

Committed patchset #5 (id:90001)

[commit-bot: I haz the power, 2016-11-18 04:48:27]

Description was changed from

==========
Add ink drop ripple to status tray

This is still not handling view resizes that happen when an ink drop is
active (e.g., in status tray). A workaround is implemented, but proper
fix would need a follow-up CL.

This CL also changes round rect mask to take insets instead of bounds so
that there would be no need to update mask bounds when size of host view
is updated.

BUG=612544
TEST=manual
==========

to

==========
Add ink drop ripple to status tray

This is still not handling view resizes that happen when an ink drop is
active (e.g., in status tray). A workaround is implemented, but proper
fix would need a follow-up CL.

This CL also changes round rect mask to take insets instead of bounds so
that there would be no need to update mask bounds when size of host view
is updated.

BUG=612544
TEST=manual

Committed: https://crrev.com/c109436f07a06f711d50c982e2284f1460abd8a9
Cr-Commit-Position: refs/heads/master@{#433113}
==========

[commit-bot: I haz the power, 2016-11-18 04:48:28]

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

[mohsen, 2016-11-18 05:20:56]

Forgot to send my response to comments!

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

https://codereview.chromium.org/2510603003/diff/40001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.h:167: gfx::Rect
GetBackgroundBounds() const;
On 2016/11/17 at 21:48:01, bruthig wrote:
> With the name 'GetBackgroundBounds()' I would expect this to be used by
PaintMaterial() and for the creation of the InkDropMask, but
'GetBackgroundBounds()' is adding some extra padding to work around dynamically
resizing hosts and shouldn't be used in these places. So I still think this
would be better named as GetInkDropBounds()

Right. Done.

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

https://codereview.chromium.org/2510603003/diff/80001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.cc:604: // coordinates.
On 2016/11/17 at 23:55:21, tdanderson wrote:
> Please adjust whitespace as discussed.
> 
> How about "|insets| are relative to contents bounds. Change them to be
relative to local bounds."

Done.

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

https://codereview.chromium.org/2510603003/diff/80001/ash/common/system/tray/...
ash/common/system/tray/tray_background_view.h:162: // Helper function that
returns background insets relative to local bounds.
On 2016/11/17 at 23:55:21, tdanderson wrote:
> nit: perhaps "calculates" instead of "returns" ?

Done.

https://codereview.chromium.org/2510603003/diff/80001/ash/common/system/tray/...
File ash/common/system/tray/tray_popup_utils.h (right):

https://codereview.chromium.org/2510603003/diff/80001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.h:165: // Returns the effective ink drop
bounds for |host| according to the
On 2016/11/17 at 23:55:22, tdanderson wrote:
> nit: this comment references "bounds" but the function returns gfx::Insets.
Can you please update the comment accordingly?

Oops! Forgot to fix the comment! Thanks. Done.