GTK implementation of remembering platform app window geometry.

GTK implementation of remembering platform app window geometry.
This adds the necesary code to shell_window_gtk to remember window geometry, and a browser test to test the behavior.

BUG=140658
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154537

[Marijn Kruisselbrink, 2012-08-27 18:15:16]

Here are the gtk specific bits, as well as a browser test to test it; the
various hard-coded delays in the test make it a bit fragile, but I'm not sure if
there is a way to get around that

[asargent_no_longer_on_chrome, 2012-08-29 00:01:56]

+estade tp look at gtk-specific parts

https://chromiumcodereview.appspot.com/10871087/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10871087/diff/1/chrome/browser/extensi...
chrome/browser/extensions/platform_app_browsertest.cc:382: }
Instead of always waiting 1 second to read back the bounds, is there some "this
window is ready" event we can listen for? (estade might know)

Another option would be to poll in the test javascript using setInterval every
200ms or so to see if we get the right answer, and time out after some
reasonable period of time (we could pick 5-10 seconds, or I think the googletest
framework lets us ask in C++ code if the currently running test has exceeded its
timeline. )

[Marijn Kruisselbrink, 2012-08-29 00:08:10]

On 2012/08/29 00:01:56, Antony Sargent wrote:
> +estade tp look at gtk-specific parts
> 
>
https://chromiumcodereview.appspot.com/10871087/diff/1/chrome/browser/extensi...
> File chrome/browser/extensions/platform_app_browsertest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10871087/diff/1/chrome/browser/extensi...
> chrome/browser/extensions/platform_app_browsertest.cc:382: }
> Instead of always waiting 1 second to read back the bounds, is there some
"this
> window is ready" event we can listen for? (estade might know)
> 
> Another option would be to poll in the test javascript using setInterval every
> 200ms or so to see if we get the right answer, and time out after some
> reasonable period of time (we could pick 5-10 seconds, or I think the
googletest
> framework lets us ask in C++ code if the currently running test has exceeded
its
> timeline. )
Polling from the C++ side should work too and might be cleaner then doing it
from the javascript side, just exiting the loop as soon as GetBounds returns the
value we expect.

[asargent_no_longer_on_chrome, 2012-08-29 18:19:34]

That sounds reasonable.

On Tue, Aug 28, 2012 at 5:08 PM, <mek@chromium.org> wrote:

