Handle view resize for ripple

Handle view resize for ripple

This change updates ink drop ripples (especially, flood fill ripple) to
handle host view's size change to update their layer size. The main
change is that the flood fill ripple gets insets instead of bounds to
make it relative to the host's bounds. If a ripple is in progress, it
will snap to its final size immediately.

Ink drop highlight is still using a workaround to handle resize and will
be fixed in a follow-up CL.

BUG=666175
TEST=manual

Committed: https://crrev.com/7dfd81cc1c15c4f3f9954c4180e24356395aed4c
Cr-Commit-Position: refs/heads/master@{#435002}

[mohsen, 2016-11-28 19:51:59]

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

[commit-bot: I haz the power, 2016-11-28 19:52:57]

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-28 19:56:35]

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

[mohsen, 2016-11-28 19:56:36]

Please take a look...

[commit-bot: I haz the power, 2016-11-28 20:26:12]

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

[commit-bot: I haz the power, 2016-11-28 20:26:12]

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

[mohsen, 2016-11-28 20:50:59]

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

[commit-bot: I haz the power, 2016-11-28 20:51:56]

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

[bruthig, 2016-11-28 22:13:46]

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

https://codereview.chromium.org/2533053002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:417: //
https://crbug.com/666175).
Consider spawning a new bug to track the highlight specific work especially if
we're going to defer it and not merge back to M56.  It should make it easier for
TPM to follow the existing bug with which will have a merge request.

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

https://codereview.chromium.org/2533053002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_popup_utils.cc:332: const gfx::Insets insets =
TrayPopupUtils::GetInkDropInsets(ink_drop_style);
nit: Can you tighten the scope for |insets|. i.e. add curly braces to the
INSET_BOUNDS/FILL_BOUNDS case so you can make |insets| a local var within that
scope. Or just call GetInkDropInsets() inline in the FloodFillInkDropRipple
ctor.

https://codereview.chromium.org/2533053002/diff/1/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/2533053002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:171: gfx::Rect
CalculateInkDropBounds(const gfx::Size& size) {
:( I kind of wish I had left myself a comment as to why the ink drop
ripple/highlight had to be sized differently than the host.... If you know why
can you please add a comment?

https://codereview.chromium.org/2533053002/diff/1/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/2533053002/diff/1/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_item_view.cc:523: size(),
gfx::Insets(), GetInkDropCenterBasedOnLastEvent(),
nit: Consider adding a FloodFillInkDropRipple ctor that doesn't accept the
|clip_bounds| param and uses a default value of gfx::Insets().

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
File ui/views/animation/flood_fill_ink_drop_ripple.cc (right):

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
ui/views/animation/flood_fill_ink_drop_ripple.cc:171:
painted_layer_.SetTransform(GetMaxSizeTargetTransform());
So will opacity animations continue here or do we need to snap them to the
correct values as well?

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
File ui/views/animation/flood_fill_ink_drop_ripple.h (right):

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
ui/views/animation/flood_fill_ink_drop_ripple.h:38: // rectangle of the given
size. The circle is clipped to the rectangles bounds.
This doc should be updated. i.e. 'size' is very ambiguous. Can you add some
thorough docs as to what |host_size| and |clip_insets| are, how they are used to
calculate the effective painted bounds and, why we use them i.e. to accommodate
dynamically resizes.

It might make sense to move these docs to the ctor, but I don't feel too
strongly about this.

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
File ui/views/animation/flood_fill_ink_drop_ripple_unittest.cc (right):

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
ui/views/animation/flood_fill_ink_drop_ripple_unittest.cc:23: // |clip_insets|.
nit: It doesn't really make sense for |clip_insets| to define a coordinate
space. Perhaps replace |clip_insets| with 'clip bounds' or 'effective bounds'.

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

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_ripple.h:57: // Called by ink drop hosts when their
size is changed.
nit: This comment is somewhat inaccurate.  This function is called by the
InkDropImpl not directly by hosts. Can you capture why this function exists and
why/when a subclass should override it.  Something along the lines of:

// Handles dynamic host resizes.  Subclasses can override to update an
// already visible ripple when the host is resized.

[commit-bot: I haz the power, 2016-11-28 22:49:01]

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

[commit-bot: I haz the power, 2016-11-28 22:49:01]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-11-29 00:02:33]

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

[commit-bot: I haz the power, 2016-11-29 00:04:44]

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-29 00:07:12]

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

https://codereview.chromium.org/2533053002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_background_view.cc:417: //
https://crbug.com/666175).
On 2016/11/28 at 22:13:45, bruthig wrote:
> Consider spawning a new bug to track the highlight specific work especially if
we're going to defer it and not merge back to M56.  It should make it easier for
TPM to follow the existing bug with which will have a merge request.

Done.

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

