Implement NativeWidgetMac::ReorderNativeViews

ui/views/widget/native_widget_mac_unittest.mm

 // Copyright 2014 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.
 
 #import "ui/views/widget/native_widget_mac.h"
 
 #import <Cocoa/Cocoa.h>
 
 #import "base/mac/foundation_util.h"
 #import "base/mac/scoped_nsobject.h"
 #import "base/mac/scoped_objc_class_swizzler.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/test_timeouts.h"
 #import "testing/gtest_mac.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #import "ui/base/cocoa/constrained_window/constrained_window_animation.h"
 #import "ui/base/cocoa/window_size_constants.h"
 #import "ui/events/test/cocoa_test_event_utils.h"
 #include "ui/events/test/event_generator.h"
 #import "ui/gfx/mac/coordinate_conversion.h"
 #include "ui/views/bubble/bubble_delegate.h"
 #import "ui/views/cocoa/bridged_native_widget.h"
 #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"
 #include "ui/views/test/widget_test.h"
 #include "ui/views/widget/native_widget_mac.h"
 #include "ui/views/widget/native_widget_private.h"
 #include "ui/views/window/dialog_delegate.h"
 
 // Donates an implementation of -[NSAnimation stopAnimation] which calls the
 // original implementation, then quits a nested run loop.
@@ skipping 164 matching lines @@
 
   int gained_visible_count_ = 0;
   int lost_visible_count_ = 0;
   int target_gained_visible_count_ = 0;
   int target_lost_visible_count_ = 0;
   base::RunLoop* run_loop_ = nullptr;
 
   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();
   NSWindow* ns_window = widget->GetNativeWindow();
   WidgetChangeObserver observer(widget);
 
   // Should initially be hidden.
   EXPECT_FALSE(widget->IsVisible());
   EXPECT_FALSE([ns_window isVisible]);
   EXPECT_EQ(0, observer.gained_visible_count());
@@ skipping 1105 matching lines @@
 
   [widget->GetNativeWindow() makeFirstResponder:child_view];
   EXPECT_FALSE(manager->GetFocusedView());
 
   [widget->GetNativeWindow() makeFirstResponder:widget->GetNativeView()];
   EXPECT_EQ(manager->GetFocusedView(), widget->GetRootView());
 
   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
 
 @implementation TestStopAnimationWaiter
 - (void)setWindowStateForEnd {
   views::test::ScopedSwizzleWaiter::GetMethodAndMarkCalled()(self, _cmd);
 }
 @end
 
 @implementation NativeWidgetMacTestWindow
@@ skipping 17 matching lines @@
   lastDirtyRect_ = dirtyRect;
 }
 
 @end
 
 @implementation FocusableTestNSView
 - (BOOL)acceptsFirstResponder {
   return YES;
 }
 @end