[reland] Bring up BrowserActionInteractiveTest on Mac.

chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc

Index: chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc
diff --git a/chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc b/chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc
index 1c0ffcdd78f937b7ee85456f64f3c8410731dd8b..250c27b1e8d14cf3e3efb028208397bedd519820 100644
--- a/chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc
+++ b/chrome/browser/extensions/api/extension_action/browser_action_interactive_test.cc
@@ -28,6 +28,7 @@
 #include "extensions/common/permissions/permissions_data.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "extensions/test/result_catcher.h"
+#include "ui/base/ui_features.h"
 
 #if defined(OS_WIN)
 #include "ui/views/win/hwnd_util.h"
@@ -44,6 +45,12 @@ class BrowserActionInteractiveTest : public ExtensionApiTest {
   BrowserActionInteractiveTest() {}
   ~BrowserActionInteractiveTest() override {}
 
+  // BrowserTestBase:
+  void SetUpOnMainThread() override {
+    ExtensionApiTest::SetUpOnMainThread();
+    EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
+  }
+
  protected:
   // Function to control whether to run popup tests for the current platform.
   // These tests require RunExtensionSubtest to work as expected and the browser
@@ -53,17 +60,20 @@ class BrowserActionInteractiveTest : public ExtensionApiTest {
     // TODO(justinlin): http://crbug.com/177163
 #if defined(OS_WIN) && !defined(NDEBUG)
     return false;
-#elif defined(OS_MACOSX)
-    // TODO(justinlin): Browser window do not become active on Mac even when
-    // Activate() is called on them. Enable when/if it's possible to fix.
-    return false;
 #else
     return true;
 #endif
   }
 
+  void EnsurePopupActive() {
+    BrowserActionTestUtil test_util(browser());
+    EXPECT_TRUE(test_util.HasPopup());
+    EXPECT_TRUE(test_util.WaitForPopup());
+    EXPECT_TRUE(test_util.HasPopup());
+  }
+
   // Open an extension popup via the chrome.browserAction.openPopup API.
-  void OpenExtensionPopupViaAPI() {
+  void OpenPopupViaAPI() {
     // Setup the notification observer to wait for the popup to finish loading.
     content::WindowedNotificationObserver frame_observer(
         content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
@@ -72,7 +82,41 @@ class BrowserActionInteractiveTest : public ExtensionApiTest {
     ASSERT_TRUE(RunExtensionSubtest("browser_action/open_popup",
                                     "open_popup_succeeds.html")) << message_;
     frame_observer.Wait();
+    EnsurePopupActive();
+  }
+
+  // Open an extension popup by clicking the browser action button.
+  void OpenPopupViaToolbar() {
+    content::WindowedNotificationObserver frame_observer(
+        content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
+        content::NotificationService::AllSources());
+    BrowserActionTestUtil(browser()).Press(0);
+    frame_observer.Wait();
+    EnsurePopupActive();
+  }
+
+  // Click on the omnibox to close the extension popup.
+  void ClosePopupViaClick() {
     EXPECT_TRUE(BrowserActionTestUtil(browser()).HasPopup());
+    content::WindowedNotificationObserver observer(
+        extensions::NOTIFICATION_EXTENSION_HOST_DESTROYED,
+        content::NotificationService::AllSources());
+
+#if defined(OS_MACOSX)
+    // ClickOnView() in an inactive window is not robust on Mac. The click does
+    // not guarantee window activation on trybots. So activate explicitly.
+    EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));

[Devlin, 2017/05/30 15:12:27]

One worry: Extension popups are non-modal and dismiss on deactivation (which is
all kind of hand-rolled for the class, unfortunately).  If we call
BringBrowserWindowToFront(), since that focuses a different NativeWindow, it
seems like that could result in dismissing the popup directly (since the popup
would no longer be the focused window).  Is that a concern?  If not, can we
explain why in a comment?

At a higher level, this might not be entirely bad - ClosePopupViaClick()
basically tests that the popup is dismissed via focus loss, which this would
still accomplish, and ideally we'd notice if clicking the omnibox no longer
focuses the browser's native window.  So if this is necessary and this alone
*does* dismiss the popup, that wouldn't be entirely bad - but we should rename
things appropriately (e.g. ClosePopupViaFocusLoss()), and maybe coalesce more
code (call BringBrowserWindowToFront for all platforms?  You'd know better than
I if that's sane).

[tapted, 2017/05/31 10:53:35]

Yeah I think the goal of this test step is merely to trigger that dismissal in a
cross-platform way. The mechanism doesn't matter so much.

Using BringBrowserWindowToFront on all platforms is problematic since if the
popup is not a 'toplevel' window the browser widget is seen as always active.
I've renamed to ClosePopupViaFocusLoss() and added comments - see what you
think.

+#endif
+
+    ui_test_utils::ClickOnView(browser(), VIEW_ID_OMNIBOX);
+
+    // The window disappears immediately.
+    EXPECT_FALSE(BrowserActionTestUtil(browser()).HasPopup());
+
+    // Wait for the notification to achieve a consistent state and verify that
+    // the popup was properly torn down.
+    observer.Wait();
+    base::RunLoop().RunUntilIdle();
   }
 };
 
@@ -87,7 +131,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, TestOpenPopup) {
   // Setup extension message listener to wait for javascript to finish running.
   ExtensionTestMessageListener listener("ready", true);
   {
-    OpenExtensionPopupViaAPI();
+    OpenPopupViaAPI();
     EXPECT_TRUE(browserActionBar.HasPopup());
     browserActionBar.HidePopup();
   }
@@ -205,15 +249,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
   ASSERT_TRUE(RunExtensionSubtest("browser_action/open_popup",
                                   "open_popup_fails.html")) << message_;
   EXPECT_TRUE(listener.WaitUntilSatisfied());
-
-  content::WindowedNotificationObserver frame_observer(
-      content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
-      content::NotificationService::AllSources());
-  // Open popup in the first extension.
-  BrowserActionTestUtil(browser()).Press(0);
-  frame_observer.Wait();
-  EXPECT_TRUE(BrowserActionTestUtil(browser()).HasPopup());
-
+  OpenPopupViaToolbar();
   ResultCatcher catcher;
   // Return control to javascript to validate that opening a popup fails now.
   listener.Reply("show another");
@@ -227,7 +263,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
   if (!ShouldRunPopupTest())
     return;
 
-  OpenExtensionPopupViaAPI();
+  OpenPopupViaAPI();
   ExtensionService* service = extensions::ExtensionSystem::Get(
       browser()->profile())->extension_service();
   ASSERT_FALSE(
@@ -243,19 +279,8 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, BrowserClickClosesPopup1) {
   if (!ShouldRunPopupTest())
     return;
-
-  // Open an extension popup via the chrome.browserAction.openPopup API.
-  content::WindowedNotificationObserver frame_observer(
-      content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
-      content::NotificationService::AllSources());
-  ASSERT_TRUE(RunExtensionSubtest("browser_action/open_popup",
-                                  "open_popup_succeeds.html")) << message_;
-  frame_observer.Wait();
-  EXPECT_TRUE(BrowserActionTestUtil(browser()).HasPopup());
-
-  // Click on the omnibox to close the extension popup.
-  ui_test_utils::ClickOnView(browser(), VIEW_ID_OMNIBOX);
-  EXPECT_FALSE(BrowserActionTestUtil(browser()).HasPopup());
+  OpenPopupViaAPI();
+  ClosePopupViaClick();
 }
 
 // Test that the extension popup is closed when the browser window is clicked.
@@ -275,18 +300,8 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
       "browser_action/popup")));
   const Extension* extension = GetSingleLoadedExtension();
   ASSERT_TRUE(extension) << message_;
