Added ink drop hover/ripple to menu hosting bookmark buttons.

Added ink drop hover/ripple to menu hosting bookmark buttons.

BUG=592058
TEST=InkDropAnimationControllerImplTest.*
TEST=InkDropAnimationTest.*
TEST=CustomButtonTest.InkDropAfterShowingContextMenu
TEST=CustomButtonTest.InkDropAfterTryingToShowContextMenu
TEST=Manual

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

[bruthig, 2016-03-03 21:00:07]

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

[bruthig, 2016-03-03 21:00:08]

varkha@, can you take a first look?
I plan to have a look at it myself to see if any tests can/should be added.

[bruthig, 2016-03-08 17:39:41]

Description was changed from

==========
Added ink drop hover/ripple to activate-able bookmark buttons.

BUG=551641, 523058, 523055
TEST=Manual
==========

to

==========
Added ink drop hover/ripple to activate-able bookmark buttons.

BUG=551641, 523058, 523055
TEST=InkDropAnimationTest.SnapToActivatedWithoutActiveAnimations
TEST=InkDropAnimationTest.SnapToActivatedWithActiveAnimations
TEST=Manual
==========

[bruthig, 2016-03-09 17:32:02]

Description was changed from

==========
Added ink drop hover/ripple to activate-able bookmark buttons.

BUG=551641, 523058, 523055
TEST=InkDropAnimationTest.SnapToActivatedWithoutActiveAnimations
TEST=InkDropAnimationTest.SnapToActivatedWithActiveAnimations
TEST=Manual
==========

to

==========
Added ink drop hover/ripple to activate-able bookmark buttons.

BUG=592058
TEST=InkDropAnimationTest.SnapToActivatedWithoutActiveAnimations
TEST=InkDropAnimationTest.SnapToActivatedWithActiveAnimations
TEST=Manual
==========

[varkha, 2016-03-11 17:57:17]

General question: will we need a snap treatment for other states?

https://codereview.chromium.org/1757993004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/1757993004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:245: return
GetThemeProvider()
When is there no ThemeProvider? Maybe a comment.

https://codereview.chromium.org/1757993004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:319: //
BookmarkFolderButton -------------------------------------------------------
Move this down and add another section.

https://codereview.chromium.org/1757993004/diff/160001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller.h (right):

https://codereview.chromium.org/1757993004/diff/160001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller.h:40: // Immediately snaps the
InkDropState to ACTIVATED.
Maybe some comment why it is necessary to treat ACTIVATED state differently.

https://codereview.chromium.org/1757993004/diff/160001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/1757993004/diff/160001/ui/views/controls/butt...
ui/views/controls/button/menu_button.h:112: void IncrementPressedLocked(bool
animate_ink_drop);
I think it will be clearer if you reverse the parameter and pass in bool
snap_ink_drop_immediately.

https://codereview.chromium.org/1757993004/diff/160001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/1757993004/diff/160001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller.cc:467: pressed_lock_.reset(new
MenuButton::PressedLock(button, true));
nit: Some better readability on the true / false arguments, maybe by passing in
a const variable with a clear name.

[bruthig, 2016-03-11 17:59:39]

Description was changed from

==========
Added ink drop hover/ripple to activate-able bookmark buttons.

BUG=592058
TEST=InkDropAnimationTest.SnapToActivatedWithoutActiveAnimations
TEST=InkDropAnimationTest.SnapToActivatedWithActiveAnimations
TEST=Manual
==========

to

==========
Added ink drop hover/ripple to activate-able bookmark buttons.

BUG=592058

TEST=WIP
TEST=InkDropAnimationTest.SnapToActivatedWithoutActiveAnimations
TEST=InkDropAnimationTest.SnapToActivatedWithActiveAnimations
TEST=Manual
==========

[bruthig, 2016-03-11 18:02:32]

Description was changed from

==========
Added ink drop hover/ripple to activate-able bookmark buttons.

BUG=592058

TEST=WIP
TEST=InkDropAnimationTest.SnapToActivatedWithoutActiveAnimations
TEST=InkDropAnimationTest.SnapToActivatedWithActiveAnimations
TEST=Manual
==========

to

==========
Added ink drop hover/ripple to menu hosting bookmark buttons.

BUG=592058

TEST=WIP
TEST=InkDropAnimationTest.SnapToActivatedWithoutActiveAnimations
TEST=InkDropAnimationTest.SnapToActivatedWithActiveAnimations
TEST=Manual
==========

[bruthig, 2016-03-11 18:02:42]

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

