Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2069103004:
MacViews: Attach child windows when parent is on the screen. (Closed)
DescriptionMacViews: Ensure child windows are only managed with real changes to
-[NSWindow isVisible].
After restoring from a miniaturize, child windows are currently reattached too
early. This is because AppKit draws the window before updating the result of
-[NSWindow isVisible], and we need to participate in that draw. But updating
child window relationships at this time causes layering issues when the window
has finished restoring.
BUG=620266
Committed: https://crrev.com/7a3314d270102d9a6dc6572846f406290021d776
Cr-Commit-Position: refs/heads/master@{#403166}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Removed OnVisibilityChangedTo(). Added test. #
Total comments: 26
Patch Set 3 : Fixed nits in test. #
Total comments: 2
Patch Set 4 : Disable test as potentialy flacky. #
Messages
Total messages: 25 (8 generated)
Description was changed from ========== MacViews: Attach child windows when parent is on the screen. It fixes a problem with restore child windows after minimize. BUG=620266 ========== to ========== MacViews: Attach child windows when parent is on the screen. It fixes a problem with restore child windows after minimize. Method [ViewsNSWindowDelegate onWindowOrderWillChange] seems to be absolute. Firstly it was used to prevent hidden Widget redraw, but now [NSWindow setAutodisplay] is used. BUG=620266 ==========
kirr@yandex-team.ru changed reviewers: + tapted@chromium.org
PTAL I'm not sure that it's a right way to fix window restore. But it works, with only one views_unittest failed (NativeWidgetMacTest.WindowModalSheet). I also did not notice any changes on window restoring; views::UI was blinking before and still blinking now :)
https://codereview.chromium.org/2069103004/diff/1/ui/views/cocoa/views_nswind... File ui/views/cocoa/views_nswindow_delegate.mm (left): https://codereview.chromium.org/2069103004/diff/1/ui/views/cocoa/views_nswind... ui/views/cocoa/views_nswindow_delegate.mm:39: parent_->OnVisibilityChangedTo(orderingMode != NSWindowOut); So you're correct that this shouldn't be calling OnVisibilityChangedTo(true) -- if [NSWindow visible] is still reporting NO, then that's likely to screw up the child window relationships. But this was added to ensure we have a valid `paint` before the window becomes visible -- we still need that hook. i.e. it's still a bug that windows come out of minimize "blank", and that windows appear on screen first with blank background before the content appears. We need to generalize the stuff that's done for modal sheets in void BridgedNativeWidget::ShowAsModalSheet() -- the code there ensures the window contents are available. We need to do the same for windows first appearing on screen, and for windows coming out of minimize. Do you want to give this a go?
https://codereview.chromium.org/2069103004/diff/1/ui/views/cocoa/views_nswind... File ui/views/cocoa/views_nswindow_delegate.mm (left): https://codereview.chromium.org/2069103004/diff/1/ui/views/cocoa/views_nswind... ui/views/cocoa/views_nswindow_delegate.mm:39: parent_->OnVisibilityChangedTo(orderingMode != NSWindowOut); Sorry for delay. I've checked blank window blinking one more time. First thing is that now there is a blinking. So removing this line should not change anything to worse. Also I've tried to fix it as you say, by showing a layer and waiting for a frame and then change window visibility. Here is a draft https://gist.github.com/anonymous/1607657661087e0ca73e0352495c47cf It's working. But I think that it's better to fix it as render_widget_host_view does. It leaves the CALayer visible and suspend compositor. Maybe some aura code expect that native widget visibility is equal to layer visibility. I think it's better to try and see if tests fails. So, can we commit this CL as it is (maybe with test)? And fix blinking in another one.
It's fine to fix the blinking in a follow-up but we can't land changes that cause test regressions. https://codereview.chromium.org/2069103004/diff/1/ui/views/cocoa/views_nswind... File ui/views/cocoa/views_nswindow_delegate.mm (right): https://codereview.chromium.org/2069103004/diff/1/ui/views/cocoa/views_nswind... ui/views/cocoa/views_nswindow_delegate.mm:43: parent_->OnVisibilityChangedTo(true); Let's make this parent_->OnVisibilityChanged() as well and remove OnVisibilityChangedTo from the API. If the bug is that reparenting fails when the argument to OnVisibilityChangedTo doesn't match [window_ isVisible] then we should make it impossible for that to be triggered. https://codereview.chromium.org/2069103004/diff/1/ui/views/widget/native_widg... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/2069103004/diff/1/ui/views/widget/native_widg... ui/views/widget/native_widget_mac_unittest.mm:938: #endif This needs a proper fix - can you investigate why it fails?
https://codereview.chromium.org/2069103004/diff/1/ui/views/cocoa/views_nswind... File ui/views/cocoa/views_nswindow_delegate.mm (right): https://codereview.chromium.org/2069103004/diff/1/ui/views/cocoa/views_nswind... ui/views/cocoa/views_nswindow_delegate.mm:43: parent_->OnVisibilityChangedTo(true); On 2016/06/21 00:08:35, tapted wrote: > Let's make this parent_->OnVisibilityChanged() as well and remove > OnVisibilityChangedTo from the API. If the bug is that reparenting fails when > the argument to OnVisibilityChangedTo doesn't match [window_ isVisible] then we > should make it impossible for that to be triggered. Done. I've also removed [NSWindow display] overload. Seems like it's not calling on os x 10.11 at least in views_unittests. https://codereview.chromium.org/2069103004/diff/1/ui/views/widget/native_widg... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/2069103004/diff/1/ui/views/widget/native_widg... ui/views/widget/native_widget_mac_unittest.mm:938: #endif On 2016/06/21 00:08:35, tapted wrote: > This needs a proper fix - can you investigate why it fails? Test is expected that in the orderOut call views::Widget is already hidden, but it's not because I've removed onWindowOrderWillChange. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:413: base::MessageLoop::current()->PostDelayedTask( 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?
kirr@yandex-team.ru changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:377: // and restored using makeKeyAndOrderFront. nit: -makeKeyAndOrderFront:. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:378: // Child window could become occluded (because of cocoa bug?). You can say: Parent-child window relationships in AppKit are not supported when window visibility changes. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:379: TEST_F(NativeWidgetMacTest, OrderFrontAfterMiniturize) { Miniturize -> Miniaturize (or Minimize) https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:380: Widget* widget = new Widget; You should be able to avoid using InitParams with something like Widget* parent = CreateTopLevelPlatformWidget(); https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:385: Widget* child_widget = new Widget; Widget* child = CreateChildPlatformWidget(widget->GetNativeView()); https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:393: child_widget->SetBounds(gfx::Rect(110, 110, 100, 100)); 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. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:395: widget->Show(); show the child too? https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:396: base::RunLoop().RunUntilIdle(); is this needed? https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:402: base::RunLoop().RunUntilIdle(); this shouldn't be needed - miniaturize is synchronous https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:413: base::MessageLoop::current()->PostDelayedTask( On 2016/06/21 16:33:47, kirr wrote: > 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? 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 https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:422: EXPECT_TRUE([child_ns_window occlusionState] & NSWindowOcclusionStateVisible); 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(..)
https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:377: // and restored using makeKeyAndOrderFront. On 2016/06/22 04:09:15, tapted wrote: > nit: -makeKeyAndOrderFront:. Done. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:378: // Child window could become occluded (because of cocoa bug?). On 2016/06/22 04:09:14, tapted wrote: > You can say: > > Parent-child window relationships in AppKit are not supported when window > visibility changes. Done. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:379: TEST_F(NativeWidgetMacTest, OrderFrontAfterMiniturize) { On 2016/06/22 04:09:14, tapted wrote: > Miniturize -> Miniaturize (or Minimize) Done. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:380: Widget* widget = new Widget; On 2016/06/22 04:09:14, tapted wrote: > You should be able to avoid using InitParams with something like > > Widget* parent = CreateTopLevelPlatformWidget(); Done. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:385: Widget* child_widget = new Widget; On 2016/06/22 04:09:14, tapted wrote: > > Widget* child = CreateChildPlatformWidget(widget->GetNativeView()); Done. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:395: widget->Show(); On 2016/06/22 04:09:14, tapted wrote: > show the child too? I think no. Test visually works fine without it. But without widget_->Show() widget stay inactive. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:396: base::RunLoop().RunUntilIdle(); On 2016/06/22 04:09:14, tapted wrote: > is this needed? 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. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:402: base::RunLoop().RunUntilIdle(); On 2016/06/22 04:09:14, tapted wrote: > this shouldn't be needed - miniaturize is synchronous It's also not for the expectations below, bot for occlusion state. I've not investigated what exactly happens there, sorry. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:413: base::MessageLoop::current()->PostDelayedTask( 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. https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:422: EXPECT_TRUE([child_ns_window occlusionState] & NSWindowOcclusionStateVisible); 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.
https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:395: widget->Show(); On 2016/06/22 12:59:04, kirr wrote: > On 2016/06/22 04:09:14, tapted wrote: > > show the child too? > > I think no. > Test visually works fine without it. > But without widget_->Show() widget stay inactive. 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 https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:413: base::MessageLoop::current()->PostDelayedTask( On 2016/06/22 12:59:04, kirr wrote: > 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. 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). https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:422: EXPECT_TRUE([child_ns_window occlusionState] & NSWindowOcclusionStateVisible); On 2016/06/22 12:59:04, kirr wrote: > 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. 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.
https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/2069103004/diff/20001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:422: EXPECT_TRUE([child_ns_window occlusionState] & NSWindowOcclusionStateVisible); 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.
Well, since AppKit lies to us about window visibility, it might be reasonable that the API also doesn't give us a way to test this properly. But might be some other things to try - I can have a try, but I ran out of time today. I think currently the problem is the child is not only obscured, but that it's also "detached". We can verify whether or not the child is detached by doing something like - activate the parent window - child should raise on top, or - move the parent window, child should move with it if the child is detached, it will stay where it is. These might be easier things to check than obscure states. (also let's take off sky - he's a busy fellow, and I don't think we need him for OWNERS for the files being touched here).
tapted@chromium.org changed reviewers: - sky@chromium.org
On 2016/06/24 07:26:36, tapted wrote: > Well, since AppKit lies to us about window visibility, it might be reasonable > that the API also doesn't give us a way to test this properly. But might be some > other things to try - I can have a try, but I ran out of time today. > > I think currently the problem is the child is not only obscured, but that it's > also "detached". We can verify whether or not the child is detached by doing > something like > - activate the parent window - child should raise on top, or > - move the parent window, child should move with it > > if the child is detached, it will stay where it is. These might be easier things > to check than obscure states. > > (also let's take off sky - he's a busy fellow, and I don't think we need him for > OWNERS for the files being touched here). Child window is not detached. If we move parent window, child will appears on top of it.
Sorry for the delay. I found some time to investigate this a bit more, but just ran into dead-ends. So this lgtm, we just need to update the CL description and disable the tests so that it doesn't mess with the CQ. https://codereview.chromium.org/2069103004/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget.h (right): https://codereview.chromium.org/2069103004/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. cl description should go something like """MacViews: Ensure child windows are only managed with real changes to -[NSWindow isVisible]. MacViews: Ensure child windows are only managed with real changes to -[NSWindow isVisible]. After restoring from a miniaturize, child windows are currently reattached too early. This is because AppKit draws the window before updating the result of -[NSWindow isVisible], and we need to participate in that draw. But updating child window relationships at this time causes layering issues when the window has finished restoring. Instead, wait until -[NSWindow isVisible] becomes YES. """ make sure it's wrapped at < 80 chars. https://codereview.chromium.org/2069103004/diff/40001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/2069103004/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:380: TEST_F(NativeWidgetMacTest, OrderFrontAfterMiniaturize) { Let's just make this DISABLED_OrderFrontAfterMiniaturize Add to the comment above something like "Disabled because it relies on APIs and state changes that are unavoidably flaky."
Description was changed from ========== MacViews: Attach child windows when parent is on the screen. It fixes a problem with restore child windows after minimize. Method [ViewsNSWindowDelegate onWindowOrderWillChange] seems to be absolute. Firstly it was used to prevent hidden Widget redraw, but now [NSWindow setAutodisplay] is used. BUG=620266 ========== to ========== MacViews: Ensure child windows are only managed with real changes to -[NSWindow isVisible]. After restoring from a miniaturize, child windows are currently reattached too early. This is because AppKit draws the window before updating the result of -[NSWindow isVisible], and we need to participate in that draw. But updating child window relationships at this time causes layering issues when the window has finished restoring. BUG=620266 ==========
On 2016/06/30 12:02:37, tapted wrote: > Sorry for the delay. I found some time to investigate this a bit more, but just > ran into dead-ends. > > So this lgtm, we just need to update the CL description and disable the tests so > that it doesn't mess with the CQ. > Ok. Thanks for investigating of the test and for description.
The CQ bit was checked by kirr@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2069103004/#ps60001 (title: "Disable test as potentialy flacky.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== MacViews: Ensure child windows are only managed with real changes to -[NSWindow isVisible]. After restoring from a miniaturize, child windows are currently reattached too early. This is because AppKit draws the window before updating the result of -[NSWindow isVisible], and we need to participate in that draw. But updating child window relationships at this time causes layering issues when the window has finished restoring. BUG=620266 ========== to ========== MacViews: Ensure child windows are only managed with real changes to -[NSWindow isVisible]. After restoring from a miniaturize, child windows are currently reattached too early. This is because AppKit draws the window before updating the result of -[NSWindow isVisible], and we need to participate in that draw. But updating child window relationships at this time causes layering issues when the window has finished restoring. BUG=620266 Committed: https://crrev.com/7a3314d270102d9a6dc6572846f406290021d776 Cr-Commit-Position: refs/heads/master@{#403166} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7a3314d270102d9a6dc6572846f406290021d776 Cr-Commit-Position: refs/heads/master@{#403166} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
