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

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

This simplifies the teardown codepaths and increases parity with
NativeWidgetAura by increasing the separation between the calls to
OnNativeWidgetDestroying() and  OnNativeWidgetDestroyed().

Previously NativeWidgetMac relied on different ways of calling `delete`
on a BridgedNativeWidget in order to perform a synchronous close of the
window in CloseNow(). Now it just calls -[NSWindow close], giving it the
same codepath as a native-triggered close (e.g. clicking the close
button in the window frame).

BUG=604628
Committed: https://crrev.com/76adce4f223e7342acc632cd5abe181eb4751d99
Cr-Commit-Position: refs/heads/master@{#426407}

[tapted, 2016-10-05 10:36:55]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 10:37:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 11:14:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 11:14:22]

Dry run: 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_...)

[tapted, 2016-10-13 08:15:13]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 08:15:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 08:30:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 08:30:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tapted, 2016-10-18 01:01:51]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 01:03:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 01:29:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 01:29:12]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-10-18 03:04:48]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 03:05:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 03:50:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 03:50:59]

Dry run: This issue passed the CQ dry run.

[tapted, 2016-10-18 05:00:19]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 05:00:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2016-10-18 05:05:58]

Description was changed from

==========
Playing around with BridgedNativeWidget lifetime

BUG=604628
==========

to

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

This simplifies the teardown codepaths and increases parity with
NativeWidgetAura by increasing the separation between the calls to
OnNativeWidgetDestroying() and  OnNativeWidgetDestroyed().

Previously NativeWidgetMac relied on different ways of calling `delete`
on a BridgedNativeWidget in order to perform a synchronous close of the
window in CloseNow(). Now it just calls -[NSWindow close], giving it the
same codepath as a native-triggered close (e.g. clicking the close
button in the window frame).

BUG=604628
==========

[tapted, 2016-10-18 05:05:58]

tapted@chromium.org changed reviewers:
+ karandeepb@chromium.org

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

Hi Karan, please take a look

https://codereview.chromium.org/2393843002/diff/100001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_unittest.mm:686: // Test that
BridgedNativeWidget remains sane if Init() is never called.
This was actually a lie :) -- Init() was previously called in TestBase::SetUp,
but that is skipped now.

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac.mm:377: DCHECK(!IsWindowModalSheet());
This can probably be fixed now... but should get some tests added at the same
time.

[commit-bot: I haz the power, 2016-10-18 05:43:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 05:43:30]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2016-10-18 10:00:30]

https://codereview.chromium.org/2393843002/diff/100001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:357: // that the hosted NSWindow is
closed before the destructor is called.
Technically, the window hasn't closed yet right? We are still "inside" [NSWindow
close] and the delegate's windowWillClose method. Maybe this can be clarified
better.

