Fixed attach logic so that DOMView can be initialized before or after added to Widget.

 * Attach native view in Init if Widget exists.
 * Attach native view when the view is added to Widget if
   it's initialized but not yet attached.

Detach native view when it's removed from parent view so that
it can be re-attached.

BUG=none
TEST=none

Review URL: http://codereview.chromium.org/3152053

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

3 files changed, 109 insertions, 1 deletions

diff --git a/chrome/browser/views/dom_view.cc b/chrome/browser/views/dom_view.cc
index 4725cf9..9c42201 100644
--- a/chrome/browser/views/dom_view.cc
+++ b/chrome/browser/views/dom_view.cc
@@ -23,7 +23,9 @@ bool DOMView::Init(Profile* profile, SiteInstance* instance) {
 
   initialized_ = true;
   tab_contents_.reset(CreateTabContents(profile, instance));
-  views::NativeViewHost::Attach(tab_contents_->GetNativeView());
+  // Attach the native_view now if the view is already added to Widget.
+  if (GetWidget())
+    Attach(tab_contents_->GetNativeView());
   return true;
 }
 
@@ -47,3 +49,14 @@ bool DOMView::SkipDefaultKeyEventProcessing(const views::KeyEvent& e) {
 void DOMView::Focus() {
   tab_contents_->Focus();
 }
+
+void DOMView::ViewHierarchyChanged(bool is_add, views::View* parent,
+                                   views::View* child) {
+  // Attach the native_view when this is added to Widget if
+  // the native view has not been attached yet and tab_contents_ exists.
+  views::NativeViewHost::ViewHierarchyChanged(is_add, parent, child);
+  if (is_add && GetWidget() && !native_view() && tab_contents_.get())
+    Attach(tab_contents_->GetNativeView());
+  else if (!is_add && child == this && native_view())
+    Detach();
+}
diff --git a/chrome/browser/views/dom_view.h b/chrome/browser/views/dom_view.h
index 09d8b2a..4a804f8 100644
--- a/chrome/browser/views/dom_view.h
+++ b/chrome/browser/views/dom_view.h
@@ -40,6 +40,8 @@ class DOMView : public views::NativeViewHost {
   // Overridden from View.
   virtual bool SkipDefaultKeyEventProcessing(const views::KeyEvent& e);
   virtual void Focus();
+  virtual void ViewHierarchyChanged(bool is_add, views::View* parent,
+                                    views::View* child);
 
   // Returns new allocated TabContents instance, caller is responsible deleting.
   // Override in derived classes to replace TabContents with derivative.
diff --git a/chrome/browser/views/dom_view_browsertest.cc b/chrome/browser/views/dom_view_browsertest.cc
new file mode 100644
index 0000000..8aa51b6
--- /dev/null
+++ b/chrome/browser/views/dom_view_browsertest.cc
@@ -0,0 +1,93 @@
+// Copyright (c) 2010 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 "chrome/browser/browser.h"
+#include "chrome/browser/views/dom_view.h"
+#include "chrome/test/in_process_browser_test.h"
+#include "chrome/test/ui_test_utils.h"
+#include "views/widget/root_view.h"
+#include "views/widget/widget.h"
+
+using namespace views;
+
+class DOMViewTest : public InProcessBrowserTest {
+ public:
+  Widget* CreatePopupWindow() {
+    Widget* window =
+        Widget::CreatePopupWidget(Widget::NotTransparent,
+                                  Widget::AcceptEvents,
+                                  Widget::DeleteOnDestroy,
+                                  Widget::DontMirrorOriginInRTL);
+    window->Init(NULL, gfx::Rect(0, 0, 400, 400));
+    return window;
+  }
+};
+
+// Tests if creating and deleting dom_view
+// does not crash and leak memory.
+IN_PROC_BROWSER_TEST_F(DOMViewTest, TestShowAndHide) {
+  Widget* one = CreatePopupWindow();
+
+  DOMView* dom_view = new DOMView();
+  one->GetRootView()->AddChildView(dom_view);
+
+  dom_view->Init(browser()->profile(), NULL);
+  dom_view->LoadURL(GURL("http://www.google.com"));
+  ui_test_utils::WaitForNotification(NotificationType::LOAD_STOP);
+  one->Show();
+
+  ui_test_utils::RunAllPendingInMessageLoop();
+
+  one->Hide();
+}
+
+// Tests if removing from tree then deleting dom_view
+// does not crash and leak memory.
+IN_PROC_BROWSER_TEST_F(DOMViewTest, TestRemoveAndDelete) {
+  Widget* one = CreatePopupWindow();
+
+  DOMView* dom_view = new DOMView();
+  one->GetRootView()->AddChildView(dom_view);
+
+  dom_view->Init(browser()->profile(), NULL);
+  dom_view->LoadURL(GURL("http://www.google.com"));
+  ui_test_utils::WaitForNotification(NotificationType::LOAD_STOP);
+  one->Show();
+
+  ui_test_utils::RunAllPendingInMessageLoop();
+
+  one->GetRootView()->RemoveChildView(dom_view);
+
+  delete dom_view;
+
+  one->Hide();
+}
+
+// Tests if reparenting dom_view does not crash and does not leak
+// memory.
+IN_PROC_BROWSER_TEST_F(DOMViewTest, TestReparent) {
+  Widget* one = CreatePopupWindow();
+
+  DOMView* dom_view = new DOMView();
+  one->GetRootView()->AddChildView(dom_view);
+
+  dom_view->Init(browser()->profile(), NULL);
+  dom_view->LoadURL(GURL("http://www.google.com"));
+  ui_test_utils::WaitForNotification(NotificationType::LOAD_STOP);
+  one->Show();
+
+  ui_test_utils::RunAllPendingInMessageLoop();
+
+  one->GetRootView()->RemoveChildView(dom_view);
+  one->Hide();
+
+  // Re-attach to another Widget.
+  Widget* two = CreatePopupWindow();
+  two->GetRootView()->AddChildView(dom_view);
+  two->Show();
+
+  ui_test_utils::RunAllPendingInMessageLoop();
+
+  two->Hide();
+}