Convert ToolbarActionsBarBubbleViews to BubbleDialogDelegate

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // 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:
   std::unique_ptr<views::Widget> CreateAnchorWidget() {
     std::unique_ptr<views::Widget> anchor_widget(new views::Widget());
     views::Widget::InitParams params =
         CreateParams(views::Widget::InitParams::TYPE_WINDOW);
     params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
     anchor_widget->Init(params);
     return anchor_widget;
   }
 
   base::string16 HeadingString() { return base::ASCIIToUTF16("Heading"); }
   base::string16 BodyString() { return base::ASCIIToUTF16("Body"); }
   base::string16 ActionString() { return base::ASCIIToUTF16("Action"); }
   base::string16 DismissString() { return base::ASCIIToUTF16("Dismiss"); }
 };
 
 TEST_F(ToolbarActionsBarBubbleViewsTest, TestBubbleLayoutActionButton) {
   std::unique_ptr<views::Widget> anchor_widget = CreateAnchorWidget();
   TestToolbarActionsBarBubbleDelegate delegate(HeadingString(), BodyString(),
                                                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();
 }
 
 TEST_F(ToolbarActionsBarBubbleViewsTest,
        TestBubbleLayoutActionAndDismissButton) {
   std::unique_ptr<views::Widget> anchor_widget = CreateAnchorWidget();
   TestToolbarActionsBarBubbleDelegate delegate(HeadingString(), BodyString(),
                                                ActionString());
   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();
 }
 
 TEST_F(ToolbarActionsBarBubbleViewsTest, TestShowAndCloseBubble) {
   std::unique_ptr<views::Widget> anchor_widget = CreateAnchorWidget();
   TestToolbarActionsBarBubbleDelegate delegate(HeadingString(), BodyString(),
                                                ActionString());
   delegate.set_dismiss_button_text(DismissString());
   ToolbarActionsBarBubbleViews* bubble = new ToolbarActionsBarBubbleViews(
       anchor_widget->GetContentsView(), delegate.GetDelegate());
 
   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,
             *delegate.close_action());
   EXPECT_TRUE(bubble_observer.widget_closed());
 }
 
 TEST_F(ToolbarActionsBarBubbleViewsTest, TestClickActionButton) {
   std::unique_ptr<views::Widget> anchor_widget = CreateAnchorWidget();
   TestToolbarActionsBarBubbleDelegate delegate(HeadingString(), BodyString(),
                                                ActionString());
   delegate.set_dismiss_button_text(DismissString());
   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());
   EXPECT_EQ(ToolbarActionsBarBubbleDelegate::CLOSE_EXECUTE,
             *delegate.close_action());
   EXPECT_TRUE(bubble_observer.widget_closed());
 }
 
 TEST_F(ToolbarActionsBarBubbleViewsTest, TestCloseOnDeactivation) {
   std::unique_ptr<views::Widget> anchor_widget = CreateAnchorWidget();
   TestToolbarActionsBarBubbleDelegate delegate(HeadingString(), BodyString(),
                                                ActionString());
   delegate.set_dismiss_button_text(DismissString());
   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());
   // Close the bubble by activating another widget. The delegate should be
   // told it was dismissed.
   anchor_widget->Activate();
   base::RunLoop().RunUntilIdle();
   ASSERT_TRUE(delegate.close_action());
   EXPECT_EQ(ToolbarActionsBarBubbleDelegate::CLOSE_DISMISS_DEACTIVATION,
             *delegate.close_action());
   EXPECT_TRUE(bubble_observer.widget_closed());
 }
 
 TEST_F(ToolbarActionsBarBubbleViewsTest, TestDontCloseOnDeactivation) {
   scoped_ptr<views::Widget> anchor_widget = CreateAnchorWidget();
   TestToolbarActionsBarBubbleDelegate delegate(HeadingString(), BodyString(),
                                                ActionString());
   delegate.set_dismiss_button_text(DismissString());
   delegate.set_close_on_deactivate(false);
   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());
   // Activate another widget. The bubble shouldn't close.
   anchor_widget->Activate();
   base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(delegate.close_action());
   bubble_widget->Close();
   base::RunLoop().RunUntilIdle();
 }