https://codereview.chromium.org/2393843002/diff/100001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_unittest.mm:694: delete native_widget_mac_;
nit: Create a local std::unique_ptr and assign its pointer to native_widget_mac_
to avoid explicit deletion.

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_mac.h (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac.h:41: // destroyed. This is usually called
from the NSWindowDelegate.
nit: Maybe change the comment to- "This is usually called from the |bridge_|
when the window delegate receives windowWillClose: message" or something
similar.

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac.mm:103: void
NativeWidgetMac::OnWindowDestroyed() {
DCHECK(bridge_)?

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac.mm:367: [window close];
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.

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac.mm:385: [window close];
Should we orderOut: here to avoid crashes like http://crbug.com/156101?

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac_unittest.mm:1158: // removed. The parent
should still be open (children are always closed
"the NSWindow relation will already be removed". Update this comment.

[tapted, 2016-10-19 04:44:19]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 04:46:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2016-10-19 06:20:26]

tryservers are kaput, but I don't think I broke anything :p

https://codereview.chromium.org/2393843002/diff/100001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:357: // that the hosted NSWindow is
closed before the destructor is called.
On 2016/10/18 10:00:29, karandeepb wrote:
> Technically, the window hasn't closed yet right? We are still "inside"
[NSWindow
> close] and the delegate's windowWillClose method. Maybe this can be clarified
> better. 

Done.

https://codereview.chromium.org/2393843002/diff/100001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_unittest.mm:694: delete native_widget_mac_;
On 2016/10/18 10:00:29, karandeepb wrote:
> nit: Create a local std::unique_ptr and assign its pointer to
native_widget_mac_
> to avoid explicit deletion.

Done.

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_mac.h (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac.h:41: // destroyed. This is usually called
from the NSWindowDelegate.
On 2016/10/18 10:00:29, karandeepb wrote:
> nit: Maybe change the comment to- "This is usually called from the |bridge_|
> when the window delegate receives windowWillClose: message" or something
> similar.

Done. (the sheetDidEnd: delegate method invokes this too, but also via
BridgedNativeWidget::OnWindowWillClose)

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac.mm:103: void
NativeWidgetMac::OnWindowDestroyed() {
On 2016/10/18 10:00:29, karandeepb wrote:
> DCHECK(bridge_)?

Done.

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac.mm:367: [window close];
On 2016/10/18 10:00:29, karandeepb wrote:
> 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? 

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.

> Maybe we can add
> tests to verify that Widget::CloseNow and Widget::Close do deallocate the
window
> but not during [window close].

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.

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

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.

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac.mm:385: [window close];
On 2016/10/18 10:00:29, karandeepb wrote:
> Should we orderOut: here to avoid crashes like http://crbug.com/156101

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.

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac_unittest.mm:1158: // removed. The parent
should still be open (children are always closed
On 2016/10/18 10:00:29, karandeepb wrote:
> "the NSWindow relation will already be removed". Update this comment.

Done. And actually added a OnWidgetDestroyed() override as well, so we can check
the old condition as well.

[commit-bot: I haz the power, 2016-10-19 10:08:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 10:08:37]

Dry run: This issue passed the CQ dry run.

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

LGTM.

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac.mm:367: [window close];
On 2016/10/19 06:20:25, tapted wrote:
> On 2016/10/18 10:00:29, karandeepb wrote:
> > 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? 
> 
> 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.
> 
> > Maybe we can add
> > tests to verify that Widget::CloseNow and Widget::Close do deallocate the
> window
> > but not during [window close].
> 
> 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.
> 
> > Also, does anything prevent us from scheduling CloseNow itself here? This
> would
> > make it more similar to the NativeWidgetAura implementation.
> 
> 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.

Lots of subtle stuff. Acknowledged.

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac.mm:385: [window close];
On 2016/10/19 06:20:25, tapted wrote:
> On 2016/10/18 10:00:29, karandeepb wrote:
> > Should we orderOut: here to avoid crashes like http://crbug.com/156101
> 
> 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.

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:36:07]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2016-10-20 04:36:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
File ui/views/widget/native_widget_mac.mm (right):

https://codereview.chromium.org/2393843002/diff/100001/ui/views/widget/native...
ui/views/widget/native_widget_mac.mm:385: [window close];
On 2016/10/20 00:50:23, karandeepb wrote:
> On 2016/10/19 06:20:25, tapted wrote:
> > On 2016/10/18 10:00:29, karandeepb wrote:
> > > Should we orderOut: here to avoid crashes like http://crbug.com/156101
> > 
> > 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.
> 
> Acknowledged. Although, the Widget::Close documentation seems to suggest that
it
> should hide the widget and then asynchronously close it.

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

[commit-bot: I haz the power, 2016-10-20 04:50:19]

Description was changed from

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

This simplifies the teardown codepaths and increases parity with
NativeWidgetAura by increasing the separation between the calls to
OnNativeWidgetDestroying() and  OnNativeWidgetDestroyed().

Previously NativeWidgetMac relied on different ways of calling `delete`
on a BridgedNativeWidget in order to perform a synchronous close of the
window in CloseNow(). Now it just calls -[NSWindow close], giving it the
same codepath as a native-triggered close (e.g. clicking the close
button in the window frame).

BUG=604628
==========

to

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

This simplifies the teardown codepaths and increases parity with
NativeWidgetAura by increasing the separation between the calls to
OnNativeWidgetDestroying() and  OnNativeWidgetDestroyed().

Previously NativeWidgetMac relied on different ways of calling `delete`
on a BridgedNativeWidget in order to perform a synchronous close of the
window in CloseNow(). Now it just calls -[NSWindow close], giving it the
same codepath as a native-triggered close (e.g. clicking the close
button in the window frame).

BUG=604628
==========

[commit-bot: I haz the power, 2016-10-20 04:50:21]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-21 13:15:45]

Description was changed from

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

This simplifies the teardown codepaths and increases parity with
NativeWidgetAura by increasing the separation between the calls to
OnNativeWidgetDestroying() and  OnNativeWidgetDestroyed().

Previously NativeWidgetMac relied on different ways of calling `delete`
on a BridgedNativeWidget in order to perform a synchronous close of the
window in CloseNow(). Now it just calls -[NSWindow close], giving it the
same codepath as a native-triggered close (e.g. clicking the close
button in the window frame).

BUG=604628
==========

to

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

This simplifies the teardown codepaths and increases parity with
NativeWidgetAura by increasing the separation between the calls to
OnNativeWidgetDestroying() and  OnNativeWidgetDestroyed().

Previously NativeWidgetMac relied on different ways of calling `delete`
on a BridgedNativeWidget in order to perform a synchronous close of the
window in CloseNow(). Now it just calls -[NSWindow close], giving it the
same codepath as a native-triggered close (e.g. clicking the close
button in the window frame).

BUG=604628

Committed: https://crrev.com/76adce4f223e7342acc632cd5abe181eb4751d99
Cr-Commit-Position: refs/heads/master@{#426407}
==========

[commit-bot: I haz the power, 2016-10-21 13:15:46]

Patchset 7 (id:??) landed as
https://crrev.com/76adce4f223e7342acc632cd5abe181eb4751d99
Cr-Commit-Position: refs/heads/master@{#426407}