Fixing crash Report - Magic Signature: views::View::ConvertPointToScreen

ui/views/bubble/bubble_delegate_unittest.cc

Index: ui/views/bubble/bubble_delegate_unittest.cc
diff --git a/ui/views/bubble/bubble_delegate_unittest.cc b/ui/views/bubble/bubble_delegate_unittest.cc
index 5cf4a377ec521ae4380d288f3d651bca70993efa..e93543e15357f09d0f51d340d61870076bc8401f 100644
--- a/ui/views/bubble/bubble_delegate_unittest.cc
+++ b/ui/views/bubble/bubble_delegate_unittest.cc
@@ -29,6 +29,14 @@ class TestBubbleDelegateView : public BubbleDelegateView {
   }
   virtual ~TestBubbleDelegateView() {}
 
+  void SetAnchorRectForTest(gfx::Rect rect) {
+    SetAnchorRect(rect);
+  }
+
+  void SetAnchorViewForTest(View* view) {
+    SetAnchorView(view);
+  }
+
   // BubbleDelegateView overrides:
   virtual View* GetInitiallyFocusedView() OVERRIDE { return view_; }
   virtual gfx::Size GetPreferredSize() OVERRIDE { return gfx::Size(200, 200); }
@@ -109,6 +117,57 @@ TEST_F(BubbleDelegateTest, CloseAnchorWidget) {
   EXPECT_TRUE(bubble_observer.widget_closed());
 }
 
+TEST_F(BubbleDelegateTest, CloseAnchorViewTest) {
+  // Create an anchor widget which has multiple views, one of them will be used

[msw, 2013/09/27 18:20:31]

I don't see multiple views, revise this comment.

[Mr4D (OOO till 08-26), 2013/09/27 20:29:35]

Well there is already the content view. Therefore there are multiple, but
changed.

+  // as anchor view.
+  scoped_ptr<Widget> anchor_widget(CreateTestWidget());
+  scoped_ptr<View> anchor_view(new View());

[msw, 2013/09/27 18:20:31]

Do not use a scoped_ptr here, the View is owned by the widget.

[Mr4D (OOO till 08-26), 2013/09/27 20:29:35]

That is not correct. The view will get removed from the hierarchy and
"RemoveChildView" is NOT deleting the view. So it still belongs to us and we
need to get rid of it. Sure I could remove the scoped_ptr and simply delete it -
is that what you are asking for?

[msw, 2013/09/27 21:21:52]

Nope, I incorrectly assumed that RemoveChildView would delete the view. Thank
you for correcting me, you can keep the scoped_ptr.

+  anchor_widget->GetContentsView()->AddChildView(anchor_view.get());
+  TestBubbleDelegateView* bubble_delegate = new TestBubbleDelegateView(
+      anchor_view.get());
+  // Preventing close on deactivate should not prevent closing with the anchor.

[msw, 2013/09/27 18:20:31]

Why are you testing this and calling set_close_on_deactivate(false); here?

[Mr4D (OOO till 08-26), 2013/09/27 20:29:35]

Done.

+  bubble_delegate->set_close_on_deactivate(false);
+  Widget* bubble_widget = BubbleDelegateView::CreateBubble(bubble_delegate);
+  EXPECT_EQ(bubble_delegate, bubble_widget->widget_delegate());

[msw, 2013/09/27 18:20:31]

Remove the expectations checked by other tests, they're not needed here.

[Mr4D (OOO till 08-26), 2013/09/27 20:29:35]

Done.

+  EXPECT_EQ(bubble_widget, bubble_delegate->GetWidget());
+  EXPECT_EQ(anchor_widget, bubble_delegate->anchor_widget());
+  test::TestWidgetObserver bubble_observer(bubble_widget);

[msw, 2013/09/27 18:20:31]

Remove this observer, it's not needed for this test.

[Mr4D (OOO till 08-26), 2013/09/27 20:29:35]

Done.

+  EXPECT_FALSE(bubble_observer.widget_closed());
+  // Check that the anchor view is correct and set up an anchor view rect.

[msw, 2013/09/27 18:20:31]

It's probably good to test that the anchor_view bounds override an anchor rect,
(and also that updated anchor_view positions are reflected in subsequent calls
to GetAnchorRect) but that behavior should be checked in a separate test.

[Mr4D (OOO till 08-26), 2013/09/27 20:29:35]

Not quite sure I get what you are asking for here. I assume that you want that
location changes of the view will lead to different rectangles. Will add that.

[msw, 2013/09/27 21:21:52]

I'm asking you to simplify this test. The interaction between SetAnchorRect and
SetAnchorView should be done in another test. This test should simply check that
GetAnchorRect returns the same value (|view_rect|), and more importantly doesn't
crash, after the View is deleted.

+  // Make sure that this rect will get ignored (as long as the anchor view is
+  // attached).
+  EXPECT_EQ(anchor_view, bubble_delegate->AnchorView());
+  EXPECT_TRUE(bubble_delegate->has_anchor_view());
+  gfx::Rect set_anchor_rect = gfx::Rect(10, 10, 100, 100);

[msw, 2013/09/27 18:20:31]

nit: const.

[Mr4D (OOO till 08-26), 2013/09/27 20:29:35]

Done.

+  bubble_delegate->SetAnchorRectForTest(set_anchor_rect);
+  gfx::Rect view_rect = bubble_delegate->GetAnchorRect();

[msw, 2013/09/27 18:20:31]

nit: const.

[Mr4D (OOO till 08-26), 2013/09/27 20:29:35]

Done.

+  EXPECT_NE(view_rect.ToString(), set_anchor_rect.ToString());
+
+  // Create the bubble.
+  bubble_widget->Show();
+  EXPECT_EQ(anchor_widget, bubble_delegate->anchor_widget());
+  EXPECT_FALSE(bubble_observer.widget_closed());
+
+  // Remove now the anchor view and make sure that the original found rect
+  // is still kept, so that the bubble does not jump when the view gets deleted.
+  anchor_widget->GetContentsView()->RemoveChildView(anchor_view.get());
+  anchor_view.reset();
+  EXPECT_TRUE(bubble_delegate->has_anchor_view());
+  EXPECT_EQ(NULL, bubble_delegate->AnchorView());
+  EXPECT_EQ(view_rect.ToString(), bubble_delegate->GetAnchorRect().ToString());
+
+  // Inform now the bubble that the anchor view is gone and expect it to

[msw, 2013/09/27 18:20:31]

I think this behavior is unnecessary; is it ever used in practice?
Couldn't any user simply set a new anchor rect instead?

[Mr4D (OOO till 08-26), 2013/09/27 20:29:35]

You asked for this yesterday. Done. removed.

+  // remember the originally set anchor rect which was ignored till now.
+  bubble_delegate->SetAnchorViewForTest(NULL);
+  EXPECT_FALSE(bubble_delegate->has_anchor_view());
+  EXPECT_EQ(set_anchor_rect.ToString(),
+            bubble_delegate->GetAnchorRect().ToString());
+
+  // Ensure that closing the anchor widget also closes the bubble itself.

[msw, 2013/09/27 18:20:31]

This test does not need to check this.

[Mr4D (OOO till 08-26), 2013/09/27 20:29:35]

Done.

+  anchor_widget->CloseNow();
+  EXPECT_TRUE(bubble_observer.widget_closed());
+}
+
 TEST_F(BubbleDelegateTest, ResetAnchorWidget) {
   scoped_ptr<Widget> anchor_widget(CreateTestWidget());
   BubbleDelegateView* bubble_delegate = new BubbleDelegateView(