Reland: Implement correct Panel closing sequence for Mac.

chrome/browser/ui/panels/panel_app_browsertest.cc

Index: chrome/browser/ui/panels/panel_app_browsertest.cc
diff --git a/chrome/browser/ui/panels/panel_app_browsertest.cc b/chrome/browser/ui/panels/panel_app_browsertest.cc
index 85bc6642878debfb067ef2c40cb86e267989f9a4..d6c86dcd5c747eea133ba897a64fde89b1649869 100644
--- a/chrome/browser/ui/panels/panel_app_browsertest.cc
+++ b/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 wan to release it as soon as

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

typo: wan

[Dmitry Titov, 2011/08/22 18:34:56]

Done.

+    // 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)));

[Avi (use Gerrit), 2011/08/19 22:05:01]

Curious why you changed these to EXPECT. Is it worth continuing if the extension
loading fails or the service can't get the extension?

[Dmitry Titov, 2011/08/22 18:34:56]

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.

 
     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());
 }