Linux: avoid browser windows moving around by the size of WM decorations over restart.

Linux: avoid browser windows moving around by the size of WM decorations over restart.
Use a debounce timer to get the true window position shortly after the last reconfigure event is delivered, and save that.
BUG=18771
TEST=none


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

[tony, 2009-09-11 18:11:23]

Where in the code are x & y used that is causing the positioning to happen? 
We're not supposed to be positioning the window at all, so it seems like we
should fix it there instead.

Out of curiosity, do you know why it doesn't repro on all window managers? 
E.g., I can't seem to repro this in metacity and kwin3 on hardy.  I'm worried
that with this change, my windows will now be positioned incorrectly.

[Evan Martin, 2009-09-11 18:14:37]

Tony is right -- we shouldn't be using these coordinates.

(I am also concerned that getting this position is another round trip to the X
server.)

[Mike Mammarella, 2009-09-11 18:40:24]

Looks like ultimately it's being caused by the call to
Browser::set_override_bounds() in SessionRestoreImpl::OnGotSession(). You have
to set the "Restore the pages that were open last" preference for it to do that.

What to do, wrap that in #if !defined(OS_LINUX)? And add a comment explaining
why, pointing at the comments in browser_window_gtk.cc?

On 2009/09/11 18:14:37, Evan Martin wrote:
> Tony is right -- we shouldn't be using these coordinates.
> 
> (I am also concerned that getting this position is another round trip to the X
> server.)

[tony, 2009-09-11 18:48:21]

On 2009/09/11 18:40:24, Mike Mammarella wrote:
> Looks like ultimately it's being caused by the call to
> Browser::set_override_bounds() in SessionRestoreImpl::OnGotSession(). You have
> to set the "Restore the pages that were open last" preference for it to do
that.
> 
> What to do, wrap that in #if !defined(OS_LINUX)? And add a comment explaining
> why, pointing at the comments in browser_window_gtk.cc?

Hah, this is evan's bug!  The fix should be somewhere in browser_window_gtk.cc
in SetGeometryHints.  I think the bounds_overridden is the wrong signal to use. 
See http://codereview.chromium.org/159316 where evan was trying to fix a
different bug.

[Evan Martin, 2009-09-11 18:51:21]

On 2009/09/11 18:48:21, tony wrote:
> On 2009/09/11 18:40:24, Mike Mammarella wrote:
> > Looks like ultimately it's being caused by the call to
> > Browser::set_override_bounds() in SessionRestoreImpl::OnGotSession(). You
have
> > to set the "Restore the pages that were open last" preference for it to do
> that.
> > 
> > What to do, wrap that in #if !defined(OS_LINUX)? And add a comment
explaining
> > why, pointing at the comments in browser_window_gtk.cc?
> 
> Hah, this is evan's bug!  The fix should be somewhere in browser_window_gtk.cc
> in SetGeometryHints.  I think the bounds_overridden is the wrong signal to
use. 
> See http://codereview.chromium.org/159316 where evan was trying to fix a
> different bug.

Do we also have a bug where dropping windows offsets them?  Maybe the "restore
bounds" code is wrong in general.  From my memory of looking at this bug I was
pretty sure that bounds_overridden was the right place to put this trick.

[tony, 2009-09-11 18:58:07]

On 2009/09/11 18:51:21, Evan Martin wrote:
> Do we also have a bug where dropping windows offsets them?  Maybe the "restore
> bounds" code is wrong in general.  From my memory of looking at this bug I was
> pretty sure that bounds_overridden was the right place to put this trick.

I guess the problem is you can't distinguish between session restore setting the
override bounds and ContinueDraggingDetachedTab setting the override bounds. 
Maybe the right fix is to just #if around set_override_bounds in
session_restore.cc, or maybe there's another signal we can use.

[Evan Martin, 2009-09-11 19:04:01]

On 2009/09/11 18:58:07, tony wrote:
> On 2009/09/11 18:51:21, Evan Martin wrote:
> > Do we also have a bug where dropping windows offsets them?  Maybe the
"restore
> > bounds" code is wrong in general.  From my memory of looking at this bug I
was
> > pretty sure that bounds_overridden was the right place to put this trick.
> 
> I guess the problem is you can't distinguish between session restore setting
the
> override bounds and ContinueDraggingDetachedTab setting the override bounds. 
> Maybe the right fix is to just #if around set_override_bounds in
> session_restore.cc, or maybe there's another signal we can use.

Maybe I'm misunderstanding, but what I'm saying is that both session restore and
dropping tabs should have the same logic for positioning a tab.  Oh, but you're
saying ContinueDragging needs it to be different? Hrmmmm.

[tony, 2009-09-11 19:16:12]

Oh, what I'm saying is that session restore probably shouldn't be positioning
windows since we don't normally position windows (except during tab drag out). 
What is the behavior that we want?

[tony, 2009-09-11 23:44:15]

http://codereview.chromium.org/203027/diff/1004/4
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/203027/diff/1004/4#newcode292
Line 292: browser->set_override_bounds((*i)->bounds);
Can we just not call set_override_bounds on Linux?

[Mike Mammarella, 2009-09-11 23:46:54]

That's what I did in patch set 2, but then if you have more than one browser
window, all but one of them will not get their sizes restored. We definitely
want to restore the sizes, just not the positions.

On 2009/09/11 23:44:15, tony wrote:
> http://codereview.chromium.org/203027/diff/1004/4
> File chrome/browser/sessions/session_restore.cc (right):
> 
> http://codereview.chromium.org/203027/diff/1004/4#newcode292
> Line 292: browser->set_override_bounds((*i)->bounds);
> Can we just not call set_override_bounds on Linux?

[tony, 2009-09-12 00:46:43]

