Stop relying on ExtensionActionTestHelper::GetPopupSize in tests.

The only test (BrowserAction*Test.BrowserActionPopup) that used to rely
on ExtensionActionTestHelper::GetPopupSize was disabled for a long time.
In the meantime, GetPopupSize stopped working (e.g. it would crash with
a null dereference when not being able to find a native widget
associated with the popup).

This CL re-enables the test (the focus-related flakiness should be fixed
by r754788) and makes the test use
content::WebContents::GetContainerBounds instead of
ExtensionActionTestHelper::GetPopupSize.  After this CL, the
GetPopupSize helper test method is no longer needed and can be removed.

Additionally, the CL removes one additional cause of test flakiness by
making sure that the resizing of the popup has actually propagated all
the way to the content::WebContents::GetContainerBounds by 1) waiting
until layout happens (by having the test go through
requestAnimationFrame) and 2) by waiting until the layout results have
gone through compositing (by having the test wait for
MainThreadFrameObserver).

Bug: 1021172
Change-Id: I84cdd6934d621dfed9ec783fbd0e21d172387fee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2129211
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#755886}

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

@@ -570,45 +570,68 @@ IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, DestroyHWNDDoesNotCrash) {
 }
 #endif  // OS_WIN
 
-// TODO(lukasza): https://crbug.com/1021172: Enable the test below after fixing
-// issues it has accumulated over the long time it was kept disabled.  In
-// particular, the GetPopupSize below may crash (dereferencing nullptr after
-// failing to find the right native widget) or return a wrong size (including
-// window borders if operating on the native window instead of the native
-// widget).
-IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest,
-                       DISABLED_BrowserActionPopup) {
+class MainFrameSizeWaiter : public content::WebContentsObserver {
+ public:
+  MainFrameSizeWaiter(content::WebContents* web_contents,
+                      const gfx::Size& size_to_wait_for)
+      : content::WebContentsObserver(web_contents),
+        size_to_wait_for_(size_to_wait_for) {}
+
+  void Wait() {
+    if (current_size() != size_to_wait_for_)
+      run_loop_.Run();
+  }
+
+ private:
+  gfx::Size current_size() {
+    return web_contents()->GetContainerBounds().size();
+  }
+
+  void MainFrameWasResized(bool width_changed) override {
+    if (current_size() == size_to_wait_for_)
+      run_loop_.Quit();
+  }
+
+  gfx::Size size_to_wait_for_;
+  base::RunLoop run_loop_;
+};
+
+IN_PROC_BROWSER_TEST_F(BrowserActionInteractiveTest, BrowserActionPopup) {
+  if (!ShouldRunPopupTest())
+    return;
+
   ASSERT_TRUE(
       LoadExtension(test_data_dir_.AppendASCII("browser_action/popup")));
-  std::unique_ptr<ExtensionActionTestHelper> actions_bar =
-      ExtensionActionTestHelper::Create(browser());
   const Extension* extension = GetSingleLoadedExtension();
   ASSERT_TRUE(extension) << message_;
 
-  // The extension's popup's size grows by |growFactor| each click.
-  const int growFactor = 500;
+  // The extension's popup's size grows by |kGrowFactor| each click.
+  const int kGrowFactor = 500;
+  std::unique_ptr<ExtensionActionTestHelper> actions_bar =
+      ExtensionActionTestHelper::Create(browser());
   gfx::Size minSize = actions_bar->GetMinPopupSize();
-  gfx::Size middleSize = gfx::Size(growFactor, growFactor);
+  gfx::Size middleSize = gfx::Size(kGrowFactor, kGrowFactor);
   gfx::Size maxSize = actions_bar->GetMaxPopupSize();
 
   // Ensure that two clicks will exceed the maximum allowed size.
-  ASSERT_GT(minSize.height() + growFactor * 2, maxSize.height());
-  ASSERT_GT(minSize.width() + growFactor * 2, maxSize.width());
+  ASSERT_GT(minSize.height() + kGrowFactor * 2, maxSize.height());
+  ASSERT_GT(minSize.width() + kGrowFactor * 2, maxSize.width());
 
   // Simulate a click on the browser action and verify the size of the resulting
-  // popup.  The first one tries to be 0x0, so it should be the min values.
-  ASSERT_TRUE(OpenPopupViaToolbar());
-  EXPECT_EQ(minSize, actions_bar->GetPopupSize());
-  EXPECT_TRUE(actions_bar->HidePopup());
-
-  ASSERT_TRUE(OpenPopupViaToolbar());
-  EXPECT_EQ(middleSize, actions_bar->GetPopupSize());
-  EXPECT_TRUE(actions_bar->HidePopup());
-
-  // One more time, but this time it should be constrained by the max values.
-  ASSERT_TRUE(OpenPopupViaToolbar());
-  EXPECT_EQ(maxSize, actions_bar->GetPopupSize());
-  EXPECT_TRUE(actions_bar->HidePopup());
+  // popup.
+  const gfx::Size kExpectedSizes[] = {minSize, middleSize, maxSize};
+  for (size_t i = 0; i < base::size(kExpectedSizes); i++) {
+    const gfx::Size& kExpectedSize = kExpectedSizes[i];
+    SCOPED_TRACE(testing::Message()
+                 << "Test #" << i << ": size = " << kExpectedSize.ToString());
+
+    content::WebContentsAddedObserver popup_observer;
+    actions_bar->Press(0);
+    content::WebContents* popup = popup_observer.GetWebContents();
+    MainFrameSizeWaiter(popup, kExpectedSize).Wait();
+    EXPECT_EQ(kExpectedSize, popup->GetContainerBounds().size());
+    ASSERT_TRUE(actions_bar->HidePopup());
+  }
 }
 
 // Test that a browser action popup can download data URLs. See

chrome/browser/ui/extensions/extension_action_test_helper.h

@@ -70,9 +70,6 @@ class ExtensionActionTestHelper {
   // Returns whether a browser action popup is being shown currently.
   virtual bool HasPopup() = 0;
 
-  // Returns the size of the current browser action popup.
-  virtual gfx::Size GetPopupSize() = 0;
-
   // Hides the given popup and returns whether the hide was successful.
   virtual bool HidePopup() = 0;
 

chrome/browser/ui/views/extensions/extensions_menu_test_util.cc

@@ -23,7 +23,6 @@
 #include "ui/gfx/image/image.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/view.h"
-#include "ui/views/widget/widget.h"
 
 // A view wrapper class that owns the ExtensionsToolbarContainer.
 // This is used when we don't have a "real" browser window, because the
@@ -144,12 +143,6 @@ bool ExtensionsMenuTestUtil::HasPopup() {
   return !!GetPopupNativeView();
 }
 
-gfx::Size ExtensionsMenuTestUtil::GetPopupSize() {
-  gfx::NativeView popup = GetPopupNativeView();
-  views::Widget* widget = views::Widget::GetWidgetForNativeView(popup);
-  return widget->GetWindowBoundsInScreen().size();
-}
-
 bool ExtensionsMenuTestUtil::HidePopup() {
   // ExtensionsToolbarContainer::HideActivePopup() is private. Get around it by
   // casting to an ExtensionsContainer.

chrome/browser/ui/views/extensions/extensions_menu_test_util.h

@@ -35,7 +35,6 @@ class ExtensionsMenuTestUtil : public ExtensionActionTestHelper {
   std::string GetTooltip(int index) override;
   gfx::NativeView GetPopupNativeView() override;
   bool HasPopup() override;
-  gfx::Size GetPopupSize() override;
   bool HidePopup() override;
   bool ActionButtonWantsToRun(size_t index) override;
   void SetWidth(int width) override;

chrome/browser/ui/views/toolbar/extension_action_test_helper_views.cc

@@ -24,7 +24,6 @@
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/image.h"
 #include "ui/views/test/test_views.h"
-#include "ui/views/widget/widget.h"
 
 // A helper class that owns an instance of a BrowserActionsContainer; this is
 // used when testing without an associated browser window, or if this is for
@@ -130,12 +129,6 @@ bool ExtensionActionTestHelperViews::HasPopup() {
   return GetPopupNativeView() != nullptr;
 }
 
-gfx::Size ExtensionActionTestHelperViews::GetPopupSize() {
-  gfx::NativeView popup = GetPopupNativeView();
-  views::Widget* widget = views::Widget::GetWidgetForNativeView(popup);
-  return widget->GetWindowBoundsInScreen().size();
-}
-
 bool ExtensionActionTestHelperViews::HidePopup() {
   GetToolbarActionsBar()->HideActivePopup();
   return !HasPopup();

chrome/browser/ui/views/toolbar/extension_action_test_helper_views.h

@@ -30,7 +30,6 @@ class ExtensionActionTestHelperViews : public ExtensionActionTestHelper {
   std::string GetTooltip(int index) override;
   gfx::NativeView GetPopupNativeView() override;
   bool HasPopup() override;
-  gfx::Size GetPopupSize() override;
   bool HidePopup() override;
   bool ActionButtonWantsToRun(size_t index) override;
   void SetWidth(int width) override;

chrome/test/data/extensions/api_test/browser_action/popup/popup.js

@@ -2,11 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-function notify_pass() {
-  // We should be done.  Let the test harness know.
-  chrome.test.notifyPass();
-}
-
 function run_tests() {
   // Compute the size of the popup.
   var width = 0;
@@ -29,10 +24,6 @@ function run_tests() {
   width += 500;
   localStorage.height = JSON.stringify(height);
   localStorage.width = JSON.stringify(width);
-
-  // Allow for the pop-up resize to happen (asynchronously)
-  // before saying that the test is done.
-  window.setTimeout(notify_pass, 0);
 }
 
 window.addEventListener("load", function() {