MacViews: Support -[NSWindow close] on sheets.

MacViews: Support -[NSWindow close] on sheets.

Usually a sheet is closed with a call to endSheet:, which blocks the UI
thread to run the close animation, then runs the didEndSelector passed
to -[NSApp beginSheet:..]. Calling -[NSWindow close] bypasses this and
shouldn't be used. However, there are times where we need to close a
sheet more violently. E.g.: When -[NSWindow close] is invoked on the
parent window currently hosting a sheet, or during a dialog test that
wants to invoke the synchronous Widget::CloseNow() rather than
asynchronous Widget::Close(). Attempts to do this used to DCHECK().

Now, support -[NSWindow close] of a sheet by posting a task to
-[NSWindow endSheet:] on the parent when the sheet is closed. This has
tricky lifetime implications, but is necessary to ensure the modal
session on the parent window is unblocked.

BUG=681049
Review-Url: https://codereview.chromium.org/2629593005
Cr-Commit-Position: refs/heads/master@{#443718}
Committed: https://chromium.googlesource.com/chromium/src/+/caeaf7c8f5c0daf2db7fd1a0dad1828dac26d62c

[tapted, 2017-01-13 17:09:19]

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

[commit-bot: I haz the power, 2017-01-13 17:11:09]

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

[tapted, 2017-01-13 18:07:30]

Description was changed from

==========
MacViews: Support -[NSWindow close] on sheets.

BUG=
==========

to

==========
MacViews: Support -[NSWindow close] on sheets.

Usually a sheet is closed with a call to endSheet:, which blocks the UI
thread to run the close animation, then runs the didEndSelector passed
to -[NSApp beginSheet:..]. Calling -[NSWindow close] bypasses this and
shouldn't be used. However, there are times where we need to close a
sheet more violently. E.g.: When -[NSWindow close] is invoked on the
parent window currently hosting a sheet, or during a dialog test that
wants to invoke the synchronous Widget::CloseNow() rather than
asynchronous Widget::Close(). Attempts to do this used to DCHECK().

Now, support -[NSWindow close] of a sheet by posting a task to
-[NSWindow endSheet:] on the parent when the sheet is closed. This has
tricky lifetime implications, but is necessary to ensure the modal
session on the parent window is unblocked.

BUG=681049
==========

[tapted, 2017-01-13 18:07:30]

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

[commit-bot: I haz the power, 2017-01-13 18:29:09]

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

[commit-bot: I haz the power, 2017-01-13 18:29:10]

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, 2017-01-13 19:26:00]

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

[commit-bot: I haz the power, 2017-01-13 19:26:44]

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

[tapted, 2017-01-13 19:36:53]

Hi Robert, please take a look

https://codereview.chromium.org/2629593005/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac.mm (left):

https://codereview.chromium.org/2629593005/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac.mm:385: // Cocoa ignores -close calls on open
sheets, so they should be closed
I think when I wrote this things were structured a bit different and the problem
was that -close wasn't triggering sheetDidEnd: . Hopefully I've catered for all
the dragons now.. (but this should only come up in tests and during process
shutdown anyway, since having a sheet will disable the close button for users).

[Robert Sesek, 2017-01-13 20:02:48]

lgtm w/ two questions

https://codereview.chromium.org/2629593005/diff/20001/ui/views/cocoa/views_ns...
File ui/views/cocoa/views_nswindow_delegate.mm (right):

https://codereview.chromium.org/2629593005/diff/20001/ui/views/cocoa/views_ns...
ui/views/cocoa/views_nswindow_delegate.mm:95: // Does nothing: Used to force
|self| to be retained in a block.
Could you instead just put another keepAlive in the block?

https://codereview.chromium.org/2629593005/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/2629593005/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_unittest.mm:1170: // Tests behavior when
closing a window that is a sheet, or that hosts a sheet.
Also worth testing re-showing a sheet on a window that had previously closed a
sheet?

