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/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 {
 
 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 32 matching lines @@
   void SetShouldShowCloseButton(bool should_show_close) {
     should_show_close_ = should_show_close;
   }
 
  private:
   Widget* widget_;
   View* contents_view_ = nullptr;  // Owned by |widget_|.
   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)
       : BubbleFrameView(gfx::Insets(), gfx::Insets(kMargin)),
         test_base_(test_base),
         available_bounds_(gfx::Rect(0, 0, 1000, 1000)) {
     SetBubbleBorder(std::unique_ptr<BubbleBorder>(
         new BubbleBorder(kArrow, BubbleBorder::NO_SHADOW, kColor)));
   }
   ~TestBubbleFrameView() override {}
 
   // View overrides:
   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();
       params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
       widget_->Init(params);
     }
 
     return widget_.get();
   }
 
   // BubbleFrameView overrides:
   gfx::Rect GetAvailableScreenBounds(const gfx::Rect& rect) const override {
     return available_bounds_;
   }
 
   TestBubbleFrameViewWidgetDelegate* widget_delegate() {
     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_;
 
   gfx::Rect available_bounds_;
 
   // Widget returned by GetWidget(). Only created if GetWidget() is called.
   mutable std::unique_ptr<TestBubbleFrameViewWidgetDelegate> widget_delegate_;
   mutable std::unique_ptr<Widget> widget_;
 
   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) {
   TestBubbleFrameView frame(this);
   EXPECT_EQ(kArrow, frame.bubble_border()->arrow());
   EXPECT_EQ(kColor, frame.bubble_border()->background_color());
 
   int margin_x = frame.content_margins().left();
   int margin_y = frame.content_margins().top();
   gfx::Insets insets = frame.bubble_border()->GetInsets();
@@ skipping 347 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
 }
 
+// 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