GTK implementation of remembering platform app window geometry.

chrome/browser/extensions/platform_app_browsertest.cc

Index: chrome/browser/extensions/platform_app_browsertest.cc
diff --git a/chrome/browser/extensions/platform_app_browsertest.cc b/chrome/browser/extensions/platform_app_browsertest.cc
index df84214388380c9c6f06d593a855986256fa0d30..a9c7d5f57a7ed61332d3b182952d009f3e8b8cb8 100644
--- a/chrome/browser/extensions/platform_app_browsertest.cc
+++ b/chrome/browser/extensions/platform_app_browsertest.cc
@@ -16,6 +16,7 @@
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/render_process_host.h"
+#include "base/threading/platform_thread.h"
 
 using content::WebContents;
 
@@ -410,4 +411,62 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, MutationEventsDisabled) {
   ASSERT_TRUE(RunPlatformAppTest("platform_apps/mutation_events")) << message_;
 }
 
+// Only implemented in GTK so far.
+#if defined(TOOLKIT_GTK)
+// Test that windows created with an id will remember and restore their
+// geometry when opening new windows.
+IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, ShellWindowRestorePosition) {
+  ExtensionTestMessageListener page2_listener("WaitForPage2", true);
+  ExtensionTestMessageListener page3_listener("WaitForPage3", true);
+  ExtensionTestMessageListener done_listener("Done1", false);
+  ExtensionTestMessageListener done2_listener("Done2", false);
+  ExtensionTestMessageListener done3_listener("Done3", false);
+
+  ASSERT_TRUE(LoadAndLaunchPlatformApp("geometry"));
+
+  // Wait for the app to be launched (although this is mostly to have a
+  // message to reply to to let the script know it should create its second
+  // window.
+  ASSERT_TRUE(page2_listener.WaitUntilSatisfied());
+
+  // Wait for the first window to verify its geometry was correctly set
+  // from the default* attributes passed to the create function.
+  ASSERT_TRUE(done_listener.WaitUntilSatisfied());
+
+  // programatically move and resize the window
+  ShellWindow* window = GetFirstShellWindow();
+  ASSERT_TRUE(window);
+  gfx::Rect bounds(137, 143, 203, 187);
+  window->GetBaseWindow()->SetBounds(bounds);
+
+  // TODO(mek): there is a delay (and on linux an X roundtrip) between setting
+  // bounds for a window and being able to read back those same bounds. This
+  // seems to work to wait long enough, but there are probably still situations
+  // where timing still works out badly.

[Evan Stade, 2012/08/29 21:51:11]

polling is better than spinning, but still sucks. Why don't you just manually
call OnDebouncedBoundsChanged?

[Marijn Kruisselbrink, 2012/08/30 16:46:12]

Just calling OnDebounceBoundsChanged is not enough, since that is just used to
get the right origin of the window, not for the size (and even if I do add code
to it to also get the size from gtk, I seem to get even more
unreliable/non-deterministic behavior). So this polling is waiting both for
(several) OnConfigure events and the subsequent debounce timer (after calling
SetBounds I seem to first get two OnConfigure events with just the position
changed, and only a couple of RunAllPendingInMessageLoop / some delay another
couple of OnConfigure events with also the size changed).

[Evan Stade, 2012/08/30 21:59:22]

the bottom line is that introducing a timeout like this will make the test
flaky, so don't do it. In many situations (debug, a system under load, etc.) one
second will not be enough. The first step to fixing the problem is to completely
understand it. Luckily, GTK is open source which helps you drill down if tracing
through chrome code doesn't provide enough clues.

[Marijn Kruisselbrink, 2012/08/30 23:26:35]

Okay, I read through GTK+ code, and the conclusion is that by design there is an
unknown delay between resizing a window and the new size being available to
gtk_window_get_size or any other method of getting the size. The way it works is
that GTK sends the resize request to the X server/window manager, and then
hopefully at some later point in time, completely beyond our/gtk's control the
window manager will respond to notify us that the size did indeed change. So
some waiting/polling is just unavoidable. I guess there might be some mechanism
already in place to kill tests that take too long so I could remove the timeout
entirely for this loop.

[Evan Stade, 2012/08/30 23:55:38]

there's nothing wrong with waiting for that size changed notification, but I
wouldn't remove the timeout.

+  const base::TimeDelta kWaitTime = base::TimeDelta::FromSeconds(1);
+  base::TimeTicks end_time = base::TimeTicks::Now() + kWaitTime;
+  while (base::TimeTicks::Now() < end_time) {
+    content::RunAllPendingInMessageLoop();
+  }
+
+  // make sure the window was properly moved&resized
+  ASSERT_EQ(bounds, window->GetBaseWindow()->GetBounds());
+
+  // tell javascript to open a second window
+  page2_listener.Reply("continue");
+
+  // wait for javascript to verify that the second window got the updated
+  // coordinates, ignoring its default* create params.

[Evan Stade, 2012/08/29 21:51:11]

what does the asterisk mean here

[Marijn Kruisselbrink, 2012/08/30 16:46:12]

Done.

+  ASSERT_TRUE(done2_listener.WaitUntilSatisfied());
+
+

[Evan Stade, 2012/08/29 21:51:11]

^H

[Marijn Kruisselbrink, 2012/08/30 16:46:12]

Done.

+  // tell javascript to open a third window

[Evan Stade, 2012/08/29 21:51:11]

sentence-like punctuation and capitalization in comments.

[Marijn Kruisselbrink, 2012/08/30 16:46:12]

Done.

+  page3_listener.Reply("continue");
+
+  // wait for javascript to verify that the third window got the restored size
+  // and explicitly specified coordinates.
+  ASSERT_TRUE(done3_listener.WaitUntilSatisfied());
+}
+#endif // defined(TOOLKIT_GTK)

[Evan Stade, 2012/08/29 21:51:11]

2 spaces before //

[Marijn Kruisselbrink, 2012/08/30 16:46:12]

Done.

+
 }  // namespace extensions