Close all panels originated by the extension when extension unloads.

chrome/browser/extensions/window_open_apitest.cc

Index: chrome/browser/extensions/window_open_apitest.cc
diff --git a/chrome/browser/extensions/window_open_apitest.cc b/chrome/browser/extensions/window_open_apitest.cc
index b3d2b1dbde04ae564c355dba575eeabd80aef070..1e3fd8241eaea729ffa798d2ae45bf9c8e600439 100644
--- a/chrome/browser/extensions/window_open_apitest.cc
+++ b/chrome/browser/extensions/window_open_apitest.cc
@@ -3,10 +3,13 @@
 // found in the LICENSE file.
 
 #include "base/command_line.h"
+#include "base/memory/scoped_vector.h"
 #include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/extensions/extension_test_message_listener.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/extensions/extension.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "net/base/mock_host_resolver.h"
 
@@ -21,16 +24,19 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, DISABLED_WindowOpen) {
   EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
 }
 
-void WaitForTabsAndPopups(Browser* browser, int num_tabs, int num_popups) {
+void WaitForTabsAndPopups(Browser* browser,
+                          int num_tabs,
+                          int num_popups,
+                          int num_panels) {
   // We start with one tab and one browser already open.
   ++num_tabs;
-  size_t num_browsers = static_cast<size_t>(num_popups) + 1;
+  size_t num_browsers = static_cast<size_t>(num_popups + num_panels) + 1;
 
   const base::TimeDelta kWaitTime = base::TimeDelta::FromSeconds(15);
   base::TimeTicks end_time = base::TimeTicks::Now() + kWaitTime;
   while (base::TimeTicks::Now() < end_time) {
-    if (BrowserList::GetBrowserCount(browser->profile()) >= num_browsers &&
-        browser->tab_count() >= num_tabs)
+    if (BrowserList::GetBrowserCount(browser->profile()) == num_browsers &&

[dcheng, 2011/10/28 19:48:17]

Why did this change from >= to ==? It seems safer to use >= since otherwise we
may end up in a situation where this loop does not terminate.

[prasadt, 2011/10/28 20:19:35]

I think the situation is the same before too, as you could have a test that
creates one few tabs. Yes, it could be a problem is some test consider more tabs
etc than what's passed in to be a success. But I don't see that to be the case.

I had to make the change because for the uninstall case, I'm looking for the
count to go down to the expected number and not up. So >= will bail out right
away which is not what I want.

+        browser->tab_count() == num_tabs)
       break;
 
     MessageLoopForUI::current()->RunAllPending();
@@ -39,6 +45,8 @@ void WaitForTabsAndPopups(Browser* browser, int num_tabs, int num_popups) {
   EXPECT_EQ(num_browsers, BrowserList::GetBrowserCount(browser->profile()));
   EXPECT_EQ(num_tabs, browser->tab_count());
 
+  int num_popups_seen = 0;
+  int num_panels_seen = 0;
   for (BrowserList::const_iterator iter = BrowserList::begin();
        iter != BrowserList::end(); ++iter) {
     if (*iter == browser)
@@ -46,7 +54,13 @@ void WaitForTabsAndPopups(Browser* browser, int num_tabs, int num_popups) {
 
     // Check for TYPE_POPUP or TYPE_PANEL.
     EXPECT_TRUE((*iter)->is_type_popup() || (*iter)->is_type_panel());
+    if ((*iter)->is_type_popup())
+      ++num_popups_seen;
+    else
+      ++num_panels_seen;
   }
+  ASSERT_EQ(num_popups, num_popups_seen);

[dcheng, 2011/10/28 19:48:17]

I would use EXPECT_EQ so the rest of the tests can continue running.

[prasadt, 2011/10/28 20:19:35]

Done.

This won't actually affect other tests as these are browser tests and each runs
in its own process.

+  ASSERT_EQ(num_panels, num_panels_seen);
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, BrowserIsApp) {
@@ -55,7 +69,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, BrowserIsApp) {
   ASSERT_TRUE(LoadExtension(
       test_data_dir_.AppendASCII("window_open").AppendASCII("browser_is_app")));
 
-  WaitForTabsAndPopups(browser(), 0, 2);
+  WaitForTabsAndPopups(browser(), 0, 2, 0);
 
   for (BrowserList::const_iterator iter = BrowserList::begin();
        iter != BrowserList::end(); ++iter) {
@@ -73,7 +87,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupDefault) {
 
   const int num_tabs = 1;
   const int num_popups = 0;
-  WaitForTabsAndPopups(browser(), num_tabs, num_popups);
+  WaitForTabsAndPopups(browser(), num_tabs, num_popups, 0);
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupLarge) {
@@ -90,7 +104,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupLarge) {
   const int num_tabs = 0;
   const int num_popups = 1;
 #endif
-  WaitForTabsAndPopups(browser(), num_tabs, num_popups);
+  WaitForTabsAndPopups(browser(), num_tabs, num_popups, 0);
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupSmall) {
@@ -102,7 +116,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenPopupSmall) {
   // On other systems this should open a new popup window.
   const int num_tabs = 0;
   const int num_popups = 1;
-  WaitForTabsAndPopups(browser(), num_tabs, num_popups);
+  WaitForTabsAndPopups(browser(), num_tabs, num_popups, 0);
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PopupBlockingExtension) {
@@ -113,7 +127,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PopupBlockingExtension) {
       test_data_dir_.AppendASCII("window_open").AppendASCII("popup_blocking")
       .AppendASCII("extension")));
 
-  WaitForTabsAndPopups(browser(), 5, 3);
+  WaitForTabsAndPopups(browser(), 5, 3, 0);
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PopupBlockingHostedApp) {
@@ -146,7 +160,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PopupBlockingHostedApp) {
   browser()->OpenURL(open_popup, GURL(), NEW_FOREGROUND_TAB,
                      content::PAGE_TRANSITION_TYPED);
 
-  WaitForTabsAndPopups(browser(), 3, 1);
+  WaitForTabsAndPopups(browser(), 3, 1, 0);
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowArgumentsOverflow) {
@@ -175,6 +189,45 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpenFocus) {
 }
 #endif
 
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest,
+                       CloseNonExtensionPanelsOnUninstall) {
+  host_resolver()->AddRule("a.com", "127.0.0.1");
+  ASSERT_TRUE(StartTestServer());
+
+  // Setup listeners to wait on strings we expect the extension pages to send.
+  std::vector<std::string> test_strings;
+  test_strings.push_back("content_tab");
+  test_strings.push_back("content_panel");
+  test_strings.push_back("content_popup");
+
+  ScopedVector<ExtensionTestMessageListener> listeners;
+  for (size_t i = 0; i < test_strings.size(); ++i) {
+    listeners.push_back(
+        new ExtensionTestMessageListener(test_strings[i], false));
+  }
+
+  const Extension* extension = LoadExtension(
+      test_data_dir_.AppendASCII("window_open").AppendASCII(
+          "close_panels_on_uninstall"));
+  ASSERT_TRUE(extension);
+
+  // Two tabs. One in extension domain and one in non-extension domain.
+  // Two popups - one in extension domain and one in non-extension domain.
+  // Two panels - one in extension domain and one in non-extension domain.
+  WaitForTabsAndPopups(browser(), 2, 2, 2);
+
+  // Wait on test messages to make sure the pages loaded.
+  for (size_t i = 0; i < listeners.size(); ++i)
+    ASSERT_TRUE(listeners[i]->WaitUntilSatisfied());
+
+  // Uninstall the extension.

[dcheng, 2011/10/28 19:48:17]

Comment not needed.

[prasadt, 2011/10/28 20:19:35]

Done.

+  UninstallExtension(extension->id());
+
+  // Wait for one tab and one popup in non-extension domain to stay open.
+  // Expect everything else, including panels, to close.
+  WaitForTabsAndPopups(browser(), 1, 1, 0);
+}
+
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WindowOpener) {
   ASSERT_TRUE(RunExtensionTest("window_open/opener")) << message_;
 }