MacViews: Attach child windows when parent is on the screen.

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"
@@ skipping 355 matching lines @@
   int paint_count() { return paint_count_; }
 
  private:
   int paint_count_ = 0;
   int target_paint_count_ = 0;
   base::RunLoop* run_loop_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(PaintCountView);
 };
 
+// Test for correct child window restore when parent window is minimized
+// and restored using makeKeyAndOrderFront.

[tapted, 2016/06/22 04:09:15]

nit: -makeKeyAndOrderFront:.

[kirr, 2016/06/22 12:59:03]

Done.

+// Child window could become occluded (because of cocoa bug?).

[tapted, 2016/06/22 04:09:14]

You can say: 

Parent-child window relationships in AppKit are not supported when window
visibility changes.

[kirr, 2016/06/22 12:59:03]

Done.

+TEST_F(NativeWidgetMacTest, OrderFrontAfterMiniturize) {

[tapted, 2016/06/22 04:09:14]

Miniturize -> Miniaturize (or Minimize)

[kirr, 2016/06/22 12:59:04]

Done.

+  Widget* widget = new Widget;

[tapted, 2016/06/22 04:09:14]

You should be able to avoid using InitParams with something like

Widget* parent = CreateTopLevelPlatformWidget();

[kirr, 2016/06/22 12:59:04]

Done.

+  Widget::InitParams init_params(Widget::InitParams::TYPE_WINDOW);
+  widget->Init(init_params);
+  NSWindow* ns_window = widget->GetNativeWindow();
+
+  Widget* child_widget = new Widget;

[tapted, 2016/06/22 04:09:14]

  Widget* child = CreateChildPlatformWidget(widget->GetNativeView());

[kirr, 2016/06/22 12:59:04]

Done.

+  Widget::InitParams child_params(Widget::InitParams::TYPE_CONTROL);
+  child_params.parent = widget->GetNativeView();
+  child_widget->Init(child_params);
+  NSWindow* child_ns_window = child_widget->GetNativeWindow();
+
+  // Set parent bounds that overlap child.
+  widget->SetBounds(gfx::Rect(100, 100, 300, 300));
+  child_widget->SetBounds(gfx::Rect(110, 110, 100, 100));

[tapted, 2016/06/22 04:09:14]

note with TYPE_CONTROL, I think the bounds will be relative to the parent (not
screen origin), but it will overlap either way, so this should be fine.

+
+  widget->Show();

[tapted, 2016/06/22 04:09:14]

show the child too?

[kirr, 2016/06/22 12:59:04]

I think no.
Test visually works fine without it.
But without widget_->Show() widget stay inactive.

[tapted, 2016/06/23 12:09:26]

hm - I'm pretty sure you need to show it. It's valid to have a child that's
hidden. If it's getting shown without it that might be a bug. Compare it to
WidgetTest.ChildStackedRelativeToParent

+  base::RunLoop().RunUntilIdle();

[tapted, 2016/06/22 04:09:14]

is this needed?

[kirr, 2016/06/22 12:59:04]

Yes, seems like it is also related to window activation.
All changes with - orderWindow:relativeTo: are sync.
But if I remove these lines test starts passing on initial code. And window
order is fine.

+
+  EXPECT_FALSE(widget->IsMinimized());
+
+  // Minimize parent.
+  [ns_window performMiniaturize:nil];
+  base::RunLoop().RunUntilIdle();

[tapted, 2016/06/22 04:09:14]

this shouldn't be needed - miniaturize is synchronous

[kirr, 2016/06/22 12:59:04]

It's also not for the expectations below, bot for occlusion state.
I've not investigated what exactly happens there, sorry.

+
+  EXPECT_TRUE(widget->IsMinimized());
+  EXPECT_FALSE(widget->IsVisible());
+  EXPECT_FALSE(child_widget->IsVisible());
+
+  // Restore parent window as AppController does.
+  [ns_window makeKeyAndOrderFront:nil];
+
+  // Wait and check that child is really visible.
+  // TODO(kirr): remove the fixed timeout.
+  base::MessageLoop::current()->PostDelayedTask(

[kirr, 2016/06/21 16:33:47]

I've added the test to show the problem.
But I don't know how to make it without fixed delay, because |child_ns_window|
become occluded at the end of system restore window animation.
Do you have any idea how to remove the delay? Maybe we can go without a test?

[tapted, 2016/06/22 04:09:14]

Can you get a stack trace from the visibility change? It should all happen under
makeKeyAndOrderFront -- the deminiaturize animation on Mac runs synchronously
AFAIK.

Otherwise you should be able to use WidgetChangeObserver and
`WaitForVisibileCounts` which is declared in this file 

[kirr, 2016/06/22 12:59:04]

Here is NSNotifications trace for the test (+ orderWindow:relativeTo:) log.
https://gist.github.com/kirr/cecc8a8f922cc62a568dc4f48e6d0143

Change occlusion state is async. I've tried to wait for second
NSWindowOcclusionStateChangedNotification for child window, and it's working
(test is fails on old code). But the problem is to find some point in a new
code.

[tapted, 2016/06/23 12:09:26]

You can get the stack with something like base::debug::StackTrace().Print(), or
stopping in a debugger. We're assuming the fix works because, now, when the
child changes visibility according to -[NSWindow isVisible], it's safe to
re-attach it as a child. Whereas before we tried to reattach it when we "knew"
it was displayed but -[NSWindow isVisible] still reports NO (lying to us...). So
the stack at the point that we try to re-attach the child should tell us what
the trigger is for AppKit finally deciding to say the window isVisible -- if the
call to makeKeyAndOrderFront on the parent window is in the stack then it's
synchronous. But if it's a message from the window server we need something
else.

(And, generally, we don't use NSWindowOcclusionStateChangedNotification anywhere
in the release code for MacViews yet, so we really shouldn't need it in the
test. We _do_ use NSWindowDidBecomeKeyNotification, which might be a better
thing to look for - but see comments below).

+      FROM_HERE, base::MessageLoop::QuitWhenIdleClosure(),
+      base::TimeDelta::FromSeconds(2));
+  base::MessageLoop::current()->Run();
+
+  EXPECT_FALSE(widget->IsMinimized());
+  EXPECT_TRUE(widget->IsVisible());
+  EXPECT_TRUE(child_widget->IsVisible());
+  // Check that child window is visible.
+  EXPECT_TRUE([child_ns_window occlusionState] & NSWindowOcclusionStateVisible);

[tapted, 2016/06/22 04:09:14]

ah, occlusion states are updated from the window server, so that actually is
asynchronous. You could use WindowedNSNotificationObserver, but I think we just
need to check the window z-order, which you can do with 

 WidgetTest::IsWindowStackedAbove(..)

[kirr, 2016/06/22 12:59:04]

WidgetTest::IsWindowStackedAbove(..) returns the same result as occlusion state.
So I've added this to.
Unfortunately it's not helped to sync things.

Test should work fine when there is no bug and should fails or become flaky if
not.

[tapted, 2016/06/23 12:09:25]

So, like you probably saw, we can't guarantee that a single iteration of
base::RunLoop().RunUntilIdle() is sufficient to process all the events when the
window server is involved - there can be latency. Sadly, we can't use a delayed
task either - the trybot running the test might be under epic load (2 seconds
might not be enough), and idle delays are bad in any case since they take up
resources unnecessarily.

Another problem is that if we need to rely on occlusion states, we can't
guarantee that another test running at the same time doesn't show its own window
and does weird things to the occlusion. views_unittests typically gets run with
some 5-20 threads simultaneously for better throughput. So this can also result
in a flaky test.

Testing the relative z-order of the windows should't be affected by the latter,
and should be OK in views_unittests.

But if we need to rely on occlusion or activation of the window, the test needs
to go in an interactive UI test -- e.g. native_widget_mac_interactive_uitest.mm
-- these always run one-at-a-time on a machine. There's a `ShowKeyWindow`
function there which shows a window and waits for it to become the key window,
which is reported by the window server. If that's a reliable trigger for the
behaviour you're testing, then you might be able to use that.

But we can't commit a test with a fixed delay - it's too likely to mess with the
CQ, causing flakes.

[kirr, 2016/06/23 16:20:11]

Ok, I agree that we can't commit with a fixed delay.
But it's seems really tricky to go without it.

Here is interactive test with logs:
http://pastebin.com/k5eRZbfS

Here is a log for this test on old code:
http://pastebin.com/GdHHb9qF

As you can see on window restore NSWindow first sent  NSWindowDidBecomeKey, then
going _NSWindowDidBecomeVisible etc and then several NSWindowOcclusion messages.
All occlusion messages are async. I leave one stack for example.

Cocoa bug - that on restore firstly both windows is visible and only after some
delay child window order changed.

+  widget->Close();
+}
+
 // Test minimized states triggered externally, implied visibility and restored
 // bounds whilst minimized.
 TEST_F(NativeWidgetMacTest, MiniaturizeExternally) {
   Widget* widget = new Widget;
   Widget::InitParams init_params(Widget::InitParams::TYPE_WINDOW);
   widget->Init(init_params);
 
   PaintCountView* view = new PaintCountView();
   widget->GetContentsView()->AddChildView(view);
   NSWindow* ns_window = widget->GetNativeWindow();
@@ skipping 537 matching lines @@
   // -[NSWindow orderOut:] call that is triggered from -[ViewsNSWindowDelegate
   // sheetDidEnd:]. |sheet_widget| will be destroyed next, so it's still safe to
   // use in the block. However, since the orderOut just happened, it's not very
   // interesting.
   observer = [[NSNotificationCenter defaultCenter]
       addObserverForName:NSWindowDidEndSheetNotification
                   object:native_parent
                    queue:nil
               usingBlock:^(NSNotification* note) {
                 EXPECT_TRUE([sheet_window delegate]);
-                EXPECT_FALSE(sheet_widget->IsVisible());
-                EXPECT_FALSE(sheet_widget->GetLayer()->IsDrawn());
                 *did_observe_ptr = true;
               }];
 
   // Pump in order to trigger -[NSWindow endSheet:..], which will block while
   // the animation runs, then delete |sheet_widget|.
   TestWidgetObserver widget_observer(sheet_widget);
   EXPECT_TRUE([sheet_window delegate]);
   base::RunLoop().RunUntilIdle();
   EXPECT_FALSE([sheet_window delegate]);
 
@@ skipping 640 matching lines @@
   lastDirtyRect_ = dirtyRect;
 }
 
 @end
 
 @implementation FocusableTestNSView
 - (BOOL)acceptsFirstResponder {
   return YES;
 }
 @end