views: implement dialog width snapping

ui/views/bubble/bubble_frame_view_unittest.cc

 // Copyright (c) 2012 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 "ui/views/bubble/bubble_frame_view.h"
 
 #include <memory>
 
 #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 {
 
 const BubbleBorder::Arrow kArrow = BubbleBorder::TOP_LEFT;
 const SkColor kColor = SK_ColorRED;
 const int kMargin = 6;
 const int kMinimumClientWidth = 100;
 const int kMinimumClientHeight = 200;
 const int kMaximumClientWidth = 300;
 const int kMaximumClientHeight = 300;
@@ skipping 447 matching lines @@
 #else
   maximum_rect.Inset(frame.bubble_border()->GetInsets());
 
   // Should ignore the contents view's maximum size and use the preferred size.
   gfx::Size expected_size(kPreferredClientWidth + kExpectedAdditionalWidth,
                           kPreferredClientHeight + kExpectedAdditionalHeight);
   EXPECT_EQ(expected_size, maximum_rect.size());
 #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