views: support dialog width snapping once and for all

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/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/views_test_base.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
 
 namespace views {
 
 typedef ViewsTestBase BubbleFrameViewTest;
 
@@ skipping 456 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
 }
 
+namespace {
+
+class TestBubbleDialogDelegateView : public BubbleDialogDelegateView {
+ public:
+  TestBubbleDialogDelegateView()
+      : BubbleDialogDelegateView(nullptr, BubbleBorder::NONE) {
+    set_shadow(BubbleBorder::NO_ASSETS);
+  }
+  ~TestBubbleDialogDelegateView() override {}
+
+  void SetAnchorView(View* anchor_view) {

[tapted, 2017/04/20 01:12:56]

`using BubbleDialogDelegateView::SetAnchorView;`

(that's a clearer signal that we just want to expose a protected member from the
base class)

[Elly Fong-Jones, 2017/04/20 19:54:00]

Ooh, I didn't even know you could do this! TIL :)

+    BubbleDialogDelegateView::SetAnchorView(anchor_view);
+  }
+
+  // This delegate is owned by the test case itself, so it should not delete
+  // itself here.

[tapted, 2017/04/20 01:12:56]

nit: here, // BubbleDialogDelegateView:

I'd put the comment above into the method body

[Elly Fong-Jones, 2017/04/20 19:54:00]

Done.

+  void DeleteDelegate() override {}
+

[tapted, 2017/04/20 01:12:56]

We should override GetDialogButtons() as well to return ui::DIALOG_BUTTON_NONE
(otherwise the width of buttons and default strings that are put there also have
an influence on the dialog size).

[Elly Fong-Jones, 2017/04/20 19:53:59]

Done.

+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestBubbleDialogDelegateView);
+};
+
+class TestLayoutProvider : public LayoutProvider {
+ public:
+  TestLayoutProvider() : LayoutProvider() {}
+  ~TestLayoutProvider() override {}
+
+  int GetSnappedDialogWidth(int min_width) const override {

[tapted, 2017/04/20 01:12:56]

nit: // LayoutProvider:

[Elly Fong-Jones, 2017/04/20 19:54:00]

Done.

+    return snap_to_ ? snap_to_ : min_width;
+  }
+
+  void set_snap_to(int width) { snap_to_ = width; }
+
+ private:
+  int snap_to_ = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(TestLayoutProvider);
+};
+
+}  // namespace
+
+TEST_F(BubbleFrameViewTest, WidthSnaps) {

[tapted, 2017/04/20 01:12:56]

nit: comment describing the test purpose

[Elly Fong-Jones, 2017/04/20 19:54:00]

Done.

+  // It would be good to test directly against HarmonyLayoutProvider, but //ui
+  // can't depend on //chrome to get at it. Instead, this test has its own
+  // LayoutProvider.

[Peter Kasting, 2017/04/19 19:26:13]

Nit: I feel like even talking about HarmonyLayoutProvider here is a bit of a
misnomer, and besides, the test layout provider here gives you more control over
this.  I'd just remove this comment.

[Elly Fong-Jones, 2017/04/20 19:53:59]

Done.

+  TestLayoutProvider provider;
+
+  std::unique_ptr<TestBubbleDialogDelegateView> delegate(
+      new TestBubbleDialogDelegateView());

[Peter Kasting, 2017/04/19 19:26:13]

Nit: Prefer = base::MakeUnique to bare new

[tapted, 2017/04/20 01:12:56]

could this just go on the stack?

TestBubbleDialogDelegateView delegate;

Then pass &delegate below

[Elly Fong-Jones, 2017/04/20 19:54:00]

Acknowledged.

[Elly Fong-Jones, 2017/04/20 19:54:00]

Ooh, it can. Done :)

+
+  std::unique_ptr<Widget> anchor(new Widget());

[tapted, 2017/04/20 01:12:56]

this can probably go on the stack too

