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

ui/views/widget/native_widget_mac.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.
 
 #include "ui/views/widget/native_widget_mac.h"
 
 #import <Cocoa/Cocoa.h>
 
 #include <utility>
 
@@ skipping 82 matching lines @@
     return [delegate nativeWidgetMac]->bridge_.get();
   }
   return nullptr;  // Not created by NativeWidgetMac.
 }
 
 bool NativeWidgetMac::IsWindowModalSheet() const {
   return GetWidget()->widget_delegate()->GetModalType() ==
          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;
 }
 
 int NativeWidgetMac::SheetPositionY() {
   NSView* view = GetNativeView();
   return
       [view convertPoint:NSMakePoint(0, NSHeight([view frame])) toView:nil].y;
 }
@@ skipping 242 matching lines @@
   // is no need to call the NSWindow or its delegate's -windowShouldClose:
   // implementation in the manner of -[NSWindow performClose:]. But,
   // like -performClose:, first remove the window from AppKit's display
   // list to avoid crashes like http://crbug.com/156101.
   [window orderOut:nil];
 
   // Many tests assume that base::RunLoop().RunUntilIdle() is always sufficient
   // to execute a close. However, in rare cases, -performSelector:..afterDelay:0
   // does not do this. So post a regular task.
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, base::BindBlock(^{
     [window close];

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

While closing a window asynchronously, we don't temporarily retain it like we do
in RemoveAndDestroyChildren and now in CloseNow. Won't the reference count for
the window reach 0, during [window close] which is unsafe? Maybe we can add
tests to verify that Widget::CloseNow and Widget::Close do deallocate the window
but not during [window close].

Also, does anything prevent us from scheduling CloseNow itself here? This would
make it more similar to the NativeWidgetAura implementation.

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

Actually, the ObjectiveC block automatically increments refcounts on NSObjects
that are captured in the closure. But that's subtle, and relies on the captured
argument being on the stack (i.e. capturing |this| won't refcount). Added a
comment to the |window| declaration so someone doesn't decide to optimise it
away.
Testing that a dealloc occurs is tricky -- there's actually no requirement that
a dealloc happens within any timeframe after a Close() or CloseNow() -- AppKit
is allowed to have whatever references it likes (e.g. NSEvents stuck on a queue
somewhere). But in controlled environments, ui::CocoaTest::TearDown() is able to
check with some complicated loops and timeouts --
bridged_native_widget_unittest.mm will be involved in the checks there that
ensure all NSWindows are removed from [NSApp windows] (pretty sure NSWindow's
dealloc does this). So there's already some coverage of this.
Yeah I thought about that. I might explore in a follow-up. To do that we'd need
to add a WeakPtrFactory on NativeWidgetMac so the posted task doesn't outlive
|this|. Currently it's safe since we only need to outlive |window| which is
ref-counted already.

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

Lots of subtle stuff. Acknowledged.

   }));
 }
 
 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() {
   ShowWithWindowState(ui::SHOW_STATE_NORMAL);
 }
 
 void NativeWidgetMac::Hide() {
   if (!bridge_)
     return;
 
@@ skipping 363 matching lines @@
   [[ViewsNSWindowCloseAnimator alloc] initWithWindow:window];
 }
 
 - (void)animationDidEnd:(NSAnimation*)animation {
   [window_ close];
   [animation_ setDelegate:nil];
   [self release];
 }
 
 @end