[commit-bot: I haz the power, 2017-01-13 20:21:52]

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

[commit-bot: I haz the power, 2017-01-13 20:21:53]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-01-13 20:51:50]

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

[tapted, 2017-01-13 20:52:51]

https://codereview.chromium.org/2629593005/diff/20001/ui/views/cocoa/views_ns...
File ui/views/cocoa/views_nswindow_delegate.mm (right):

https://codereview.chromium.org/2629593005/diff/20001/ui/views/cocoa/views_ns...
ui/views/cocoa/views_nswindow_delegate.mm:95: // Does nothing: Used to force
|self| to be retained in a block.
On 2017/01/13 20:02:47, Robert Sesek wrote:
> Could you instead just put another keepAlive in the block?

That would take (and release) an additional reference :). Currently there's just
the reference being held by the ObjC block machinery. i.e. It would work, but it
would effectively be replacing

[self doNothing];

with

[self retain];
[self release];

So I don't know if that's better. I do think `doNothing` is weird, but I didn't
want to use additional retain/release since it might suggest that the block
doesn't already have a ref to begin with that exists for the lifetime of the
block (or that we need to account for some other hidden weirdness..)

But I'm not otherwise attached to `doNothing` - e.g. I thought about calling
some random NSObject property, but thought "doNothing" gave slightly more
meaning.

https://codereview.chromium.org/2629593005/diff/20001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/2629593005/diff/20001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_unittest.mm:1170: // Tests behavior when
closing a window that is a sheet, or that hosts a sheet.
On 2017/01/13 20:02:47, Robert Sesek wrote:
> Also worth testing re-showing a sheet on a window that had previously closed a
> sheet?

Done.

https://codereview.chromium.org/2629593005/diff/40001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/2629593005/diff/40001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_unittest.mm:1196:
EXPECT_TRUE([sheet_widget->GetNativeWindow() isVisible]);
(added this to check re-showing - it fails without the RunUntilIdle() bit above
for example)

[commit-bot: I haz the power, 2017-01-13 20:52:58]

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, 2017-01-13 21:37:01]

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

[commit-bot: I haz the power, 2017-01-13 21:37:02]

Dry run: This issue passed the CQ dry run.

[Robert Sesek, 2017-01-13 21:46:01]

LGTM, take or leave the suggestion

https://codereview.chromium.org/2629593005/diff/20001/ui/views/cocoa/views_ns...
File ui/views/cocoa/views_nswindow_delegate.mm (right):

https://codereview.chromium.org/2629593005/diff/20001/ui/views/cocoa/views_ns...
ui/views/cocoa/views_nswindow_delegate.mm:95: // Does nothing: Used to force
|self| to be retained in a block.
On 2017/01/13 20:52:51, tapted wrote:
> On 2017/01/13 20:02:47, Robert Sesek wrote:
> > Could you instead just put another keepAlive in the block?
> 
> That would take (and release) an additional reference :). Currently there's
just
> the reference being held by the ObjC block machinery. i.e. It would work, but
it
> would effectively be replacing
> 
> [self doNothing];
> 
> with
> 
> [self retain];
> [self release];
> 
> So I don't know if that's better. I do think `doNothing` is weird, but I
didn't
> want to use additional retain/release since it might suggest that the block
> doesn't already have a ref to begin with that exists for the lifetime of the
> block (or that we need to account for some other hidden weirdness..)
> 
> But I'm not otherwise attached to `doNothing` - e.g. I thought about calling
> some random NSObject property, but thought "doNothing" gave slightly more
> meaning.

Hm, I see your point. Maybe rather than doNothing, you end the block with [[self
retain] release]; ?

[tapted, 2017-01-13 22:32:45]

https://codereview.chromium.org/2629593005/diff/20001/ui/views/cocoa/views_ns...
File ui/views/cocoa/views_nswindow_delegate.mm (right):

