views: implement dialog width snapping

ui/views/bubble/bubble_frame_view_unittest.cc

Index: ui/views/bubble/bubble_frame_view_unittest.cc
diff --git a/ui/views/bubble/bubble_frame_view_unittest.cc b/ui/views/bubble/bubble_frame_view_unittest.cc
index bf955dd451c640273688efb5f36fae2e0198a691..b40a6565faeec230bf7f6c7be61ca056743150d0 100644
--- a/ui/views/bubble/bubble_frame_view_unittest.cc
+++ b/ui/views/bubble/bubble_frame_view_unittest.cc
@@ -8,19 +8,55 @@
 
 #include "base/macros.h"
 #include "build/build_config.h"
+#include "ui/base/test/material_design_controller_test_api.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/views/bubble/bubble_border.h"
+#include "ui/views/bubble/bubble_dialog_delegate.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/test/test_views.h"
+#include "ui/views/test/test_views_delegate.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
+#include "ui/views/window/dialog_delegate.h"
 
 namespace views {
 
-typedef ViewsTestBase BubbleFrameViewTest;
+class BubbleFrameViewTestViewsDelegate : public TestViewsDelegate {
+ public:
+  BubbleFrameViewTestViewsDelegate() {}
+  ~BubbleFrameViewTestViewsDelegate() override {}
+
+  // TestViewsDelegate:
+  int GetSnappedDialogWidth(int min_width) const override {
+    return snapped_width_ ? snapped_width_ : min_width;
+  }
+
+  void set_snapped_width(int width) { snapped_width_ = width; }
+
+ private:
+  int snapped_width_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(BubbleFrameViewTestViewsDelegate);
+};
+
+class BubbleFrameViewTest : public ViewsTestBase {
+ public:
+  BubbleFrameViewTest() {
+    views_delegate_ = new BubbleFrameViewTestViewsDelegate;
+    set_views_delegate(base::WrapUnique(views_delegate_));
+  }
+  ~BubbleFrameViewTest() override {}
+
+  BubbleFrameViewTestViewsDelegate* views_delegate() { return views_delegate_; }
+
+ private:
+  BubbleFrameViewTestViewsDelegate* views_delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(BubbleFrameViewTest);
+};
 
 namespace {
 
@@ -73,6 +109,35 @@ class TestBubbleFrameViewWidgetDelegate : public WidgetDelegate {
   bool should_show_close_ = false;
 };
 
+// A subclass of TestBubbleFrameViewWidgetDelegate that is also a
+// DialogDelegate, used to test creation of Bubbles end-to-end, since the width
+// snapping logic for bubbles only applies to bubbles whose widget delegate is a
+// DialogDelegate.
+//
+// Having a multiple-implementation inherited class is perilous, but there does
+// not seem to be an easy way to avoid it.
+class TestBubbleFrameViewDialogDelegate
+    : public DialogDelegate,
+      public TestBubbleFrameViewWidgetDelegate {

[tapted, 2017/04/06 00:24:44]

I don't think we need this - we're not really interested in any the things that
TestBubbleFrameViewWidgetDelegate provides -- we can just inherit from a
BubbleDialogDelegateView  (or DialogDelegateView with a but more work
[ShouldUseCustomFrame returning true])

Then so long as we use set_shadow(BubbleBorder::NO_SHADOW) we can verify that
the size of the *Widget* matches the expected Harmony sizes. I think that's a
lot more compelling than the preferred size of the frame view, since layout code
can ignore preferred sizes, and the Widget size will include the non-client
frame view, which will be a closer match to the size of the dialog seen by
users.

[Peter Kasting, 2017/04/06 06:32:16]

+1

+ public:
+  TestBubbleFrameViewDialogDelegate(Widget* widget)
+      : TestBubbleFrameViewWidgetDelegate(widget) {}
+  ~TestBubbleFrameViewDialogDelegate() override {}
+
+  // These are defined on both parent classes, so they need to be disambiguated
+  // thus:
+  Widget* GetWidget() override {
+    return TestBubbleFrameViewWidgetDelegate::GetWidget();
+  }
+  const Widget* GetWidget() const override {
+    return TestBubbleFrameViewWidgetDelegate::GetWidget();
+  }
+  DialogDelegate* AsDialogDelegate() override { return this; }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestBubbleFrameViewDialogDelegate);
+};
+
 class TestBubbleFrameView : public BubbleFrameView {
  public:
   TestBubbleFrameView(ViewsTestBase* test_base)
@@ -88,8 +153,7 @@ class TestBubbleFrameView : public BubbleFrameView {
   const Widget* GetWidget() const override {
     if (!widget_) {
       widget_.reset(new Widget);
-      widget_delegate_.reset(
-          new TestBubbleFrameViewWidgetDelegate(widget_.get()));
+      widget_delegate_ = MakeWidgetDelegate(widget_.get());
       Widget::InitParams params =
           test_base_->CreateParams(Widget::InitParams::TYPE_BUBBLE);
       params.delegate = widget_delegate_.get();
@@ -109,6 +173,12 @@ class TestBubbleFrameView : public BubbleFrameView {
     return widget_delegate_.get();
   }
 
+ protected:
+  virtual std::unique_ptr<TestBubbleFrameViewWidgetDelegate> MakeWidgetDelegate(
+      Widget* widget) const {
+    return base::WrapUnique(new TestBubbleFrameViewWidgetDelegate(widget));

[Peter Kasting, 2017/04/06 06:32:16]

Nit: WrapUnique(new -> MakeUnique (2 places)

+  }
+
  private:
   ViewsTestBase* test_base_;
 
@@ -121,6 +191,18 @@ class TestBubbleFrameView : public BubbleFrameView {
   DISALLOW_COPY_AND_ASSIGN(TestBubbleFrameView);
 };
 
+class TestBubbleFrameDialogView : public TestBubbleFrameView {
+ public:
+  TestBubbleFrameDialogView(ViewsTestBase* test_base)
+      : TestBubbleFrameView(test_base) {}
+
+ protected:
+  std::unique_ptr<TestBubbleFrameViewWidgetDelegate> MakeWidgetDelegate(
+      Widget* widget) const override {
+    return base::WrapUnique(new TestBubbleFrameViewDialogDelegate(widget));
+  }
+};
+
 }  // namespace
 
 TEST_F(BubbleFrameViewTest, GetBoundsForClientView) {
@@ -488,4 +570,22 @@ TEST_F(BubbleFrameViewTest, GetMaximumSize) {
 #endif
 }
 
+// This test ensures that BubbleFrameViews are snapped to the width specified by
+// ViewsDelegate::GetSnappedDialogWidth().
+TEST_F(BubbleFrameViewTest, WidthSnapsToUnit) {
+  TestBubbleFrameDialogView frame(this);
+
+  // The "border width" here is primarily for the bubble shadow, but also for
+  // the edge stroke of the bubble. This width is *not* included in the width
+  // snapping, so it needs to be subtracted out.
+  int border_width = 10;

[Peter Kasting, 2017/04/06 06:32:16]

Nit: This looks like you're matching a shadow/stroke width from elsewhere
(either built in to a resource or computed elsewhere).  This makes the test
fragile; instead expose the necessary values from the code that actually
computes them, and then fetch them in this test.

+
+  // Check that changing the snapping unit changes the computed width to be a
+  // multiple of the snapping unit.

[Peter Kasting, 2017/04/06 06:32:16]

Nit: Technically, the code below tests only that your GetSnappedDialogWidth()
override is called.  It doesn't test that this is a "multiple of the snapping
unit".  If your override returned "the snap_width + 1", that's what we'd get
here.

Instead, I suggest the following:
* Eliminate your views delegate override
* Have a dialog that can return a variety of preferred widths
* Create an array of pairs (actual width, expected snapped width)
* Test that, when you force disable harmony mode, the actual width is the
preferred width in all cases
* Then, when you force enable harmony mode, the actual width should be the
snapped width
* Then if the dialog returns false for "should snap", you get the pre-Harmony
behavior

+  const int kTestWidth = 500;
+  EXPECT_NE(kTestWidth, frame.GetPreferredSize().width() - border_width);

[Peter Kasting, 2017/04/06 06:32:16]

Nit: EXPECT_LT?

+  views_delegate()->set_snapped_width(kTestWidth);
+  EXPECT_EQ(kTestWidth, frame.GetPreferredSize().width() - border_width);
+}
+
 }  // namespace views