Implemented prototype for new ink drop specs.

Implemented prototype for new ink drop specs.

- Updated the InkDropAnimation to composite 2 rectangles and 4 circles
 to smoothly animate between a circle and a rounded rectangle of
 different sizes.
- Created a new InkDropHost that Views can extend in order to host the
 ink drop animation.
- Updated the ToolbarButtons to work with the new InkDropAnimation API.
- Added SLOW_ACTION_PENDING and DEACTIVATED InkDropStates.

TEST=InkDropAnimationTest.*
TEST=InkDropAnimationControllerFactoryTest.*
TEST=MDInkDropAnimationControllerFactoryTest.*

BUG=517903, 522170
Committed: https://crrev.com/8e8496ec69c203243767cec3014cd7232909e805
Cr-Commit-Position: refs/heads/master@{#349100}

[bruthig, 2015-08-19 20:51:39]

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

[bruthig, 2015-08-19 20:51:40]

tdanderson@, jonross@, can you guys PTAL?

[tdanderson, 2015-08-19 22:11:19]

Looks good to me as a first draft.

https://chromiumcodereview.appspot.com/1298513003/diff/20001/chrome/browser/u...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://chromiumcodereview.appspot.com/1298513003/diff/20001/chrome/browser/u...
chrome/browser/ui/views/toolbar/toolbar_button.cc:102: // Button actions are
only triggered by left and middle mouse clicks.
Don't we also want to show feedback when pressing the right button (in
preparation for showing the history menu on the back button, for instance)?

https://chromiumcodereview.appspot.com/1298513003/diff/20001/ui/views/animati...
File ui/views/animation/ink_drop_animation.cc (right):

