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

ui/views/bubble/bubble_delegate.cc

Index: ui/views/bubble/bubble_delegate.cc
diff --git a/ui/views/bubble/bubble_delegate.cc b/ui/views/bubble/bubble_delegate.cc
index af08994d487f5905b3471e806cfb899664de10e0..b742b2266afaff98bc94182d8496520ea8551dd5 100644
--- a/ui/views/bubble/bubble_delegate.cc
+++ b/ui/views/bubble/bubble_delegate.cc
@@ -9,6 +9,7 @@
 #include "ui/gfx/rect.h"
 #include "ui/native_theme/native_theme.h"
 #include "ui/views/bubble/bubble_frame_view.h"
+#include "ui/views/focus/view_storage.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_observer.h"
 
@@ -128,8 +129,10 @@ Widget* CreateBorderWidget(BubbleDelegateView* bubble) {
 BubbleDelegateView::BubbleDelegateView()
     : close_on_esc_(true),
       close_on_deactivate_(true),
-      anchor_view_(NULL),
+      has_anchor_view_(false),
+      last_known_anchor_view_rect_(gfx::Rect()),

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

Remove this and others, the default gfx::Rect ctor is implicitly invoked.

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

Done.

       anchor_widget_(NULL),
+      anchor_rect_(gfx::Rect()),
       move_with_anchor_(false),
       arrow_(BubbleBorder::TOP_LEFT),
       shadow_(BubbleBorder::SMALL_SHADOW),
@@ -151,8 +154,10 @@ BubbleDelegateView::BubbleDelegateView(
     BubbleBorder::Arrow arrow)
     : close_on_esc_(true),
       close_on_deactivate_(true),
-      anchor_view_(anchor_view),
+      has_anchor_view_(false),
+      last_known_anchor_view_rect_(gfx::Rect()),
       anchor_widget_(NULL),
+      anchor_rect_(gfx::Rect()),
       move_with_anchor_(false),
       arrow_(arrow),
       shadow_(BubbleBorder::SMALL_SHADOW),
@@ -165,23 +170,21 @@ BubbleDelegateView::BubbleDelegateView(
       border_accepts_events_(true),
       adjust_if_offscreen_(true),
       parent_window_(NULL) {
+  SetAnchorView(anchor_view);
   AddAccelerator(ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
   UpdateColorsFromTheme(GetNativeTheme());
 }
 
 BubbleDelegateView::~BubbleDelegateView() {
-  if (anchor_widget() != NULL)
-    anchor_widget()->RemoveObserver(this);
-  anchor_widget_ = NULL;
-  anchor_view_ = NULL;
+  DetachFromAnchor();
 }
 
 // static
 Widget* BubbleDelegateView::CreateBubble(BubbleDelegateView* bubble_delegate) {
   bubble_delegate->Init();
   // Determine the anchor widget from the anchor view at bubble creation time.
-  bubble_delegate->anchor_widget_ = bubble_delegate->anchor_view() ?
-      bubble_delegate->anchor_view()->GetWidget() : NULL;
+  bubble_delegate->anchor_widget_ = bubble_delegate->AnchorView() ?
+      bubble_delegate->AnchorView()->GetWidget() : NULL;
   if (bubble_delegate->anchor_widget())
     bubble_delegate->anchor_widget()->AddObserver(bubble_delegate);
 
@@ -231,11 +234,8 @@ NonClientFrameView* BubbleDelegateView::CreateNonClientFrameView(
 }
 
 void BubbleDelegateView::OnWidgetDestroying(Widget* widget) {
-  if (anchor_widget() == widget) {
-    anchor_widget_->RemoveObserver(this);
-    anchor_view_ = NULL;
-    anchor_widget_ = NULL;
-  }
+  if (anchor_widget() == widget)
+    DetachFromAnchor();
 }
 
 void BubbleDelegateView::OnWidgetVisibilityChanging(Widget* widget,
@@ -267,11 +267,48 @@ void BubbleDelegateView::OnWidgetBoundsChanged(Widget* widget,
     SizeToContents();
 }
 
+View* BubbleDelegateView::AnchorView() const {
+  if (!has_anchor_view_)
+    return NULL;
+  return ViewStorage::GetInstance()->RetrieveView(anchor_view_storage_id_);
+}
+
+const gfx::Rect& BubbleDelegateView::AnchorRect() const {
+  return has_anchor_view_ ? last_known_anchor_view_rect_ : anchor_rect_;
+}
+
+void BubbleDelegateView::SetAnchorView(View* anchor_view) {
+  if (!anchor_view) {
+    if (has_anchor_view_) {
+      has_anchor_view_ = false;
+      last_known_anchor_view_rect_ = anchor_rect_;
+      ViewStorage::GetInstance()->RemoveView(anchor_view_storage_id_);
+    }
+    return;
+  }
+
+  if (!has_anchor_view_) {
+    anchor_view_storage_id_ = ViewStorage::GetInstance()->CreateStorageID();

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

This int needs ctor initialization; either initialize it to -1 and replace
|has_anchor_view_| with (anchor_view_storage_id_ != -1), or initialize it with a
new storage ID like this and drop |has_anchor_view_| in favor of simply checking
for GetAnchorView.

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

According to the header file -1 is not an invalid state. As there are no invalid
storage ID's it seems to be much more secure to have the boolean. Note that due
to this, there is also no ctor initialization required (since there is no
invalid value).

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

CreateStorageID always uses positive numbers, at least until it overflows, but I
guess the API sadly doesn't make any such guarantee. I suggest initializing
|anchor_view_storage_id_| to a valid storage id in the ctor, as most other
ViewStorage users do. In the worst case ViewStorage::view_storage_next_id_ will
overflow when the user shows 32k bubbles (or browser windows, etc.), and
probably continue to be valid until 64k such IDs are taken, and then it will
began to repeat and clash IDs... This seems like a low-pri bug that should be
addressed separately.

+    has_anchor_view_ = true;
+  }
+
+  ViewStorage::GetInstance()->StoreView(anchor_view_storage_id_, anchor_view);
+}
+
+void BubbleDelegateView::SetAnchorRect(const gfx::Rect& rect) {

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

Reverted this to set_anchor_rect with my suggestion in GetAnchorRect.

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

Done.

+  anchor_rect_ = rect;
+  last_known_anchor_view_rect_ = rect;
+}
+
 gfx::Rect BubbleDelegateView::GetAnchorRect() {
-  if (!anchor_view())
-    return anchor_rect_;
-  gfx::Rect anchor_bounds = anchor_view()->GetBoundsInScreen();
+  if (!AnchorView()) {

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

I think this function can remain largely the same; remove
|last_known_anchor_view_rect_| and simply update and return |anchor_rect_| when
this is called and AnchorView() exists.

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

I did that this way since you *explicitly* told me yesterday multiple times that
you do *not* want to get the location overwritten. Which is also the reason why
I added that to the unit test to make sure that your requirement was taken care
of.

Okay, since you do not care about it anymore I can of course remove this again.

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

Sorry for not being clear enough yesterday, but that's not what I asked for. I
asked you to not cache the anchor view's last known bounds at all, merely to
avoid complexity. But now that I see the code, it's quite easy to just overwrite
|anchor_rect_| on each GetAnchorRect call, as your latest patch set does, so
thanks for cleaning up the extra Rect class member.

+    // If the anchor view was going away, we have to return the last known good
+    // position.
+    return has_anchor_view_ ? last_known_anchor_view_rect_ : anchor_rect_;
+  }
+  gfx::Rect anchor_bounds = AnchorView()->GetBoundsInScreen();
   anchor_bounds.Inset(anchor_view_insets_);
+  last_known_anchor_view_rect_ = anchor_bounds;
   return anchor_bounds;
 }
 
@@ -437,4 +474,11 @@ void BubbleDelegateView::HandleVisibilityChanged(Widget* widget,
   }
 }
 
+void BubbleDelegateView::DetachFromAnchor() {
+  if (anchor_widget_)
+    anchor_widget_->RemoveObserver(this);
+  SetAnchorView(NULL);
+  anchor_widget_ = NULL;
+}
+
 }  // namespace views