Implement correct Panel closing sequence for Mac.

Enable PanelBrowserTest.CreatePanelOnOverflow on Mac.

Review URL: http://codereview.chromium.org/7694014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@97736 0039d316-1c4b-4281-b951-d872f2087c98

chrome/app/nibs/Panel.xib

@@ -2,9 +2,9 @@
 <archive type="com.apple.InterfaceBuilder3.Cocoa.XIB" version="7.10">
 	<data>
 		<int key="IBDocument.SystemTarget">1050</int>
-		<string key="IBDocument.SystemVersion">10J869</string>
+		<string key="IBDocument.SystemVersion">10K549</string>
 		<string key="IBDocument.InterfaceBuilderVersion">804</string>
-		<string key="IBDocument.AppKitVersion">1038.35</string>
+		<string key="IBDocument.AppKitVersion">1038.36</string>
 		<string key="IBDocument.HIToolboxVersion">461.00</string>
 		<object class="NSMutableDictionary" key="IBDocument.PluginVersions">
 			<string key="NS.key.0">com.apple.InterfaceBuilder.CocoaPlugin</string>
@@ -40,7 +40,7 @@
 				<string key="NSClassName">NSApplication</string>
 			</object>
 			<object class="NSWindowTemplate" id="1005">
-				<int key="NSWindowStyleMask">1</int>
+				<int key="NSWindowStyleMask">3</int>
 				<int key="NSWindowBacking">2</int>
 				<string key="NSWindowRect">{{196, 240}, {480, 270}}</string>
 				<int key="NSWTFlags">544736256</int>

chrome/browser/ui/panels/panel.cc

@@ -92,7 +92,14 @@ void Panel::SetBounds(const gfx::Rect& bounds) {
 // close on the first attempt.
 void Panel::Close() {
   native_panel_->ClosePanel();
+
+// TODO(dimich): Only implemented fully async on Mac. Need to update other
+// platforms. The panel should be removed from PanelManager when and if it
+// was actually closed. The closing can be cancelled because of onbeforeunload
+// handler on the web page.
+#if !defined(OS_MACOSX)
   manager()->Remove(this);
+#endif
 }
 
 void Panel::Activate() {

chrome/browser/ui/panels/panel_app_browsertest.cc

@@ -5,6 +5,7 @@
 
 #include "base/command_line.h"
 #include "base/file_path.h"
+#include "base/mac/scoped_nsautorelease_pool.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -15,6 +16,7 @@
 #include "chrome/browser/ui/panels/panel_manager.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 class PanelAppBrowserTest : public ExtensionBrowserTest {
@@ -25,18 +27,45 @@ class PanelAppBrowserTest : public ExtensionBrowserTest {
   }
 
   void LoadAndLaunchExtension(const char* name) {
-    ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII(name)));
+    // Opening panels on a Mac causes NSWindowController of the Panel window
+    // to be autoreleased. We need a pool drained after it's done so the test
+    // can close correctly. The NSWindowController of the Panel window controls
+    // lifetime of the Browser object so we want to release it as soon as
+    // possible. In real Chrome, this is done by message pump.
+    // On non-Mac platform, this is an empty class.
+    base::mac::ScopedNSAutoreleasePool autorelease_pool;
+
+    EXPECT_TRUE(LoadExtension(test_data_dir_.AppendASCII(name)));
 
     ExtensionService* service = browser()->profile()->GetExtensionService();
     const Extension* extension = service->GetExtensionById(
         last_loaded_extension_id_, false);
-    ASSERT_TRUE(extension);
+    EXPECT_TRUE(extension);
+
+    size_t browser_count = BrowserList::size();
 
     Browser::OpenApplication(
         browser()->profile(),
         extension,
-        extension_misc::LAUNCH_PANEL,  // Override manifest, open in panel.
+        // Overriding manifest to open in a panel.
+        extension_misc::LAUNCH_PANEL,
         NEW_WINDOW);
+
+    // Now we have a new browser instance.
+    EXPECT_EQ(browser_count + 1, BrowserList::size());
+  }
+
+  void CloseWindowAndWait(Browser* browser) {
+    // Closing a browser window may involve several async tasks. Need to use
+    // message pump and wait for the notification.
+    size_t browser_count = BrowserList::size();
+    ui_test_utils::WindowedNotificationObserver signal(
+        chrome::NOTIFICATION_BROWSER_CLOSED,
+        Source<Browser>(browser));
+    browser->CloseWindow();
+    signal.Wait();
+    // Now we have one less browser instance.
+    EXPECT_EQ(browser_count - 1, BrowserList::size());
   }
 };
 
