Reland: Implement correct Panel closing sequence for Mac.

Implement correct Panel closing sequence for Mac.
Enable PanelBrowserTest.CreatePanelOnOverflow on Mac.

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

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

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

[Dmitry Titov, 2011-08-19 21:28:55]

This implements the window closing sequence for the Panels, similar to the one
used for main browser windows. It delegates to Browser to let onbeforeunload
handlers to run and renderers exit.

XIB file change is to enable "Close" button so the [NSWindowDelegate
windowShouldClose] is called (otherwise it doesn't). We overlay the default
titlebar with our custom one so the button is there but not visible.

Avi, could you please look at it with a Cocoa eye?
Looking for Panel and generic feedback from Jenn.
Thanks!

[jennb, 2011-08-19 21:46:12]

To summarize the comments below: I prefer a solution where the window is
animated offscreen in ClosePanel() and immediately removed from panel manager. 
The hidden window is destroyed later when the tab is asynchronously closed and
ClosePanel() is called a second time.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel.cc
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:91: // TODO(dimich): we need to only remove
the panel when and if it actually
Even if the panel isn't closing immediately, can't the panel be hidden offscreen
and removed from panel manager immediately?

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_app_browsertest.cc (right):

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_app_browsertest.cc:33: // lifetime of the Browser
object so we wan to release it as soon as
typo: wan

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_window_cocoa.mm (right):

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_window_cocoa.mm:106: [window
performClose:controller_];
I prefer to animate the window offscreen here, then call performClose.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:52: // lifetime of the Browser
object so we wan to release it as soon as
typo: wan

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:175:
CloseWindowAndWait(panel1->browser());
Test doesn't care when close actually happens so no need to wait for these 2
closes.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:306:
CloseWindowAndWait(panel->browser());
Wait not needed if we remove panel from panel manager immediately.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:146: // signal),
whindow will be unconditionally closed. Clean up.
typo: whindow

[Avi (use Gerrit), 2011-08-19 22:05:00]

Cocoa stuff LGTM; take care of Jenn's concerns.

Plus, there's a typo in the change description.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_app_browsertest.cc (right):

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_app_browsertest.cc:38:
EXPECT_TRUE(LoadExtension(test_data_dir_.AppendASCII(name)));
Curious why you changed these to EXPECT. Is it worth continuing if the extension
loading fails or the service can't get the extension?

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:133: // Tab strip
isn't empty.Make browser to close all the tabs, allowing the
nit: space after period.

[Dmitry Titov, 2011-08-20 00:33:30]

On 2011/08/19 21:46:12, jennb wrote:
> To summarize the comments below: I prefer a solution where the window is
> animated offscreen in ClosePanel() and immediately removed from panel manager.

> The hidden window is destroyed later when the tab is asynchronously closed and
> ClosePanel() is called a second time.

Discussed offline.

Main browser window and popups will terminate close based on onbeforeunload
handler on the page. We need to replicate this for the Panles, unless we are
closing the panel as an emergency (when app is exiting?) - via
Panle::DestroyBrowser().

I will poke around a bit more, stay tuned :-)

[Dmitry Titov, 2011-08-22 18:34:55]

General comment addressing Jenn's suggestion about immediate hide of the window
and removal of it from the PanelManager in Panel::Close():

I verified how onbeforeunload handler behaves. It stops window closing in all
situations - close of tab, close of browser window, Quit from app menu, closing
popup. So we should do the same. Async closing as in this patch does it.

Jenn asked why BrowserWindowCocoa::Close() hides the window first. Here is the
explanation: 

When user clicks on Close button, control does not go through
BrowserWindow::Close, instead it goes through [NSWindow performClose] directly,
as action for the Close button.  The windowShouldClose callback then verifies
that onbeforeunload handlers were called, and then it is done with them, hides
the window and starts to close tabs. Later, when all tabs are closed, the
BrowserWindow::Close() is called again, from
TabStripModel::DetachTabContentsAt() when tabstrip becomes empty. This is what
causes actual closing of the window.

When Chrome is terminated via Quit menu item, BrowserList::CloseAllBrowsers()
will invoke BrowserWindow::Close() - which will indeed hide the window earlier.
Now, if there are no onbeforeunload handdlers, the windowShouldClose will again
hide the window before startign to close the tabs. If there is a onbeforeunload
handler, the dialog comes up and that un-hides the browser window back, as a
parent for modality.

