Implement NativeWidgetMac::ReorderNativeViews

ui/views/controls/native/native_view_host_mac.mm

Index: ui/views/controls/native/native_view_host_mac.mm
diff --git a/ui/views/controls/native/native_view_host_mac.mm b/ui/views/controls/native/native_view_host_mac.mm
index da057fa743ad67bb948d225aa0dd65458dc98ea4..02045f0b83ae9645630834ddffb0764133e53bab 100644
--- a/ui/views/controls/native/native_view_host_mac.mm
+++ b/ui/views/controls/native/native_view_host_mac.mm
@@ -6,18 +6,16 @@
 
 #import <Cocoa/Cocoa.h>
 
-#include "base/logging.h"
 #include "base/mac/foundation_util.h"
-#import "ui/views/cocoa/bridged_content_view.h"
+#include "ui/views/cocoa/bridged_native_widget.h"

[tapted, 2016/03/17 08:18:58]

nit: import

 #include "ui/views/controls/native/native_view_host.h"
+#include "ui/views/widget/native_widget_mac.h"
 #include "ui/views/widget/widget.h"
 
 namespace views {
 namespace {
 
-// Reparents |native_view| to be a child of the native content view of
-// |new_parent|.
-void ReparentNSView(NSView* native_view, Widget* new_parent) {
+void EnsureNativeViewHasNoChildWidgets(NSView* native_view) {
   DCHECK(native_view);
   // Mac's NativeViewHost has no support for hosting its own child widgets.
   // This check is probably overly restrictive, since the Widget containing the
@@ -29,17 +27,6 @@ void ReparentNSView(NSView* native_view, Widget* new_parent) {
     Widget::GetAllChildWidgets(native_view, &child_widgets);
     CHECK_GE(1u, child_widgets.size());  // 1 (itself) or 0 if detached.
   }
-
-  if (!new_parent) {
-    [native_view removeFromSuperview];
-    return;
-  }
-
-  BridgedContentView* new_superview =
-      base::mac::ObjCCastStrict<BridgedContentView>(
-          new_parent->GetNativeView());
-  DCHECK(new_superview);
-  [new_superview addSubview:native_view];
 }
 
 }  // namespace
@@ -57,7 +44,12 @@ void NativeViewHostMac::AttachNativeView() {
   DCHECK(host_->native_view());
   DCHECK(!native_view_);
   native_view_.reset([host_->native_view() retain]);
-  ReparentNSView(host_->native_view(), host_->GetWidget());
+
+  EnsureNativeViewHasNoChildWidgets(native_view_.get());
+  BridgedNativeWidget* bridge = NativeWidgetMac::GetBridgeForNativeWindow(
+      host_->GetWidget()->GetNativeWindow());
+  DCHECK(bridge);
+  bridge->SetAssociationForView(host_, native_view_.get());
 }
 
 void NativeViewHostMac::NativeViewDetaching(bool destroyed) {
@@ -70,7 +62,14 @@ void NativeViewHostMac::NativeViewDetaching(bool destroyed) {
   // NativeViewHost::Detach().
   DCHECK(!native_view_ || native_view_ == host_->native_view());
   [host_->native_view() setHidden:YES];
-  ReparentNSView(host_->native_view(), NULL);
+
+  EnsureNativeViewHasNoChildWidgets(host_->native_view());
+  BridgedNativeWidget* bridge = NativeWidgetMac::GetBridgeForNativeWindow(
+      host_->GetWidget()->GetNativeWindow());
+  // BridgedNativeWidget can be null when Widget is closing.
+  if (bridge)

[kirr, 2016/03/16 21:43:45]

I've also tried to use BridgedContentView for holding associative_views, and
there was also problems with removing views in widget destructor.
When using content view FocusTraversalTest fails because NativeViewHost is
content view there.

[tapted, 2016/03/17 08:18:58]

Yeah that makes sense - BridgedNativeWidget is fine. The null check concerns me
a little -- it doesn't surprise me that it's needed, I just wish I was more
certain that there wasn't a subtle problem lurking there. I think it's OK for
now - if there's problems we can try to figure out a way to obtain the
BridgedNativeWidget, or find a way to ensure everything is detached. i.e. it
would be "nice" BridgedNativeWidget could be destroyed without attachments,
since it's a stronger guarantee that there are no lifetime issues.

Does adding a

DCHECK(GetWidget()->IsClosed());

work? That would be nice to have but, again, I wouldn't be surprised if that's
only just *about* to get set.

+    bridge->ClearAssociationForView(host_);
+
   native_view_.reset();
 }