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

ui/views/bubble/bubble_delegate_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 "base/run_loop.h"
 #include "ui/base/hit_test.h"
 #include "ui/views/bubble/bubble_delegate.h"
 #include "ui/views/bubble/bubble_frame_view.h"
 #include "ui/views/test/test_widget_observer.h"
 #include "ui/views/test/views_test_base.h"
@@ skipping 11 matching lines @@
 class TestBubbleDelegateView : public BubbleDelegateView {
  public:
   TestBubbleDelegateView(View* anchor_view)
       : BubbleDelegateView(anchor_view, BubbleBorder::TOP_LEFT),
         view_(new View()) {
     view_->set_focusable(true);
     AddChildView(view_);
   }
   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); }
   virtual int GetFadeDuration() OVERRIDE { return 1; }
 
  private:
   View* view_;
 
   DISALLOW_COPY_AND_ASSIGN(TestBubbleDelegateView);
 };
@@ skipping 60 matching lines @@
   //            aura::test::TestActivationClient::OnWindowDestroyed().
   scoped_ptr<Widget> smoke_and_mirrors_widget(CreateTestWidget());
   EXPECT_FALSE(bubble_observer.widget_closed());
 #endif
 
   // Ensure that closing the anchor widget also closes the bubble itself.
   anchor_widget->CloseNow();
   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(
       anchor_widget->GetContentsView(), BubbleBorder::NONE);
 
   // Make sure the bubble widget is parented to a widget other than the anchor
   // widget so that closing the anchor widget does not close the bubble widget.
   scoped_ptr<Widget> parent_widget(CreateTestWidget());
   bubble_delegate->set_parent_window(parent_widget->GetNativeView());
   // Preventing close on deactivate should not prevent closing with the parent.
@@ skipping 117 matching lines @@
             bubble_widget->GetFocusManager()->GetFocusedView());
 
   Observe(bubble_widget);
 
   bubble_delegate->StartFade(false);
   RunNestedLoop();
   EXPECT_TRUE(bubble_destroyed());
 }
 
 }  // namespace views