Added the Material Design ink drop ripple effect to the navigation buttons in the browser toolbar.

Added the Material Design ink drop ripple effect to the navigation buttons in the browser toolbar.

The ripple effect is behind the 'top-chrome-md' experimental switch.

BUG=505587
Committed: https://crrev.com/6839695e30402d0ea6324a245a956807aeb5bdb7
Cr-Commit-Position: refs/heads/master@{#339650}

[bruthig, 2015-07-17 17:41:02]

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

[bruthig, 2015-07-17 17:41:02]

pkasting@, can you PTAL?

[Peter Kasting, 2015-07-17 18:01:58]

https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:34:
ink_drop_animation_controller_(nullptr),
Omit this.

https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:36: #if defined(OS_CHROMEOS)
Why is this OS_CHROMEOS?  Can't we do this for all platforms when mode ==
material?

https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:40:
layer()->SetFillsBoundsOpaquely(false);
Why do we need this if it's unconditionally done on line 55?

https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:163: if
(ui::MaterialDesignController::IsModeMaterial()) {
I suggest adding a comment on this block as to why we need to null the border
painters in material mode.

[bruthig, 2015-07-20 14:53:07]

pkasting@, can you PTAL?

https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):

https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:34:
ink_drop_animation_controller_(nullptr),
On 2015/07/17 18:01:58, Peter Kasting wrote:
> Omit this.

Done.

https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:36: #if defined(OS_CHROMEOS)
On 2015/07/17 18:01:58, Peter Kasting wrote:
> Why is this OS_CHROMEOS?  Can't we do this for all platforms when mode ==
> material?

The designers want the ripple effect on Chrome OS only for the time being. 
There is concern that it will conflict with operating system touch feedback on
other OS's such as Windows 10.

https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:40:
layer()->SetFillsBoundsOpaquely(false);
On 2015/07/17 18:01:58, Peter Kasting wrote:
> Why do we need this if it's unconditionally done on line 55?

Line 55 is operating on image() whereas this is operating on layer().

I have moved them both here so as to save a layer when running in non-material
mode.

https://codereview.chromium.org/1242573005/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_button.cc:163: if
(ui::MaterialDesignController::IsModeMaterial()) {
On 2015/07/17 18:01:58, Peter Kasting wrote:
> I suggest adding a comment on this block as to why we need to null the border
> painters in material mode.

Done.

[Peter Kasting, 2015-07-20 19:15:59]

lgtm

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

https://codereview.chromium.org/1242573005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:35: #if defined(OS_CHROMEOS)
Can you at least add a comment here about why we're only doing this on CrOS?  If
there is a plan to revisit this decision in the future a TODO for that might be
warranted as well.

[bruthig, 2015-07-21 13:40:41]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2015-07-21 13:40:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1242573005/#ps40001
(title: "Added comment as to why ripple was on ChromeOS only.")

[commit-bot: I haz the power, 2015-07-21 13:41:00]

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

[bruthig, 2015-07-21 13:41:03]

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

https://codereview.chromium.org/1242573005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:35: #if defined(OS_CHROMEOS)
On 2015/07/20 19:15:59, Peter Kasting wrote:
> Can you at least add a comment here about why we're only doing this on CrOS? 
If
> there is a plan to revisit this decision in the future a TODO for that might
be
> warranted as well.

Done.

[commit-bot: I haz the power, 2015-07-21 14:50:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-21 14:51:29]

Patchset 3 (id:??) landed as
https://crrev.com/6839695e30402d0ea6324a245a956807aeb5bdb7
Cr-Commit-Position: refs/heads/master@{#339650}

[Evan Stade, 2015-07-22 16:29:45]

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

[Evan Stade, 2015-07-22 16:29:46]

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

https://codereview.chromium.org/1242573005/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:174: border->SetPainter(true,
Button::STATE_PRESSED, nullptr);
for my edification, is it intentional that this also removes the pressed state
for pointer events, even though the inkdrop only works for touch events? (Is the
inkdrop going to work with pointer events as well?)

[bruthig, 2015-07-23 14:28:14]

On 2015/07/22 16:29:46, Evan Stade wrote:
>
https://codereview.chromium.org/1242573005/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/toolbar/toolbar_button.cc (right):
> 
>
https://codereview.chromium.org/1242573005/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/toolbar/toolbar_button.cc:174:
border->SetPainter(true,
> Button::STATE_PRESSED, nullptr);
> for my edification, is it intentional that this also removes the pressed state
> for pointer events, even though the inkdrop only works for touch events? (Is
the
> inkdrop going to work with pointer events as well?)

Yes x2, this was intentional and the ink drop will eventually be activated for
pointer events as well as touch events.  Admittedly this does look a little
weird currently.