Revert 97736 - Implement correct Panel closing sequence for Mac.

Enable PanelBrowserTest.CreatePanelOnOverflow on Mac.

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

TBR=dimich@chromium.org
Review URL: http://codereview.chromium.org/7711013

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

6 files changed, 30 insertions, 124 deletions

diff --git a/chrome/browser/ui/panels/panel.cc b/chrome/browser/ui/panels/panel.cc
index 118d741..102ac60b 100644
--- a/chrome/browser/ui/panels/panel.cc
+++ b/chrome/browser/ui/panels/panel.cc
@@ -92,14 +92,7 @@ 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() {
diff --git a/chrome/browser/ui/panels/panel_app_browsertest.cc b/chrome/browser/ui/panels/panel_app_browsertest.cc
index 03f696b..85bc664 100644
--- a/chrome/browser/ui/panels/panel_app_browsertest.cc
+++ b/chrome/browser/ui/panels/panel_app_browsertest.cc
@@ -5,7 +5,6 @@
 
 #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"
@@ -16,7 +15,6 @@
 #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 {
@@ -27,45 +25,18 @@ class PanelAppBrowserTest : public ExtensionBrowserTest {
   }
 
   void LoadAndLaunchExtension(const char* 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)));
+    ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII(name)));
 
     ExtensionService* service = browser()->profile()->GetExtensionService();
     const Extension* extension = service->GetExtensionById(
         last_loaded_extension_id_, false);
-    EXPECT_TRUE(extension);
-
-    size_t browser_count = BrowserList::size();
+    ASSERT_TRUE(extension);
 
     Browser::OpenApplication(
         browser()->profile(),
         extension,
-        // Overriding manifest to open in a panel.
-        extension_misc::LAUNCH_PANEL,
+        extension_misc::LAUNCH_PANEL,  // Override manifest, open in 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());
   }
 };
 
@@ -75,7 +46,7 @@ IN_PROC_BROWSER_TEST_F(PanelAppBrowserTest, OpenAppInPanel) {
 
   // No Panels initially.
   PanelManager* panel_manager = PanelManager::GetInstance();
-  ASSERT_EQ(0, panel_manager->num_panels()); // No panels initially.
+  EXPECT_EQ(0, panel_manager->num_panels()); // No panels initially.
 
   LoadAndLaunchExtension("app_with_panel_container");
 
@@ -96,8 +67,6 @@ 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());
 
-  CloseWindowAndWait(new_browser);
-
+  new_browser->CloseWindow();
   EXPECT_EQ(0, panel_manager->num_panels());
-  EXPECT_EQ(1u, BrowserList::size());
 }
diff --git a/chrome/browser/ui/panels/panel_browser_window_cocoa.h b/chrome/browser/ui/panels/panel_browser_window_cocoa.h
index 0328b06..7cb1fc0 100644
--- a/chrome/browser/ui/panels/panel_browser_window_cocoa.h
+++ b/chrome/browser/ui/panels/panel_browser_window_cocoa.h
@@ -56,11 +56,6 @@ 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);
diff --git a/chrome/browser/ui/panels/panel_browser_window_cocoa.mm b/chrome/browser/ui/panels/panel_browser_window_cocoa.mm
index 3e7521f..b94b377 100644
--- a/chrome/browser/ui/panels/panel_browser_window_cocoa.mm
+++ b/chrome/browser/ui/panels/panel_browser_window_cocoa.mm
@@ -8,7 +8,6 @@
 #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"
 
@@ -104,7 +103,13 @@ void PanelBrowserWindowCocoa::ClosePanel() {
       return;
 
   NSWindow* window = [controller_ window];
-  [window performClose:controller_];
+  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.
 }
 
 void PanelBrowserWindowCocoa::ActivatePanel() {
@@ -171,13 +176,9 @@ Browser* PanelBrowserWindowCocoa::GetPanelBrowser() const {
 
 void PanelBrowserWindowCocoa::DestroyPanelBrowser() {
   [controller_ close];
-}
-
-void PanelBrowserWindowCocoa::didCloseNativeWindow() {
-  DCHECK(!isClosed());
-  panel_->manager()->Remove(panel_.get());
   controller_ = NULL;
 }
+
 // NativePanelTesting implementation.
 
 // static
diff --git a/chrome/browser/ui/panels/panel_browsertest.cc b/chrome/browser/ui/panels/panel_browsertest.cc
index acca37b..2e88fc3 100644
--- a/chrome/browser/ui/panels/panel_browsertest.cc
+++ b/chrome/browser/ui/panels/panel_browsertest.cc
@@ -3,7 +3,6 @@
 // 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"
@@ -46,14 +45,6 @@ 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,
@@ -71,19 +62,6 @@ 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)
@@ -303,8 +281,7 @@ IN_PROC_BROWSER_TEST_F(PanelBrowserTest, CreatePanel) {
   EXPECT_GT(bounds.width(), 0);
   EXPECT_GT(bounds.height(), 0);
 
-  CloseWindowAndWait(panel->browser());
-
+  panel->Close();
   EXPECT_EQ(0, panel_manager->num_panels());
 }
 
@@ -316,7 +293,14 @@ IN_PROC_BROWSER_TEST_F(PanelBrowserTest, FindBar) {
   panel->Close();
 }
 
-IN_PROC_BROWSER_TEST_F(PanelBrowserTest, CreatePanelOnOverflow) {
+// 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) {
   TestCreatePanelOnOverflow();
 }
 
diff --git a/chrome/browser/ui/panels/panel_window_controller_cocoa.mm b/chrome/browser/ui/panels/panel_window_controller_cocoa.mm
index a9e8714..2474030 100644
--- a/chrome/browser/ui/panels/panel_window_controller_cocoa.mm
+++ b/chrome/browser/ui/panels/panel_window_controller_cocoa.mm
@@ -8,7 +8,6 @@
 
 #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"
@@ -97,6 +96,14 @@ 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);
@@ -108,47 +115,4 @@ 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