[bruthig, 2016-03-11 18:02:43]

sky@, can you please have a first look at:
 - chrome/browser/*
 - ui/views/controls/*
 
Please note that I am in the process of adding tests.

[bruthig, 2016-03-11 19:20:59]

Description was changed from

==========
Added ink drop hover/ripple to menu hosting bookmark buttons.

BUG=592058

TEST=WIP
TEST=InkDropAnimationTest.SnapToActivatedWithoutActiveAnimations
TEST=InkDropAnimationTest.SnapToActivatedWithActiveAnimations
TEST=Manual
==========

to

==========
Added ink drop hover/ripple to menu hosting bookmark buttons.

BUG=592058

TEST=InkDropAnimationControllerImplTest.*
TEST=InkDropAnimationTest.*
TEST=CustomButtonTest.InkDropAfterShowingContextMenu
TEST=CustomButtonTest.InkDropAfterTryingToShowContextMenu
TEST=Manual
==========

[sky, 2016-03-11 20:41:33]

https://codereview.chromium.org/1757993004/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/1757993004/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:213: View*
GetTooltipHandlerForPoint(const gfx::Point& point) override {
I think you need this override in BookmarkMenuButtonBase.

https://codereview.chromium.org/1757993004/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:226: bool
IsTriggerableEvent(const ui::Event& e) override {
And I think you need this one too.

https://codereview.chromium.org/1757993004/diff/180001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1757993004/diff/180001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:51:
menu_button_->IncrementPressedLocked(true);
use delegating constructor.

https://codereview.chromium.org/1757993004/diff/180001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/1757993004/diff/180001/ui/views/controls/butt...
ui/views/controls/button/menu_button.h:35: PressedLock(MenuButton* menu_button,
bool first_menu_show);
Rather than explicitly supplying the state can you determine it from whether
there is a MenuController around?

[bruthig, 2016-03-11 21:58:21]

> General question: will we need a snap treatment for other states?

Maybe, but it wasn't worth the effort to add this in now.

varkha@, sky@, I have addressed your comments, PTAL

https://codereview.chromium.org/1757993004/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/1757993004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:245: return
GetThemeProvider()
On 2016/03/11 17:57:17, varkha wrote:
> When is there no ThemeProvider? Maybe a comment.

Done.

https://codereview.chromium.org/1757993004/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:319: //
BookmarkFolderButton -------------------------------------------------------
On 2016/03/11 17:57:17, varkha wrote:
> Move this down and add another section.

Done.

https://codereview.chromium.org/1757993004/diff/160001/ui/views/animation/ink...
File ui/views/animation/ink_drop_animation_controller.h (right):

https://codereview.chromium.org/1757993004/diff/160001/ui/views/animation/ink...
ui/views/animation/ink_drop_animation_controller.h:40: // Immediately snaps the
InkDropState to ACTIVATED.
On 2016/03/11 17:57:17, varkha wrote:
> Maybe some comment why it is necessary to treat ACTIVATED state differently.

Done.

https://codereview.chromium.org/1757993004/diff/160001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/1757993004/diff/160001/ui/views/controls/butt...
ui/views/controls/button/menu_button.h:112: void IncrementPressedLocked(bool
animate_ink_drop);
On 2016/03/11 17:57:17, varkha wrote:
> I think it will be clearer if you reverse the parameter and pass in bool
> snap_ink_drop_immediately.

Done.

https://codereview.chromium.org/1757993004/diff/160001/ui/views/controls/menu...
File ui/views/controls/menu/menu_controller.cc (right):

https://codereview.chromium.org/1757993004/diff/160001/ui/views/controls/menu...
ui/views/controls/menu/menu_controller.cc:467: pressed_lock_.reset(new
MenuButton::PressedLock(button, true));
On 2016/03/11 17:57:17, varkha wrote:
> nit: Some better readability on the true / false arguments, maybe by passing
in
> a const variable with a clear name.

Done.

https://codereview.chromium.org/1757993004/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/1757993004/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:213: View*
GetTooltipHandlerForPoint(const gfx::Point& point) override {
On 2016/03/11 20:41:32, sky wrote:
> I think you need this override in BookmarkMenuButtonBase.

It's not clear to me why this would be needed. The two classes that extend
BookmarkMenuButtonBase previously extended MenuButton (not BookmarkButtonBase)
and this function was not in the call chain.

FTR Tooltips appear to display fine for BookmarkFolderButtons with this change,
and MenuButton has it's own implementation of IsTriggerableEvent().

Is this request unrelated to my change?

https://codereview.chromium.org/1757993004/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:226: bool
IsTriggerableEvent(const ui::Event& e) override {
On 2016/03/11 20:41:32, sky wrote:
> And I think you need this one too.

similar to above.

https://codereview.chromium.org/1757993004/diff/180001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.cc (right):

https://codereview.chromium.org/1757993004/diff/180001/ui/views/controls/butt...
ui/views/controls/button/menu_button.cc:51:
menu_button_->IncrementPressedLocked(true);
On 2016/03/11 20:41:32, sky wrote:
> use delegating constructor.

Done.

https://codereview.chromium.org/1757993004/diff/180001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/1757993004/diff/180001/ui/views/controls/butt...
ui/views/controls/button/menu_button.h:35: PressedLock(MenuButton* menu_button,
bool first_menu_show);
On 2016/03/11 20:41:32, sky wrote:
> Rather than explicitly supplying the state can you determine it from whether
> there is a MenuController around?

Naively, I'm not aware of any way for a MenuButton to determine if a
MenuController is around and/or active.  But even if it were possible it depends
more on whether the button is becoming pressed due to the MenuController::Run()
method or the MenuController::ShowSiblingMenu(). AFAICT the
MenuButton::PressedLock is the glue between the MenuController and the
MenuButton.

[sky, 2016-03-11 23:11:14]

LGTM

https://codereview.chromium.org/1757993004/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/1757993004/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:226: bool
IsTriggerableEvent(const ui::Event& e) override {
On 2016/03/11 21:58:20, bruthig wrote:
> On 2016/03/11 20:41:32, sky wrote:
> > And I think you need this one too.
> 
> similar to above.

You are right, ignore me.

https://codereview.chromium.org/1757993004/diff/180001/ui/views/controls/butt...
File ui/views/controls/button/menu_button.h (right):

https://codereview.chromium.org/1757993004/diff/180001/ui/views/controls/butt...
ui/views/controls/button/menu_button.h:35: PressedLock(MenuButton* menu_button,
bool first_menu_show);
On 2016/03/11 21:58:20, bruthig wrote:
> On 2016/03/11 20:41:32, sky wrote:
> > Rather than explicitly supplying the state can you determine it from whether
> > there is a MenuController around?
> 
> Naively, I'm not aware of any way for a MenuButton to determine if a
> MenuController is around and/or active.  But even if it were possible it
depends
> more on whether the button is becoming pressed due to the
MenuController::Run()
> method or the MenuController::ShowSiblingMenu(). AFAICT the
> MenuButton::PressedLock is the glue between the MenuController and the
> MenuButton.

MenuController::GetActiveInstance, but you're right on the sibling part. Ok.

[bruthig, 2016-03-11 23:12:27]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2016-03-11 23:13:17]

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

[commit-bot: I haz the power, 2016-03-11 23:54:02]

Description was changed from

==========
Added ink drop hover/ripple to menu hosting bookmark buttons.

BUG=592058

TEST=InkDropAnimationControllerImplTest.*
TEST=InkDropAnimationTest.*
TEST=CustomButtonTest.InkDropAfterShowingContextMenu
TEST=CustomButtonTest.InkDropAfterTryingToShowContextMenu
TEST=Manual
==========

to

==========
Added ink drop hover/ripple to menu hosting bookmark buttons.

BUG=592058

TEST=InkDropAnimationControllerImplTest.*
TEST=InkDropAnimationTest.*
TEST=CustomButtonTest.InkDropAfterShowingContextMenu
TEST=CustomButtonTest.InkDropAfterTryingToShowContextMenu
TEST=Manual
==========

[commit-bot: I haz the power, 2016-03-11 23:54:03]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-11 23:56:59]

Description was changed from

==========
Added ink drop hover/ripple to menu hosting bookmark buttons.

BUG=592058

TEST=InkDropAnimationControllerImplTest.*
TEST=InkDropAnimationTest.*
TEST=CustomButtonTest.InkDropAfterShowingContextMenu
TEST=CustomButtonTest.InkDropAfterTryingToShowContextMenu
TEST=Manual
==========

to

==========
Added ink drop hover/ripple to menu hosting bookmark buttons.

BUG=592058

TEST=InkDropAnimationControllerImplTest.*
TEST=InkDropAnimationTest.*
TEST=CustomButtonTest.InkDropAfterShowingContextMenu
TEST=CustomButtonTest.InkDropAfterTryingToShowContextMenu
TEST=Manual

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

[commit-bot: I haz the power, 2016-03-11 23:57:00]

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