[Mac] In BrowserWindowCocoa::Close() do not use -[NSWindow performClose:].

[Mac] In BrowserWindowCocoa::Close() do not use -[NSWindow performClose:].

|-performClose:| will mimic a click on the window's close button, which can
race a JavaScript app-modal alert during shutdown, leaving around an orphaned
Browser object.

BUG=118424
TEST=See steps in bug. Regression test added.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161874

[Robert Sesek, 2012-10-10 18:31:38]

mark: browser_window_cocoa.mm
ben: browser_browsertest.cc

[Mark Mentovai, 2012-10-10 18:43:08]

LGTM

https://codereview.chromium.org/11090029/diff/9001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/11090029/diff/9001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:178: // implement
|-windowShouldClose:|.
It wouldn’t be difficult to send the message to the window if the delegate
doesn’t implement -windowShouldClose:. The assumption that our windows have
delegates to deal with this may be valid today, but I wouldn’t count on that
remaining valid indefinitely.

https://codereview.chromium.org/11090029/diff/9001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:180: if (![delegate
respondsToSelector:@selector(windowShouldClose:)] ||
What is this weird bracket syntax?

[Robert Sesek, 2012-10-11 14:13:37]

https://codereview.chromium.org/11090029/diff/9001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/11090029/diff/9001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:178: // implement
|-windowShouldClose:|.
On 2012/10/10 18:43:09, Mark Mentovai wrote:
> It wouldn’t be difficult to send the message to the window if the delegate
> doesn’t implement -windowShouldClose:. The assumption that our windows have
> delegates to deal with this may be valid today, but I wouldn’t count on that
> remaining valid indefinitely.

Done. There's a new test for you to review, too.

https://codereview.chromium.org/11090029/diff/9001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:180: if (![delegate
respondsToSelector:@selector(windowShouldClose:)] ||
On 2012/10/10 18:43:09, Mark Mentovai wrote:
> What is this weird bracket syntax?

http://bukk.it/noidea-golf.gif

[Mark Mentovai, 2012-10-11 15:44:57]

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:179: SEL windowShouldClose =
@selector(windowShouldClose:);
window_should_close

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:184: if ([window()
performSelector:windowShouldClose withObject:window()])
Why not just [window windowShouldClose:window()]?

Or if you want to use -performSelector, why didn’t you use it when calling the
delegate?

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right):

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:131: // Tests that
BrowserWindowCocoa::Close mimicks the behavior of
mimics

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:211: 
Can you also have a test where the delegate says YES but the window says NO, and
verify that the window does close? And vice-versa?

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:212:
TEST_F(BrowserWindowCocoaCloseTest, NoResponseFromDelegatAndWindow) {
Delegate is missing an E.

[Robert Sesek, 2012-10-11 15:58:38]

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:179: SEL windowShouldClose =
@selector(windowShouldClose:);
On 2012/10/11 15:44:57, Mark Mentovai wrote:
> window_should_close

Done.

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:184: if ([window()
performSelector:windowShouldClose withObject:window()])
On 2012/10/11 15:44:57, Mark Mentovai wrote:
> Why not just [window windowShouldClose:window()]?
> 
> Or if you want to use -performSelector, why didn’t you use it when calling the
> delegate?

NSWindow does implement windowShouldClose: and it's not in its interface. That's
why the test needs a fake one.

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right):

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:131: // Tests that
BrowserWindowCocoa::Close mimicks the behavior of
On 2012/10/11 15:44:57, Mark Mentovai wrote:
> mimics

Done.

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:211: 
On 2012/10/11 15:44:57, Mark Mentovai wrote:
> Can you also have a test where the delegate says YES but the window says NO,
and
> verify that the window does close? And vice-versa?

Done, and the inverse

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:212:
TEST_F(BrowserWindowCocoaCloseTest, NoResponseFromDelegatAndWindow) {
On 2012/10/11 15:44:57, Mark Mentovai wrote:
> Delegate is missing an E.

Done.

[Mark Mentovai, 2012-10-11 16:04:03]

LGTM

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://codereview.chromium.org/11090029/diff/3002/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:184: if ([window()
performSelector:windowShouldClose withObject:window()])
On 2012/10/11 15:58:39, rsesek wrote:
> On 2012/10/11 15:44:57, Mark Mentovai wrote:
> > Why not just [window windowShouldClose:window()]?
> > 
> > Or if you want to use -performSelector, why didn’t you use it when calling
the
> > delegate?
> 
> NSWindow does implement windowShouldClose: and it's not in its interface.
That's
> why the test needs a fake one.

Oh yeah, duh, your comment even said that.

https://codereview.chromium.org/11090029/diff/6012/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right):

https://codereview.chromium.org/11090029/diff/6012/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:230: // Window should
not be closed.
I like this comment. You should put it into the other tests where the window is
supposed to stay open. Is OCMock actually testing that the window isn’t closed
(by not adding [[window_ expect] close?)

[Robert Sesek, 2012-10-11 16:06:54]

https://codereview.chromium.org/11090029/diff/6012/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm (right):

https://codereview.chromium.org/11090029/diff/6012/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_cocoa_unittest.mm:230: // Window should
not be closed.
On 2012/10/11 16:04:03, Mark Mentovai wrote:
> I like this comment. You should put it into the other tests where the window
is
> supposed to stay open. Is OCMock actually testing that the window isn’t closed
> (by not adding [[window_ expect] close?)

Yeah, mock expectations are strict. An unexpected method call is an error (which
is why I also have to stub out overlayWindow in the SetUp).

[Ben Goodger (Google), 2012-10-12 16:32:39]

lgtm

[commit-bot: I haz the power, 2012-10-12 18:19:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/11090029/13005

[commit-bot: I haz the power, 2012-10-13 01:14:34]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-15 15:31:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/11090029/13005

[commit-bot: I haz the power, 2012-10-15 16:45:58]

Change committed as 161874