MacViews: Implement CloseNow() as just -[NSWindow close].

ui/views/widget/native_widget_mac.mm

Index: ui/views/widget/native_widget_mac.mm
diff --git a/ui/views/widget/native_widget_mac.mm b/ui/views/widget/native_widget_mac.mm
index 8ddf80139c21577cc32d25d36eae81f0bf440151..84dffb680469d103b46680244df28e298dff1325 100644
--- a/ui/views/widget/native_widget_mac.mm
+++ b/ui/views/widget/native_widget_mac.mm
@@ -100,15 +100,8 @@ bool NativeWidgetMac::IsWindowModalSheet() const {
          ui::MODAL_TYPE_WINDOW;
 }
 
-void NativeWidgetMac::OnWindowWillClose() {
-  // Note: If closed via CloseNow(), |bridge_| will already be reset. If closed
-  // by the user, or via Close() and a RunLoop, notify observers while |bridge_|
-  // is still a valid pointer, then reset it.
-  if (bridge_) {
-    delegate_->OnNativeWidgetDestroying();
-    [GetNativeWindow() setDelegate:nil];
-    bridge_.reset();
-  }
+void NativeWidgetMac::OnWindowDestroyed() {

[karandeepb, 2016/10/18 10:00:29]

DCHECK(bridge_)?

[tapted, 2016/10/19 06:20:25]

Done.

+  bridge_.reset();
   delegate_->OnNativeWidgetDestroyed();
   if (ownership_ == Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET)
     delete this;
@@ -379,10 +372,18 @@ void NativeWidgetMac::CloseNow() {
   if (!bridge_)
     return;
 
-  // Notify observers while |bridged_| is still valid.
-  delegate_->OnNativeWidgetDestroying();
-  // Reset |bridge_| to NULL before destroying it.
-  std::unique_ptr<BridgedNativeWidget> bridge(std::move(bridge_));
+  // Cocoa ignores -close calls on open sheets, so they should be closed
+  // asynchronously, using Widget::Close().
+  DCHECK(!IsWindowModalSheet());

[tapted, 2016/10/18 05:09:09]

This can probably be fixed now... but should get some tests added at the same
time.

+
+  // NSWindows must be retained until -[NSWindow close] returns.
+  base::scoped_nsobject<NSWindow> window(GetNativeWindow(),
+                                         base::scoped_policy::RETAIN);
+
+  // If there's a bridge at this point, it means there must be a window as well.
+  DCHECK(window);
+  [window close];

[karandeepb, 2016/10/18 10:00:29]

Should we orderOut: here to avoid crashes like http://crbug.com/156101?

[tapted, 2016/10/19 06:20:25]

I'm actually thinking of removing the orderOut altogether, but in a follow-up
(see http://crrev.com/2061693003). I don't think http://crbug.com/156101 will
apply to MacViews, and the orderOut in Close() is spewing out visibility changes
that create extra work and trigger weird codepaths.

[karandeepb, 2016/10/20 00:50:23]

Acknowledged. Although, the Widget::Close documentation seems to suggest that it
should hide the widget and then asynchronously close it.

[tapted, 2016/10/20 04:42:44]

ooh - nice find. Yeah I guess we should be consistent with that :/

+  // Note: |this| is deleted here when ownership_ == NATIVE_WIDGET_OWNS_WIDGET.
 }
 
 void NativeWidgetMac::Show() {