Color the ink drop ripple and hover effects based on theming.

Color the ink drop ripple and hover effects based on theming.

This is a port of https://codereview.chromium.org/1682893002/

BUG=575789
Committed: https://crrev.com/c404ead1fdc621b7be3a8d131700f82efb9083af
Cr-Commit-Position: refs/heads/master@{#377495}

[Evan Stade, 2016-02-23 22:07:21]

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

[bruthig, 2016-02-23 22:48:10]

Some minor comments inline. Also we should consider syncing the creation of new
InkDropAnimation and InkDropHover's. It might look pretty weird if the hover is
a different color from the ripple.  But I'm not 100% sure if/how that could
happen.

Were you able to actually repro a crash when removing
|can_destroy_after_hidden_animation_|?  I'm pretty sure I added it in because it
was crashing in prod... but I can't remember for the life of me how.  Anyway, I
guess we'll run without it and keep it in mind if we do get a mysterious crash.

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_animation.cc (right):

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:17:
observer_->InkDropAnimationStarted(ink_drop_state);
Why not guard this with "if (observer_)" ?
Similar below.

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_animation_controller_impl.cc (right):

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_impl.cc:95:
ink_drop_animation_->AnimateToState(ink_drop_state);
Here would be a good spot to note that |ink_drop_animation_| could be null.  I
don't think it's obvious.

[Evan Stade, 2016-02-24 00:31:58]

"""
Also we should consider syncing the creation of new
InkDropAnimation and InkDropHover's. It might look pretty weird if the hover is
a different color from the ripple.  But I'm not 100% sure if/how that could
happen.
"""

it pretty much is synced for all InkDropHostViews

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_animation.cc (right):

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:17:
observer_->InkDropAnimationStarted(ink_drop_state);
On 2016/02/23 22:48:09, bruthig wrote:
> Why not guard this with "if (observer_)" ?
> Similar below.

Because I decided it was an error to not set the observer. We could
DCHECK(observer_) but that's not generally recommended when you deref on the
next line anyway.

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_animation_controller_impl.cc (right):

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_impl.cc:95:
ink_drop_animation_->AnimateToState(ink_drop_state);
On 2016/02/23 22:48:09, bruthig wrote:
> Here would be a good spot to note that |ink_drop_animation_| could be null.  I
> don't think it's obvious.

I don't think it's super helpful because the crash that would occur if you tried
to use ink_drop_animation_ here is fairly obvious (it's not as insidious as when
|this| is destroyed. In general I don't like leaving comments if I don't think
they're very warranted because comments tend to get out of sync with reality, at
which point they do more harm than good.

[bruthig, 2016-02-24 15:46:56]

> """
> Also we should consider syncing the creation of new
> InkDropAnimation and InkDropHover's. It might look pretty weird if the hover
is
> a different color from the ripple.  But I'm not 100% sure if/how that could
> happen.
> """
> 
> it pretty much is synced for all InkDropHostViews

I'm not exactly sure how they are synced.  But it's probably not a problem
currently because the hover disappears when a ripple becomes active.  It will be
more likely that we could get two different colors when the hover persists
during the ripple animation, i.e. BookmarkButtons.  But I think it's safe to
defer solving that problem.

lgtm

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_animation.cc (right):

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:17:
observer_->InkDropAnimationStarted(ink_drop_state);
On 2016/02/24 00:31:57, Evan Stade wrote:
> On 2016/02/23 22:48:09, bruthig wrote:
> > Why not guard this with "if (observer_)" ?
> > Similar below.
> 
> Because I decided it was an error to not set the observer. We could
> DCHECK(observer_) but that's not generally recommended when you deref on the
> next line anyway.

Acknowledged.

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_animation_controller_impl.cc (right):

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_impl.cc:95:
ink_drop_animation_->AnimateToState(ink_drop_state);
On 2016/02/24 00:31:57, Evan Stade wrote:
> On 2016/02/23 22:48:09, bruthig wrote:
> > Here would be a good spot to note that |ink_drop_animation_| could be null. 
I
> > don't think it's obvious.
> 
> I don't think it's super helpful because the crash that would occur if you
tried
> to use ink_drop_animation_ here is fairly obvious (it's not as insidious as
when
> |this| is destroyed. In general I don't like leaving comments if I don't think
> they're very warranted because comments tend to get out of sync with reality,
at
> which point they do more harm than good.

Acknowledged.

[Evan Stade, 2016-02-24 22:02:16]

estade@chromium.org changed reviewers:
+ sky@chromium.org

[Evan Stade, 2016-02-24 22:02:17]

+sky, PTAL

> I'm not exactly sure how they are synced.

what I meant was that GetInkDropBaseColor() is used for both, so the color will
tend to be the same.

[sky, 2016-02-25 00:38:32]

https://codereview.chromium.org/1724963002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1724963002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:151:
image()->SetFillsBoundsOpaquely(true);
this isn't needed (because you destroy the layer on the next line).

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.h:34: virtual SkColor
GetInkDropBaseColor() const;
As your implementation has a NOTREACHED, is there a reason you didn't want to
make this pure virtual?

[Evan Stade, 2016-02-25 01:34:28]

https://codereview.chromium.org/1724963002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1724963002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:151:
image()->SetFillsBoundsOpaquely(true);
On 2016/02/25 00:38:32, sky wrote:
> this isn't needed (because you destroy the layer on the next line).

good call. This is a copy-pasta error so I've updated several other affected
places as well.

I'm wondering if we need SetFillsBoundsOpaquely to take a parameter at all ---
do we ever want to set it to true? It seems not, given that's the default and
almost all views just pick one value and stick to it. I've started a secondary
CL to see if I can change that or at least clean up some more unnecessary calls.

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_host_view.h (right):

https://codereview.chromium.org/1724963002/diff/40001/ui/views/animation/ink_...
ui/views/animation/ink_drop_host_view.h:34: virtual SkColor
GetInkDropBaseColor() const;
On 2016/02/25 00:38:32, sky wrote:
> As your implementation has a NOTREACHED, is there a reason you didn't want to
> make this pure virtual?

yes, originally it was pure virtual, but there was a test button class that
inherited from Button which complained. I figured it would be valid to create a
button that didn't have a ripple, and if no ripple is added this should never be
called. If this is never called, it's annoying to have to provide an override.

[sky, 2016-02-25 02:26:55]

LGTM

https://codereview.chromium.org/1724963002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc (right):

https://codereview.chromium.org/1724963002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc:151:
image()->SetFillsBoundsOpaquely(true);
On 2016/02/25 01:34:28, Evan Stade wrote:
> On 2016/02/25 00:38:32, sky wrote:
> > this isn't needed (because you destroy the layer on the next line).
> 
> good call. This is a copy-pasta error so I've updated several other affected
> places as well.
> 
> I'm wondering if we need SetFillsBoundsOpaquely to take a parameter at all ---
> do we ever want to set it to true? It seems not, given that's the default and
> almost all views just pick one value and stick to it. I've started a secondary
> CL to see if I can change that or at least clean up some more unnecessary
calls.

Seems like a weird limitation on view and layer. There are valid reasons to
toggle.

[Evan Stade, 2016-02-25 02:27:54]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-02-25 02:27:54]

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/1724963002/#ps60001
(title: "sky review")

[commit-bot: I haz the power, 2016-02-25 02:33:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1724963002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1724963002/60001

[commit-bot: I haz the power, 2016-02-25 03:54:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-25 03:55:13]

Description was changed from

==========
Color the ink drop ripple and hover effects based on theming.

This is a port of https://codereview.chromium.org/1682893002/

BUG=575789
==========

to

==========
Color the ink drop ripple and hover effects based on theming.

This is a port of https://codereview.chromium.org/1682893002/

BUG=575789

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

[commit-bot: I haz the power, 2016-02-25 03:55:15]

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