https://codereview.chromium.org/2533053002/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_popup_utils.cc:332: const gfx::Insets insets =
TrayPopupUtils::GetInkDropInsets(ink_drop_style);
On 2016/11/28 at 22:13:45, bruthig wrote:
> nit: Can you tighten the scope for |insets|. i.e. add curly braces to the
INSET_BOUNDS/FILL_BOUNDS case so you can make |insets| a local var within that
scope. Or just call GetInkDropInsets() inline in the FloodFillInkDropRipple
ctor.

Done.

https://codereview.chromium.org/2533053002/diff/1/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/2533053002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:171: gfx::Rect
CalculateInkDropBounds(const gfx::Size& size) {
On 2016/11/28 at 22:13:45, bruthig wrote:
> :( I kind of wish I had left myself a comment as to why the ink drop
ripple/highlight had to be sized differently than the host.... If you know why
can you please add a comment?

Apparently, this is because https://crbug.com/595317. Added a comment according
to my understanding of that bug. WDYT?

https://codereview.chromium.org/2533053002/diff/1/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/2533053002/diff/1/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_item_view.cc:523: size(),
gfx::Insets(), GetInkDropCenterBasedOnLastEvent(),
On 2016/11/28 at 22:13:45, bruthig wrote:
> nit: Consider adding a FloodFillInkDropRipple ctor that doesn't accept the
|clip_bounds| param and uses a default value of gfx::Insets().

Done.

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
File ui/views/animation/flood_fill_ink_drop_ripple.cc (right):

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
ui/views/animation/flood_fill_ink_drop_ripple.cc:171:
painted_layer_.SetTransform(GetMaxSizeTargetTransform());
On 2016/11/28 at 22:13:46, bruthig wrote:
> So will opacity animations continue here or do we need to snap them to the
correct values as well?

I think the opacity animations continue and it makes sense. First, for
transform, the existing animation doesn't make sense anymore after resize and
should be updated, but, for now, we just snap it to the final state as a
workaround. However, that's not the case for opacity animations. We can continue
the same animations and there is no need to snap them to the final values.
Second, unlike snapping transform animations which is not much noticeable to the
user during a resize, I think snapping opacity animations would be noticeable
and it's better to let the animation run its course. WDYT?

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
File ui/views/animation/flood_fill_ink_drop_ripple.h (right):

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
ui/views/animation/flood_fill_ink_drop_ripple.h:38: // rectangle of the given
size. The circle is clipped to the rectangles bounds.
On 2016/11/28 at 22:13:46, bruthig wrote:
> This doc should be updated. i.e. 'size' is very ambiguous. Can you add some
thorough docs as to what |host_size| and |clip_insets| are, how they are used to
calculate the effective painted bounds and, why we use them i.e. to accommodate
dynamically resizes.
> 
> It might make sense to move these docs to the ctor, but I don't feel too
strongly about this.

Done. Take a look...

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
File ui/views/animation/flood_fill_ink_drop_ripple_unittest.cc (right):

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
ui/views/animation/flood_fill_ink_drop_ripple_unittest.cc:23: // |clip_insets|.
On 2016/11/28 at 22:13:46, bruthig wrote:
> nit: It doesn't really make sense for |clip_insets| to define a coordinate
space. Perhaps replace |clip_insets| with 'clip bounds' or 'effective bounds'.

Updated comments.

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

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/ink_drop...
ui/views/animation/ink_drop_ripple.h:57: // Called by ink drop hosts when their
size is changed.
On 2016/11/28 at 22:13:46, bruthig wrote:
> nit: This comment is somewhat inaccurate.  This function is called by the
InkDropImpl not directly by hosts. Can you capture why this function exists and
why/when a subclass should override it.  Something along the lines of:
> 
> // Handles dynamic host resizes.  Subclasses can override to update an
> // already visible ripple when the host is resized.

Updated.

[bruthig, 2016-11-29 01:37:49]

lgtm

https://codereview.chromium.org/2533053002/diff/1/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/2533053002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:171: gfx::Rect
CalculateInkDropBounds(const gfx::Size& size) {
On 2016/11/29 00:07:12, mohsen wrote:
> On 2016/11/28 at 22:13:45, bruthig wrote:
> > :( I kind of wish I had left myself a comment as to why the ink drop
> ripple/highlight had to be sized differently than the host.... If you know why
> can you please add a comment?
> 
> Apparently, this is because https://crbug.com/595317. Added a comment
according
> to my understanding of that bug. WDYT?

lgtm, thx!

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
File ui/views/animation/flood_fill_ink_drop_ripple.cc (right):

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
ui/views/animation/flood_fill_ink_drop_ripple.cc:171:
painted_layer_.SetTransform(GetMaxSizeTargetTransform());
On 2016/11/29 00:07:12, mohsen wrote:
> On 2016/11/28 at 22:13:46, bruthig wrote:
> > So will opacity animations continue here or do we need to snap them to the
> correct values as well?
> 
> I think the opacity animations continue and it makes sense. First, for
> transform, the existing animation doesn't make sense anymore after resize and
> should be updated, but, for now, we just snap it to the final state as a
> workaround. However, that's not the case for opacity animations. We can
continue
> the same animations and there is no need to snap them to the final values.
> Second, unlike snapping transform animations which is not much noticeable to
the
> user during a resize, I think snapping opacity animations would be noticeable
> and it's better to let the animation run its course. WDYT?

sgtm

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
File ui/views/animation/flood_fill_ink_drop_ripple.h (right):

https://codereview.chromium.org/2533053002/diff/1/ui/views/animation/flood_fi...
ui/views/animation/flood_fill_ink_drop_ripple.h:38: // rectangle of the given
size. The circle is clipped to the rectangles bounds.
On 2016/11/29 00:07:12, mohsen wrote:
> On 2016/11/28 at 22:13:46, bruthig wrote:
> > This doc should be updated. i.e. 'size' is very ambiguous. Can you add some
> thorough docs as to what |host_size| and |clip_insets| are, how they are used
to
> calculate the effective painted bounds and, why we use them i.e. to
accommodate
> dynamically resizes.
> > 
> > It might make sense to move these docs to the ctor, but I don't feel too
> strongly about this.
> 
> Done. Take a look...

looks good, thx

[mohsen, 2016-11-29 01:44:09]

mohsen@chromium.org changed reviewers:
+ jamescook@chromium.org, sky@chromium.org

[mohsen, 2016-11-29 01:44:10]

+jamescook@ for changes in /ash/.
+sky@ for changes in /chrome/browser/ui/views/ and /ui/views/controls/.

[James Cook, 2016-11-29 01:56:57]

ash lgtm

[commit-bot: I haz the power, 2016-11-29 02:13:47]

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

[commit-bot: I haz the power, 2016-11-29 02:13:48]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-29 05:12:34]

https://codereview.chromium.org/2533053002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/2533053002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:171: const gfx::Insets
kInkDropInsets(1, 0);
Doesn't this result in a static initializer? Style guide says constants like
this should be POD.

[mohsen, 2016-11-29 05:48:44]

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

[commit-bot: I haz the power, 2016-11-29 05:49:07]

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-29 05:52:27]

https://codereview.chromium.org/2533053002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/2533053002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:171: const gfx::Insets
kInkDropInsets(1, 0);
On 2016/11/29 at 05:12:34, sky wrote:
> Doesn't this result in a static initializer? Style guide says constants like
this should be POD.

Right, my bad! However, I think style guide allows constexpr non-POD objects,
right? In this case, changed to constexpr.

[commit-bot: I haz the power, 2016-11-29 06:56:51]

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

[commit-bot: I haz the power, 2016-11-29 06:56:52]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-29 14:18:12]

On 2016/11/29 05:52:27, mohsen wrote:
>
https://codereview.chromium.org/2533053002/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):
> 
>
https://codereview.chromium.org/2533053002/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:171: const gfx::Insets
> kInkDropInsets(1, 0);
> On 2016/11/29 at 05:12:34, sky wrote:
> > Doesn't this result in a static initializer? Style guide says constants like
> this should be POD.
> 
> Right, my bad! However, I think style guide allows constexpr non-POD objects,
> right? In this case, changed to constexpr.

Apparently you are right. LGTM

[mohsen, 2016-11-29 16:03:20]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-11-29 16:03:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, bruthig@chromium.org
Link to the patchset: https://codereview.chromium.org/2533053002/#ps60001
(title: "const -> constexpr")

[commit-bot: I haz the power, 2016-11-29 16:03:31]

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-29 16:08:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480435400406370,
"parent_rev": "c5287f93c5d5b6d74ce0a0ed737c4a8537caba74", "commit_rev":
"a9c700106106e31d9280ba0bfa05672f8a8a7d73"}