-
-  // Open an extension popup by clicking the browser action button.
-  content::WindowedNotificationObserver frame_observer(
-      content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
-      content::NotificationService::AllSources());
-  BrowserActionTestUtil(browser()).Press(0);
-  frame_observer.Wait();
-  EXPECT_TRUE(BrowserActionTestUtil(browser()).HasPopup());
-
-  // Click on the omnibox to close the extension popup.
-  ui_test_utils::ClickOnView(browser(), VIEW_ID_OMNIBOX);
-  EXPECT_FALSE(BrowserActionTestUtil(browser()).HasPopup());
+  OpenPopupViaToolbar();
+  ClosePopupViaClick();
 }
 
 // Test that the extension popup is closed on browser tab switches.
@@ -299,7 +314,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, TabSwitchClosesPopup) {
   ASSERT_EQ(2, browser()->tab_strip_model()->count());
   EXPECT_EQ(browser()->tab_strip_model()->GetWebContentsAt(1),
             browser()->tab_strip_model()->GetActiveWebContents());
-  OpenExtensionPopupViaAPI();
+  OpenPopupViaAPI();
 
   content::WindowedNotificationObserver observer(
       extensions::NOTIFICATION_EXTENSION_HOST_DESTROYED,
@@ -317,7 +332,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
     return;
 
   // First, we open a popup.
-  OpenExtensionPopupViaAPI();
+  OpenPopupViaAPI();
   BrowserActionTestUtil browser_action_test_util(browser());
   EXPECT_TRUE(browser_action_test_util.HasPopup());
 
@@ -336,9 +351,15 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
   EXPECT_FALSE(browser_action_test_util.HasPopup());
 }
 
-#if defined(TOOLKIT_VIEWS)
+// BrowserActionTestUtil::InspectPopup() is not implemented for a Cocoa browser.
+#if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER)
+#define MAYBE_CloseBrowserWithDevTools CloseBrowserWithDevTools
+#else
+#define MAYBE_CloseBrowserWithDevTools DISABLED_CloseBrowserWithDevTools
+#endif
 // Test closing the browser while inspecting an extension popup with dev tools.
-IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, CloseBrowserWithDevTools) {
+IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
+                       MAYBE_CloseBrowserWithDevTools) {
   if (!ShouldRunPopupTest())
     return;
 
@@ -359,7 +380,6 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, CloseBrowserWithDevTools) {
   // Close the browser window, this should not cause a crash.
   chrome::CloseWindow(browser());
 }
-#endif  // TOOLKIT_VIEWS
 
 #if defined(OS_WIN)
 // Test that forcibly closing the browser and popup HWND does not cause a crash.
@@ -369,7 +389,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
   if (!ShouldRunPopupTest())
     return;
 
-  OpenExtensionPopupViaAPI();
+  OpenPopupViaAPI();
   BrowserActionTestUtil test_util(browser());
   const gfx::NativeView view = test_util.GetPopupNativeView();
   EXPECT_NE(static_cast<gfx::NativeView>(NULL), view);