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

ui/views/controls/button/custom_button_unittest.cc

Index: ui/views/controls/button/custom_button_unittest.cc
diff --git a/ui/views/controls/button/custom_button_unittest.cc b/ui/views/controls/button/custom_button_unittest.cc
index 418ff6e1c6b28cafad81a4e267f4adb97d239460..544b9c436f4ba1c0b50b7cf6ecfbdeec6965e084 100644
--- a/ui/views/controls/button/custom_button_unittest.cc
+++ b/ui/views/controls/button/custom_button_unittest.cc
@@ -13,6 +13,7 @@
 #include "ui/events/event_utils.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/views/animation/ink_drop_host.h"
+#include "ui/views/animation/ink_drop_impl.h"
 #include "ui/views/animation/test/ink_drop_host_view_test_api.h"
 #include "ui/views/animation/test/test_ink_drop.h"
 #include "ui/views/animation/test/test_ink_drop_host.h"
@@ -72,8 +73,20 @@ class TestCustomButton : public CustomButton, public ButtonListener {
     canceled_ = true;
   }
 
+  // InkDropHostView:
+  void AddInkDropLayer(ui::Layer* ink_drop_layer) override {

[bruthig, 2016/10/18 14:31:26]

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...

[tapted, 2016/10/19 02:59:31]

Done.

+    ++ink_drop_layer_add_count_;
+    CustomButton::AddInkDropLayer(ink_drop_layer);
+  }
+  void RemoveInkDropLayer(ui::Layer* ink_drop_layer) override {
+    ++ink_drop_layer_remove_count_;
+    CustomButton::RemoveInkDropLayer(ink_drop_layer);
+  }
+
   bool pressed() { return pressed_; }
   bool canceled() { return canceled_; }
+  int ink_drop_layer_add_count() { return ink_drop_layer_add_count_; }
+  int ink_drop_layer_remove_count() { return ink_drop_layer_remove_count_; }
 
   void Reset() {
     pressed_ = false;
@@ -87,6 +100,9 @@ class TestCustomButton : public CustomButton, public ButtonListener {
   bool pressed_ = false;
   bool canceled_ = false;
 
+  int ink_drop_layer_add_count_ = 0;
+  int ink_drop_layer_remove_count_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(TestCustomButton);
 };
 
@@ -126,6 +142,14 @@ class CustomButtonTest : public ViewsTestBase {
     widget_->SetContentsView(button_);
   }
 
+  void CreateButtonWithRealInkDrop() {
+    delete button_;
+    button_ = new TestCustomButton(false);
+    InkDropHostViewTestApi(button_).SetInkDrop(
+        base::MakeUnique<InkDropImpl>(button_));
+    widget_->SetContentsView(button_);
+  }
+
  protected:
   Widget* widget() { return widget_.get(); }
   TestCustomButton* button() { return button_; }
@@ -571,6 +595,43 @@ TEST_F(CustomButtonTest, InkDropStaysHiddenWhileDragging) {
   SetDraggedView(nullptr);
 }
 
+// Test that hiding an already-hidden ink drop doesn't add layers.
+TEST_F(CustomButtonTest, AlwaysHiddenInkDropHasNoLayers) {

[bruthig, 2016/10/18 14:31:26]

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 02:59:31]

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.

+  CreateButtonWithRealInkDrop();
+
+  EXPECT_TRUE(button()->visible());
+  EXPECT_FALSE(button()->layer());
+
+  button()->SetVisible(false);
+  EXPECT_FALSE(button()->visible());
+
+  // The animation may immediately end and remove the layer. So count calls as
+  // well as checking for the presence of a layer.
+  EXPECT_FALSE(button()->layer());
+  EXPECT_EQ(0, button()->ink_drop_layer_add_count());
+  EXPECT_EQ(0, button()->ink_drop_layer_remove_count());
+
+  // Sanity check that making the button visible and clicking it should add a
+  // layer for the ripple.
+  button()->SetVisible(true);
+  EXPECT_TRUE(button()->visible());
+
+  // No expected change when just making the button visible.
+  EXPECT_FALSE(button()->layer());
+  EXPECT_EQ(0, button()->ink_drop_layer_add_count());
+  EXPECT_EQ(0, button()->ink_drop_layer_remove_count());
+
+  ui::test::EventGenerator generator(widget()->GetNativeWindow());
+  generator.PressLeftButton();
+  EXPECT_TRUE(button()->layer());
+  EXPECT_EQ(1, button()->ink_drop_layer_add_count());
+
+  // The animation is running here, so the layer won't be removed.
+  EXPECT_EQ(0, button()->ink_drop_layer_remove_count());
+
+  // This test doesn't wait for the animation to finish.
+}
+
 // Todo(karandeepb): On Mac, a button should get clicked on a Space key press
 // (and not release). Modify this test after fixing crbug.com/607429.
 // Test that Space Key behaves correctly on a focused button.