Add MD ink drop ripple to shelf app items

Add MD ink drop ripple to shelf app items

BUG=612539
Committed: https://crrev.com/497110a79cf9e4a9ffef70842055a0445310eaf3
Cr-Commit-Position: refs/heads/master@{#400228}

[mohsen, 2016-06-07 21:07:21]

Description was changed from

==========
Add ripple to shelf items

BUG=595009
==========

to

==========
Add ripple to shelf app items

BUG=595009
==========

[mohsen, 2016-06-07 21:07:44]

Description was changed from

==========
Add ripple to shelf app items

BUG=595009
==========

to

==========
Add ink drop ripple to shelf app items

BUG=595009
==========

[mohsen, 2016-06-07 21:13:06]

Patchset #4 (id:60001) has been deleted

[mohsen, 2016-06-07 21:13:13]

Patchset #3 (id:40001) has been deleted

[mohsen, 2016-06-07 21:16:34]

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

[mohsen, 2016-06-07 21:16:35]

Please take a look...

[mohsen, 2016-06-08 21:17:48]

Patchset #4 (id:100001) has been deleted

[mohsen, 2016-06-08 21:27:14]

I've rebased the CL on top off r398375. Please take a look...

[bruthig, 2016-06-09 03:26:00]

I think we should add some tests to validate the ink drop state.  I have some
ideas but we can discuss offline tomorrow.

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/overflow_but...
File ash/shelf/overflow_button.h (right):

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/overflow_but...
ash/shelf/overflow_button.h:21: OverflowButton(ShelfView* shelf_view);
explicit

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_button.cc
File ash/shelf/shelf_button.cc (right):

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_button...
ash/shelf/shelf_button.cc:514: bool ShelfButton::ShouldShowInkDropHighlight()
const {
I can't remember what we discussed in person.  Am I correct that we are
deferring creating a ShouldShowInkDropHighlight() method until we switch the
keyboard focus on the shelf to use the InkDropHighlight?

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:722: if (is_repost_event_)
The interplay of |is_repost_event_|, |last_pressed_index_|,
|closing_event_time|, and IsRepostEvent() is very difficult to follow.  I'm sure
this could be made much simpler, but perhaps it should not be included in this
change.

WDYT?

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1427: bool ShelfView::ButtonPressedImpl(views::Button*
sender,
Consider renaming ButtonPressed as ButtonPressedImpl and keeping the declaration
and definition in the same spot.  It will make the diff easier to read and will
save your name from showing up beside a lot of lines in git blame ;)

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1430: // Do not handle mouse release during drag.
Would it be possible to replace most of these quick returns with a call to
WillIgnoreEventOnButton()?
i.e.
if (sender == overflow_button_) {
  ...
}

if (WillIgnoreEventOnButton(...))
  return false;

{
  ScopedTargetRootWindow scoped_target(
...

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1460: return true;
Shouldn't this be a return false?

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1509:
ink_drop->AnimateToState(views::InkDropState::DEACTIVATED);
It might make sense to put the DEACTIVATED call in ShowListMenuForView().

https://codereview.chromium.org/2033553003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/bar_control_button.h (left):

https://codereview.chromium.org/2033553003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bar_control_button.h:19: class InkDropDelegate;
I guess I missed these, thanks for cleaning them up!

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

https://codereview.chromium.org/2033553003/diff/120001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.cc:128:
SetPaintToLayer(old_paint_to_layer_);
What's the use case for this?

[mohsen, 2016-06-09 21:24:51]

Please take another look...

(I'm adding some tests. Will upload when they're ready.)

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/overflow_but...
File ash/shelf/overflow_button.h (right):

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/overflow_but...
ash/shelf/overflow_button.h:21: OverflowButton(ShelfView* shelf_view);
On 2016/06/09 at 03:25:59, bruthig wrote:
> explicit

Thanks for catching this. Done.

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_button.cc
File ash/shelf/shelf_button.cc (right):

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_button...
ash/shelf/shelf_button.cc:514: bool ShelfButton::ShouldShowInkDropHighlight()
const {
On 2016/06/09 at 03:25:59, bruthig wrote:
> I can't remember what we discussed in person.  Am I correct that we are
deferring creating a ShouldShowInkDropHighlight() method until we switch the
keyboard focus on the shelf to use the InkDropHighlight?

Yes. We don't want hover effect on shelf buttons. For focus, I'm not changing
anything yet as I'm waiting for specs.

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:722: if (is_repost_event_)
On 2016/06/09 at 03:26:00, bruthig wrote:
> The interplay of |is_repost_event_|, |last_pressed_index_|,
|closing_event_time|, and IsRepostEvent() is very difficult to follow.  I'm sure
this could be made much simpler, but perhaps it should not be included in this
change.
> 
> WDYT?

Probably, similar names of |is_repost_event_| and IsRepostEvent() which have
different meanings make it confusing. Changed the former to
|is_repost_event_on_same_item_|. Does it look better?

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1427: bool ShelfView::ButtonPressedImpl(views::Button*
sender,
On 2016/06/09 at 03:25:59, bruthig wrote:
> Consider renaming ButtonPressed as ButtonPressedImpl and keeping the
declaration and definition in the same spot.  It will make the diff easier to
read and will save your name from showing up beside a lot of lines in git blame
;)

Right. I was trying to keep the ordering suggested by style guides (function
overrides after other functions). Now that I removed ButtonPressedImpl() (see
below), I moved ButtonPressed() definition back to its original location which
should make diff easier to read. The declaration can't be moved back as it needs
to be in the public section.

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1430: // Do not handle mouse release during drag.
On 2016/06/09 at 03:25:59, bruthig wrote:
> Would it be possible to replace most of these quick returns with a call to
WillIgnoreEventOnButton()?
> i.e.
> if (sender == overflow_button_) {
>   ...
> }
> 
> if (WillIgnoreEventOnButton(...))
>   return false;
> 
> {
>   ScopedTargetRootWindow scoped_target(
> ...

It was not possible because for the double-click case we should've returned true
rather than false. However, now that I removed ButtonPressedImpl() (see below),
it is possible. Done.

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1460: return true;
On 2016/06/09 at 03:26:00, bruthig wrote:
> Shouldn't this be a return false?

The return value of the function means whether the ink drop ripple is taken care
of or not. If not, we hide the ripple. HOWEVER, hiding the ripple is not needed
anymore (it was probably needed once, but as the patch developed, the need is
gone). So, I removed ButtonPressedImpl() helper altogether and moved the code
back to ButtonPressed().

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1509:
ink_drop->AnimateToState(views::InkDropState::DEACTIVATED);
On 2016/06/09 at 03:26:00, bruthig wrote:
> It might make sense to put the DEACTIVATED call in ShowListMenuForView().

Sure. Done.

https://codereview.chromium.org/2033553003/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/bar_control_button.h (left):

https://codereview.chromium.org/2033553003/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/bar_control_button.h:19: class InkDropDelegate;
On 2016/06/09 at 03:26:00, bruthig wrote:
> I guess I missed these, thanks for cleaning them up!

No problem!

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

https://codereview.chromium.org/2033553003/diff/120001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.cc:128:
SetPaintToLayer(old_paint_to_layer_);
On 2016/06/09 at 03:26:00, bruthig wrote:
> What's the use case for this?

I think if a View had already been painting to layer before ink drop layer was
added (e.g. ShelfButton), we should not set paint-to-layer false here.
Otherwise, it might break things!

PS: ShelfButton is set to paint to layer in ShelfView::Init().

[bruthig, 2016-06-10 15:25:20]

Looking good, I will await the tests, but you could probably go ahead and add an
OWNER.

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_button.cc
File ash/shelf/shelf_button.cc (right):

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_button...
ash/shelf/shelf_button.cc:514: bool ShelfButton::ShouldShowInkDropHighlight()
const {
On 2016/06/09 21:24:50, mohsen wrote:
> On 2016/06/09 at 03:25:59, bruthig wrote:
> > I can't remember what we discussed in person.  Am I correct that we are
> deferring creating a ShouldShowInkDropHighlight() method until we switch the
> keyboard focus on the shelf to use the InkDropHighlight?
> 
> Yes. We don't want hover effect on shelf buttons. For focus, I'm not changing
> anything yet as I'm waiting for specs.

Acknowledged.

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2033553003/diff/120001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:722: if (is_repost_event_)
On 2016/06/09 21:24:51, mohsen wrote:
> On 2016/06/09 at 03:26:00, bruthig wrote:
> > The interplay of |is_repost_event_|, |last_pressed_index_|,
> |closing_event_time|, and IsRepostEvent() is very difficult to follow.  I'm
sure
> this could be made much simpler, but perhaps it should not be included in this
> change.
> > 
> > WDYT?
> 
> Probably, similar names of |is_repost_event_| and IsRepostEvent() which have
> different meanings make it confusing. Changed the former to
> |is_repost_event_on_same_item_|. Does it look better?

Definitely looks better!

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

https://codereview.chromium.org/2033553003/diff/120001/ui/views/animation/ink...
ui/views/animation/ink_drop_host_view.cc:128:
SetPaintToLayer(old_paint_to_layer_);
On 2016/06/09 21:24:51, mohsen wrote:
> On 2016/06/09 at 03:26:00, bruthig wrote:
> > What's the use case for this?
> 
> I think if a View had already been painting to layer before ink drop layer was
> added (e.g. ShelfButton), we should not set paint-to-layer false here.
> Otherwise, it might break things!
> 
> PS: ShelfButton is set to paint to layer in ShelfView::Init().

Hmm VERY good point! It seems that this SetPaintToLayer() method is enabling
distributed ownership of whether the View should paint to a layer or not and is
tough to reason about and get correct.  Can you add a TODO, perhaps on
View::SetPaintToLayer(), to consider making it more robust for these use cases?
e.g. Adding a counter and only destroying the layer when the count gets to 0.

This change looks good for this CL though.

[mohsen, 2016-06-10 18:20:07]

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

[mohsen, 2016-06-10 18:20:08]

sky@: Can you take an OWNERS look?

[mohsen, 2016-06-13 15:57:05]

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

[mohsen, 2016-06-13 15:57:06]

jamescook@: Can you please take a look? sky@ is OOO for now.

[mohsen, 2016-06-13 16:20:07]

Added tests...

[mohsen, 2016-06-13 16:48:08]

Description was changed from

==========
Add ink drop ripple to shelf app items

BUG=595009
==========

to

==========
Add ink drop ripple to shelf app items

BUG=612539
==========

[James Cook, 2016-06-13 19:55:35]

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/overflow_but...
File ash/shelf/overflow_button.h (right):

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/overflow_but...
ash/shelf/overflow_button.h:55: ShelfView* shelf_view_;
Aside: This is fine for this CL, but in the medium-term I would like to reduce
the amount of interdependency between the various Shelf* classes. Specifically,
I'd like to move to WmShelf being the main "shelf controller" class that
everyone depends on.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (left):

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view.h...
ash/shelf/shelf_view.h:61: public views::ButtonListener,
optional: You could also introduce an InkDropButtonListener class that has the
ButtonPressed() signature you need. That would reduce the amount of coupling
with ShelfView and provide an example of what you want ButtonListener to look
like eventually.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (right):

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view.h...
ash/shelf/shelf_view.h:143: bool ShouldIgnoreEventOnButton(views::View* view,
const ui::Event& event);
Consider inverting the boolean sense of this and calling it
ShouldEventActivateButton() or something similar.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view.h...
ash/shelf/shelf_view.h:161: // synchronous and blocks the call. Fix this after
menu is converted to
Are you actually planning to convert menus to be async? Everywhere or just in
the shelf? If that's not actually on the radar for someone I would remove that
part of the comment.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2004: // Should match the smae constant in
shelf_view.cc.
I would expose the constant, either in shelf_view.h or a shared constants file
somewhere (shelf_constants.h?).

Also smae -> same

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2095: }  // namespace
nit: blank line above

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2099: ShelfViewInkDropTest() :
ink_drop_(nullptr) {}
initialize browser_button_ to nullptr?

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2114: ink_drop_ = nullptr;
optional: You could skip nulling these out.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2132: // Tests that clicking on a shelf item
that does not show a menu transitions ink
Thanks for documenting the purpose of each test. It makes things easier to
review, and easier for the next engineer.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2222: ui::MouseEvent
drag_event_indise(ui::ET_MOUSE_DRAGGED, press_location,
nit: indise -> inside

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2260:
base::Bind(&ShelfViewInkDropTest::CloseMenu, base::Unretained(this)));
You might add a comment below that a menu will be spawned on release.

[mohsen, 2016-06-14 06:24:39]

(I've also rebased the code which has messed up diff with previous patch sets in
some files a bit. Sorry about that!).

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/overflow_but...
File ash/shelf/overflow_button.h (right):

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/overflow_but...
ash/shelf/overflow_button.h:55: ShelfView* shelf_view_;
On 2016/06/13 at 19:55:35, James Cook wrote:
> Aside: This is fine for this CL, but in the medium-term I would like to reduce
the amount of interdependency between the various Shelf* classes. Specifically,
I'd like to move to WmShelf being the main "shelf controller" class that
everyone depends on.

I guess this should have been addressed by introduction of InkDropButtonListener
interface.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (left):

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view.h...
ash/shelf/shelf_view.h:61: public views::ButtonListener,
On 2016/06/13 at 19:55:35, James Cook wrote:
> optional: You could also introduce an InkDropButtonListener class that has the
ButtonPressed() signature you need. That would reduce the amount of coupling
with ShelfView and provide an example of what you want ButtonListener to look
like eventually.

Good idea. Done.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (right):

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view.h...
ash/shelf/shelf_view.h:143: bool ShouldIgnoreEventOnButton(views::View* view,
const ui::Event& event);
On 2016/06/13 at 19:55:35, James Cook wrote:
> Consider inverting the boolean sense of this and calling it
ShouldEventActivateButton() or something similar.

Done.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view.h...
ash/shelf/shelf_view.h:161: // synchronous and blocks the call. Fix this after
menu is converted to
On 2016/06/13 at 19:55:35, James Cook wrote:
> Are you actually planning to convert menus to be async? Everywhere or just in
the shelf? If that's not actually on the radar for someone I would remove that
part of the comment.

Yes, there is a plan to convert all menus to be async.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2004: // Should match the smae constant in
shelf_view.cc.
On 2016/06/13 at 19:55:35, James Cook wrote:
> I would expose the constant, either in shelf_view.h or a shared constants file
somewhere (shelf_constants.h?).
> 
> Also smae -> same

How about making it a private constant member of ShelfView that can be accessed
here via ShelfViewTestAPI class? (Done)

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2095: }  // namespace
On 2016/06/13 at 19:55:35, James Cook wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2099: ShelfViewInkDropTest() :
ink_drop_(nullptr) {}
On 2016/06/13 at 19:55:35, James Cook wrote:
> initialize browser_button_ to nullptr?

Oops. Done.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2114: ink_drop_ = nullptr;
On 2016/06/13 at 19:55:35, James Cook wrote:
> optional: You could skip nulling these out.

Done.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2222: ui::MouseEvent
drag_event_indise(ui::ET_MOUSE_DRAGGED, press_location,
On 2016/06/13 at 19:55:35, James Cook wrote:
> nit: indise -> inside

Done.

https://codereview.chromium.org/2033553003/diff/200001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2260:
base::Bind(&ShelfViewInkDropTest::CloseMenu, base::Unretained(this)));
On 2016/06/13 at 19:55:35, James Cook wrote:
> You might add a comment below that a menu will be spawned on release.

Sure. Done.

[bruthig, 2016-06-14 14:37:33]

Just a few minor things.  All in all though looks great!!  Those tests are
AWESOME!

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_button.cc
File ash/shelf/shelf_button.cc (right):

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_button...
ash/shelf/shelf_button.cc:56: const int kInkDropLargeCornerRadius = 4;
Can you add a kInkDropSmallCornerRadius here as well?  I'm planning to make
InkDropHostView::CreateInkDropRipple() return a FloodFill style by default and
remove InkDropHostView::kInkDropSmallCornerRadius.

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1711: // None of the checks in
|ShouldEventActivateButton| affects overflow button.
nit: |ShouldEventActivateButton| -> ShouldEventActivateButton()

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (right):

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view.h...
ash/shelf/shelf_view.h:143: // Returns true if the event on the shelf item is
going to activate the item.
nit: event -> |event|

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2014: // of state changes requested on it.
Can you just add a note that the InkDropSpy will not track any auto transitions
performed by the contained |ink_drop|.

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2109: void SetUp() override {
We should probably initialize the MaterialDesignController just so any mode
checks are consistent with the button having an ink drop.

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2122:
ShelfViewTestAPI(ShelfTestAPI(shelf).shelf_view()).CloseMenu();
I think |shelf_view_| and |test_api_| are accessible in ShelfViewTest.

[James Cook, 2016-06-14 16:34:29]

LGTM for OWNERS. Please address bruthig's feedback before landing.

In the future, please upload a rebase as a separate patchset, either before or
after your patchset with changes. That makes it easier for me to review the
diffs. Thanks!

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/ink_drop_but...
File ash/shelf/ink_drop_button_listener.h (right):

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/ink_drop_but...
ash/shelf/ink_drop_button_listener.h:35:
DISALLOW_COPY_AND_ASSIGN(InkDropButtonListener);
nit: You don't need this if you have a pure virtual method. Nor do you need the
constructor.

[mohsen, 2016-06-14 20:40:31]

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/ink_drop_but...
File ash/shelf/ink_drop_button_listener.h (right):

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/ink_drop_but...
ash/shelf/ink_drop_button_listener.h:35:
DISALLOW_COPY_AND_ASSIGN(InkDropButtonListener);
On 2016/06/14 at 16:34:28, James Cook wrote:
> nit: You don't need this if you have a pure virtual method. Nor do you need
the constructor.

Done.

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_button.cc
File ash/shelf/shelf_button.cc (right):

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_button...
ash/shelf/shelf_button.cc:56: const int kInkDropLargeCornerRadius = 4;
On 2016/06/14 at 14:37:33, bruthig wrote:
> Can you add a kInkDropSmallCornerRadius here as well?  I'm planning to make
InkDropHostView::CreateInkDropRipple() return a FloodFill style by default and
remove InkDropHostView::kInkDropSmallCornerRadius.

If I add it here, the name will clash with the existing one in InkDropHostView.
It's probably better to add it here whenever it is removed there.

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1711: // None of the checks in
|ShouldEventActivateButton| affects overflow button.
On 2016/06/14 at 14:37:33, bruthig wrote:
> nit: |ShouldEventActivateButton| -> ShouldEventActivateButton()

Done.

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view.h
File ash/shelf/shelf_view.h (right):

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view.h...
ash/shelf/shelf_view.h:143: // Returns true if the event on the shelf item is
going to activate the item.
On 2016/06/14 at 14:37:33, bruthig wrote:
> nit: event -> |event|

Done.

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2014: // of state changes requested on it.
On 2016/06/14 at 14:37:33, bruthig wrote:
> Can you just add a note that the InkDropSpy will not track any auto
transitions performed by the contained |ink_drop|.

Done.

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2109: void SetUp() override {
On 2016/06/14 at 14:37:33, bruthig wrote:
> We should probably initialize the MaterialDesignController just so any mode
checks are consistent with the button having an ink drop.

Right. Done.

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2122:
ShelfViewTestAPI(ShelfTestAPI(shelf).shelf_view()).CloseMenu();
On 2016/06/14 at 14:37:33, bruthig wrote:
> I think |shelf_view_| and |test_api_| are accessible in ShelfViewTest.

Done. Also removed CloseMenu() helper to run CloseMenu() directly on
|test_api_|.

[mohsen, 2016-06-14 21:33:38]

Add a TODO I forgot to add in the previous patchset.

[bruthig, 2016-06-15 14:21:40]

Minor nit, but LGTM

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_button.cc
File ash/shelf/shelf_button.cc (right):

https://codereview.chromium.org/2033553003/diff/240001/ash/shelf/shelf_button...
ash/shelf/shelf_button.cc:56: const int kInkDropLargeCornerRadius = 4;
On 2016/06/14 20:40:30, mohsen wrote:
> On 2016/06/14 at 14:37:33, bruthig wrote:
> > Can you add a kInkDropSmallCornerRadius here as well?  I'm planning to make
> InkDropHostView::CreateInkDropRipple() return a FloodFill style by default and
> remove InkDropHostView::kInkDropSmallCornerRadius.
> 
> If I add it here, the name will clash with the existing one in
InkDropHostView.
> It's probably better to add it here whenever it is removed there.

Acknowledged.

https://codereview.chromium.org/2033553003/diff/280001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2033553003/diff/280001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1716: int view_index =
view_model_->GetIndexOfView(sender);
nit: You should be able to delete the |view_index| variable and just initialize
|last_pressed_index_| here.

[mohsen, 2016-06-15 16:38:43]

mohsen@chromium.org changed reviewers:
+ msw@chromium.org, sadrul@chromium.org

[mohsen, 2016-06-15 16:38:44]

+msw@: For OWNERS in chrome/browser/ui/views...
+sadrul@: For OWNERS in ui/views...

https://codereview.chromium.org/2033553003/diff/280001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2033553003/diff/280001/ash/shelf/shelf_view.c...
ash/shelf/shelf_view.cc:1716: int view_index =
view_model_->GetIndexOfView(sender);
On 2016/06/15 at 14:21:40, bruthig wrote:
> nit: You should be able to delete the |view_index| variable and just
initialize |last_pressed_index_| here.

Right. Done.

[msw, 2016-06-15 17:37:06]

c/b/ui/views lgtm

[mohsen, 2016-06-16 00:06:20]

Description was changed from

==========
Add ink drop ripple to shelf app items

BUG=612539
==========

to

==========
Add MD ink drop ripple to shelf app items

BUG=612539
==========

[sadrul, 2016-06-16 15:56:27]

Remove the TODO from view.h. With that, lgtm

https://codereview.chromium.org/2033553003/diff/320001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2033553003/diff/320001/ui/views/view.h#newcod...
ui/views/view.h:330: // painting to layer.
We should not do this here. Normally, there would be just one place where this
decision is made for a View. This extra checks should live in the places where
something else also needs to affect this decision.

[mohsen, 2016-06-16 16:26:26]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2033553003/340001

[mohsen, 2016-06-16 16:28:47]

https://codereview.chromium.org/2033553003/diff/320001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/2033553003/diff/320001/ui/views/view.h#newcod...
ui/views/view.h:330: // painting to layer.
On 2016/06/16 at 15:56:27, sadrul wrote:
> We should not do this here. Normally, there would be just one place where this
decision is made for a View. This extra checks should live in the places where
something else also needs to affect this decision.

OK. Removed.

[commit-bot: I haz the power, 2016-06-16 17:28:43]

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

[commit-bot: I haz the power, 2016-06-16 17:28:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mohsen, 2016-06-16 18:59:26]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-06-16 18:59:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, bruthig@chromium.org, msw@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2033553003/#ps340001
(title: "Removed TODO")

[commit-bot: I haz the power, 2016-06-16 18:59:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2033553003/340001

[commit-bot: I haz the power, 2016-06-16 19:29:44]

Committed patchset #15 (id:340001)

[commit-bot: I haz the power, 2016-06-16 19:30:04]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 19:35:39]

Description was changed from

==========
Add MD ink drop ripple to shelf app items

BUG=612539
==========

to

==========
Add MD ink drop ripple to shelf app items

BUG=612539

Committed: https://crrev.com/497110a79cf9e4a9ffef70842055a0445310eaf3
Cr-Commit-Position: refs/heads/master@{#400228}
==========

[commit-bot: I haz the power, 2016-06-16 19:35:41]

Patchset 15 (id:??) landed as
https://crrev.com/497110a79cf9e4a9ffef70842055a0445310eaf3
Cr-Commit-Position: refs/heads/master@{#400228}