Convert ToolbarActionsBarBubbleViews to BubbleDialogDelegate

chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc

Index: chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc
diff --git a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc
index 16f59b24fa91fd7cef37b231a289b8257c4b186a..f4abed0996e0b4cbf38a00d95a7facfe8a56501f 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views_unittest.cc
@@ -2,28 +2,19 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h"
+
 #include "base/memory/scoped_ptr.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/ui/toolbar/test_toolbar_actions_bar_bubble_delegate.h"
-#include "chrome/browser/ui/views/toolbar/toolbar_actions_bar_bubble_views.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/test/event_generator.h"
-#include "ui/views/bubble/bubble_delegate.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/test/test_widget_observer.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/widget/widget.h"
-
-namespace {
-
-gfx::Point GetCenterInScreenCoordinates(const views::View* view) {
-  gfx::Point center(view->width() / 2, view->height() / 2);
-  views::View::ConvertPointToScreen(view, &center);
-  return center;
-}
-
-}  // namespace
+#include "ui/views/window/dialog_client_view.h"
 
 class ToolbarActionsBarBubbleViewsTest : public views::ViewsTestBase {
  protected:
@@ -48,16 +39,11 @@ TEST_F(ToolbarActionsBarBubbleViewsTest, TestBubbleLayoutActionButton) {
                                                ActionString());
   ToolbarActionsBarBubbleViews* bubble = new ToolbarActionsBarBubbleViews(
       anchor_widget->GetContentsView(), delegate.GetDelegate());
-  views::BubbleDelegateView::CreateBubble(bubble);
+  views::BubbleDialogDelegateView::CreateBubble(bubble);
   bubble->Show();
 
-  EXPECT_TRUE(bubble->heading_label());
-  EXPECT_EQ(HeadingString(), bubble->heading_label()->text());

[Devlin, 2016/04/13 00:38:58]

thinking about this, I'd prefer to keep the text checks here.  I'm sure
BubbleDialogDelegateView probably has them, too, but this also serves to test
that the delegate passes up the right string for each button.  And, really, it's
just a super cheap check in a unittest, so I'd say that it's worth it.

[Evan Stade, 2016/04/13 15:31:03]

Here is my thinking:

I'm not a fan of unit tests that just parrot the implementation because I don't
think they add any security against regressions, they just make it twice as much
manual effort to make any code changes. EXPECT_EQ(HeadingString(),
bubble->GetWindowTitle()) is an example of such parroting. The cost is not in
runtime but in reading and maintaining the tests. By distilling the test to the
two important EXPECTs I think clarity improves.

The second cost is that you have to expose content_label() just for the purposes
of testing. This makes the prod code harder to grok because it's not clear where
content_label() is used (even if you put "// just for testing", people may not
read the comments and it adds clutter to the header file).

[Devlin, 2016/04/13 16:33:40]

I agree with the spirit (i.e., change detector tests are bad), but I think that
there's enough intermediary steps here that make it worthwhile.  For instance, I
don't think we should check
EXPECT_EQ(HeadingString(), delegate.GetHeadingString()),
but checking the button text checks that the right text is returned and applied
for the right button, which, while not complicated, is enough of a path that I
think it's worth testing, and that if it ever changes, the test should be
updated.
I'm a little less worried about this.  For one thing, codesearch helps here
since you can just check the xrefs.  If you wanted, you could also rename the
accessors to be content_label_for_testing().  Or, I know that sky@ often favors
just friending the test class and not exposing them generally, which is also
fine.  And, at the end of the day, exposing
const Label* content_label() const
really isn't that dangerous. ;)

[Evan Stade, 2016/04/13 22:10:39]

OK, I restored the button text checks.
cs xrefs miss a lot (stuff used only on windows or mac). And just having a
content_label_ member var (instead of local var) makes me think "hmm this must
be referenced somewhere besides construction time", when that's false.

-  EXPECT_TRUE(bubble->content_label());
-  EXPECT_EQ(BodyString(), bubble->content_label()->text());
-  EXPECT_TRUE(bubble->action_button());
-  EXPECT_EQ(ActionString(), bubble->action_button()->GetText());
-  EXPECT_FALSE(bubble->dismiss_button());
+  EXPECT_TRUE(bubble->GetDialogClientView()->ok_button());
+  EXPECT_FALSE(bubble->GetDialogClientView()->cancel_button());
 
   bubble->GetWidget()->Close();
   base::RunLoop().RunUntilIdle();
@@ -71,17 +57,11 @@ TEST_F(ToolbarActionsBarBubbleViewsTest,
   delegate.set_dismiss_button_text(DismissString());
   ToolbarActionsBarBubbleViews* bubble = new ToolbarActionsBarBubbleViews(
       anchor_widget->GetContentsView(), delegate.GetDelegate());
-  views::BubbleDelegateView::CreateBubble(bubble);
+  views::BubbleDialogDelegateView::CreateBubble(bubble);
   bubble->Show();
 
-  EXPECT_TRUE(bubble->heading_label());
-  EXPECT_EQ(HeadingString(), bubble->heading_label()->text());
-  EXPECT_TRUE(bubble->content_label());
-  EXPECT_EQ(BodyString(), bubble->content_label()->text());
-  EXPECT_TRUE(bubble->action_button());
-  EXPECT_EQ(ActionString(), bubble->action_button()->GetText());
-  EXPECT_TRUE(bubble->dismiss_button());
-  EXPECT_EQ(DismissString(), bubble->dismiss_button()->GetText());
+  EXPECT_TRUE(bubble->GetDialogClientView()->ok_button());
+  EXPECT_TRUE(bubble->GetDialogClientView()->cancel_button());
 
   bubble->GetWidget()->Close();
   base::RunLoop().RunUntilIdle();
@@ -98,13 +78,13 @@ TEST_F(ToolbarActionsBarBubbleViewsTest, TestShowAndCloseBubble) {
   EXPECT_FALSE(delegate.shown());
   EXPECT_FALSE(delegate.close_action());
   views::Widget* bubble_widget =
-      views::BubbleDelegateView::CreateBubble(bubble);
+      views::BubbleDialogDelegateView::CreateBubble(bubble);
   views::test::TestWidgetObserver bubble_observer(bubble_widget);
   bubble->Show();
   EXPECT_TRUE(delegate.shown());
   EXPECT_FALSE(delegate.close_action());
 
-  bubble_widget->Close();
+  bubble->GetDialogClientView()->CancelWindow();
   base::RunLoop().RunUntilIdle();
   ASSERT_TRUE(delegate.close_action());
   EXPECT_EQ(ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_USER_ACTION,
@@ -120,14 +100,17 @@ TEST_F(ToolbarActionsBarBubbleViewsTest, TestClickActionButton) {
   ToolbarActionsBarBubbleViews* bubble = new ToolbarActionsBarBubbleViews(
       anchor_widget->GetContentsView(), delegate.GetDelegate());
   views::Widget* bubble_widget =
-      views::BubbleDelegateView::CreateBubble(bubble);
+      views::BubbleDialogDelegateView::CreateBubble(bubble);
   views::test::TestWidgetObserver bubble_observer(bubble_widget);
   bubble->Show();
 
   EXPECT_FALSE(delegate.close_action());
   ui::test::EventGenerator generator(GetContext(),
                                      anchor_widget->GetNativeWindow());
-  generator.MoveMouseTo(GetCenterInScreenCoordinates(bubble->action_button()));
+  generator.MoveMouseTo(bubble->GetDialogClientView()
+                            ->ok_button()
+                            ->GetBoundsInScreen()
+                            .CenterPoint());
   generator.ClickLeftButton();
   base::RunLoop().RunUntilIdle();
   ASSERT_TRUE(delegate.close_action());
@@ -144,7 +127,7 @@ TEST_F(ToolbarActionsBarBubbleViewsTest, TestCloseOnDeactivation) {
   ToolbarActionsBarBubbleViews* bubble = new ToolbarActionsBarBubbleViews(
       anchor_widget->GetContentsView(), delegate.GetDelegate());
   views::Widget* bubble_widget =
-      views::BubbleDelegateView::CreateBubble(bubble);
+      views::BubbleDialogDelegateView::CreateBubble(bubble);
   views::test::TestWidgetObserver bubble_observer(bubble_widget);
   bubble->Show();
 
@@ -168,7 +151,7 @@ TEST_F(ToolbarActionsBarBubbleViewsTest, TestDontCloseOnDeactivation) {
   ToolbarActionsBarBubbleViews* bubble = new ToolbarActionsBarBubbleViews(
       anchor_widget->GetContentsView(), delegate.GetDelegate());
   views::Widget* bubble_widget =
-      views::BubbleDelegateView::CreateBubble(bubble);
+      views::BubbleDialogDelegateView::CreateBubble(bubble);
   views::test::TestWidgetObserver bubble_observer(bubble_widget);
   bubble->Show();