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

ui/views/cocoa/bridged_native_widget.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/cocoa/bridged_native_widget.h"
 
 #import <objc/runtime.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 540 matching lines @@
 
 void BridgedNativeWidget::SetVisibilityState(WindowVisibilityState new_state) {
   // Ensure that:
   //  - A window with an invisible parent is not made visible.
   //  - A parent changing visibility updates child window visibility.
   //    * But only when changed via this function - ignore changes via the
   //      NSWindow API, or changes propagating out from here.
   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;
   }
 
   DCHECK(wants_to_be_visible_);
   // If the parent (or an ancestor) is hidden, return and wait for it to become
   // visible.
   if (parent() && !parent()->IsVisibleParent())
     return;
@@ skipping 249 matching lines @@
 
   window_visible_ = window_visible;
 
   // If arriving via SetVisible(), |wants_to_be_visible_| should already be set.
   // If made visible externally (e.g. Cmd+H), just roll with it. Don't try (yet)
   // to distinguish being *hidden* externally from being hidden by a parent
   // window - we might not need that.
   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.
 
     // When becoming invisible, remove the entry in any parent's childWindow
     // list. Cocoa's childWindow management breaks down when child windows are
     // hidden.
     if (parent_)
       [parent_->GetNSWindow() removeChildWindow:window_];
   }
@@ skipping 568 matching lines @@
   [bridged_view_ setMouseDownCanMoveWindow:draggable];
   // AppKit will not update its cache of mouseDownCanMoveWindow unless something
   // changes. Previously we tried adding an NSView and removing it, but for some
   // reason it required reposting the mouse-down event, and didn't always work.
   // Calling the below seems to be an effective solution.
   [window_ setMovableByWindowBackground:NO];
   [window_ setMovableByWindowBackground:YES];
 }
 
 }  // namespace views