Enabled ink drop highlight for focus on bookmark bar buttons.

Enabled ink drop highlight for focus on bookmark bar buttons.

This is fixing a regression caused by
https://codereview.chromium.org/2447523002.

BUG=666192
Committed: https://crrev.com/a57939b6193ccb89fc70aa624a6f29140efac422
Cr-Commit-Position: refs/heads/master@{#432994}

[bruthig, 2016-11-17 18:39:18]

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

[bruthig, 2016-11-17 18:39:46]

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

[bruthig, 2016-11-17 18:39:47]

pkasting@, can you take a quick look?

[commit-bot: I haz the power, 2016-11-17 18:40:15]

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

[Peter Kasting, 2016-11-17 19:31:57]

LGTM

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

https://codereview.chromium.org/2508243002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:222:
ink_drop->SetShowHighlightOnFocus(true);
Should CreateDefaultFloodFillInkDropImpl() do this by default?  Should it take
an arg as to whether the created ink drop should show highlight on focus, so
callers have to explicitly choose?

[bruthig, 2016-11-17 19:43:33]

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

https://codereview.chromium.org/2508243002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:222:
ink_drop->SetShowHighlightOnFocus(true);
On 2016/11/17 19:31:57, Peter Kasting wrote:
> Should CreateDefaultFloodFillInkDropImpl() do this by default?  Should it take
> an arg as to whether the created ink drop should show highlight on focus, so
> callers have to explicitly choose?

At a high level there is a lot of rework opportunities with how the InkDrop
parts are constructed.  I'm hoping to replace the virtual CreateInkDrop*()
functions in InkDropHost/InkDropHostView with a factory design pattern. This
will hopefully make it more obvious/explicit/etc to clients as to how the ink
drop is configured.

With respect to the specific request tho, UI elements that use the ink drop
highlight to show keyboard focus are quite special and there are really only 2
or 3 surfaces that do it, namely toolbar buttons and bookmark bar buttons.  I
wouldn't want to over burden all the other elements with an extra param for such
a special case.

I plan to leave it as is unless you have strong objections.

[commit-bot: I haz the power, 2016-11-17 21:29:17]

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

[commit-bot: I haz the power, 2016-11-17 21:29:17]

Dry run: This issue passed the CQ dry run.

[bruthig, 2016-11-17 21:32:41]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2016-11-17 21:35:01]

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

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

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-17 22:32:48]

Description was changed from

==========
Enabled ink drop highlight for focus on bookmark bar buttons.

This is fixing a regression caused by
https://codereview.chromium.org/2447523002.

BUG=666192
==========

to

==========
Enabled ink drop highlight for focus on bookmark bar buttons.

This is fixing a regression caused by
https://codereview.chromium.org/2447523002.

BUG=666192

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

[commit-bot: I haz the power, 2016-11-17 22:32:49]

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