On 2009/09/11 23:46:54, Mike Mammarella wrote:
> That's what I did in patch set 2, but then if you have more than one browser
> window, all but one of them will not get their sizes restored. We definitely
> want to restore the sizes, just not the positions.

It looks like we still get the size restored when I comment that line out in
session_restore.cc.  I think, that's the code path that gets run when "Open the
home page" is checked and that seems to remember the window size.

[Mike Mammarella, 2009-09-12 01:01:27]

For a single window, or multiple windows? I just tried it again to be sure, and
it's definitely the case that only one window gets its size restored. The others
start at the default size (full height, almost full width), and sometimes even
the first window will inherit a strange size.

(To correctly test this you have to use the wrench->exit option to quit, so that
all the windows will be saved and restored.)

[tony, 2009-09-12 01:09:12]

On 2009/09/12 01:01:27, Mike Mammarella wrote:
> For a single window, or multiple windows? I just tried it again to be sure,
and
> it's definitely the case that only one window gets its size restored. The
others
> start at the default size (full height, almost full width), and sometimes even
> the first window will inherit a strange size.
> 
> (To correctly test this you have to use the wrench->exit option to quit, so
that
> all the windows will be saved and restored.)

Ah yes, you are right.  LGTM, although I feel like this is the wrong way to hack
around it.  It seems like the special case is pulling tabs out and maybe there
should be a bool for that instead of special casing session restore?  I'm not
sure, since I don't know what the other uses of override_bounds_ is.

[Mike Mammarella, 2009-09-12 01:27:11]

On 2009/09/12 01:09:12, tony wrote:

> Ah yes, you are right.  LGTM, although I feel like this is the wrong way to
hack
> around it.  It seems like the special case is pulling tabs out and maybe there
> should be a bool for that instead of special casing session restore?  I'm not
> sure, since I don't know what the other uses of override_bounds_ is.

It seemed to me that this was the safest way, avoiding changing the behavior for
any other possible uses of override_bounds_ and on other platforms.

[Evan Stade, 2009-09-12 01:43:54]

so, the uses of override_bounds_ that I know of:

- session restore
- dragging/dropping tabs
- javascript popup windows that specify their position/size

[tony, 2009-09-14 17:09:43]

On 2009/09/12 01:43:54, Evan Stade wrote:
> so, the uses of override_bounds_ that I know of:
> 
> - session restore
> - dragging/dropping tabs
> - javascript popup windows that specify their position/size

Ok, then let's go with this change.  I think we want the latter two to still
work and this just special cases the first.  LGTM!

[Mike Mammarella, 2009-09-14 17:41:57]

So over the weekend I was talking with a friend of mine who pointed out several
applications that do, in fact, restore their positions: GIMP, audacity, pidgin,
dia, and gkrellm. There are probably others.

So it's not really as clear-cut that we shouldn't restore window position as we
thought; these applications all seem to suggest that applications that maintain
state across restarts should also restore window position. (In contrast to
things like xterm, gedit, etc. that restore no state.)

However, the documentation for gtk_window_get_position() says that the correct
way to do this is to use the session management protocol (XSMP); I will look
into that. (My understanding is that then we'd still not restore the position
ourselves - it would be done for us.)

On 2009/09/14 17:09:43, tony wrote:
> On 2009/09/12 01:43:54, Evan Stade wrote:
> > so, the uses of override_bounds_ that I know of:
> > 
> > - session restore
> > - dragging/dropping tabs
> > - javascript popup windows that specify their position/size
> 
> Ok, then let's go with this change.  I think we want the latter two to still
> work and this just special cases the first.  LGTM!

[Evan Martin, 2009-09-14 19:23:14]

On 2009/09/14 17:41:57, Mike Mammarella wrote:
> So over the weekend I was talking with a friend of mine who pointed out
several
> applications that do, in fact, restore their positions: GIMP, audacity,
pidgin,
> dia, and gkrellm. There are probably others.
> 
> So it's not really as clear-cut that we shouldn't restore window position as
we
> thought; these applications all seem to suggest that applications that
maintain
> state across restarts should also restore window position. (In contrast to
> things like xterm, gedit, etc. that restore no state.)
> 
> However, the documentation for gtk_window_get_position() says that the correct
> way to do this is to use the session management protocol (XSMP); I will look
> into that. (My understanding is that then we'd still not restore the position
> ourselves - it would be done for us.)

I have the vague recollection that XSMP is a disaster that nobody uses.  But
good luck!

[Mike Mammarella, 2009-09-18 01:41:37]

Decided against XSMP. Here's one more try at a good solution.

[tony, 2009-09-18 18:45:12]

I like this change better.  It's trying to make sure we save the right bounds
rather than hacking it on restore.

Overall, LGTM.

http://codereview.chromium.org/203027/diff/10001/11002
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/203027/diff/10001/11002#newcode128
Line 128: // When a window is moved or resized, GTK will call
MainWindowConfigured below.
Nit: I expect this comment to either be in the header or near the where we start
the timer.

http://codereview.chromium.org/203027/diff/10001/11002#newcode1387
Line 1387: bounds_.set_origin(origin);
Should we update restored_bounds_ too?

[Mike Mammarella, 2009-09-18 21:18:39]

http://codereview.chromium.org/203027/diff/10001/11002
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/203027/diff/10001/11002#newcode128
Line 128: // When a window is moved or resized, GTK will call
MainWindowConfigured below.
On 2009/09/18 18:45:12, tony wrote:
> Nit: I expect this comment to either be in the header or near the where we
start
> the timer.

Done.

http://codereview.chromium.org/203027/diff/10001/11002#newcode1387
Line 1387: bounds_.set_origin(origin);
On 2009/09/18 18:45:12, tony wrote:
> Should we update restored_bounds_ too?
> 

Done.