Fix WidgetTest.ChildStackedRelativeToParent on 10.13

Since 10.13, a trip to the runloop seems to be been necessary to ensure
[NSApp orderedWindows] updates inside WidgetTest::IsWindowStackedAbove().

Unforunately, flushing the runloop exposes the test to window server
events which can cause asynchronous activation events to be interleaved
with other parts of the test. To deflake, run the test interactively,
and sync with the window server whenever a new window is raised.

Earlier attempt in https://crrev.com/c/590332 caused flakes on 10.12.

Bug: 749905
Change-Id: Ifde8800e0966233cfeea41515b66b0fc2926317d
Reviewed-on: https://chromium-review.googlesource.com/899806Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534582}

ui/views/test/widget_test.cc

@@ -26,6 +26,10 @@ Widget* CreateTopLevelPlatformWidgetWithStubbedCapture(
 
 }  // namespace
 
+void WidgetTest::WidgetCloser::operator()(Widget* widget) const {
+  widget->CloseNow();
+}
+
 WidgetTest::WidgetTest() {}
 WidgetTest::~WidgetTest() {}
 

ui/views/test/widget_test.h

@@ -5,6 +5,8 @@
 #ifndef UI_VIEWS_TEST_WIDGET_TEST_H_
 #define UI_VIEWS_TEST_WIDGET_TEST_H_
 