So my impression is that BrowserWindow::Close() hiding the window is at least
not a very useful code and can be removed.

In case of emergency closing (power off), the BrowserWindow::DestroyBrowser() is
used that is explicitly synchronous.

Looking at the code path that happens in presence of onbeforeunload handlers, I
do think we should land this patch and fix GTK and Windows implementations to
remove Panel from PanelManager when in fact it can be removed, not right away. I
can work on this in a separate patch after this one, and it also needs a test
with onbeforeunload handler.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel.cc
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:91: // TODO(dimich): we need to only remove
the panel when and if it actually
On 2011/08/19 21:46:12, jennb wrote:
> Even if the panel isn't closing immediately, can't the panel be hidden
offscreen
> and removed from panel manager immediately?

See comment at the top.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_app_browsertest.cc (right):

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_app_browsertest.cc:33: // lifetime of the Browser
object so we wan to release it as soon as
On 2011/08/19 21:46:12, jennb wrote:
> typo: wan

Done.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_app_browsertest.cc:38:
EXPECT_TRUE(LoadExtension(test_data_dir_.AppendASCII(name)));
It is a helper function, and it returns void. ASSERT_XXX macros return a value,
and this is how Gtest terminates the test when ASSERT happens (it simply returns
from the test method). Apparently it's ok to return value from a void function
but using ASSERT_TRUE and friends in the void helper function is kind of wrong
because it won't stop the test.

So using EXPECT_TRUE at least produces expected result.

I have ASSERT later in the test method, right after calling the helper one.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_window_cocoa.mm (right):

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_window_cocoa.mm:106: [window
performClose:controller_];
On 2011/08/19 21:46:12, jennb wrote:
> I prefer to animate the window offscreen here, then call performClose.

See comment at the top.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:52: // lifetime of the Browser
object so we wan to release it as soon as
On 2011/08/19 21:46:12, jennb wrote:
> typo: wan

Done.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:175:
CloseWindowAndWait(panel1->browser());
On 2011/08/19 21:46:12, jennb wrote:
> Test doesn't care when close actually happens so no need to wait for these 2
> closes.

Done. Put panel->Close() back.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browsertest.cc:306:
CloseWindowAndWait(panel->browser());
On 2011/08/19 21:46:12, jennb wrote:
> Wait not needed if we remove panel from panel manager immediately.

See comment at the top.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:133: // Tab strip
isn't empty.Make browser to close all the tabs, allowing the
On 2011/08/19 22:05:01, Avi wrote:
> nit: space after period.

Done.

http://codereview.chromium.org/7694014/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:146: // signal),
whindow will be unconditionally closed. Clean up.
On 2011/08/19 21:46:12, jennb wrote:
> typo: whindow

Done.

[jennb, 2011-08-22 19:13:02]

LGTM

Thanks for the detailed explanation about close process. I'm ok with having a
separate patch to make GTK and Views also not remove panel from panel manager
until panel has actually closed.

http://codereview.chromium.org/7694014/diff/6001/chrome/browser/ui/cocoa/brow...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7694014/diff/6001/chrome/browser/ui/cocoa/brow...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:116: LOG(INFO) <<
"BrowserWindowCocoa::Close()";
Did you mean to keep this debugging aid in the patch?

http://codereview.chromium.org/7694014/diff/6001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/7694014/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel.cc:97: // closes. It is now only implemented on
Mac (unless it's synchronous on other
Change the 2nd sentence to state that changes are needed for other platforms.
Else it's not clear what the TODO action should be.

[Dmitry Titov, 2011-08-23 18:44:23]

This patch was landed and reverted because unit_tests on Mac failed. 

The updated patch is the same except 1 file -
panel_browser_window_cocoa_unittest.cc
I combined 'create panel' and 'show panel' into a single function that has
NSAutoreleasePool now. This ensures that NSWindowController of the panel window
is released, releaseing the browser and terminating the close sequence
correctly.

Also, instead of just panel->Close() method of closing panels, I wait for a
notification that browser was closed. This makes CocoaTest happy because all the
Cocoa windows are closed at the end of the test.

Jenn, could you take another look please?

[jennb, 2011-08-23 20:59:35]

LGTM

[Dmitry Titov, 2011-08-23 22:17:02]

Now it got reverted because using freed object in the test has caused the test
to succeed locally but fail on the bot.
Updated, will re-land.