https://chromiumcodereview.appspot.com/1298513003/diff/20001/ui/views/animati...
ui/views/animation/ink_drop_animation.cc:205:
top_left_circle_layer_(AddPaintLayer(circle_layer_delegate_.get())),
I may have already asked before (can't remember), but is it necessary for all of
these layers to exist all of the time? In particular, do we need to initialize
them in the constructor or can they be created when needed (on user input) and
then destroyed when no longer needed (user input finished)?

https://chromiumcodereview.appspot.com/1298513003/diff/20001/ui/views/animati...
ui/views/animation/ink_drop_animation.cc:238: ink_drop_state_ = state;
You should be able to factor this line out to after line 293 (you break in each
case where you set it).

https://chromiumcodereview.appspot.com/1298513003/diff/20001/ui/views/animati...
ui/views/animation/ink_drop_animation.cc:290: DCHECK(false) << "Unhandled
InkDropState=" << static_cast<int>(state);
Probably use NOTREACHED instead here?

https://chromiumcodereview.appspot.com/1298513003/diff/20001/ui/views/animati...
ui/views/animation/ink_drop_animation.cc:310:
AnimateToTransform(top_left_circle_layer_.get(), transforms.top_left_circle,
Does the order of these function calls matter? (if so, consider documenting
that)

https://chromiumcodereview.appspot.com/1298513003/diff/20001/ui/views/animati...
ui/views/animation/ink_drop_animation.cc:371: const float circle_radius =
circle_layer_delegate_->radius();
Probably already on your to-do list, but some inline documentation might be
worthwhile here.

https://chromiumcodereview.appspot.com/1298513003/diff/20001/ui/views/animati...
File ui/views/animation/ink_drop_animation.h (right):

https://chromiumcodereview.appspot.com/1298513003/diff/20001/ui/views/animati...
ui/views/animation/ink_drop_animation.h:45: // Sets the origin of the ink drop.
Is this the origin in terms of where the input event was (e.g., mouse location)?
Relative to which coordinate space?

https://chromiumcodereview.appspot.com/1298513003/diff/20001/ui/views/animati...
ui/views/animation/ink_drop_animation.h:100: gfx::Size large_size_;
I'm not clear from the documentation what "large" and "small" sized animations
are.

https://chromiumcodereview.appspot.com/1298513003/diff/20001/ui/views/animati...
File ui/views/animation/ink_drop_animation_controller_factory.cc (right):

https://chromiumcodereview.appspot.com/1298513003/diff/20001/ui/views/animati...
ui/views/animation/ink_drop_animation_controller_factory.cc:79: // concern it
will conflict with OS level touch feedback in a bad way.
Since this is behind a flag anyway, I don't think there's any harm in allowing
on all three platforms just to see what it actually looks like (unless "conflict
with OS level touch feedback in a bad way" actually means "crash").

[jonross, 2015-08-19 22:13:36]

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:103: if
(event.IsLeftMouseButton() || event.IsMiddleMouseButton()) {
Do you need to check the exclusivity of these clicks?

Event::IsOnlyLeftMouseButton / IsOnlyMiddleMouseButton

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:298:
views::InkDropState::ACTIVATED);
Consider refactoring these animations to be around the if block. Moving the
result early exits outside as well.

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:300:
menu_runner_->RunMenuAt(GetWidget(),
Blocking iirc?

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:341: // TODO(bruthig): Get the
constants from the theme provider.
If you don't do this in the current review, please make them constants of the
file.

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

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:33: const SkColor kInkDropColor =
SK_ColorBLACK;
Testing?

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:44: 1,    // InkDropState::HIDDEN
Consider each of these being a const, then having the array just be the consts.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:84: SkColor color() const { return
color_; }
Needed?

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:135: class RectangleLayerDelegate :
public ui::LayerDelegate {
Mostly duplicate code. Consider making a parent of these.

Then circle/square could just override onpaintlayer.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:238: ink_drop_state_ = state;
Move these out of the switch.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:311: duration, preemption_strategy);
We are starting to get duplication from the number of layers. Should we store
these in an array? Then iterate when applying the same effects.?

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:344:
SetTransforms(CalculateCircleTransforms(gfx::Size(1, 1)));
Does this do enough to warrant it being a standalone method?

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:426: return transforms;
Is this performing a move?

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

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_factory.cc:20: class
VIEWS_EXPORT InkDropAnimationControllerStub
Thoughts on just having IDAC be this, instead of pure virtual?

[bruthig, 2015-08-20 20:35:36]

I've addressed all comments except requests for documentation. I plan on doing
one more pass to address all the documentation TODO's and requests.

This patch set should be functional, so if you want to pull it and side load for
demos it's good to show!

jonross@, tdanderson@, PTAL :)

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:102: // Button actions are
only triggered by left and middle mouse clicks.
On 2015/08/19 22:11:18, tdanderson wrote:
> Don't we also want to show feedback when pressing the right button (in
> preparation for showing the history menu on the back button, for instance)?

That will be triggered by the AnimateToState(ACTIVATED) in the
ToolbarButton::ShowDropDownMenu() method.

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:103: if
(event.IsLeftMouseButton() || event.IsMiddleMouseButton()) {
On 2015/08/19 22:13:36, jonross wrote:
> Do you need to check the exclusivity of these clicks?
> 
> Event::IsOnlyLeftMouseButton / IsOnlyMiddleMouseButton

Negative, actions are still triggered when a button is released while both the
left and middle buttons are pressed. Specifically if the user left clicks and
holds and the animation proceeds to a long press on the "Back" button and then
they middle click and release, then the most recent url in the history is
brought up in a new tab.  This is equivalent to a simple middle click and
release on the "Back" buttonm so IMO it should show an ACTION_PENDING ->
QUICK_ACTION animation sequence which this allows for.

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:298:
views::InkDropState::ACTIVATED);
On 2015/08/19 22:13:36, jonross wrote:
> Consider refactoring these animations to be around the if block. Moving the
> result early exits outside as well.

Done.

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:300:
menu_runner_->RunMenuAt(GetWidget(),
On 2015/08/19 22:13:36, jonross wrote:
> Blocking iirc?

Correct.

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:341: // TODO(bruthig): Get the
constants from the theme provider.
On 2015/08/19 22:13:36, jonross wrote:
> If you don't do this in the current review, please make them constants of the
> file.

Done.

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

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:33: const SkColor kInkDropColor =
SK_ColorBLACK;
On 2015/08/19 22:13:36, jonross wrote:
> Testing?

Not 100% sure what you mean here. Assuming you are asking why I'm using
SK_ColorBLACK instead of SK_ColorLTGRAY.  The specs call for #000000 12% alpha. 
This may have changed from when the original prototype was spec'ed out.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:44: 1,    // InkDropState::HIDDEN
On 2015/08/19 22:13:36, jonross wrote:
> Consider each of these being a const, then having the array just be the
consts.

I've replaced the array with a switch statement.  The pros of the switch are (1)
if someone adds a new state then they will get a compile time error if they
don't update it and (2) a default case is not necessary in the AnimateToState()
switch statement.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:84: SkColor color() const { return
color_; }
On 2015/08/19 22:13:36, jonross wrote:
> Needed?

Removed, but required by the base class now.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:135: class RectangleLayerDelegate :
public ui::LayerDelegate {
On 2015/08/19 22:13:36, jonross wrote:
> Mostly duplicate code. Consider making a parent of these.
> 
> Then circle/square could just override onpaintlayer.

Done.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:205:
top_left_circle_layer_(AddPaintLayer(circle_layer_delegate_.get())),
On 2015/08/19 22:11:18, tdanderson wrote:
> I may have already asked before (can't remember), but is it necessary for all
of
> these layers to exist all of the time? In particular, do we need to initialize
> them in the constructor or can they be created when needed (on user input) and
> then destroyed when no longer needed (user input finished)?

You are correct, it is not necessary to keep the layers around at all times. 
Issue www.crbug.com/522175 tracks the work to optimize this.  However, the plan
is that the InkDropAnimation itself will be created/destroyed (or
acquired/released to a pool) instead of the InkDropAnimation creating/destroying
it's Layers.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:238: ink_drop_state_ = state;
On 2015/08/19 22:13:36, jonross wrote:
> Move these out of the switch.

I can't set the |ink_drop_state_|  after the switch because some cases in the
switch make recursive calls to AnimateToState().

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:290: DCHECK(false) << "Unhandled
InkDropState=" << static_cast<int>(state);
On 2015/08/19 22:11:18, tdanderson wrote:
> Probably use NOTREACHED instead here?

Removed, no longer needed.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:310:
AnimateToTransform(top_left_circle_layer_.get(), transforms.top_left_circle,
On 2015/08/19 22:11:19, tdanderson wrote:
> Does the order of these function calls matter? (if so, consider documenting
> that)

The order doesn't matter.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:311: duration, preemption_strategy);
On 2015/08/19 22:13:36, jonross wrote:
> We are starting to get duplication from the number of layers. Should we store
> these in an array? Then iterate when applying the same effects.?

Done.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:344:
SetTransforms(CalculateCircleTransforms(gfx::Size(1, 1)));
On 2015/08/19 22:13:36, jonross wrote:
> Does this do enough to warrant it being a standalone method?

The purpose of making this a standalone method is because there are two
different spots that should invoke this code.  I'm trying to make it easier for
someone who may need to update it and they don't realize that it is the same
operation because the code was in-lined.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:426: return transforms;
On 2015/08/19 22:13:36, jonross wrote:
> Is this performing a move?

Doesn't exist anymore.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_animation.h (right):

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:45: // Sets the origin of the ink drop.
On 2015/08/19 22:11:19, tdanderson wrote:
> Is this the origin in terms of where the input event was (e.g., mouse
location)?
> Relative to which coordinate space?

Attempted to clarify in comment.

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

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_factory.cc:20: class
VIEWS_EXPORT InkDropAnimationControllerStub
On 2015/08/19 22:13:36, jonross wrote:
> Thoughts on just having IDAC be this, instead of pure virtual?

I'm of the opinion that I want to make it hard for users to do things they
shouldn't.  In this case I don't want them to incorrectly (or unknowingly)
create and use an IDAC that is actually a non-functional stub and the only
indication of that fact is documentation or implementation details.  By using a
pure virtual base class and an IDAC with a 'Stub' suffix it should be painfully
obvious what the purpose of each of the classes is.

It's not immediately obvious to me what the benefits would be of implementing it
like that.  Perhaps you can elaborate?

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_factory.cc:79: // concern it
will conflict with OS level touch feedback in a bad way.
On 2015/08/19 22:11:19, tdanderson wrote:
> Since this is behind a flag anyway, I don't think there's any harm in allowing
> on all three platforms just to see what it actually looks like (unless
"conflict
> with OS level touch feedback in a bad way" actually means "crash").

This was the request of the designers.  If we do change it we should probably
loop them in and I'm assuming we would have to update the hover borders that
will be currently be shown on those platforms.  Is it ok if we defer that for a
follow-up review if necessary?

[jonross, 2015-08-21 15:14:49]

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:103: if
(event.IsLeftMouseButton() || event.IsMiddleMouseButton()) {
On 2015/08/20 20:35:35, bruthig (OOO Aug 21 - 30) wrote:
> On 2015/08/19 22:13:36, jonross wrote:
> > Do you need to check the exclusivity of these clicks?
> > 
> > Event::IsOnlyLeftMouseButton / IsOnlyMiddleMouseButton
> 
> Negative, actions are still triggered when a button is released while both the
> left and middle buttons are pressed. Specifically if the user left clicks and
> holds and the animation proceeds to a long press on the "Back" button and then
> they middle click and release, then the most recent url in the history is
> brought up in a new tab.  This is equivalent to a simple middle click and
> release on the "Back" buttonm so IMO it should show an ACTION_PENDING ->
> QUICK_ACTION animation sequence which this allows for.

Acknowledged.

https://codereview.chromium.org/1298513003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:300:
menu_runner_->RunMenuAt(GetWidget(),
On 2015/08/20 20:35:35, bruthig (OOO Aug 21 - 30) wrote:
> On 2015/08/19 22:13:36, jonross wrote:
> > Blocking iirc?
> 
> Correct.

Acknowledged.

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

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:33: const SkColor kInkDropColor =
SK_ColorBLACK;
On 2015/08/20 20:35:35, bruthig (OOO Aug 21 - 30) wrote:
> On 2015/08/19 22:13:36, jonross wrote:
> > Testing?
> 
> Not 100% sure what you mean here. Assuming you are asking why I'm using
> SK_ColorBLACK instead of SK_ColorLTGRAY.  The specs call for #000000 12%
alpha. 
> This may have changed from when the original prototype was spec'ed out.

Ah, yeah the current spec does want the alpha black. SGTM

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:84: SkColor color() const { return
color_; }
On 2015/08/20 20:35:35, bruthig (OOO Aug 21 - 30) wrote:
> On 2015/08/19 22:13:36, jonross wrote:
> > Needed?
> 
> Removed, but required by the base class now.

Acknowledged.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:238: ink_drop_state_ = state;
On 2015/08/20 20:35:35, bruthig (OOO Aug 21 - 30) wrote:
> On 2015/08/19 22:13:36, jonross wrote:
> > Move these out of the switch.
> 
> I can't set the |ink_drop_state_|  after the switch because some cases in the
> switch make recursive calls to AnimateToState().

That sounds dangerous.

However I was thinking over just having the setting be before the switch.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:344:
SetTransforms(CalculateCircleTransforms(gfx::Size(1, 1)));
On 2015/08/20 20:35:35, bruthig (OOO Aug 21 - 30) wrote:
> On 2015/08/19 22:13:36, jonross wrote:
> > Does this do enough to warrant it being a standalone method?
> 
> The purpose of making this a standalone method is because there are two
> different spots that should invoke this code.  I'm trying to make it easier
for
> someone who may need to update it and they don't realize that it is the same
> operation because the code was in-lined.

Acknowledged.

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

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation_controller_factory.cc:20: class
VIEWS_EXPORT InkDropAnimationControllerStub
On 2015/08/20 20:35:35, bruthig (OOO Aug 21 - 30) wrote:
> On 2015/08/19 22:13:36, jonross wrote:
> > Thoughts on just having IDAC be this, instead of pure virtual?
> 
> I'm of the opinion that I want to make it hard for users to do things they
> shouldn't.  In this case I don't want them to incorrectly (or unknowingly)
> create and use an IDAC that is actually a non-functional stub and the only
> indication of that fact is documentation or implementation details.  By using
a
> pure virtual base class and an IDAC with a 'Stub' suffix it should be
painfully
> obvious what the purpose of each of the classes is.
> 
> It's not immediately obvious to me what the benefits would be of implementing
it
> like that.  Perhaps you can elaborate?

I was mostly thinking of condensing the code. Where the default IDAC would
always just be a no-opt. However I'm fine with your reasoning for this
differentiation. SGTM.

https://codereview.chromium.org/1298513003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1298513003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:308: result =
menu_runner_->RunMenuAt(GetWidget(), NULL,
nit: nullptr

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

https://codereview.chromium.org/1298513003/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:49: const int
kDeactivatedStateAnimationDurationMs = 250;
I think you should still use an array. However have that array be defined in
terms of these constants:

const int kAnimationDurationsMs[static_cast<int>(views::InkDropState::COUNT)] = 

kHiddenStateAnimationDurationMs,
kActionPendingStateAnimationDurationMs,
....
};

Then you can revert the switch on line 66.

However your point on the switch being a compilation check is valid. So I'd be
fine without my suggestion. We can talk when you're back.

[bruthig, 2015-09-03 21:21:04]

bruthig@chromium.org changed reviewers:
+ pkasting@chromium.org, sadrul@chromium.org

[bruthig, 2015-09-03 21:21:05]

jonross@/tdanderson@: An you please take a look from a feature correctness
perspective.

pkasting@: Can you provide an ONWERs review for:
 - chrome/*

sadrul@: Can you provide an OWNERs review for:
 - ui/views/*

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

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:238: ink_drop_state_ = state;
On 2015/08/21 15:14:48, jonross wrote:
> On 2015/08/20 20:35:35, bruthig (OOO Aug 21 - 30) wrote:
> > On 2015/08/19 22:13:36, jonross wrote:
> > > Move these out of the switch.
> > 
> > I can't set the |ink_drop_state_|  after the switch because some cases in
the
> > switch make recursive calls to AnimateToState().
> 
> That sounds dangerous.
> 
> However I was thinking over just having the setting be before the switch.

I've moved it to before the switch statement, added a comment and a unit test ;)

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:371: const float circle_radius =
circle_layer_delegate_->radius();
On 2015/08/19 22:11:19, tdanderson wrote:
> Probably already on your to-do list, but some inline documentation might be
> worthwhile here.

I've tried to address this with some refactoring and limited documentation.  Let
me know if you think it could benefit from more.

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
File ui/views/animation/ink_drop_animation.h (right):

https://codereview.chromium.org/1298513003/diff/20001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.h:100: gfx::Size large_size_;
On 2015/08/19 22:11:19, tdanderson wrote:
> I'm not clear from the documentation what "large" and "small" sized animations
> are.

I made an attempt to better explain this with variable and class level
documentation.

https://codereview.chromium.org/1298513003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1298513003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:308: result =
menu_runner_->RunMenuAt(GetWidget(), NULL,
On 2015/08/21 15:14:48, jonross wrote:
> nit: nullptr

Done.

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

https://codereview.chromium.org/1298513003/diff/60001/ui/views/animation/ink_...
ui/views/animation/ink_drop_animation.cc:49: const int
kDeactivatedStateAnimationDurationMs = 250;
On 2015/08/21 15:14:49, jonross wrote:
> I think you should still use an array. However have that array be defined in
> terms of these constants:
> 
> const int kAnimationDurationsMs[static_cast<int>(views::InkDropState::COUNT)]
= 
> 
> kHiddenStateAnimationDurationMs,
> kActionPendingStateAnimationDurationMs,
> ....
> };
> 
> Then you can revert the switch on line 66.
> 
> However your point on the switch being a compilation check is valid. So I'd be
> fine without my suggestion. We can talk when you're back.

Discussed offline, will leave it as-is and see if the OWNERS have any concerns.

[Peter Kasting, 2015-09-03 21:53:06]

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/back_button.cc (right):

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/back_button.cc:36:
ToolbarButton::LayoutInkDrop();
As a reader I'm unsure whether this line has to be before the next one (it
almost seems like it should be after the next one since I'd have thought setting
the ink drop center correctly would be something we'd need to do before
layout?).

Then we have the opposite ordering in ToolbarButton::NotifyClick(), where we
call the superclass method after messing with the animation controller.

If either of these places requires its current ordering, comment to that effect.
 Consider making the orderings match if possible.

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/back_button.cc:38: (width() - margin_leading_) /
2 + margin_leading_, height() / 2));
Nit: Seems like you can just do (width() + margin_leading_) / 2

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:30: const int
kInkDropLargeSize = 32;
Nit: Move all these to LayoutInkDrop() since that's the only consumer.

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:108: // Button actions are
only triggered by left and middle mouse clicks.
Today in Chrome I can right-click the back button and immediately get a dropdown
of my session history.  Does that falsify this statement?

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:307: result =
menu_runner_->RunMenuAt(GetWidget(), nullptr,
Is it safe to move this block below the conditional, since both arms do the same
thing?  Or does RunMenuAt() need to be called while |menu_delegate| is alive?

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:320:
ink_drop_animation_controller_->AnimateToState(
This isn't safe if the result is MENU_DELETED, in that case |this| is null.  You
must return before calling this.

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:338: gfx::Size ink_drop_size =
gfx::Size(kInkDropLargeSize, kInkDropLargeSize);
Nit: I don't see why you made a temp for this instead of doing it inline like is
done for the small size.

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.h (right):

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.h:72: void NotifyClick(const
ui::Event& event) override;
Nit: No subclasses need to call this, so make this a private override rather
than protected.

[bruthig, 2015-09-09 18:00:36]

I've addressed pkasting@'s concerns from patch set 7.

pkasting@: Can you please review changes in:
 - chrome/*

sadrul@: Can you provide an OWNERs review for:
 - ui/views/*

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/back_button.cc (right):

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/back_button.cc:36:
ToolbarButton::LayoutInkDrop();
On 2015/09/03 21:53:06, Peter Kasting wrote:
> As a reader I'm unsure whether this line has to be before the next one (it
> almost seems like it should be after the next one since I'd have thought
setting
> the ink drop center correctly would be something we'd need to do before
> layout?).
> 
> Then we have the opposite ordering in ToolbarButton::NotifyClick(), where we
> call the superclass method after messing with the animation controller.
> 
> If either of these places requires its current ordering, comment to that
effect.
>  Consider making the orderings match if possible.

I've re-worked this a little bit. FTR the ordering in
ToolbarButton::NotifyClick() doesn't matter.

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/back_button.cc:38: (width() - margin_leading_) /
2 + margin_leading_, height() / 2));
On 2015/09/03 21:53:06, Peter Kasting wrote:
> Nit: Seems like you can just do (width() + margin_leading_) / 2

Done.

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:30: const int
kInkDropLargeSize = 32;
On 2015/09/03 21:53:06, Peter Kasting wrote:
> Nit: Move all these to LayoutInkDrop() since that's the only consumer.

Done.

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:108: // Button actions are
only triggered by left and middle mouse clicks.
On 2015/09/03 21:53:06, Peter Kasting wrote:
> Today in Chrome I can right-click the back button and immediately get a
dropdown
> of my session history.  Does that falsify this statement?

No it does not, the menu shown due to a right-click is handled by
ToolbarButton::OnMousePressed()->ToolbarButton::ShowDropDownMenu().  I've tried
to reduce the ambiguity by adding the 'views::' prefix.

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:307: result =
menu_runner_->RunMenuAt(GetWidget(), nullptr,
On 2015/09/03 21:53:06, Peter Kasting wrote:
> Is it safe to move this block below the conditional, since both arms do the
same
> thing?  Or does RunMenuAt() need to be called while |menu_delegate| is alive?

Negative, as you suspect |menu_delegate| must be alive when RunMenuAt() is
called.

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:320:
ink_drop_animation_controller_->AnimateToState(
On 2015/09/03 21:53:06, Peter Kasting wrote:
> This isn't safe if the result is MENU_DELETED, in that case |this| is null. 
You
> must return before calling this.

Done. I've also added a comment to try and make this more obvious.

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:338: gfx::Size ink_drop_size =
gfx::Size(kInkDropLargeSize, kInkDropLargeSize);
On 2015/09/03 21:53:06, Peter Kasting wrote:
> Nit: I don't see why you made a temp for this instead of doing it inline like
is
> done for the small size.

Done.  Oversight on my part.

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.h (right):

https://codereview.chromium.org/1298513003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.h:72: void NotifyClick(const
ui::Event& event) override;
On 2015/09/03 21:53:06, Peter Kasting wrote:
> Nit: No subclasses need to call this, so make this a private override rather
> than protected.

Incorrect, HomeButton::OnNotifyClick() calls ToolbarButton::OnNotifyClick().

[Peter Kasting, 2015-09-09 22:50:46]

LGTM

https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:303:
views::MenuRunner::RunResult result = views::MenuRunner::MENU_DELETED;
Nit: For clarity, I'd move this line to be between the comment below and the
subsequent conditional, and I'd omit the initial value so it's clear both arms
must set this.

https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:327: // a result of the
ToolbarButton's destructor being called.
Nit: You probably don't need this as this ought to be clear to anyone familiar
with menus.  In either case, I'd remove the blank line above this since this is
sort of the final part of "handling running the menu".

https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:347: return gfx::Point(width()
/ 2, height() / 2);
Nit: Or just:

  return GetLocalBounds().CenterPoint();

https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.h (right):

https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.h:72: // views::LabelButton:
Nit: I would put this heading over both the overrides here and eliminate the
comment above entirely:

  // views::LabelButton:
  bool ShouldEnterPushedState(const ui::Event& event) override;
  void NotifyClick(const ui::Event& event) override;

Any commentary on ShouldEnterPushedState can be given in the implementation.

[bruthig, 2015-09-10 14:57:39]

I have addressed pkasting@'s nits from patch set 9 and updated the CL
description.

pkasting@: No need to review.

sadrul@: Can you PTAL at:
 - ui/*

https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:303:
views::MenuRunner::RunResult result = views::MenuRunner::MENU_DELETED;
On 2015/09/09 22:50:46, Peter Kasting wrote:
> Nit: For clarity, I'd move this line to be between the comment below and the
> subsequent conditional, and I'd omit the initial value so it's clear both arms
> must set this.

Done.

https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:327: // a result of the
ToolbarButton's destructor being called.
On 2015/09/09 22:50:46, Peter Kasting wrote:
> Nit: You probably don't need this as this ought to be clear to anyone familiar
> with menus.  In either case, I'd remove the blank line above this since this
is
> sort of the final part of "handling running the menu".

Done.

https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.cc:347: return gfx::Point(width()
/ 2, height() / 2);
On 2015/09/09 22:50:46, Peter Kasting wrote:
> Nit: Or just:
> 
>   return GetLocalBounds().CenterPoint();

Done.

https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/toolbar_button.h (right):

https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/toolbar_button.h:72: // views::LabelButton:
On 2015/09/09 22:50:46, Peter Kasting wrote:
> Nit: I would put this heading over both the overrides here and eliminate the
> comment above entirely:
> 
>   // views::LabelButton:
>   bool ShouldEnterPushedState(const ui::Event& event) override;
>   void NotifyClick(const ui::Event& event) override;
> 
> Any commentary on ShouldEnterPushedState can be given in the implementation.

Done.

[jonross, 2015-09-10 14:58:57]

On 2015/09/10 14:57:39, bruthig wrote:
> I have addressed pkasting@'s nits from patch set 9 and updated the CL
> description.
> 
> pkasting@: No need to review.
> 
> sadrul@: Can you PTAL at:
>  - ui/*
> 
>
https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
> File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):
> 
>
https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/toolbar/toolbar_button.cc:303:
> views::MenuRunner::RunResult result = views::MenuRunner::MENU_DELETED;
> On 2015/09/09 22:50:46, Peter Kasting wrote:
> > Nit: For clarity, I'd move this line to be between the comment below and the
> > subsequent conditional, and I'd omit the initial value so it's clear both
arms
> > must set this.
> 
> Done.
> 
>
https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/toolbar/toolbar_button.cc:327: // a result of the
> ToolbarButton's destructor being called.
> On 2015/09/09 22:50:46, Peter Kasting wrote:
> > Nit: You probably don't need this as this ought to be clear to anyone
familiar
> > with menus.  In either case, I'd remove the blank line above this since this
> is
> > sort of the final part of "handling running the menu".
> 
> Done.
> 
>
https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/toolbar/toolbar_button.cc:347: return
gfx::Point(width()
> / 2, height() / 2);
> On 2015/09/09 22:50:46, Peter Kasting wrote:
> > Nit: Or just:
> > 
> >   return GetLocalBounds().CenterPoint();
> 
> Done.
> 
>
https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
> File chrome/browser/ui/views/toolbar/toolbar_button.h (right):
> 
>
https://codereview.chromium.org/1298513003/diff/160001/chrome/browser/ui/view...
> chrome/browser/ui/views/toolbar/toolbar_button.h:72: // views::LabelButton:
> On 2015/09/09 22:50:46, Peter Kasting wrote:
> > Nit: I would put this heading over both the overrides here and eliminate the
> > comment above entirely:
> > 
> >   // views::LabelButton:
> >   bool ShouldEnterPushedState(const ui::Event& event) override;
> >   void NotifyClick(const ui::Event& event) override;
> > 
> > Any commentary on ShouldEnterPushedState can be given in the implementation.
> 
> Done.

LGTM

[tdanderson, 2015-09-10 15:37:51]

LGTM with two nits

https://chromiumcodereview.appspot.com/1298513003/diff/180001/ui/views/animat...
File ui/views/animation/ink_drop_animation.h (right):

https://chromiumcodereview.appspot.com/1298513003/diff/180001/ui/views/animat...
ui/views/animation/ink_drop_animation.h:85: // Resets the Transforms on all the
owned Layers to a minium size.
nit: minium -> minimum

https://chromiumcodereview.appspot.com/1298513003/diff/180001/ui/views/animat...
File ui/views/animation/ink_drop_animation_controller_factory.h (right):

https://chromiumcodereview.appspot.com/1298513003/diff/180001/ui/views/animat...
ui/views/animation/ink_drop_animation_controller_factory.h:22: //
|ink_drop_host_|.
Do you mean |ink_drop_host| here? And can you clarify what "might be attached
to" means here? Reading the API documentation here I'd want to know exactly what
happens when I call this function and not have to look for documentation
elsewhere.

[sadrul, 2015-09-14 20:05:41]

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation.cc (right):

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:134: void OnPaintLayer(const
ui::PaintContext& context) override;
Should this be left as pure virtual?

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:210: gfx::Size size() const { return
size_; }
const gfx::Size& size()

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:235: canvas->DrawRect(gfx::Rect(0, 0,
size_.width(), size_.height()), paint);
gfx::Rect(size_)

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:362:
CalculateCircleTransforms(gfx::Size(1, 1), transforms);
Explain why size(1,1) is used here (instead of, say, the size of the view/host)?

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:368:
painted_layers_[i]->SetTransform(transforms[i]);
Can you clarify why you need to set transforms on individual layers? Where are
these transforms changed for these layers?

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:389: << "The circle's diameter should
not be greater than the total height.";
DCHECK*

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:475:
painted_layers_[painted_shape].reset(layer);
Looks like you are creating 6 layers at startup. Can you clarify what the
lifetime of these layers are?

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation.h (right):

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:63: TOP_LEFT_CIRCLE,
= 0

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:80: InkDropTransforms transforms,
const

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:90: void SetTransforms(InkDropTransforms
transforms);
const

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:97: void CalculateCircleTransforms(const
gfx::SizeF size,
const &

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:98: InkDropTransforms transforms) const;
const

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:102: void CalculateRectTransforms(const
gfx::SizeF size,
const &

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:104: InkDropTransforms transforms)
const;
const

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:108: void
GetCurrentTansforms(InkDropTransforms transforms) const;
*Transforms

InkDropTransforms* since out-param.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller_impl.cc (right):

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:65:
ink_drop_host_->RemoveInkDropLayer(ink_drop_animation_->root_layer());
I guess it's a bit unfortunate that you need to remember to manually do this
before destruction. Would be nice if InkDropAnimation() took care of this
itself.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_state.h (right):

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_state.h:31: DEACTIVATED,
These could've been in a separate smaller CL?

[sadrul, 2015-09-14 20:06:25]

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation.cc (right):

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:475:
painted_layers_[painted_shape].reset(layer);
On 2015/09/14 20:05:40, sadrul wrote:
> Looks like you are creating 6 layers at startup. Can you clarify what the
> lifetime of these layers are?

We discussed this offline. Please make sure there are relevant TODOs (I think
there already are?)

[bruthig, 2015-09-15 19:47:10]

sadrul@, Can you PTAL?

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation.cc (right):

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:134: void OnPaintLayer(const
ui::PaintContext& context) override;
On 2015/09/14 20:05:40, sadrul wrote:
> Should this be left as pure virtual?

Done.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:210: gfx::Size size() const { return
size_; }
On 2015/09/14 20:05:40, sadrul wrote:
> const gfx::Size& size()

Done.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:235: canvas->DrawRect(gfx::Rect(0, 0,
size_.width(), size_.height()), paint);
On 2015/09/14 20:05:40, sadrul wrote:
> gfx::Rect(size_)

Done.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:362:
CalculateCircleTransforms(gfx::Size(1, 1), transforms);
On 2015/09/14 20:05:40, sadrul wrote:
> Explain why size(1,1) is used here (instead of, say, the size of the
view/host)?

Discussed offline.  This should make more sense once the ink drop ripple has
been documented on a chromium.org.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:368:
painted_layers_[i]->SetTransform(transforms[i]);
On 2015/09/14 20:05:40, sadrul wrote:
> Can you clarify why you need to set transforms on individual layers? Where are
> these transforms changed for these layers?

(I'm assuming this is not a request for added documentation.) The transforms are
calculated  by the CalculateCircleTransforms() and CalculateRectTransforms()
methods and the actual Layer's transforms are set in either SetTransforms() or
AnimateToTransforms().

[Discussed offline why each layer needs to have it's own transform set on it.]

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.cc:389: << "The circle's diameter should
not be greater than the total height.";
On 2015/09/14 20:05:40, sadrul wrote:
> DCHECK*

Done.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation.h (right):

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:63: TOP_LEFT_CIRCLE,
On 2015/09/14 20:05:40, sadrul wrote:
> = 0

Done.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:80: InkDropTransforms transforms,
On 2015/09/14 20:05:40, sadrul wrote:
> const

Done.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:85: // Resets the Transforms on all the
owned Layers to a minium size.
On 2015/09/10 15:37:51, tdanderson wrote:
> nit: minium -> minimum

Done.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:90: void SetTransforms(InkDropTransforms
transforms);
On 2015/09/14 20:05:40, sadrul wrote:
> const

Done.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:97: void CalculateCircleTransforms(const
gfx::SizeF size,
On 2015/09/14 20:05:40, sadrul wrote:
> const &

Done.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:98: InkDropTransforms transforms) const;
On 2015/09/14 20:05:40, sadrul wrote:
> const

|transforms| is an output parameter.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:102: void CalculateRectTransforms(const
gfx::SizeF size,
On 2015/09/14 20:05:40, sadrul wrote:
> const &

Done.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:104: InkDropTransforms transforms)
const;
On 2015/09/14 20:05:41, sadrul wrote:
> const

|transforms| is an output parameter.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation.h:108: void
GetCurrentTansforms(InkDropTransforms transforms) const;
On 2015/09/14 20:05:40, sadrul wrote:
> *Transforms
> 
> InkDropTransforms* since out-param.

Done.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller_factory.h (right):

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_factory.h:22: //
|ink_drop_host_|.
On 2015/09/10 15:37:51, tdanderson wrote:
> Do you mean |ink_drop_host| here? And can you clarify what "might be attached
> to" means here? Reading the API documentation here I'd want to know exactly
what
> happens when I call this function and not have to look for documentation
> elsewhere.

Attempted to improve documentation. What do you think?

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller_impl.cc (right):

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:65:
ink_drop_host_->RemoveInkDropLayer(ink_drop_animation_->root_layer());
On 2015/09/14 20:05:41, sadrul wrote:
> I guess it's a bit unfortunate that you need to remember to manually do this
> before destruction. Would be nice if InkDropAnimation() took care of this
> itself.

It was intentional that the InkDropAnimation doesn't know anything about an
InkDropHost.  The InkDropAnimationController is responsible for managing the
lifetime of the InkDropAnimation instance.  I designed it this way to make it
easy to transition to a pool of InkDropAnimations, in which case the
InkDropAnimation destructors will not be executed when they should be detached
from the host.

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_state.h (right):

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_state.h:31: DEACTIVATED,
On 2015/09/14 20:05:41, sadrul wrote:
> These could've been in a separate smaller CL?

Fair point.

[sadrul, 2015-09-16 11:19:05]

lgtm

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller_impl.cc (right):

https://codereview.chromium.org/1298513003/diff/180001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller_impl.cc:65:
ink_drop_host_->RemoveInkDropLayer(ink_drop_animation_->root_layer());
On 2015/09/15 19:47:09, bruthig wrote:
> On 2015/09/14 20:05:41, sadrul wrote:
> > I guess it's a bit unfortunate that you need to remember to manually do this
> > before destruction. Would be nice if InkDropAnimation() took care of this
> > itself.
> 
> It was intentional that the InkDropAnimation doesn't know anything about an
> InkDropHost.  The InkDropAnimationController is responsible for managing the
> lifetime of the InkDropAnimation instance.  I designed it this way to make it
> easy to transition to a pool of InkDropAnimations, in which case the
> InkDropAnimation destructors will not be executed when they should be detached
> from the host.

OK. Sounds good.

[bruthig, 2015-09-16 12:09:22]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-09-16 12:09:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, tdanderson@chromium.org, jonross@chromium.org
Link to the patchset: https://codereview.chromium.org/1298513003/#ps200001
(title: "Addressed comments from patch set 10.")

[commit-bot: I haz the power, 2015-09-16 12:09:37]

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

[commit-bot: I haz the power, 2015-09-16 12:15:12]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-09-16 12:15:47]

Patchset 11 (id:??) landed as
https://crrev.com/8e8496ec69c203243767cec3014cd7232909e805
Cr-Commit-Position: refs/heads/master@{#349100}

[commit-bot: I haz the power, 2015-09-23 12:54:43]

Patchset 11 (id:??) landed as
https://crrev.com/8e8496ec69c203243767cec3014cd7232909e805
Cr-Commit-Position: refs/heads/master@{#349100}