[reland] Bring up BrowserActionInteractiveTest on Mac.

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

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "build/build_config.h"
 #include "chrome/browser/extensions/browser_action_test_util.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ skipping 10 matching lines @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/notification_types.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_set.h"
 #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"
 #endif
 
 namespace extensions {
 namespace {
 
 // chrome.browserAction API tests that interact with the UI in such a way that
 // they cannot be run concurrently (i.e. openPopup API tests that require the
 // window be focused/active).
 class BrowserActionInteractiveTest : public ExtensionApiTest {
  public:
   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
   // window to able to be made active automatically. Returns false for platforms
   // where these conditions are not met.
   bool ShouldRunPopupTest() {
     // 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,
         content::NotificationService::AllSources());
     // Show first popup in first window and expect it to have loaded.
     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();
   }
 };
 
 // Tests opening a popup using the chrome.browserAction.openPopup API. This test
 // opens a popup in the starting window, closes the popup, creates a new window
 // and opens a popup in the new window. Both popups should succeed in opening.
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, TestOpenPopup) {
   if (!ShouldRunPopupTest())
     return;
 
   BrowserActionTestUtil browserActionBar(browser());
   // Setup extension message listener to wait for javascript to finish running.
   ExtensionTestMessageListener listener("ready", true);
   {
-    OpenExtensionPopupViaAPI();
+    OpenPopupViaAPI();
     EXPECT_TRUE(browserActionBar.HasPopup());
     browserActionBar.HidePopup();
   }
 
   EXPECT_TRUE(listener.WaitUntilSatisfied());
   Browser* new_browser = NULL;
   {
     content::WindowedNotificationObserver frame_observer(
         content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
         content::NotificationService::AllSources());
@@ skipping 97 matching lines @@
       "browser_action/popup")));
   const Extension* extension = GetSingleLoadedExtension();
   ASSERT_TRUE(extension) << message_;
 
   ExtensionTestMessageListener listener("ready", true);
   // Load the test extension which will do nothing except notifyPass() to
   // return control here.
   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");
   ASSERT_TRUE(catcher.GetNextResult()) << message_;
 }
 
 // Test that openPopup does not grant tab permissions like for browser action
 // clicks if the activeTab permission is set.
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
                        TestOpenPopupDoesNotGrantTabPermissions) {
   if (!ShouldRunPopupTest())
     return;
 
-  OpenExtensionPopupViaAPI();
+  OpenPopupViaAPI();
   ExtensionService* service = extensions::ExtensionSystem::Get(
       browser()->profile())->extension_service();
   ASSERT_FALSE(
       service->GetExtensionById(last_loaded_extension_id(), false)
           ->permissions_data()
           ->HasAPIPermissionForTab(
               SessionTabHelper::IdForTab(
                   browser()->tab_strip_model()->GetActiveWebContents()),
               APIPermission::kTab));
 }
 
 // Test that the extension popup is closed when the browser window is clicked.
 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.
 #if defined(OS_WIN)
 // Flaky on Windows: http://crbug.com/639130
 #define MAYBE_BrowserClickClosesPopup2 DISABLED_BrowserClickClosesPopup2
 #else
 #define MAYBE_BrowserClickClosesPopup2 BrowserClickClosesPopup2
 #endif
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
                        MAYBE_BrowserClickClosesPopup2) {
   if (!ShouldRunPopupTest())
     return;
 
   // Load a first extension that can open a popup.
   ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII(
       "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.
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, TabSwitchClosesPopup) {
   if (!ShouldRunPopupTest())
     return;
 
   // Add a second tab to the browser and open an extension popup.
   chrome::NewTab(browser());
   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,
       content::NotificationService::AllSources());
   // Change active tabs, the extension popup should close.
   browser()->tab_strip_model()->ActivateTabAt(0, true);
   observer.Wait();
 
   EXPECT_FALSE(BrowserActionTestUtil(browser()).HasPopup());
 }
 
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
                        DeleteBrowserActionWithPopupOpen) {
   if (!ShouldRunPopupTest())
     return;
 
   // First, we open a popup.
-  OpenExtensionPopupViaAPI();
+  OpenPopupViaAPI();
   BrowserActionTestUtil browser_action_test_util(browser());
   EXPECT_TRUE(browser_action_test_util.HasPopup());
 
   // Then, find the extension that created it.
   content::WebContents* active_web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
   ASSERT_TRUE(active_web_contents);
   GURL url = active_web_contents->GetLastCommittedURL();
   const Extension* extension = ExtensionRegistry::Get(browser()->profile())->
       enabled_extensions().GetExtensionOrAppByURL(url);
   ASSERT_TRUE(extension);
 
   // Finally, uninstall the extension, which causes the view to be deleted and
   // the popup to go away. This should not crash.
   UninstallExtension(extension->id());
   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;
 
   // Load a first extension that can open a popup.
   ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII(
       "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()).InspectPopup(0);
   frame_observer.Wait();
   EXPECT_TRUE(BrowserActionTestUtil(browser()).HasPopup());
 
   // 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.
 // http://crbug.com/400646
 IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
                        DISABLED_DestroyHWNDDoesNotCrash) {
   if (!ShouldRunPopupTest())
     return;
 
-  OpenExtensionPopupViaAPI();
+  OpenPopupViaAPI();
   BrowserActionTestUtil test_util(browser());
   const gfx::NativeView view = test_util.GetPopupNativeView();
   EXPECT_NE(static_cast<gfx::NativeView>(NULL), view);
   const HWND hwnd = views::HWNDForNativeView(view);
   EXPECT_EQ(hwnd,
             views::HWNDForNativeView(browser()->window()->GetNativeWindow()));
   EXPECT_EQ(TRUE, ::IsWindow(hwnd));
 
   // Create a new browser window to prevent the message loop from terminating.
   browser()->OpenURL(content::OpenURLParams(GURL("about:"), content::Referrer(),
                                             WindowOpenDisposition::NEW_WINDOW,
                                             ui::PAGE_TRANSITION_TYPED, false));
 
   // Forcibly closing the browser HWND should not cause a crash.
   EXPECT_EQ(TRUE, ::CloseWindow(hwnd));
   EXPECT_EQ(TRUE, ::DestroyWindow(hwnd));
   EXPECT_EQ(FALSE, ::IsWindow(hwnd));
 }
 #endif  // OS_WIN
 
 }  // namespace
 }  // namespace extensions