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

ui/views/bubble/bubble_delegate.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_delegate.h"
 
 #include "ui/gfx/animation/slide_animation.h"
 #include "ui/gfx/color_utils.h"
 #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"
 
 #if defined(OS_WIN)
 #include "ui/base/win/shell.h"
 #endif
 
 // The duration of the fade animation in milliseconds.
 static const int kHideFadeDurationMS = 200;
 
@@ skipping 99 matching lines @@
   return border_widget;
 }
 
 }  // namespace
 
 #endif
 
 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),
       color_explicitly_set_(false),
       margins_(kDefaultMargin, kDefaultMargin, kDefaultMargin, kDefaultMargin),
       original_opacity_(255),
       border_widget_(NULL),
       use_focusless_(false),
       accept_events_(true),
       border_accepts_events_(true),
       adjust_if_offscreen_(true),
       parent_window_(NULL) {
   AddAccelerator(ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
   UpdateColorsFromTheme(GetNativeTheme());
 }
 
 BubbleDelegateView::BubbleDelegateView(
     View* anchor_view,
     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),
       color_explicitly_set_(false),
       margins_(kDefaultMargin, kDefaultMargin, kDefaultMargin, kDefaultMargin),
       original_opacity_(255),
       border_widget_(NULL),
       use_focusless_(false),
       accept_events_(true),
       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);
 
   Widget* bubble_widget = CreateBubbleWidget(bubble_delegate);
 
 #if defined(OS_WIN)
 #if defined(USE_AURA)
   // If glass is enabled, the bubble is allowed to extend outside the bounds of
   // the parent frame and let DWM handle compositing.  If not, then we don't
   // want to allow the bubble to extend the frame because it will be clipped.
@@ skipping 29 matching lines @@
 NonClientFrameView* BubbleDelegateView::CreateNonClientFrameView(
     Widget* widget) {
   BubbleFrameView* frame = new BubbleFrameView(margins());
   const BubbleBorder::Arrow adjusted_arrow = base::i18n::IsRTL() ?
       BubbleBorder::horizontal_mirror(arrow()) : arrow();
   frame->SetBubbleBorder(new BubbleBorder(adjusted_arrow, shadow(), color()));
   return frame;
 }
 
 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,
                                                     bool visible) {
 #if defined(OS_WIN)
   // On Windows we need to handle this before the bubble is visible or hidden.
   // Please see the comment on the OnWidgetVisibilityChanging function. On
   // other platforms it is fine to handle it after the bubble is shown/hidden.
   HandleVisibilityChanged(widget, visible);
 #endif
@@ skipping 11 matching lines @@
   if (close_on_deactivate() && widget == GetWidget() && !active)
     GetWidget()->Close();
 }
 
 void BubbleDelegateView::OnWidgetBoundsChanged(Widget* widget,
                                                const gfx::Rect& new_bounds) {
   if (move_with_anchor() && anchor_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;
 }
 
 void BubbleDelegateView::StartFade(bool fade_in) {
 #if defined(USE_AURA)
   // Use AURA's window layer animation instead of fading. This ensures that
   // hosts which rely on the layer animation callbacks to close the window
   // work correctly.
   if (fade_in)
     GetWidget()->Show();
@@ skipping 145 matching lines @@
     if (border_widget_)
       border_widget_->ShowInactive();
     if (anchor_widget() && anchor_widget()->GetTopLevelWidget())
       anchor_widget()->GetTopLevelWidget()->DisableInactiveRendering();
   } else {
     if (border_widget_)
       border_widget_->Hide();
   }
 }
 
+void BubbleDelegateView::DetachFromAnchor() {
+  if (anchor_widget_)
+    anchor_widget_->RemoveObserver(this);
+  SetAnchorView(NULL);
+  anchor_widget_ = NULL;
+}
+
 }  // namespace views