+#include <memory>
+
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "build/build_config.h"
@@ -34,6 +36,14 @@ namespace test {
 
 class WidgetTest : public ViewsTestBase {
  public:
+  // This class can be used as a deleter for std::unique_ptr<Widget>
+  // to call function Widget::CloseNow automatically.
+  struct WidgetCloser {
+    void operator()(Widget* widget) const;
+  };
+
+  using WidgetAutoclosePtr = std::unique_ptr<Widget, WidgetCloser>;
+
   WidgetTest();
   ~WidgetTest() override;
 

ui/views/test/widget_test_mac.mm

@@ -48,6 +48,10 @@ bool WidgetTest::IsNativeWindowVisible(gfx::NativeWindow window) {
 
 // static
 bool WidgetTest::IsWindowStackedAbove(Widget* above, Widget* below) {
+  // Since 10.13, a trip to the runloop has been necessary to ensure [NSApp
+  // orderedWindows] has been updated.
+  base::RunLoop().RunUntilIdle();
+
   EXPECT_TRUE(above->IsVisible());
   EXPECT_TRUE(below->IsVisible());
 

ui/views/widget/widget_interactive_uitest.cc

@@ -641,6 +641,82 @@ TEST_F(WidgetTestInteractive, ViewFocusOnWidgetActivationChanges) {
   widget2->CloseNow();
 }
 
+// Test z-order of child widgets relative to their parent.
+TEST_F(WidgetTestInteractive, ChildStackedRelativeToParent) {
+  WidgetAutoclosePtr parent(CreateTopLevelPlatformWidget());
+  Widget* child = CreateChildPlatformWidget(parent->GetNativeView());
+
+  parent->SetBounds(gfx::Rect(160, 100, 320, 200));
+  child->SetBounds(gfx::Rect(50, 50, 30, 20));
+
+  // Child shown first. Initially not visible, but on top of parent when shown.
+  // Use ShowInactive whenever showing the child, otherwise the usual activation
+  // logic will just put it on top anyway. Here, we want to ensure it is on top
+  // of its parent regardless.
+  child->ShowInactive();
+  EXPECT_FALSE(child->IsVisible());
+
+  ShowSync(parent.get());
+  EXPECT_TRUE(child->IsVisible());
+  EXPECT_TRUE(IsWindowStackedAbove(child, parent.get()));
+  EXPECT_FALSE(IsWindowStackedAbove(parent.get(), child));  // Sanity check.
+
+  WidgetAutoclosePtr popover(CreateTopLevelPlatformWidget());
+  popover->SetBounds(gfx::Rect(150, 90, 340, 240));
+  ShowSync(popover.get());
+
+  // NOTE: for aura-mus-client stacking of top-levels is not maintained in the
+  // client, so z-order of top-levels can't be determined.
+  const bool check_toplevel_z_order = !IsMus();
+  if (check_toplevel_z_order)
+    EXPECT_TRUE(IsWindowStackedAbove(popover.get(), child));
+  EXPECT_TRUE(IsWindowStackedAbove(child, parent.get()));
+
+  // Showing the parent again should raise it and its child above the popover.
+  ShowSync(parent.get());
+  EXPECT_TRUE(IsWindowStackedAbove(child, parent.get()));
+  if (check_toplevel_z_order)
+    EXPECT_TRUE(IsWindowStackedAbove(parent.get(), popover.get()));
+
+  // Test grandchildren.
+  Widget* grandchild = CreateChildPlatformWidget(child->GetNativeView());
+  grandchild->SetBounds(gfx::Rect(5, 5, 15, 10));
+  grandchild->ShowInactive();
+  EXPECT_TRUE(IsWindowStackedAbove(grandchild, child));
+  EXPECT_TRUE(IsWindowStackedAbove(child, parent.get()));
+  if (check_toplevel_z_order)
+    EXPECT_TRUE(IsWindowStackedAbove(parent.get(), popover.get()));
+
+  ShowSync(popover.get());
+  if (check_toplevel_z_order)
+    EXPECT_TRUE(IsWindowStackedAbove(popover.get(), grandchild));
+  EXPECT_TRUE(IsWindowStackedAbove(grandchild, child));
+
+  ShowSync(parent.get());
+  EXPECT_TRUE(IsWindowStackedAbove(grandchild, child));
+  if (check_toplevel_z_order)
+    EXPECT_TRUE(IsWindowStackedAbove(child, popover.get()));
+
+  // Test hiding and reshowing.
+  parent->Hide();
+  EXPECT_FALSE(grandchild->IsVisible());
+  ShowSync(parent.get());
+
+  EXPECT_TRUE(IsWindowStackedAbove(grandchild, child));
+  EXPECT_TRUE(IsWindowStackedAbove(child, parent.get()));
+  if (check_toplevel_z_order)
+    EXPECT_TRUE(IsWindowStackedAbove(parent.get(), popover.get()));
+
+  grandchild->Hide();
+  EXPECT_FALSE(grandchild->IsVisible());
+  grandchild->ShowInactive();
+
+  EXPECT_TRUE(IsWindowStackedAbove(grandchild, child));
+  EXPECT_TRUE(IsWindowStackedAbove(child, parent.get()));
+  if (check_toplevel_z_order)
+    EXPECT_TRUE(IsWindowStackedAbove(parent.get(), popover.get()));
+}
+
 #if defined(OS_WIN)
 
 // Test view focus retention when a widget's HWND is disabled and re-enabled.

ui/views/widget/widget_unittest.cc

@@ -61,12 +61,6 @@ gfx::Point ConvertPointFromWidgetToView(View* view, const gfx::Point& p) {
   return tmp;
 }
 
-// This class can be used as a deleter for std::unique_ptr<Widget>
-// to call function Widget::CloseNow automatically.
-struct WidgetCloser {
-  inline void operator()(Widget* widget) const { widget->CloseNow(); }
-};
-
 class TestBubbleDialogDelegateView : public BubbleDialogDelegateView {
  public:
   TestBubbleDialogDelegateView(View* anchor)
@@ -82,8 +76,6 @@ class TestBubbleDialogDelegateView : public BubbleDialogDelegateView {
   mutable bool reset_controls_called_;
 };
 
-using WidgetAutoclosePtr = std::unique_ptr<Widget, WidgetCloser>;
-
 }  // namespace
 
 // A view that keeps track of the events it receives, and consumes all scroll
@@ -286,82 +278,6 @@ TEST_F(WidgetTest, ChildBoundsRelativeToParent) {
   EXPECT_EQ(toplevel_bounds, child->GetWindowBoundsInScreen());
 }
 
-// Test z-order of child widgets relative to their parent.
-TEST_F(WidgetTest, ChildStackedRelativeToParent) {
-  WidgetAutoclosePtr parent(CreateTopLevelPlatformWidget());
-  Widget* child = CreateChildPlatformWidget(parent->GetNativeView());
-
-  parent->SetBounds(gfx::Rect(160, 100, 320, 200));
-  child->SetBounds(gfx::Rect(50, 50, 30, 20));
-
-  // Child shown first. Initially not visible, but on top of parent when shown.
-  // Use ShowInactive whenever showing the child, otherwise the usual activation
-  // logic will just put it on top anyway. Here, we want to ensure it is on top
-  // of its parent regardless.
-  child->ShowInactive();
-  EXPECT_FALSE(child->IsVisible());
-
-  parent->Show();
-  EXPECT_TRUE(child->IsVisible());
-  EXPECT_TRUE(IsWindowStackedAbove(child, parent.get()));
-  EXPECT_FALSE(IsWindowStackedAbove(parent.get(), child));  // Sanity check.
-
-  WidgetAutoclosePtr popover(CreateTopLevelPlatformWidget());
-  popover->SetBounds(gfx::Rect(150, 90, 340, 240));
-  popover->Show();
-
-  // NOTE: for aura-mus-client stacking of top-levels is not maintained in the
-  // client, so z-order of top-levels can't be determined.
-  const bool check_toplevel_z_order = !IsMus();
-  if (check_toplevel_z_order)
-    EXPECT_TRUE(IsWindowStackedAbove(popover.get(), child));
-  EXPECT_TRUE(IsWindowStackedAbove(child, parent.get()));
-
-  // Showing the parent again should raise it and its child above the popover.
-  parent->Show();
-  EXPECT_TRUE(IsWindowStackedAbove(child, parent.get()));
-  if (check_toplevel_z_order)
-    EXPECT_TRUE(IsWindowStackedAbove(parent.get(), popover.get()));
-
-  // Test grandchildren.
-  Widget* grandchild = CreateChildPlatformWidget(child->GetNativeView());
-  grandchild->SetBounds(gfx::Rect(5, 5, 15, 10));
-  grandchild->ShowInactive();
-  EXPECT_TRUE(IsWindowStackedAbove(grandchild, child));
-  EXPECT_TRUE(IsWindowStackedAbove(child, parent.get()));
-  if (check_toplevel_z_order)
-    EXPECT_TRUE(IsWindowStackedAbove(parent.get(), popover.get()));
-
-  popover->Show();
-  if (check_toplevel_z_order)
-    EXPECT_TRUE(IsWindowStackedAbove(popover.get(), grandchild));
-  EXPECT_TRUE(IsWindowStackedAbove(grandchild, child));
-
-  parent->Show();
-  EXPECT_TRUE(IsWindowStackedAbove(grandchild, child));
-  if (check_toplevel_z_order)
-    EXPECT_TRUE(IsWindowStackedAbove(child, popover.get()));
-
-  // Test hiding and reshowing.
-  parent->Hide();
-  EXPECT_FALSE(grandchild->IsVisible());
-  parent->Show();
-
-  EXPECT_TRUE(IsWindowStackedAbove(grandchild, child));
-  EXPECT_TRUE(IsWindowStackedAbove(child, parent.get()));
-  if (check_toplevel_z_order)
-    EXPECT_TRUE(IsWindowStackedAbove(parent.get(), popover.get()));
-
-  grandchild->Hide();
-  EXPECT_FALSE(grandchild->IsVisible());
-  grandchild->ShowInactive();
-
-  EXPECT_TRUE(IsWindowStackedAbove(grandchild, child));
-  EXPECT_TRUE(IsWindowStackedAbove(child, parent.get()));
-  if (check_toplevel_z_order)
-    EXPECT_TRUE(IsWindowStackedAbove(parent.get(), popover.get()));
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // Widget ownership tests.
 //
@@ -2116,7 +2032,7 @@ bool RunGetNativeThemeFromDestructor(const Widget::InitParams& in_params,
                                      bool is_first_run) {
   bool needs_second_run = false;
   // Destroyed by CloseNow() below.
-  WidgetAutoclosePtr widget(new Widget);
+  WidgetTest::WidgetAutoclosePtr widget(new Widget);
   Widget::InitParams params(in_params);
   // Deletes itself when the Widget is destroyed.
   params.delegate = new GetNativeThemeFromDestructorView;