MacViews: Protect against orderOut: on an NSWindow with an attached sheet.

ui/views/cocoa/bridged_native_widget.mm

Index: ui/views/cocoa/bridged_native_widget.mm
diff --git a/ui/views/cocoa/bridged_native_widget.mm b/ui/views/cocoa/bridged_native_widget.mm
index 5dded116ff5474a67c59f354da9a31290e809bf1..83fff461b16e1c83a43717e52dc7ef7cefaca474 100644
--- a/ui/views/cocoa/bridged_native_widget.mm
+++ b/ui/views/cocoa/bridged_native_widget.mm
@@ -558,6 +558,13 @@ void BridgedNativeWidget::SetVisibilityState(WindowVisibilityState new_state) {
   wants_to_be_visible_ = new_state != HIDE_WINDOW;
 
   if (new_state == HIDE_WINDOW) {
+    // Calling -orderOut: on a window with an attached sheet encounters broken
+    // AppKit behavior. The sheet effectively becomes "lost".
+    // See http://crbug.com/667602. Alternatives: call -setAlphaValue:0 and
+    // -setIgnoresMouseEvents:YES on the NSWindow, or dismiss the sheet before
+    // hiding.
+    DCHECK(![window_ attachedSheet]);

[karandeepb, 2016/11/28 05:19:32]

Won't this trigger for the case given in issue 667602 or has that been fixed
somewhere else?

[tapted, 2016/11/28 06:15:41]

Yes, but only with chrome://flags/#mac-views-native-app-windows which is
unlikely to launch given the packaged app deprecation on Mac (although it's
useful for testing WebContents behaviour with MacViews). The goal is to detect
any future things that may rely on the broken behaviour in an obvious way.

[karandeepb, 2016/11/28 23:37:57]

Acknowledged.

+
     [window_ orderOut:nil];
     DCHECK(!window_visible_);
     return;
@@ -827,7 +834,9 @@ void BridgedNativeWidget::OnVisibilityChanged() {
   if (window_visible_) {
     wants_to_be_visible_ = true;
 
-    if (parent_)
+    // Sheets don't need a parentWindow set, and setting one causes graphical
+    // glitches (http://crbug.com/605098).

[karandeepb, 2016/11/28 05:19:32]

I can't see it being concluded on issue 605098 that the glitch was due to the
sheet having a parent window. Is this a new conclusion? 

[tapted, 2016/11/28 06:15:41]

Yup :). Turns out this was the culprit. It went away in r403166 because adding
the early exit in OnVisibilityChanged() turned the *first* visibility change
into a No-op [skipping over this addChildWindow], but not later visibility
changes. http://crbug.com/605098 is not completely gone in Canary - if you show
Edit Bookmark, then [NSApp hide] with cmd+h, then re-show Chrome, *then* dismiss
the sheet you get the same glitch, because the logic here adds it as a child
window.

[karandeepb, 2016/11/28 23:37:57]

Acknowledged. Maybe mention this explicitly in the CL description, since this
fixes some behavior.

[tapted, 2016/11/29 03:28:51]

Done.

+    if (parent_ && ![window_ isSheet])
       [parent_->GetNSWindow() addChildWindow:window_ ordered:NSWindowAbove];
   } else {
     ReleaseCapture();  // Capture on hidden windows is not permitted.