@@ -46,7 +75,7 @@ IN_PROC_BROWSER_TEST_F(PanelAppBrowserTest, OpenAppInPanel) {
 
   // No Panels initially.
   PanelManager* panel_manager = PanelManager::GetInstance();
-  EXPECT_EQ(0, panel_manager->num_panels()); // No panels initially.
+  ASSERT_EQ(0, panel_manager->num_panels()); // No panels initially.
 
   LoadAndLaunchExtension("app_with_panel_container");
 
@@ -67,6 +96,8 @@ IN_PROC_BROWSER_TEST_F(PanelAppBrowserTest, OpenAppInPanel) {
   // Now also check that PanelManager has one new Panel under management.
   EXPECT_EQ(1, panel_manager->num_panels());
 
-  new_browser->CloseWindow();
+  CloseWindowAndWait(new_browser);
+
   EXPECT_EQ(0, panel_manager->num_panels());
+  EXPECT_EQ(1u, BrowserList::size());
 }

chrome/browser/ui/panels/panel_browser_window_cocoa.h

@@ -56,6 +56,11 @@ class PanelBrowserWindowCocoa : public NativePanel {
   Panel* panel() { return panel_.get(); }
   Browser* browser() const { return browser_.get(); }
 
+  // Callback from PanelWindowControllerCocoa that native window was actually
+  // closed. The window may not close right away because of onbeforeunload
+  // handlers.
+  void didCloseNativeWindow();
+
  private:
   friend class PanelBrowserWindowCocoaTest;
   FRIEND_TEST_ALL_PREFIXES(PanelBrowserWindowCocoaTest, CreateClose);

chrome/browser/ui/panels/panel_browser_window_cocoa.mm

@@ -8,6 +8,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/cocoa/find_bar/find_bar_bridge.h"
 #include "chrome/browser/ui/panels/panel.h"
+#include "chrome/browser/ui/panels/panel_manager.h"
 #import "chrome/browser/ui/panels/panel_window_controller_cocoa.h"
 #include "content/common/native_web_keyboard_event.h"
 
@@ -103,13 +104,7 @@ void PanelBrowserWindowCocoa::ClosePanel() {
       return;
 
   NSWindow* window = [controller_ window];
-  NSRect frame = [window frame];
-  frame.size.height = kMinimumWindowSize;
-  // TODO(dimich): make this async. Currently, multiple panels will serially
-  // (and annoyingly) close when user exits Chrome.
-  [window setFrame:frame display:YES animate:YES];
-  browser_->OnWindowClosing();
-  DestroyPanelBrowser();  // not immediately, though.
+  [window performClose:controller_];
 }
 
 void PanelBrowserWindowCocoa::ActivatePanel() {
@@ -176,9 +171,13 @@ Browser* PanelBrowserWindowCocoa::GetPanelBrowser() const {
 
 void PanelBrowserWindowCocoa::DestroyPanelBrowser() {
   [controller_ close];
-  controller_ = NULL;
 }
 
+void PanelBrowserWindowCocoa::didCloseNativeWindow() {
+  DCHECK(!isClosed());
+  panel_->manager()->Remove(panel_.get());
+  controller_ = NULL;
+}
 // NativePanelTesting implementation.
 
 // static

chrome/browser/ui/panels/panel_browsertest.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/command_line.h"
+#include "base/mac/scoped_nsautorelease_pool.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -45,6 +46,14 @@ class PanelBrowserTest : public InProcessBrowserTest {
 
  protected:
   Panel* CreatePanel(const std::string& name, const gfx::Rect& bounds) {
+    // Opening panels on a Mac causes NSWindowController of the Panel window
+    // to be autoreleased. We need a pool drained after it's done so the test
+    // can close correctly. The NSWindowController of the Panel window controls
+    // lifetime of the Browser object so we want to release it as soon as
+    // possible. In real Chrome, this is done by message pump.
+    // On non-Mac platform, this is an empty class.
+    base::mac::ScopedNSAutoreleasePool autorelease_pool;
+
     Browser* panel_browser = Browser::CreateForApp(Browser::TYPE_PANEL,
                                                    name,
                                                    bounds,
@@ -62,6 +71,19 @@ class PanelBrowserTest : public InProcessBrowserTest {
     return panel;
   }
 
+  void CloseWindowAndWait(Browser* browser) {
+    // Closing a browser window may involve several async tasks. Need to use
+    // message pump and wait for the notification.
+    size_t browser_count = BrowserList::size();
+    ui_test_utils::WindowedNotificationObserver signal(
+        chrome::NOTIFICATION_BROWSER_CLOSED,
+        Source<Browser>(browser));
+    browser->CloseWindow();
+    signal.Wait();
+    // Now we have one less browser instance.
+    EXPECT_EQ(browser_count - 1, BrowserList::size());
+  }
+
   // Creates a testing extension.
   scoped_refptr<Extension> CreateExtension(const FilePath::StringType& path) {
 #if defined(OS_WIN)
@@ -281,7 +303,8 @@ IN_PROC_BROWSER_TEST_F(PanelBrowserTest, CreatePanel) {
   EXPECT_GT(bounds.width(), 0);
   EXPECT_GT(bounds.height(), 0);
 
-  panel->Close();
+  CloseWindowAndWait(panel->browser());
+
   EXPECT_EQ(0, panel_manager->num_panels());
 }
 
@@ -293,14 +316,7 @@ IN_PROC_BROWSER_TEST_F(PanelBrowserTest, FindBar) {
   panel->Close();
 }
 
-// TODO(jianli): Investigate and enable it for Mac.
-#ifdef OS_MACOSX
-#define MAYBE_CreatePanelOnOverflow DISABLED_CreatePanelOnOverflow
-#else
-#define MAYBE_CreatePanelOnOverflow CreatePanelOnOverflow
-#endif
-
-IN_PROC_BROWSER_TEST_F(PanelBrowserTest, MAYBE_CreatePanelOnOverflow) {
+IN_PROC_BROWSER_TEST_F(PanelBrowserTest, CreatePanelOnOverflow) {
   TestCreatePanelOnOverflow();
 }
 

chrome/browser/ui/panels/panel_window_controller_cocoa.mm

@@ -8,6 +8,7 @@
 
 #include "base/logging.h"
 #include "base/mac/mac_util.h"
+#include "chrome/browser/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/browser.h"
 #import "chrome/browser/ui/cocoa/find_bar/find_bar_bridge.h"
 #import "chrome/browser/ui/cocoa/find_bar/find_bar_cocoa_controller.h"
@@ -96,14 +97,6 @@ const int kMinimumWindowSize = 1;
   [findBarCocoaController positionFindBarViewAtMaxY:maxY maxWidth:maxWidth];
 }
 
-- (void)closePanel {
-  windowShim_->panel()->Close();
-}
-
-- (void)windowWillClose:(NSNotification*)notification {
-  [self autorelease];
-}
-
 - (NSView*)tabContentsView {
   TabContents* contents = windowShim_->browser()->GetSelectedTabContents();
   CHECK(contents);
@@ -115,4 +108,47 @@ const int kMinimumWindowSize = 1;
 - (PanelTitlebarViewCocoa*)titlebarView {
   return titlebar_view_;
 }
+
+// Handler for the custom Close button.
+- (void)closePanel {
+  windowShim_->panel()->Close();
+}
+
+// Called when the user wants to close the panel or from the shutdown process.
+// The Browser object is in control of whether or not we're allowed to close. It
+// may defer closing due to several states, such as onbeforeUnload handlers
+// needing to be fired. If closing is deferred, the Browser will handle the
+// processing required to get us to the closing state and (by watching for
+// all the tabs going away) will again call to close the window when it's
+// finally ready.
+// This callback is only called if the standard Close button is enabled in XIB.
+- (BOOL)windowShouldClose:(id)sender {
+  Browser* browser = windowShim_->browser();
+  // Give beforeunload handlers the chance to cancel the close before we hide
+  // the window below.
+  if (!browser->ShouldCloseWindow())
+    return NO;
+
+  if (!browser->tabstrip_model()->empty()) {
+    // Tab strip isn't empty. Make browser to close all the tabs, allowing the
+    // renderer to shut down and call us back again.
+    // The tab strip of Panel is not visible and contains only one tab but
+    // it still has to be closed.
+    browser->OnWindowClosing();
+    return NO;
+  }
+
+  // the tab strip is empty, it's ok to close the window
+  return YES;
+}
+
+// When windowShouldClose returns YES (or if controller receives direct 'close'
+// signal), window will be unconditionally closed. Clean up.
+- (void)windowWillClose:(NSNotification*)notification {
+  DCHECK(windowShim_->browser()->tabstrip_model()->empty());
+
+  windowShim_->didCloseNativeWindow();
+  [self autorelease];
+}
+
 @end