Enable AutoResize for Constrained Web Dialogs

chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc

Index: chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc
diff --git a/chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc b/chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc
index 5570984094b8f76ba6d46db87a6e54f4e11814d3..e3d8508239b3e03364cc5d0a67bc42914b81a1fb 100644
--- a/chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc
+++ b/chrome/browser/ui/webui/constrained_web_dialog_ui_browsertest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/callback.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
@@ -12,6 +13,8 @@
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "ui/web_dialogs/test/test_web_dialog_delegate.h"
 
 using content::WebContents;
@@ -43,6 +46,30 @@ class ConstrainedWebDialogBrowserTest : public InProcessBrowserTest {
  public:
   ConstrainedWebDialogBrowserTest() {}
 
+  bool IsEqualSizes(gfx::Size expected,

[msw, 2014/12/09 20:34:41]

Why does this need to be a ConstrainedWebDialogBrowserTest member? Can this just
be an anonymous namespace function in this test file and then the Bind calls can
avoid using base::Unretained(this) on the test class instance.

[apacible, 2014/12/10 18:39:29]

Done.

+                    ConstrainedWebDialogDelegate* dialog_delegate) {
+    return expected == dialog_delegate->GetPreferredSize();
+  }
+
+  // Runs the current MessageLoop until |condition| is true or timeout.
+  bool RunLoopUntil(const base::Callback<bool()>& condition) {

[msw, 2014/12/09 20:34:41]

This seems like a bad practice that may yield flaky failures and poor test
performance. Can these tests employ a web contents observer or similar instead?

[apacible, 2014/12/10 18:39:29]

The dialog resizing is asynchronous, so we want to block any assert calls until
the resizing is done. RunLoopUntil will prevent flakiness since the sizing will
definitely be done before any asserts are called.

/cc miu, who suggested this.

[miu, 2014/12/10 19:28:22]

To be clear, the resizing is initiated by the render process.  So, there's no
way to know when that resize message will reach the browser process and then hop
from the IO thread to a UI thread task.  The code here simply blocks the caller,
running the UI MessageLoop until that expected task has posted and been run.

[msw, 2014/12/11 01:13:57]

I was hoping this could use something like WaitForResizeComplete but I'm not an
expert here, so I'll defer to you and others.

[apacible, 2014/12/11 03:49:24]

I looked into WaitForResizeComplete, but it's aura only. I didn't find anything
else similar that I could use here.

+    const base::TimeTicks start_time = base::TimeTicks::Now();
+    while (!condition.Run()) {
+      const base::TimeTicks current_time = base::TimeTicks::Now();
+      if (current_time - start_time > base::TimeDelta::FromSeconds(5)) {
+        ADD_FAILURE() << "Condition not met within five seconds.";
+        return false;
+      }
+
+      base::MessageLoop::current()->PostDelayedTask(
+          FROM_HERE,
+          base::MessageLoop::QuitClosure(),
+          base::TimeDelta::FromMilliseconds(20));
+      content::RunMessageLoop();
+    }
+    return true;
+  }
+
  protected:
   bool IsShowingWebContentsModalDialog(WebContents* web_contents) const {
     WebContentsModalDialogManager* web_contents_modal_dialog_manager =
@@ -61,7 +88,7 @@ IN_PROC_BROWSER_TEST_F(ConstrainedWebDialogBrowserTest, BasicTest) {
   ASSERT_TRUE(web_contents);
 
   ConstrainedWebDialogDelegate* dialog_delegate =
-      CreateConstrainedWebDialog(browser()->profile(), delegate, web_contents);
+      ShowConstrainedWebDialog(browser()->profile(), delegate, web_contents);
   ASSERT_TRUE(dialog_delegate);
   EXPECT_TRUE(dialog_delegate->GetNativeDialog());
   EXPECT_TRUE(IsShowingWebContentsModalDialog(web_contents));
@@ -78,7 +105,7 @@ IN_PROC_BROWSER_TEST_F(ConstrainedWebDialogBrowserTest,
   ASSERT_TRUE(web_contents);
 
   ConstrainedWebDialogDelegate* dialog_delegate =
-      CreateConstrainedWebDialog(browser()->profile(), delegate, web_contents);
+      ShowConstrainedWebDialog(browser()->profile(), delegate, web_contents);
   ASSERT_TRUE(dialog_delegate);
   scoped_ptr<WebContents> new_tab(dialog_delegate->GetWebContents());
   ASSERT_TRUE(new_tab.get());
@@ -93,3 +120,155 @@ IN_PROC_BROWSER_TEST_F(ConstrainedWebDialogBrowserTest,
   new_tab.reset();
   EXPECT_TRUE(observer.contents_destroyed());
 }
+
+#if !defined(OS_MACOSX)
+// Tests that dialog autoresizes based on web contents when autoresizing
+// is enabled.
+IN_PROC_BROWSER_TEST_F(ConstrainedWebDialogBrowserTest,
+                       ContentResizeInAutoResizingDialog) {
+  std::string url = "data:text/html,<!doctype html>"
+      "<body></body>"
+      "<style>"
+      "body { height: 150px; width: 150px; }"
+      "</style>"
+      "<script>"
+      "  function changeDimensions(width, height) {"
+      "    window.document.body.style.width = width + 'px';"
+      "    window.document.body.style.height = height + 'px';"
+      "  }"
+      "  document.title='ready';"
+      "</script>";
+
+  // The delegate deletes itself.
+  WebDialogDelegate* delegate = new ui::test::TestWebDialogDelegate(

[msw, 2014/12/09 20:34:41]

nit: break line after assignment operator

[apacible, 2014/12/10 18:39:29]

Done.

[msw, 2014/12/11 01:13:57]

Ping (not actually done).

[apacible, 2014/12/11 03:49:24]

Oops, sorry. Done.

+      GURL(url));
+  WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(web_contents);
+
+  // Observes the next created WebContents.
+  content::TestNavigationObserver observer(NULL);
+  observer.StartWatchingNewWebContents();
+
+  gfx::Size min_size = gfx::Size(100, 100);
+  gfx::Size max_size = gfx::Size(200, 200);
+  gfx::Size initial_dialog_size;
+  delegate->GetDialogSize(&initial_dialog_size);
+
+  ConstrainedWebDialogDelegate* dialog_delegate =
+      CreateConstrainedWebDialogWithAutoResize(browser()->profile(), delegate,
+                                               web_contents, min_size,
+                                               max_size);
+  ASSERT_TRUE(dialog_delegate);
+  EXPECT_TRUE(dialog_delegate->GetNativeDialog());
+  ASSERT_FALSE(IsShowingWebContentsModalDialog(web_contents));
+  EXPECT_EQ(min_size, dialog_delegate->GetMinimumSize());
+  EXPECT_EQ(max_size, dialog_delegate->GetMaximumSize());
+
+  // Check for initial sizing. Dialog was created as a 400x400 dialog.
+  EXPECT_EQ(gfx::Size(), web_contents->GetPreferredSize());
+  ASSERT_EQ(initial_dialog_size, dialog_delegate->GetPreferredSize());
+
+  observer.Wait();
+
+  // Wait until the entire WebContents has loaded.
+  base::string16 ready_title(base::ASCIIToUTF16("ready"));

[msw, 2014/12/09 20:34:41]

Is there no better pattern to observe webcontents loading in tests?
In the same browser_test_utils.h I see WaitForLoadStop*, etc.
(avoiding a wait would be nice, but not necessary, I suppose...)

[apacible, 2014/12/10 18:39:29]

I tried using WaitForLoadStop*, but it doesn't always wait until all of the
WebContents has actually loaded. I consistently got "Uncaught ReferenceError:
changeDimensions is not defined" since the rest of the test was trying to run
before the entire WebContents has loaded.

[msw, 2014/12/11 01:13:57]

There's probably some mechanism (maybe a test util?) to wait until scripts are
loaded, but I don't know where to look for that offhand. Why don't you just pass
the inlined script in your call to ExecuteScript, rather than including the
script in the data URL's HTML? Then you can probably use WaitForLoadStop instead
of the more roundabout TitleWatcher.

[apacible, 2014/12/11 03:49:24]

Done. I moved the JS to a call in ExecuteScript and removed it from the URL's
HTML. I also changed TitleWatcher to WaitForLoadStop.

+  content::TitleWatcher watcher(dialog_delegate->GetWebContents(),
+                                ready_title);
+  ignore_result(watcher.WaitAndGetTitle());
+
+  ASSERT_TRUE(IsShowingWebContentsModalDialog(web_contents));
+
+  // Dialog size to WebContents size + 16 for both width and height.

[msw, 2014/12/09 20:34:41]

This seems fragile, can this value be determined within the test?

[apacible, 2014/12/10 18:39:29]

I haven't found a way to determine the value yet, but still looking into this...

[apacible, 2014/12/10 20:32:58]

Per offline conversation with msw -- made the dialog border (16) into a constant
in patch 10.

+  // Resize to content's originally set dimensions.
+  EXPECT_TRUE(RunLoopUntil(base::Bind(
+      &ConstrainedWebDialogBrowserTest::IsEqualSizes,
+      base::Unretained(this),
+      gfx::Size(166, 166), // 150 + 16
+      dialog_delegate)));
+  ASSERT_EQ(gfx::Size(166, 166), dialog_delegate->GetPreferredSize());
+
+  // Resize to dimensions within expected bounds.
+  EXPECT_TRUE(ExecuteScript(dialog_delegate->GetWebContents(),
+      "changeDimensions(175, 175);"));
+  EXPECT_TRUE(RunLoopUntil(base::Bind(
+      &ConstrainedWebDialogBrowserTest::IsEqualSizes,
+      base::Unretained(this),
+      gfx::Size(191, 191), // 175 + 16
+      dialog_delegate)));
+  ASSERT_EQ(gfx::Size(191, 191), dialog_delegate->GetPreferredSize());
+
+  // Resize to dimensions smaller than the minimum bounds.
+  EXPECT_TRUE(ExecuteScript(dialog_delegate->GetWebContents(),
+      "changeDimensions(50, 50);"));
+  EXPECT_TRUE(RunLoopUntil(base::Bind(
+      &ConstrainedWebDialogBrowserTest::IsEqualSizes,
+      base::Unretained(this),
+      min_size,
+      dialog_delegate)));
+  ASSERT_EQ(min_size, dialog_delegate->GetPreferredSize());
+
+  // Resize to dimensions greater than the maximum bounds.
+  EXPECT_TRUE(ExecuteScript(dialog_delegate->GetWebContents(),
+      "changeDimensions(250, 250);"));
+  EXPECT_TRUE(RunLoopUntil(base::Bind(
+      &ConstrainedWebDialogBrowserTest::IsEqualSizes,
+      base::Unretained(this),
+      max_size,
+      dialog_delegate)));
+  ASSERT_EQ(max_size, dialog_delegate->GetPreferredSize());
+}
+
+// Tests that dialog does not autoresize when autoresizing is not enabled.
+IN_PROC_BROWSER_TEST_F(ConstrainedWebDialogBrowserTest,
+                       ContentResizeInNonAutoResizingDialog) {
+  std::string url = "data:text/html,<!doctype html>"

[msw, 2014/12/09 20:34:41]

nit: make this a shared local/class static const char kTestDataURL[] or similar.

[apacible, 2014/12/10 18:39:29]

Done.

+      "<body></body>"
+      "<style>"
+      "body { height: 150px; width: 150px; }"
+      "</style>"
+      "<script>"
+      "  function changeDimensions(width, height) {"
+      "    window.document.body.style.width = width + 'px';"
+      "    window.document.body.style.height = height + 'px';"
+      "  }"
+      "  document.title='ready';"
+      "</script>";
+
+  // The delegate deletes itself.
+  WebDialogDelegate* delegate = new ui::test::TestWebDialogDelegate(
+      GURL(url));
+  WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(web_contents);
+
+  ConstrainedWebDialogDelegate* dialog_delegate =
+      ShowConstrainedWebDialog(browser()->profile(), delegate, web_contents);
+  ASSERT_TRUE(dialog_delegate);
+  EXPECT_TRUE(dialog_delegate->GetNativeDialog());
+  EXPECT_TRUE(IsShowingWebContentsModalDialog(web_contents));
+
+  // Wait until the entire WebContents has loaded.
+  base::string16 ready_title(base::ASCIIToUTF16("ready"));
+  content::TitleWatcher watcher(dialog_delegate->GetWebContents(),
+                                ready_title);
+  ignore_result(watcher.WaitAndGetTitle());
+
+  gfx::Size initial_dialog_size;
+  delegate->GetDialogSize(&initial_dialog_size);
+
+  // Check for initial sizing. Dialog was created as a 400x400 dialog.
+  EXPECT_EQ(gfx::Size(), web_contents->GetPreferredSize());
+  ASSERT_EQ(initial_dialog_size, dialog_delegate->GetPreferredSize());
+
+  // Resize <body> to dimension smaller than dialog.
+  EXPECT_TRUE(ExecuteScript(dialog_delegate->GetWebContents(),
+      "changeDimensions(100, 100);"));
+  ASSERT_EQ(initial_dialog_size, dialog_delegate->GetPreferredSize());

[msw, 2014/12/09 20:34:41]

Shouldn't there theoretically be the same wait/callback mechanism to ensure that
the initiator web contents has resized and run its callback to offer the dialog
the chance to resize (and then test that it was not, in fact, resized?).
Otherwise, I think you could make this dialog resizable and this test would
still pass.

[apacible, 2014/12/10 18:39:29]

Done.

+
+  // Resize <body> to dimension larger than dialog.
+  EXPECT_TRUE(ExecuteScript(dialog_delegate->GetWebContents(),
+      "changeDimensions(500, 500);"));
+  ASSERT_EQ(initial_dialog_size, dialog_delegate->GetPreferredSize());
+}
+#endif  // !OS_MACOSX