Desktop aura: Break aura::Window::SetParent in two.

ui/views/widget/native_widget_aura.cc

Index: ui/views/widget/native_widget_aura.cc
diff --git a/ui/views/widget/native_widget_aura.cc b/ui/views/widget/native_widget_aura.cc
index c9b9cfda763f18630d39b096a1ea475a8ee8cfe7..7b120c54d009fa529ccbd0f263c128947bb1a380 100644
--- a/ui/views/widget/native_widget_aura.cc
+++ b/ui/views/widget/native_widget_aura.cc
@@ -108,24 +108,18 @@ void NativeWidgetAura::InitNativeWidget(const Widget::InitParams& params) {
   delegate_->OnNativeWidgetCreated();
 
   gfx::Rect window_bounds = params.bounds;
-  if (params.child) {
-    window_->SetParent(params.GetParent());
-  } else {
+  gfx::NativeView parent = params.GetParent();
+  if (!params.child) {
     // Set up the transient child before the window is added. This way the
     // LayoutManager knows the window has a transient parent.
-    gfx::NativeView parent = params.GetParent();
     if (parent && parent->type() != aura::client::WINDOW_TYPE_UNKNOWN) {
       parent->AddTransientChild(window_);
       parent = NULL;
     }
     // SetAlwaysOnTop before SetParent so that always-on-top container is used.
     SetAlwaysOnTop(params.keep_on_top);
-    // If the parent is not specified, find the default parent for
-    // the |window_| using the desired |window_bounds|.
-    if (!parent) {
-      parent = aura::client::GetStackingClient(params.GetParent())->
-          GetDefaultParent(params.context, window_, window_bounds);
-    } else if (window_bounds == gfx::Rect()) {
+    // Make sure we have a real |window_bounds|.
+    if (window_bounds == gfx::Rect()) {
       // If a parent is specified but no bounds are given,
       // use the origin of the parent's display so that the widget
       // will be added to the same display as the parent.
@@ -133,7 +127,16 @@ void NativeWidgetAura::InitNativeWidget(const Widget::InitParams& params) {
           GetDisplayNearestWindow(parent).bounds();
       window_bounds.set_origin(bounds.origin());
     }
-    window_->SetParent(parent);
+  }
+
+  if (parent) {
+    parent->AddChild(window_);
+  } else {
+    // TODO(erg): Remove this NULL check once we've made everything in views
+    // actually pass us a context.
+    aura::RootWindow* root_window =
+        params.context ? params.context->GetRootWindow() : NULL;
+    window_->SetDefaultParentByTargetRoot(root_window, window_bounds);
   }
 
   // Wait to set the bounds until we have a parent. That way we can know our
@@ -1022,7 +1025,18 @@ void NativeWidgetPrivate::ReparentNativeView(gfx::NativeView native_view,
     (*it)->NotifyNativeViewHierarchyChanged(false, previous_parent);
   }
 
-  native_view->SetParent(new_parent);
+  if (new_parent) {
+    new_parent->AddChild(native_view);
+  } else {
+    // I am unsure what we should really be doing here. The following looks
+    // weird, but it's the equivalent of what aura has always done. (The
+    // previous behaviour of aura::Window::SetParent() used NULL as a special
+    // value that meant ask the StackingClient where things should go.) jam@
+    // reasonably thought that was stupid and replaced it with a RemoveChild
+    // call, which broke things hard.
+    native_view->SetDefaultParentByTargetRoot(native_view->GetRootWindow(),

[sky, 2012/11/21 23:35:59]

I think this is wrong. In particular this means parenting to NULL leaves
|native_view| in the same root. If the root is destroyed, then |native_view| is
destroyed too, which we don't want.

[Elliot Glaysher, 2012/11/22 00:26:56]

So currently this works because the StackingClient is a singleton (and will be
for a few more patches). Are you suggesting using |new_parent|'s root window for
context here?

[sky, 2012/11/22 00:59:50]

The problem is new_parent is NULL. I'm assuming SetDefaultParentbyTargetRoot is
going to result in leaving native_view in the same RootView, right? That is what
I'm saying is wrong. Maybe I'm missing something.

[Elliot Glaysher, 2012/11/26 21:04:54]

After talking to scottmg and jam, I'm convinced that this is  equivalent to what
we do today, and the best we can do without some more major overhauls.

According to those two, it's an invariant that we have to be attached to a
RootWindow, which is what the previous SetParent(NULL) call does: it asks a
global where to place the window in some RootWindow.

This will usually be the same RootWindow as |native_view|.

[sky, 2012/11/26 22:29:09]

I agree. I think it's just your comment that needs updating here.

+                                              gfx::Rect());
+  }
 
   // And now, notify them that they have a brand new parent.
   for (Widget::Widgets::iterator it = widgets.begin();