MD Buttons: Don't add layers for hidden -> hidden transitions.

MD Buttons: Don't add layers for hidden -> hidden transitions.

InkDropHostView::VisibilityChanged() is invoked when never-transitioned
(or even never-shown) views are set hidden. This can happen with calls
to View::SetVisibility(false), Widget::SetVisible(false) or
Widget::Close*(). These Views may never be shown, so don't add a layer
to animate ink drops in these cases.

BUG=619798
Committed: https://crrev.com/2cb97f14a013d26b5139d2144537d591181a7f4e
Cr-Commit-Position: refs/heads/master@{#426345}

[tapted, 2016-10-18 00:29:35]

Description was changed from

==========
MD Buttons: Don't add layers for hidden -> hidden transitions.

InkDropHostView::VisibilityChanged() is invoked when never-shown views
are set hidden. This can happen with calls to View::SetVisibility(false),
Widget::SetVisible(false) or Widget::Close*(). These Views may never be
shown, so don't add a layer to animate ink drops in these cases.

BUG=619798
==========

to

==========
MD Buttons: Don't add layers for hidden -> hidden transitions.

InkDropHostView::VisibilityChanged() is invoked when never-transitioned
(or even never-shown) views are set hidden. This can happen with calls
to View::SetVisibility(false), Widget::SetVisible(false) or
Widget::Close*(). These Views may never be shown, so don't add a layer
to animate ink drops in these cases.

BUG=619798
==========

[tapted, 2016-10-18 00:29:35]

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

[tapted, 2016-10-18 00:32:56]

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

[commit-bot: I haz the power, 2016-10-18 00:33:34]

Dry run: 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-10-18 01:17:18]

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

[commit-bot: I haz the power, 2016-10-18 01:17:19]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-10-18 02:31:13]

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

[commit-bot: I haz the power, 2016-10-18 02:32:13]

Dry run: 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-10-18 03:15:29]

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

[commit-bot: I haz the power, 2016-10-18 03:15:29]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-10-18 03:25:26]

HI Ben, please take a look

[bruthig, 2016-10-18 14:31:27]

A couple of requests in regards to the tests, otherwise looks good.

https://codereview.chromium.org/2431493002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/custom_button_unittest.cc (right):

https://codereview.chromium.org/2431493002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/custom_button_unittest.cc:77: void
AddInkDropLayer(ui::Layer* ink_drop_layer) override {
Feel free to add this behavior to the TestInkDropHost instead.
See
https://cs.chromium.org/chromium/src/ui/views/animation/test/test_ink_drop_ho...

https://codereview.chromium.org/2431493002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/custom_button_unittest.cc:599: TEST_F(CustomButtonTest,
AlwaysHiddenInkDropHasNoLayers) {
I think this test would make more sense in InkDropImplTest and we should
probably add one that verifies a layer is not added when SetHover(false) is
called when the highlight is already hidden.

[tapted, 2016-10-19 00:55:42]

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

[tapted, 2016-10-19 01:33:07]

The CQ bit was unchecked by tapted@chromium.org

[commit-bot: I haz the power, 2016-10-19 01:33:54]

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

[tapted, 2016-10-19 01:35:15]

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

[commit-bot: I haz the power, 2016-10-19 01:36:02]

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

[tapted, 2016-10-19 02:59:32]

https://codereview.chromium.org/2431493002/diff/20001/ui/views/controls/butto...
File ui/views/controls/button/custom_button_unittest.cc (right):

https://codereview.chromium.org/2431493002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/custom_button_unittest.cc:77: void
AddInkDropLayer(ui::Layer* ink_drop_layer) override {
On 2016/10/18 14:31:26, bruthig wrote:
> Feel free to add this behavior to the TestInkDropHost instead.
> See
>
https://cs.chromium.org/chromium/src/ui/views/animation/test/test_ink_drop_ho...

Done.

https://codereview.chromium.org/2431493002/diff/20001/ui/views/controls/butto...
ui/views/controls/button/custom_button_unittest.cc:599: TEST_F(CustomButtonTest,
AlwaysHiddenInkDropHasNoLayers) {
On 2016/10/18 14:31:26, bruthig wrote:
> I think this test would make more sense in InkDropImplTest and we should
> probably add one that verifies a layer is not added when SetHover(false) is
> called when the highlight is already hidden.

Hm - I wanted to include coverage of the NativeWidgetMac codepath, since that
was where the DCHECK was being hit. I guess in reality that was during Close()
codepaths, but it was feeling a bit like test overkill. However, moving the
AnimateToState(hidden) stuff to InkDropImplTest makes it neater, so I've updated
the test here to test the Close() codepaths (e.g. it will now DCHECK on Mac
without the fix in ink_drop_impl.cc whereas the new InkDropImplTest will only
fail the test expecations) - see what you think.

[commit-bot: I haz the power, 2016-10-19 05:18:08]

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

[commit-bot: I haz the power, 2016-10-19 05:18:09]

Dry run: Exceeded global retry quota

[bruthig, 2016-10-19 13:48:50]

lgtm

[tapted, 2016-10-19 23:10:46]

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

[tapted, 2016-10-19 23:10:47]

+sky for OWNERS in  ui/views/controls/button/custom_button_unittest.cc

[sky, 2016-10-19 23:45:15]

LGTM

[tapted, 2016-10-19 23:47:06]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-10-19 23:47:43]

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-10-20 00:10:54]

Description was changed from

==========
MD Buttons: Don't add layers for hidden -> hidden transitions.

InkDropHostView::VisibilityChanged() is invoked when never-transitioned
(or even never-shown) views are set hidden. This can happen with calls
to View::SetVisibility(false), Widget::SetVisible(false) or
Widget::Close*(). These Views may never be shown, so don't add a layer
to animate ink drops in these cases.

BUG=619798
==========

to

==========
MD Buttons: Don't add layers for hidden -> hidden transitions.

InkDropHostView::VisibilityChanged() is invoked when never-transitioned
(or even never-shown) views are set hidden. This can happen with calls
to View::SetVisibility(false), Widget::SetVisible(false) or
Widget::Close*(). These Views may never be shown, so don't add a layer
to animate ink drops in these cases.

BUG=619798
==========

[commit-bot: I haz the power, 2016-10-20 00:10:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 13:13:34]

Description was changed from

==========
MD Buttons: Don't add layers for hidden -> hidden transitions.

InkDropHostView::VisibilityChanged() is invoked when never-transitioned
(or even never-shown) views are set hidden. This can happen with calls
to View::SetVisibility(false), Widget::SetVisible(false) or
Widget::Close*(). These Views may never be shown, so don't add a layer
to animate ink drops in these cases.

BUG=619798
==========

to

==========
MD Buttons: Don't add layers for hidden -> hidden transitions.

InkDropHostView::VisibilityChanged() is invoked when never-transitioned
(or even never-shown) views are set hidden. This can happen with calls
to View::SetVisibility(false), Widget::SetVisible(false) or
Widget::Close*(). These Views may never be shown, so don't add a layer
to animate ink drops in these cases.

BUG=619798

Committed: https://crrev.com/2cb97f14a013d26b5139d2144537d591181a7f4e
Cr-Commit-Position: refs/heads/master@{#426345}
==========

[commit-bot: I haz the power, 2016-10-21 13:13:35]

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