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..d9a513cea006ce2c2af83d6d955b850140c612ef 100644
--- a/ui/views/bubble/bubble_frame_view_unittest.cc
+++ b/ui/views/bubble/bubble_frame_view_unittest.cc
@@ -8,19 +8,51 @@
 
 #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/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"
 
 namespace views {
 
-typedef ViewsTestBase BubbleFrameViewTest;
+class BubbleFrameViewTest;

[tapted, 2017/03/29 23:16:16]

nit: forward dec not used?

[Elly Fong-Jones, 2017/04/05 18:17:53]

Done.

+
+class BubbleFrameViewTestViewsDelegate : public TestViewsDelegate {
+ public:
+  BubbleFrameViewTestViewsDelegate() {}
+  ~BubbleFrameViewTestViewsDelegate() override {}
+
+  int GetSnappedDialogWidth(int width) const override {

[tapted, 2017/03/29 23:16:16]

nit: // TestViewsDelegate:

[Elly Fong-Jones, 2017/04/05 18:17:53]

Done.

+    return ((width + width_snapping_unit_ - 1) / width_snapping_unit_) *
+           width_snapping_unit_;
+  }
+
+  void set_width_snapping_unit(int unit) { width_snapping_unit_ = unit; }
+
+ private:
+  int width_snapping_unit_ = 1;
+};

[tapted, 2017/03/29 23:16:16]

nit: DISALLOW_COPY...

[Elly Fong-Jones, 2017/04/05 18:17:53]

Done.

+
+class BubbleFrameViewTest : public ViewsTestBase {
+ public:
+  BubbleFrameViewTest() {
+    views_delegate_ = new BubbleFrameViewTestViewsDelegate;
+    set_views_delegate(std::unique_ptr<TestViewsDelegate>(views_delegate_));

[tapted, 2017/03/29 23:16:16]

base::WrapUnique?

[Elly Fong-Jones, 2017/04/05 18:17:53]

Done.

+  }
+  ~BubbleFrameViewTest() override {}
+
+  BubbleFrameViewTestViewsDelegate* views_delegate() { return views_delegate_; }
+
+ private:
+  BubbleFrameViewTestViewsDelegate* views_delegate_;
+};

[tapted, 2017/03/29 23:16:16]

nit: DISALLOW_COPY...

[Elly Fong-Jones, 2017/04/05 18:17:53]

Done.

 
 namespace {
 
@@ -488,4 +520,22 @@ TEST_F(BubbleFrameViewTest, GetMaximumSize) {
 #endif
 }
 
+TEST_F(BubbleFrameViewTest, WidthSnapsToUnit) {

[tapted, 2017/03/29 23:16:16]

nit: comment before

[Elly Fong-Jones, 2017/04/05 18:17:53]

Done.

+  TestBubbleFrameView frame(this);

[tapted, 2017/03/29 23:16:16]

Can we use BubbleDialogDelegateView::CreateBubble() in the test, and compare the
dialog size using views::Widget APIs? There are a ton of flags/settings in the
codepaths leading up to the creation of the dialog frame -- I feel like this
would give us the best reassurance that all elements influencing the dialog size
are covered.

We might just need some platform-specific code to subtract the "right" shadow,
or simply use BubbleDialogDelegateView::set_shadow(BubbleBorder::NO_ASSETS)
explicitly so we can assume there is never a shadow.

[Elly Fong-Jones, 2017/04/05 18:17:53]

Which setup code do you mean? There is some in (eg)
TestBubbleFrameView::GetWidget(), but I think I would end up simply duplicating
that code in this test...

Are you talking about doing something like:

  unique_ptr<Widget> w(new Widget());
  ... make some init params ...
  w->Init();

  BubbleDialogDelegateView d(w);

and then calling:

  BubbleDialogDelegateView::CreateBubble(&d)

and checking the resulting widget's size? I don't really understand what that
would be testing, since won't ::CreateBubble simply use the widget we created
above?

I ended up doing something like this, but I am not that pleased with the result.

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

BubbleDialogDelegateView doesn't need a Widget to construct itself -- that's
done by BubbleDialogDelegateView::CreateBubble(). The problem is the Widget that
CreateBubble creates uses anon::CreateBubbleWidget() which sets a bunch of
Widget params that could have subtle influences on the native/non-native
nonclientframeviews that are added to the window that is eventually displayed,
and what the resulting "width" of the dialog will be when it's shown to the
user.

+
+  // 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 = frame.GetPreferredSize().width() - kPreferredClientWidth -
+                     kExpectedAdditionalWidth;
+
+  // Check that changing the snapping unit changes the computed width to be a
+  // multiple of the snapping unit.

[Peter Kasting, 2017/03/29 03:51:16]

This feels like you're mostly testing the functionality of the test.  That is,
it would be just as effective for this test to implement GetSnappedDialogWidth()
as always returning 5, and then check here that the returned size is 5.  Even
more bluntly, basically this is a mock test with an EXPECT_CALL; the only part
of the non-test code it verifies is that GetPreferredSize() calls
GetSnappedDialogWidth().

If that is worth testing, then I would simplify the test to just test that.  If
that's not worth testing, I'd eliminate this, or rewrite it to test something
that is worth testing.

[Elly Fong-Jones, 2017/04/05 18:17:53]

I reworked this test a little bit to be simpler, but I think we do still need
it.

+  views_delegate()->set_width_snapping_unit(19);
+  EXPECT_EQ((frame.GetPreferredSize().width() - border_width) % 19, 0);
+
+  views_delegate()->set_width_snapping_unit(17);
+  EXPECT_EQ((frame.GetPreferredSize().width() - border_width) % 17, 0);
+}
+
 }  // namespace views