[Elly Fong-Jones, 2017/04/20 19:54:00]

Done.

+  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_WINDOW);
+  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;

[tapted, 2017/04/20 01:12:56]

nit: `views::` not required

[Elly Fong-Jones, 2017/04/20 19:54:00]

Done.

+  anchor->Init(params);
+  anchor->Show();
+
+  // This is a bit awkward: |anchor| needs to outlive |delegate|, but |delegate|

[Peter Kasting, 2017/04/19 19:26:13]

I'm confused.  If |anchor| needs to outlive |delegate|, this code won't do it
correctly -- you created |anchor| last, so it will be destroyed first.

[Elly Fong-Jones, 2017/04/20 19:54:00]

After more thought, this entire comment was nonsense, so I've removed it.

+  // needs |anchor|'s ContentView, so instead of passing
+  // |anchor->GetContentsView()| directly to the |TestBubbleDialogDelegateView|
+  // constructor, the anchor view has to be set here, after |anchor| is
+  // constructed.
+  delegate->SetAnchorView(anchor->GetContentsView());

[tapted, 2017/04/20 01:12:56]

Why do we need an anchor view? can we just leave it null? Or call
SetAnchorRect(..) instead?

[Elly Fong-Jones, 2017/04/20 19:54:00]

I'm surprised that it works without an anchor view O_o.

+
+  const int kTestWidth = 300;

[Peter Kasting, 2017/04/19 19:26:13]

Nit: Prefer constexpr for compile-time constants

[Elly Fong-Jones, 2017/04/20 19:54:00]

Done.

+
+  Widget* w0 = BubbleDialogDelegateView::CreateBubble(delegate.get());
+  w0->Show();
+  EXPECT_NE(w0->GetWindowBoundsInScreen().width(), kTestWidth);

[Peter Kasting, 2017/04/19 19:26:13]

Nit: (expected, actual) (2 places)

[tapted, 2017/04/20 01:12:56]

Can we add a TestBubbleDialogDelegateView::GetPreferredSize() override (e.g.
200,200) and test against that size explicitly?

delegate.set_margins(gfx::Insets());

should simplify the width calculations, although it still depends on button
widths without GetDialogButtons() overridden

[Elly Fong-Jones, 2017/04/20 19:53:59]

nice :D Done.

[Elly Fong-Jones, 2017/04/20 19:54:00]

Acknowledged.

+
+  provider.set_snap_to(kTestWidth);
+
+  // The Widget's snapped width should exactly match the width returned by the
+  // LayoutProvider.
+  Widget* w1 = BubbleDialogDelegateView::CreateBubble(delegate.get());
+  w1->Show();
+  EXPECT_EQ(w1->GetWindowBoundsInScreen().width(), kTestWidth);
+}

[tapted, 2017/04/20 01:12:56]

w0->CloseNow();
w1->CloseNow();

(otherwise.. I think you currently have a UAF during the Widget destruction done
when the root window is torn down and Widget wants to call
delegate.DeleteDelegate()) which would be deleted by the unique_ptr here)

[Elly Fong-Jones, 2017/04/20 19:53:59]

Does this also serve to delete w0 and w1? I can't tell if I'm leaking them right
now.

[tapted, 2017/04/21 00:20:31]

Yup. Widget destruction triggers a CloseNow() when WIDGET_OWNS_NATIVE_WIDGET;
CreateBubble() doesn't make widgets in this mode, but instead
NATIVE_WIDGET_OWNS_WIDGET, so a CloseNow() on the Widget will synchronously
delete the Widget* instead.

Note I think DCHECKS (or trybots) or `is_asan = true` should complain about the
earlier code, but a regular release build might not.  E.g. (on Mac) there's a
DCHECK at
https://cs.chromium.org/chromium/src/ui/views/test/views_test_helper_mac.mm?q...
that should get hit. Maybe I should make that a CHECK...

+
 }  // namespace views