Correct issues with NativeWidgetViews ownership.

NativeWidgetView was owned by the view hierarchy which complicated the ownership of NativeWidgetViews since it would try to delete NativeWidgetViews from its destructor.

NativeWidgetView has been changed to schedule a close when it is removed from its containing hierarchy. This may be problematic for reparenting but should do for now. We will need to finesse the add/remove API to correct this type of issue.

NativeWidgetViews has been hardened in a few places that assumed they could reach up to a parent widget, even after the widget had been removed from its parent.

BUG=none
TEST=attached unittests should all pass for realz (verified in debugger, can be verified on linux views trybots too).
Review URL: http://codereview.chromium.org/7348011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@92246 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 27 insertions, 21 deletions

diff --git a/views/widget/native_widget_views.cc b/views/widget/native_widget_views.cc
index 71ed959..6c6bbcc 100644
--- a/views/widget/native_widget_views.cc
+++ b/views/widget/native_widget_views.cc
@@ -23,29 +23,23 @@ NativeWidgetViews::NativeWidgetViews(internal::NativeWidgetDelegate* delegate)
       minimized_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(close_widget_factory_(this)),
       hosting_widget_(NULL),
-      ownership_(Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET),
-      delete_native_view_(true) {
+      ownership_(Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET) {
 }
 
 NativeWidgetViews::~NativeWidgetViews() {
   delegate_->OnNativeWidgetDestroying();
-  if (delete_native_view_) {
-    view_->parent()->RemoveChildView(view_);
-    // We must prevent the NativeWidgetView from attempting to delete us.
-    view_->set_delete_native_widget(false);
-    delete view_;
-  }
   delegate_->OnNativeWidgetDestroyed();
   if (ownership_ == Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET)
     delete delegate_;
+  view_.reset();
 }
 
 View* NativeWidgetViews::GetView() {
-  return view_;
+  return view_.get();
 }
 
 const View* NativeWidgetViews::GetView() const {
-  return view_;
+  return view_.get();
 }
 
 void NativeWidgetViews::OnActivate(bool active) {
@@ -58,7 +52,7 @@ void NativeWidgetViews::OnActivate(bool active) {
 
 void NativeWidgetViews::InitNativeWidget(const Widget::InitParams& params) {
   ownership_ = params.ownership;
-  view_ = new internal::NativeWidgetView(this);
+  view_.reset(new internal::NativeWidgetView(this));
   view_->SetBoundsRect(params.bounds);
   view_->SetPaintToLayer(true);
 
@@ -70,7 +64,7 @@ void NativeWidgetViews::InitNativeWidget(const Widget::InitParams& params) {
     parent_view = ViewsDelegate::views_delegate->GetDefaultParentView();
     hosting_widget_ = parent_view->GetWidget();
   }
-  parent_view->AddChildView(view_);
+  parent_view->AddChildView(view_.get());
 
   // TODO(beng): SetInitParams().
 }
@@ -109,7 +103,12 @@ gfx::NativeWindow NativeWidgetViews::GetNativeWindow() const {
 Widget* NativeWidgetViews::GetTopLevelWidget() {
   if (view_->parent() == ViewsDelegate::views_delegate->GetDefaultParentView())
     return GetWidget();
-  return view_->GetWidget()->GetTopLevelWidget();
+  // During Widget destruction, this function may be called after |view_| is
+  // detached from a Widget, at which point this NativeWidget's Widget becomes
+  // the effective toplevel.
+  Widget* containing_widget = view_->GetWidget();
+  return containing_widget ? containing_widget->GetTopLevelWidget()
+                           : GetWidget();
 }
 
 const ui::Compositor* NativeWidgetViews::GetCompositor() const {
@@ -130,7 +129,9 @@ void NativeWidgetViews::CalculateOffsetToAncestorWithLayer(gfx::Point* offset,
 }
 
 void NativeWidgetViews::ViewRemoved(View* view) {
-  return GetParentNativeWidget()->ViewRemoved(view);
+  internal::NativeWidgetPrivate* parent = GetParentNativeWidget();
+  if (parent)
+    parent->ViewRemoved(view);
 }
 
 void NativeWidgetViews::SetNativeWindowProperty(const char* name, void* value) {
@@ -143,7 +144,8 @@ void* NativeWidgetViews::GetNativeWindowProperty(const char* name) const {
 }
 
 TooltipManager* NativeWidgetViews::GetTooltipManager() const {
-  return GetParentNativeWidget()->GetTooltipManager();
+  const internal::NativeWidgetPrivate* parent = GetParentNativeWidget();
+  return parent ? parent->GetTooltipManager() : NULL;
 }
 
 bool NativeWidgetViews::IsScreenReaderActive() const {
@@ -246,7 +248,7 @@ void NativeWidgetViews::MoveAbove(gfx::NativeView native_view) {
 }
 
 void NativeWidgetViews::MoveToTop() {
-  view_->parent()->ReorderChildView(view_, -1);
+  view_->parent()->ReorderChildView(view_.get(), -1);
 }
 
 void NativeWidgetViews::SetShape(gfx::NativeRegion region) {
@@ -264,7 +266,7 @@ void NativeWidgetViews::Close() {
 void NativeWidgetViews::CloseNow() {
   // TODO(beng): what about the other case??
   if (ownership_ == Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET)
-    delete view_;
+    delete this;
 }
 
 void NativeWidgetViews::EnableClose(bool enable) {
@@ -396,14 +398,18 @@ void NativeWidgetViews::SetCursor(gfx::NativeCursor cursor) {
 // NativeWidgetViews, private:
 
 internal::NativeWidgetPrivate* NativeWidgetViews::GetParentNativeWidget() {
-  return static_cast<internal::NativeWidgetPrivate*>(
-      view_->GetWidget()->native_widget());
+  Widget* containing_widget = view_->GetWidget();
+  return containing_widget ? static_cast<internal::NativeWidgetPrivate*>(
+      containing_widget->native_widget()) :
+      NULL;
 }
 
 const internal::NativeWidgetPrivate*
     NativeWidgetViews::GetParentNativeWidget() const {
-  return static_cast<const internal::NativeWidgetPrivate*>(
-      view_->GetWidget()->native_widget());
+  const Widget* containing_widget = view_->GetWidget();
+  return containing_widget ? static_cast<const internal::NativeWidgetPrivate*>(
+      containing_widget->native_widget()) :
+      NULL;
 }
 
 }  // namespace views