https://codereview.chromium.org/2629593005/diff/20001/ui/views/cocoa/views_ns...
ui/views/cocoa/views_nswindow_delegate.mm:95: // Does nothing: Used to force
|self| to be retained in a block.
On 2017/01/13 21:46:00, Robert Sesek wrote:
> On 2017/01/13 20:52:51, tapted wrote:
> > On 2017/01/13 20:02:47, Robert Sesek wrote:
> > > Could you instead just put another keepAlive in the block?
> > 
> > That would take (and release) an additional reference :). Currently there's
> just
> > the reference being held by the ObjC block machinery. i.e. It would work,
but
> it
> > would effectively be replacing
> > 
> > [self doNothing];
> > 
> > with
> > 
> > [self retain];
> > [self release];
> > 
> > So I don't know if that's better. I do think `doNothing` is weird, but I
> didn't
> > want to use additional retain/release since it might suggest that the block
> > doesn't already have a ref to begin with that exists for the lifetime of the
> > block (or that we need to account for some other hidden weirdness..)
> > 
> > But I'm not otherwise attached to `doNothing` - e.g. I thought about calling
> > some random NSObject property, but thought "doNothing" gave slightly more
> > meaning.
> 
> Hm, I see your point. Maybe rather than doNothing, you end the block with
[[self
> retain] release]; ?

Done!

[tapted, 2017-01-13 22:32:53]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-01-13 22:32:54]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2629593005/#ps60001
(title: "no doNothing")

[commit-bot: I haz the power, 2017-01-13 22:33:31]

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, 2017-01-13 23:21:15]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484346773823210,
"parent_rev": "16af7e87fbb7d49f79185b33209249f4877f8950", "commit_rev":
"caeaf7c8f5c0daf2db7fd1a0dad1828dac26d62c"}

[commit-bot: I haz the power, 2017-01-13 23:21:47]

Description was changed from

==========
MacViews: Support -[NSWindow close] on sheets.

Usually a sheet is closed with a call to endSheet:, which blocks the UI
thread to run the close animation, then runs the didEndSelector passed
to -[NSApp beginSheet:..]. Calling -[NSWindow close] bypasses this and
shouldn't be used. However, there are times where we need to close a
sheet more violently. E.g.: When -[NSWindow close] is invoked on the
parent window currently hosting a sheet, or during a dialog test that
wants to invoke the synchronous Widget::CloseNow() rather than
asynchronous Widget::Close(). Attempts to do this used to DCHECK().

Now, support -[NSWindow close] of a sheet by posting a task to
-[NSWindow endSheet:] on the parent when the sheet is closed. This has
tricky lifetime implications, but is necessary to ensure the modal
session on the parent window is unblocked.

BUG=681049
==========

to

==========
MacViews: Support -[NSWindow close] on sheets.

Usually a sheet is closed with a call to endSheet:, which blocks the UI
thread to run the close animation, then runs the didEndSelector passed
to -[NSApp beginSheet:..]. Calling -[NSWindow close] bypasses this and
shouldn't be used. However, there are times where we need to close a
sheet more violently. E.g.: When -[NSWindow close] is invoked on the
parent window currently hosting a sheet, or during a dialog test that
wants to invoke the synchronous Widget::CloseNow() rather than
asynchronous Widget::Close(). Attempts to do this used to DCHECK().

Now, support -[NSWindow close] of a sheet by posting a task to
-[NSWindow endSheet:] on the parent when the sheet is closed. This has
tricky lifetime implications, but is necessary to ensure the modal
session on the parent window is unblocked.

BUG=681049

Review-Url: https://codereview.chromium.org/2629593005
Cr-Commit-Position: refs/heads/master@{#443718}
Committed:
https://chromium.googlesource.com/chromium/src/+/caeaf7c8f5c0daf2db7fd1a0dad1...
==========

[commit-bot: I haz the power, 2017-01-13 23:21:58]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/caeaf7c8f5c0daf2db7fd1a0dad1...