Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2448243003:
MacViews: Reattach children of native windows when they are exposed. (Closed)
|
Created:
4 years, 1 month ago by tapted Modified:
4 years, 1 month ago Reviewers:
karandeepb CC:
chromium-reviews, tfarina, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Reattach children of native windows when they are exposed.
Since NSWindow's childWindow breaks when windows are ordered out (except
via -[NSApp hide] it seems), we detach child windows in all cases where
a Widget becomes hidden. Currently they're only re-attached for
NSWindows backed by views::Widget.
To fix, watch for the parent window being exposed, and update
relationships accordingly. This is done implicitly, by making the child
Widget visible again, if it wants to be.
BUG=659161
Committed: https://crrev.com/6c06b0559798ae1120beb416075cffb88ea92874
Cr-Commit-Position: refs/heads/master@{#428002}
Patch Set 1 #Patch Set 2 : Neater #Patch Set 3 : +test #
Total comments: 16
Patch Set 4 : respond to comments #
Messages
Total messages: 27 (18 generated)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MacViews: Reattach children of native windows when they are exposed. Since NSWindow's childWindow breaks when windows are ordered out, we detach child windows in this case. Currently they're only re-attached for NSWindows backed by views::Widget. To fix, watch for the parent window being exposed, and update relationships accordingly. BUG=659161 ========== to ========== MacViews: Reattach children of native windows when they are exposed. Since NSWindow's childWindow breaks when windows are ordered out (except via -[NSApp hide] it seems), we detach child windows in all cases where a Widget becomes hidden. Currently they're only re-attached for NSWindows backed by views::Widget. To fix, watch for the parent window being exposed, and update relationships accordingly. This is done implicitly, by making the child Widget visible again, if it wants to be. BUG=659161 ==========
tapted@chromium.org changed reviewers: + karandeepb@chromium.org
Hi Karan, please take a look
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks good. The logic was a bit hard to follow, so I have suggested some DCHECKs for sanity check and to verify my understanding is correct. Also, how does minimize work correctly, if we aren't currently reattaching child windows for parent windows not backed by views::Widget. Don't we detach child windows on minimize? https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... File ui/views/cocoa/widget_owner_nswindow_adapter.mm (right): https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:17: // views::Widget will close. Modify this comment. It's also observing changes to the occlusion state now. https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:68: [[NSNotificationCenter defaultCenter] Maybe add a comment explaining why we need to observe changes to the occlusion state. https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:98: if (![anchor_window_ isVisible]) Just to clarify, the parent-child relationship breaks when the window is hidden. It does not break when the window is not hidden but only occluded, right? And what we want to do is to reattach the child when the parent window becomes "visible" (as opposed to just being non-occluded)? Buy since there is no proper visibility change notification, we are using occlusion state. https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:102: DCHECK(child_->wants_to_be_visible()); Should we DCHECK([child->ns_window() parentWindow]); https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:108: So now the parent is visible, and the child is hidden but wants to be visible (it is not visible because it had a hidden parent). Can we add a DCHECK here that the occlusion state of the anchor window has changed to NSWindowOcclusionStateVisible? https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:110: DCHECK(![child_->ns_window() parentWindow]); Can this DCHECK move before the preceding if condition? https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:115: DCHECK([child_->ns_window() parentWindow]); DCHECK(child_->window_visible())? https://codereview.chromium.org/2448243003/diff/40001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/2448243003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:775: WidgetChangeObserver child_observer(child); nit: Move this below EXPECT_TRUE(child->IsVisible()).
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/27 03:21:17, karandeepb wrote: > Looks good. The logic was a bit hard to follow, so I have suggested some DCHECKs > for sanity check and to verify my understanding is correct. > > Also, how does minimize work correctly, if we aren't currently reattaching child > windows for parent windows not backed by views::Widget. Don't we detach child > windows on minimize? I think only orderOut: breaks things -- not minimize. We don't hide children of native parents when the window is minimized -- AppKit takes care of hiding child windows for us in this case. So there may still be an anomaly that we don't notify the ui::Layer of a child widget that it's not visible when its native parent is minimized, but that might not be needed for anything. https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... File ui/views/cocoa/widget_owner_nswindow_adapter.mm (right): https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:17: // views::Widget will close. On 2016/10/27 03:21:17, karandeepb wrote: > Modify this comment. It's also observing changes to the occlusion state now. Done. https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:68: [[NSNotificationCenter defaultCenter] On 2016/10/27 03:21:17, karandeepb wrote: > Maybe add a comment explaining why we need to observe changes to the occlusion > state. Done. https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:98: if (![anchor_window_ isVisible]) On 2016/10/27 03:21:17, karandeepb wrote: > Just to clarify, the parent-child relationship breaks when the window is hidden. > It does not break when the window is not hidden but only occluded, right? Yup. AppKit fails to update zorder/position for child windows after an orderOut, so we intentionally break the relationship in those cases. But it's fine just for occlusion. > > And what we want to do is to reattach the child when the parent window becomes > "visible" (as opposed to just being non-occluded)? Buy since there is no proper > visibility change notification, we are using occlusion state. Yep - exactly. There is a WindowDidExpose notification but it doesn't work. Visibility changes on Mac are a pain, but occlusions works pretty well. It came in 10.9, so lucky we don't support further back than that :) https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:102: DCHECK(child_->wants_to_be_visible()); On 2016/10/27 03:21:17, karandeepb wrote: > Should we DCHECK([child->ns_window() parentWindow]); Done. https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:108: On 2016/10/27 03:21:17, karandeepb wrote: > So now the parent is visible, and the child is hidden but wants to be visible > (it is not visible because it had a hidden parent). Can we add a DCHECK here > that the occlusion state of the anchor window has changed to > NSWindowOcclusionStateVisible? I don't think this is the same. The parent window can be "visible", but fully occluded -- we don't hide child widgets in that situation, so we shouldn't block re-showing them either. And since occlusion is generated by the window server, there's possible race conditions that could mean a window could be fully-occluded the first time it becomes "visible". https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:110: DCHECK(![child_->ns_window() parentWindow]); On 2016/10/27 03:21:17, karandeepb wrote: > Can this DCHECK move before the preceding if condition? Done. https://codereview.chromium.org/2448243003/diff/40001/ui/views/cocoa/widget_o... ui/views/cocoa/widget_owner_nswindow_adapter.mm:115: DCHECK([child_->ns_window() parentWindow]); On 2016/10/27 03:21:17, karandeepb wrote: > DCHECK(child_->window_visible())? Done. https://codereview.chromium.org/2448243003/diff/40001/ui/views/widget/native_... File ui/views/widget/native_widget_mac_unittest.mm (right): https://codereview.chromium.org/2448243003/diff/40001/ui/views/widget/native_... ui/views/widget/native_widget_mac_unittest.mm:775: WidgetChangeObserver child_observer(child); On 2016/10/27 03:21:17, karandeepb wrote: > nit: Move this below EXPECT_TRUE(child->IsVisible()). Done.
LGTM.
The CQ bit was unchecked by tapted@chromium.org
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tapted@chromium.org
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.
Description was changed from ========== MacViews: Reattach children of native windows when they are exposed. Since NSWindow's childWindow breaks when windows are ordered out (except via -[NSApp hide] it seems), we detach child windows in all cases where a Widget becomes hidden. Currently they're only re-attached for NSWindows backed by views::Widget. To fix, watch for the parent window being exposed, and update relationships accordingly. This is done implicitly, by making the child Widget visible again, if it wants to be. BUG=659161 ========== to ========== MacViews: Reattach children of native windows when they are exposed. Since NSWindow's childWindow breaks when windows are ordered out (except via -[NSApp hide] it seems), we detach child windows in all cases where a Widget becomes hidden. Currently they're only re-attached for NSWindows backed by views::Widget. To fix, watch for the parent window being exposed, and update relationships accordingly. This is done implicitly, by making the child Widget visible again, if it wants to be. BUG=659161 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Reattach children of native windows when they are exposed. Since NSWindow's childWindow breaks when windows are ordered out (except via -[NSApp hide] it seems), we detach child windows in all cases where a Widget becomes hidden. Currently they're only re-attached for NSWindows backed by views::Widget. To fix, watch for the parent window being exposed, and update relationships accordingly. This is done implicitly, by making the child Widget visible again, if it wants to be. BUG=659161 ========== to ========== MacViews: Reattach children of native windows when they are exposed. Since NSWindow's childWindow breaks when windows are ordered out (except via -[NSApp hide] it seems), we detach child windows in all cases where a Widget becomes hidden. Currently they're only re-attached for NSWindows backed by views::Widget. To fix, watch for the parent window being exposed, and update relationships accordingly. This is done implicitly, by making the child Widget visible again, if it wants to be. BUG=659161 Committed: https://crrev.com/6c06b0559798ae1120beb416075cffb88ea92874 Cr-Commit-Position: refs/heads/master@{#428002} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6c06b0559798ae1120beb416075cffb88ea92874 Cr-Commit-Position: refs/heads/master@{#428002} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
