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

ui/views/widget/native_widget_aura.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/widget/native_widget_aura.h"
 
 #include "base/bind.h"
 #include "base/string_util.h"
 #include "third_party/skia/include/core/SkRegion.h"
 #include "ui/aura/client/activation_change_observer.h"
@@ skipping 90 matching lines @@
   if (params.type == Widget::InitParams::TYPE_BUBBLE)
     aura::client::SetHideOnDeactivate(window_, true);
   window_->SetTransparent(params.transparent);
   window_->Init(params.layer_type);
   if (params.type == Widget::InitParams::TYPE_CONTROL)
     window_->Show();
 
   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.
       gfx::Rect bounds = gfx::Screen::GetScreenFor(parent)->
           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
   // true state/bounds (the LayoutManager may enforce a particular
   // state/bounds).
   if (IsMaximized())
     SetRestoreBounds(window_, window_bounds);
   else
     SetBounds(window_bounds);
   window_->set_ignore_events(!params.accept_events);
@@ skipping 868 matching lines @@
   Widget::Widgets widgets;
   GetAllChildWidgets(native_view, &widgets);
 
   // First notify all the widgets that they are being disassociated
   // from their previous parent.
   for (Widget::Widgets::iterator it = widgets.begin();
       it != widgets.end(); ++it) {
     (*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();
       it != widgets.end(); ++it) {
     (*it)->NotifyNativeViewHierarchyChanged(true, new_parent);
   }
 }
 
 // static
 bool NativeWidgetPrivate::IsMouseButtonDown() {
   return aura::Env::GetInstance()->is_mouse_button_down();
 }
 
 // static
 bool NativeWidgetPrivate::IsTouchDown() {
   return aura::Env::GetInstance()->is_touch_down();
 }
 
 }  // namespace internal
 }  // namespace views