[commit-bot: I haz the power, 2016-11-29 16:08:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-29 16:11:57]

Description was changed from

==========
Handle view resize for ripple

This change updates ink drop ripples (especially, flood fill ripple) to
handle host view's size change to update their layer size. The main
change is that the flood fill ripple gets insets instead of bounds to
make it relative to the host's bounds. If a ripple is in progress, it
will snap to its final size immediately.

Ink drop highlight is still using a workaround to handle resize and will
be fixed in a follow-up CL.

BUG=666175
TEST=manual
==========

to

==========
Handle view resize for ripple

This change updates ink drop ripples (especially, flood fill ripple) to
handle host view's size change to update their layer size. The main
change is that the flood fill ripple gets insets instead of bounds to
make it relative to the host's bounds. If a ripple is in progress, it
will snap to its final size immediately.

Ink drop highlight is still using a workaround to handle resize and will
be fixed in a follow-up CL.

BUG=666175
TEST=manual

Committed: https://crrev.com/7dfd81cc1c15c4f3f9954c4180e24356395aed4c
Cr-Commit-Position: refs/heads/master@{#435002}
==========

[commit-bot: I haz the power, 2016-11-29 16:11:59]

Patchset 4 (id:??) landed as
https://crrev.com/7dfd81cc1c15c4f3f9954c4180e24356395aed4c
Cr-Commit-Position: refs/heads/master@{#435002}