Implement NativeWidgetMac::ReorderNativeViews

ui/views/widget/native_widget_mac_unittest.mm

Index: ui/views/widget/native_widget_mac_unittest.mm
diff --git a/ui/views/widget/native_widget_mac_unittest.mm b/ui/views/widget/native_widget_mac_unittest.mm
index e62394eeb9495cae774ef8a765be0f63bfeb9679..344fc93a9349013cbfe076f113d0cd0c24672965 100644
--- a/ui/views/widget/native_widget_mac_unittest.mm
+++ b/ui/views/widget/native_widget_mac_unittest.mm
@@ -27,6 +27,7 @@
 #import "ui/views/cocoa/native_widget_mac_nswindow.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/label.h"
+#include "ui/views/controls/native/native_view_host.h"
 #include "ui/views/native_cursor.h"
 #include "ui/views/test/native_widget_factory.h"
 #include "ui/views/test/test_widget_observer.h"
@@ -211,6 +212,43 @@ class WidgetChangeObserver : public TestWidgetObserver {
   DISALLOW_COPY_AND_ASSIGN(WidgetChangeObserver);
 };
 
+class NativeHostHolder {
+ public:
+  NativeHostHolder(NSView* view, NativeViewHost* host)
+      : view(view), host(host) {
+    host->set_owned_by_client();
+  }
+
+  void AttachNativeView() {
+    DCHECK(!host->native_view());
+    host->Attach(view.get());
+  }
+
+  void Detach() { host->Detach(); }
+
+  gfx::NativeView GetView() const { return view.get(); }

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

nit: these accessors can be in hacker_style -- that's more typical for inline
stuff. i.e. GetView() -> view()

+  NativeViewHost* GetHost() const { return host.get(); }

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

GetHost -> host()

+
+ private:
+  base::scoped_nsobject<NSView> view;

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

view->view_

+  scoped_ptr<NativeViewHost> host;

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

host->host_

+};

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

nit: DISALLOW_COPY_AND_ASSIGN(..)

+
+void CheckViewsOrderEqual(NSArray* actual, NSArray* expected) {

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

Comment why this can't just be isEqualToArray.  (I'm assuming because there are
`extra` views in there, but where do they come from?)

+  if ([expected count] == 1) {
+    EXPECT_TRUE([actual containsObject:[expected firstObject]]);
+    return;
+  }
+
+  for (NSUInteger i = 0; i < [expected count] - 1; ++i) {
+    NSUInteger first = [actual indexOfObject:[expected objectAtIndex:i]];
+    NSUInteger second = [actual indexOfObject:[expected objectAtIndex:i + 1]];
+    EXPECT_LT(first, second);
+    EXPECT_NE(first, NSNotFound);
+    EXPECT_NE(second, NSNotFound);
+  }
+}
+
 // Test visibility states triggered externally.
 TEST_F(NativeWidgetMacTest, HideAndShowExternally) {
   Widget* widget = CreateTopLevelPlatformWidget();
@@ -1336,6 +1374,37 @@ TEST_F(NativeWidgetMacTest, ChangeFocusOnChangeFirstResponder) {
   widget->CloseNow();
 }
 
+// Test that NativeViewHost::Attach/Detach method save NativeView z-order

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

nit:  Test that NativeViewHost::Attach()/Detach() method saves the NativeView
z-order

+TEST_F(NativeWidgetMacTest, NativeViewsOrder) {
+  Widget* widget = CreateTopLevelPlatformWidget();
+
+  const int native_views_count = 3;

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

optional nit: kNativeViewCount

+  std::vector<scoped_ptr<NativeHostHolder>> hosts;
+  for (int i = 0; i < native_views_count; ++i) {
+    scoped_ptr<NativeHostHolder> holder(
+        new NativeHostHolder([[NSView alloc] init], new NativeViewHost));

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

it would be nicer to move the initialization into NativeHostHolder -- we try not
to pass ownership of objects via raw pointers. NativeHostHolder can just have a
default constructor.

+    widget->GetRootView()->AddChildView(holder->GetHost());
+    holder->AttachNativeView();
+    hosts.push_back(std::move(holder));
+  }
+  CheckViewsOrderEqual(
+      [widget->GetNativeView() subviews],
+      [NSArray arrayWithObjects:hosts[0]->GetView(), hosts[1]->GetView(),

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

you can use an array literal, like

@[ hosts[0]->view(), hosts[1]->view(), hosts[2]->view() ]

+                                hosts[2]->GetView(), nil]);
+
+  hosts[1]->Detach();
+  CheckViewsOrderEqual(
+      [widget->GetNativeView() subviews],
+      [NSArray arrayWithObjects:hosts[0]->GetView(), hosts[2]->GetView(), nil]);
+
+  hosts[1]->AttachNativeView();
+  CheckViewsOrderEqual(
+      [widget->GetNativeView() subviews],
+      [NSArray arrayWithObjects:hosts[0]->GetView(), hosts[1]->GetView(),
+                                hosts[2]->GetView(), nil]);
+  widget->CloseNow();
+}
+

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

I think there should be a bit more coverage. There's maybe some ideas in
window_reorderer_unittest.cc, but they seem overly complicated.

Perhaps just something that populates a Widget::GetContentView() with the views
in a NativeHostHolder using AddChildView/AddChildViewAt. Then call
Widget::GetContentView::ReorderChildView and ensure the native subview order is
preserved. Perhaps add a couple of unassociated native NSViews to ensure they
stack at the top, too.

Note scoped_ptr in NativeHostHolder could get in the way - you could either
explicitly do View::DoRemoveChildView(..) to prevent deletion, or just ensure
|hosts| is destroyed before the widget is closed -- that should delete all the
views and auto-remove them from the hierarchy.

 }  // namespace test
 }  // namespace views