> On 2012/08/29 00:01:56, Antony Sargent wrote:
>
>> +estade tp look at gtk-specific parts
>>
>
>
> https://chromiumcodereview.**appspot.com/10871087/diff/1/**
>
chrome/browser/extensions/**platform_app_browsertest.cc<https://chromiumcodereview.appspot.com/10871087/diff/1/chrome/browser/extensions/platform_app_browsertest.cc>
>
>> File chrome/browser/extensions/**platform_app_browsertest.cc (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/10871087/diff/1/**
>
chrome/browser/extensions/**platform_app_browsertest.cc#**newcode382<https://chromiumcodereview.appspot.com/10871087/diff/1/chrome/browser/extensions/platform_app_browsertest.cc#newcode382>
>
>> chrome/browser/extensions/**platform_app_browsertest.cc:**382: }
>> Instead of always waiting 1 second to read back the bounds, is there some
>>
> "this
>
>> window is ready" event we can listen for? (estade might know)
>>
>
>  Another option would be to poll in the test javascript using setInterval
>> every
>> 200ms or so to see if we get the right answer, and time out after some
>> reasonable period of time (we could pick 5-10 seconds, or I think the
>>
> googletest
>
>> framework lets us ask in C++ code if the currently running test has
>> exceeded
>>
> its
>
>> timeline. )
>>
> Polling from the C++ side should work too and might be cleaner then doing
> it
> from the javascript side, just exiting the loop as soon as GetBounds
> returns the
> value we expect.
>
>
>
https://chromiumcodereview.**appspot.com/10871087/<https://chromiumcodereview...
>

[Marijn Kruisselbrink, 2012-08-29 21:43:11]

Okay, implemented that.
On 2012/08/29 18:19:34, Antony Sargent wrote:
> That sounds reasonable.
> 
> On Tue, Aug 28, 2012 at 5:08 PM, <mailto:mek@chromium.org> wrote:
> 
> > On 2012/08/29 00:01:56, Antony Sargent wrote:
> > Polling from the C++ side should work too and might be cleaner then doing
> > it from the javascript side, just exiting the loop as soon as GetBounds
> > returns the value we expect.

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

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:445: // where timing still
works out badly.
polling is better than spinning, but still sucks. Why don't you just manually
call OnDebouncedBoundsChanged?

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:459: // coordinates,
ignoring its default* create params.
what does the asterisk mean here

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:462: 
^H

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:463: // tell javascript to
open a third window
sentence-like punctuation and capitalization in comments.

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:470: #endif //
defined(TOOLKIT_GTK)
2 spaces before //

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ui/...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:255: if (!IsFullscreen() &&
!IsMaximized())
don't copy paste code. Refactor as much as you can into gtk_window_util.h/cc
(shouldn't be that hard as these functions are common to BaseWindows)

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

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:445: // where timing still
works out badly.
On 2012/08/29 21:51:11, Evan Stade wrote:
> polling is better than spinning, but still sucks. Why don't you just manually
> call OnDebouncedBoundsChanged?
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).

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:459: // coordinates,
ignoring its default* create params.
On 2012/08/29 21:51:11, Evan Stade wrote:
> what does the asterisk mean here

Done.

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:462: 
On 2012/08/29 21:51:11, Evan Stade wrote:
> ^H

Done.

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:463: // tell javascript to
open a third window
On 2012/08/29 21:51:11, Evan Stade wrote:
> sentence-like punctuation and capitalization in comments.

Done.

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:470: #endif //
defined(TOOLKIT_GTK)
On 2012/08/29 21:51:11, Evan Stade wrote:
> 2 spaces before //

Done.

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ui/...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ui/...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:255: if (!IsFullscreen() &&
!IsMaximized())
On 2012/08/29 21:51:11, Evan Stade wrote:
> don't copy paste code. Refactor as much as you can into gtk_window_util.h/cc
> (shouldn't be that hard as these functions are common to BaseWindows)
Yeah, I noticed there seeming to be quite some duplication between
shell_window_gtk and browser_window_gtk. I factored out some code into
gtk_window_util now. I guess it might not feasible/useful to have some common
base class to share more code?

[Evan Stade, 2012-08-30 21:59:21]

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:445: // where timing still
works out badly.
On 2012/08/30 16:46:12, Marijn Kruisselbrink wrote:
> On 2012/08/29 21:51:11, Evan Stade wrote:
> > polling is better than spinning, but still sucks. Why don't you just
manually
> > call OnDebouncedBoundsChanged?
> 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).

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]

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:445: // where timing still
works out badly.
On 2012/08/30 21:59:22, Evan Stade wrote:
> On 2012/08/30 16:46:12, Marijn Kruisselbrink wrote:
> > On 2012/08/29 21:51:11, Evan Stade wrote:
> > > polling is better than spinning, but still sucks. Why don't you just
> manually
> > > call OnDebouncedBoundsChanged?
> > 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).
> 
> 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.
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.

https://chromiumcodereview.appspot.com/10871087/diff/24003/chrome/browser/ext...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10871087/diff/24003/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:460:
window->SaveWindowPosition();
alternatively I could make the test a friend of ShellWindowGtk and do a
static_cast<ShellWindowGtk*>(window->GetBaseWindow())->OnDebouncedBoundsChanged()
but that seems even uglier.

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

lgtm

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10871087/diff/11001/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:445: // where timing still
works out badly.
On 2012/08/30 23:26:35, Marijn Kruisselbrink wrote:
> On 2012/08/30 21:59:22, Evan Stade wrote:
> > On 2012/08/30 16:46:12, Marijn Kruisselbrink wrote:
> > > On 2012/08/29 21:51:11, Evan Stade wrote:
> > > > polling is better than spinning, but still sucks. Why don't you just
> > manually
> > > > call OnDebouncedBoundsChanged?
> > > 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).
> > 
> > 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.
> 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.

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

https://chromiumcodereview.appspot.com/10871087/diff/24003/chrome/browser/ext...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10871087/diff/24003/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:460:
window->SaveWindowPosition();
On 2012/08/30 23:26:35, Marijn Kruisselbrink wrote:
> alternatively I could make the test a friend of ShellWindowGtk and do a
>
static_cast<ShellWindowGtk*>(window->GetBaseWindow())->OnDebouncedBoundsChanged()
> but that seems even uglier.

meh, it's ok, it's a test.

https://chromiumcodereview.appspot.com/10871087/diff/24003/chrome/browser/ui/...
File chrome/browser/ui/gtk/gtk_window_util.cc (right):

https://chromiumcodereview.appspot.com/10871087/diff/24003/chrome/browser/ui/...
chrome/browser/ui/gtk/gtk_window_util.cc:187: void
UpdateWindowPosition(BaseWindow* window, gfx::Rect* bounds,
one param per line

https://chromiumcodereview.appspot.com/10871087/diff/24003/chrome/browser/ui/...
chrome/browser/ui/gtk/gtk_window_util.cc:193: (*restored_bounds) = *bounds;
no parens

[Marijn Kruisselbrink, 2012-08-31 17:20:42]

Antony: any more comments for the chrome/browser/extensions part?

[asargent_no_longer_on_chrome, 2012-08-31 18:14:34]

LGTM

https://chromiumcodereview.appspot.com/10871087/diff/19004/chrome/browser/ext...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10871087/diff/19004/chrome/browser/ext...
chrome/browser/extensions/platform_app_browsertest.cc:453: const base::TimeDelta
kWaitTime = base::TimeDelta::FromSeconds(10);
nit: instead of using the fixed value of 10 seconds, let's instead use
TestTimeouts::action_timeout() - this value gets adjusted apropriately when
tests are being run in various environments (slower hardware, under valgrind,
etc.)

[commit-bot: I haz the power, 2012-08-31 18:31:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10871087/17003

[commit-bot: I haz the power, 2012-08-31 18:31:50]

Failed to apply patch for chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/gtk/extensions/shell_window_gtk.cc
Hunk #1 succeeded at 17 (offset 1 line).
Hunk #2 FAILED at 37.
Hunk #3 succeeded at 145 (offset 8 lines).
Hunk #4 succeeded at 220 (offset 7 lines).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc.rej

[commit-bot: I haz the power, 2012-08-31 18:59:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10871087/21002

[commit-bot: I haz the power, 2012-08-31 23:37:38]

Change committed as 154537