Reposition and/or resize window if it becomes off-screen....

Reposition and/or resize window if it becomes off-screen
due to monitor configuration change since the last run.

BUG=17822
TEST=Use multiple monitors. Start Chrome, move the window to a non-primary
monitor, and exit Chrome. Disconnect or disable the monitor and start Chrome
again. Chrome should appear at the origin of a remaining monitor. Also try
chaning the resolution of the non-primary monitor. As far as the window fits
within the monitor, it should be shown as-is. If not, it is moved to the origin
and resized, if necessary.


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

[Yuzo, 2009-07-28 02:37:39]

Hi, Peter,

Can you review this?
(I'll be out of office on July 30 & 31 in Japan time.
In case I cannot commit this by then, please consider
having someone copy and submit this.)

Yuzo

[Peter Kasting, 2009-07-28 02:40:35]

This isn't sufficient to handle the problems Brian was showing me.  If he moved
a window mostly onto a monitor that was disappearing, it would still be just as
hard to reposition when Chrome restarted, but this heuristic wouldn't catch it.

I think instead of trying to guess whether a monitor is gone, we should just
save that info.  It's not hard.

[Yuzo, 2009-07-28 09:09:35]

Thank you for the review.

Can you take another look?
I've changed the code such that work area is recorded and checked.

Yuzo

On 2009/07/28 02:40:35, pkasting wrote:
> This isn't sufficient to handle the problems Brian was showing me.  If he
moved
> a window mostly onto a monitor that was disappearing, it would still be just
as
> hard to reposition when Chrome restarted, but this heuristic wouldn't catch
it.
> 
> I think instead of trying to guess whether a monitor is gone, we should just
> save that info.  It's not hard.

[Peter Kasting, 2009-07-28 18:19:34]

You may wonder why below I say to save all monitor rects.  Consider the example
of Brian's demo to me, where his primary monitor was his laptop, whose work area
never changed; he was only connecting and disconnecting his secondary monitor. 
In a case like this (which I claim is the typical case), your existing patch
won't detect any change.

http://codereview.chromium.org/160246/diff/42/47
File chrome/browser/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/160246/diff/42/47#newcode1318
Line 1318: WindowSizer::CreateDefaultMonitorInfoProvider());
This isn't complete.  You can't just save the work area for the default monitor,
you have to save it for all monitors.

http://codereview.chromium.org/160246/diff/42/45
File chrome/browser/views/chrome_views_delegate.cc (right):

http://codereview.chromium.org/160246/diff/42/45#newcode38
Line 38: WindowSizer::CreateDefaultMonitorInfoProvider());
This isn't complete.  You can't just save the work area for the default monitor,
you have to save it for all monitors.

http://codereview.chromium.org/160246/diff/42/44
File chrome/browser/window_sizer.cc (right):

http://codereview.chromium.org/160246/diff/42/44#newcode25
Line 25: virtual bool GetPersistentState(gfx::Rect* bounds, bool* maximized,
Nit: Please place each arg on its own line.

http://codereview.chromium.org/160246/diff/42/44#newcode55
Line 55: if (wp_pref) {
This isn't complete.  You need to read the work areas for all monitors.

http://codereview.chromium.org/160246/diff/42/44#newcode254
Line 254: const gfx::Rect& other_bounds, const gfx::Rect& saved_work_area,
Nit: Please place each arg on its own line.

http://codereview.chromium.org/160246/diff/42/44#newcode264
Line 264: // If the saved_work_area is different from the current work area and
Instead of this algorithm, you need something like:

for each saved work area {
  if (work area not found in current monitors) && (window rect intersects work
area) {
    adjust window to be completely visible on current monitor;
    return;
  }
}

You probably also want a less aggressive "adjust" algorithm than "set origin to
the work area origin".  For example:

bounds->set_width(std::min(bounds->width(), work_area.width()));
bounds->set_height(std::min(bounds->height(), work_area.height()));
if (bounds->left < work_area.left)
  bounds->left = work_area.left;
else
  bounds->left = std::min(bounds->left, work_area.right - bounds->width());
if (bounds->top < work_area.top)
  bounds->top = work_area.top;
else
  bounds->top = std::min(bounds->top, work_area.bottom - bounds->height());

http://codereview.chromium.org/160246/diff/42/46
File chrome/browser/window_sizer.h (right):

http://codereview.chromium.org/160246/diff/42/46#newcode90
Line 90: virtual bool GetPersistentState(gfx::Rect* bounds, bool* maximized,
Nit: Please place each arg on its own line.

http://codereview.chromium.org/160246/diff/42/46#newcode91
Line 91: gfx::Rect* work_area) const = 0;
You need to return a vector of work areas, not just one.

http://codereview.chromium.org/160246/diff/42/46#newcode165
Line 165: // If |saved_work_area| is non-empty, it is used to determine whethter
the
Nit: whethter -> whether

http://codereview.chromium.org/160246/diff/42/46#newcode170
Line 170: const gfx::Rect& other_bounds, const gfx::Rect& saved_work_area,
Nit: Please place each arg on its own line.

[Yuzo, 2009-07-29 01:56:02]

Peter,

Thank you for the review.

As for saving monitor geometry, I think saving the geometry of the monitor
closest to the bounds should be enough. We don't care about other monitor(s), do
we?

If the monitor closest to the bounds are unchanged or big enough to contain
the whole bounds, we don't need any relocation/resizing.

I'll address other comments.

Yuzo

On 2009/07/28 18:19:34, Peter Kasting wrote:
> You may wonder why below I say to save all monitor rects.  Consider the
example
> of Brian's demo to me, where his primary monitor was his laptop, whose work
area
> never changed; he was only connecting and disconnecting his secondary monitor.

> In a case like this (which I claim is the typical case), your existing patch
> won't detect any change.
> 
> http://codereview.chromium.org/160246/diff/42/47
> File chrome/browser/gtk/browser_window_gtk.cc (right):
> 
> http://codereview.chromium.org/160246/diff/42/47#newcode1318
> Line 1318: WindowSizer::CreateDefaultMonitorInfoProvider());
> This isn't complete.  You can't just save the work area for the default
monitor,
> you have to save it for all monitors.
> 
> http://codereview.chromium.org/160246/diff/42/45
> File chrome/browser/views/chrome_views_delegate.cc (right):
> 
> http://codereview.chromium.org/160246/diff/42/45#newcode38
> Line 38: WindowSizer::CreateDefaultMonitorInfoProvider());
> This isn't complete.  You can't just save the work area for the default
monitor,
> you have to save it for all monitors.
> 
> http://codereview.chromium.org/160246/diff/42/44
> File chrome/browser/window_sizer.cc (right):
> 
> http://codereview.chromium.org/160246/diff/42/44#newcode25
> Line 25: virtual bool GetPersistentState(gfx::Rect* bounds, bool* maximized,
> Nit: Please place each arg on its own line.
> 
> http://codereview.chromium.org/160246/diff/42/44#newcode55
> Line 55: if (wp_pref) {
> This isn't complete.  You need to read the work areas for all monitors.
> 
> http://codereview.chromium.org/160246/diff/42/44#newcode254
> Line 254: const gfx::Rect& other_bounds, const gfx::Rect& saved_work_area,
> Nit: Please place each arg on its own line.
> 
> http://codereview.chromium.org/160246/diff/42/44#newcode264
> Line 264: // If the saved_work_area is different from the current work area
and
> Instead of this algorithm, you need something like:
> 
> for each saved work area {
>   if (work area not found in current monitors) && (window rect intersects work
> area) {
>     adjust window to be completely visible on current monitor;
>     return;
>   }
> }
> 
> You probably also want a less aggressive "adjust" algorithm than "set origin
to
> the work area origin".  For example:
> 
> bounds->set_width(std::min(bounds->width(), work_area.width()));
> bounds->set_height(std::min(bounds->height(), work_area.height()));
> if (bounds->left < work_area.left)
>   bounds->left = work_area.left;
> else
>   bounds->left = std::min(bounds->left, work_area.right - bounds->width());
> if (bounds->top < work_area.top)
>   bounds->top = work_area.top;
> else
>   bounds->top = std::min(bounds->top, work_area.bottom - bounds->height());
> 
> http://codereview.chromium.org/160246/diff/42/46
> File chrome/browser/window_sizer.h (right):
> 
> http://codereview.chromium.org/160246/diff/42/46#newcode90
> Line 90: virtual bool GetPersistentState(gfx::Rect* bounds, bool* maximized,
> Nit: Please place each arg on its own line.
> 
> http://codereview.chromium.org/160246/diff/42/46#newcode91
> Line 91: gfx::Rect* work_area) const = 0;
> You need to return a vector of work areas, not just one.
> 
> http://codereview.chromium.org/160246/diff/42/46#newcode165
> Line 165: // If |saved_work_area| is non-empty, it is used to determine
whethter
> the
> Nit: whethter -> whether
> 
> http://codereview.chromium.org/160246/diff/42/46#newcode170
> Line 170: const gfx::Rect& other_bounds, const gfx::Rect& saved_work_area,
> Nit: Please place each arg on its own line.

[Yuzo, 2009-07-29 02:18:17]

I forgot to write that this fix is working as expected in my dual monitor
environment.

Yuzo

On 2009/07/29 01:56:02, Yuzo wrote:
> Peter,
> 
> Thank you for the review.
> 
> As for saving monitor geometry, I think saving the geometry of the monitor
> closest to the bounds should be enough. We don't care about other monitor(s),
do
> we?
> 
> If the monitor closest to the bounds are unchanged or big enough to contain
> the whole bounds, we don't need any relocation/resizing.
> 
> I'll address other comments.
> 
> Yuzo
> 
> On 2009/07/28 18:19:34, Peter Kasting wrote:
> > You may wonder why below I say to save all monitor rects.  Consider the
> example
> > of Brian's demo to me, where his primary monitor was his laptop, whose work
> area
> > never changed; he was only connecting and disconnecting his secondary
monitor.
> 
> > In a case like this (which I claim is the typical case), your existing patch
> > won't detect any change.
> > 
> > http://codereview.chromium.org/160246/diff/42/47
> > File chrome/browser/gtk/browser_window_gtk.cc (right):
> > 
> > http://codereview.chromium.org/160246/diff/42/47#newcode1318
> > Line 1318: WindowSizer::CreateDefaultMonitorInfoProvider());
> > This isn't complete.  You can't just save the work area for the default
> monitor,
> > you have to save it for all monitors.
> > 
> > http://codereview.chromium.org/160246/diff/42/45
> > File chrome/browser/views/chrome_views_delegate.cc (right):
> > 
> > http://codereview.chromium.org/160246/diff/42/45#newcode38
> > Line 38: WindowSizer::CreateDefaultMonitorInfoProvider());
> > This isn't complete.  You can't just save the work area for the default
> monitor,
> > you have to save it for all monitors.
> > 
> > http://codereview.chromium.org/160246/diff/42/44
> > File chrome/browser/window_sizer.cc (right):
> > 
> > http://codereview.chromium.org/160246/diff/42/44#newcode25
> > Line 25: virtual bool GetPersistentState(gfx::Rect* bounds, bool* maximized,
> > Nit: Please place each arg on its own line.
> > 
> > http://codereview.chromium.org/160246/diff/42/44#newcode55
> > Line 55: if (wp_pref) {
> > This isn't complete.  You need to read the work areas for all monitors.
> > 
> > http://codereview.chromium.org/160246/diff/42/44#newcode254
> > Line 254: const gfx::Rect& other_bounds, const gfx::Rect& saved_work_area,
> > Nit: Please place each arg on its own line.
> > 
> > http://codereview.chromium.org/160246/diff/42/44#newcode264
> > Line 264: // If the saved_work_area is different from the current work area
> and
> > Instead of this algorithm, you need something like:
> > 
> > for each saved work area {
> >   if (work area not found in current monitors) && (window rect intersects
work
> > area) {
> >     adjust window to be completely visible on current monitor;
> >     return;
> >   }
> > }
> > 
> > You probably also want a less aggressive "adjust" algorithm than "set origin
> to
> > the work area origin".  For example:
> > 
> > bounds->set_width(std::min(bounds->width(), work_area.width()));
> > bounds->set_height(std::min(bounds->height(), work_area.height()));
> > if (bounds->left < work_area.left)
> >   bounds->left = work_area.left;
> > else
> >   bounds->left = std::min(bounds->left, work_area.right - bounds->width());
> > if (bounds->top < work_area.top)
> >   bounds->top = work_area.top;
> > else
> >   bounds->top = std::min(bounds->top, work_area.bottom - bounds->height());
> > 
> > http://codereview.chromium.org/160246/diff/42/46
> > File chrome/browser/window_sizer.h (right):
> > 
> > http://codereview.chromium.org/160246/diff/42/46#newcode90
> > Line 90: virtual bool GetPersistentState(gfx::Rect* bounds, bool* maximized,
> > Nit: Please place each arg on its own line.
> > 
> > http://codereview.chromium.org/160246/diff/42/46#newcode91
> > Line 91: gfx::Rect* work_area) const = 0;
> > You need to return a vector of work areas, not just one.
> > 
> > http://codereview.chromium.org/160246/diff/42/46#newcode165
> > Line 165: // If |saved_work_area| is non-empty, it is used to determine
> whethter
> > the
> > Nit: whethter -> whether
> > 
> > http://codereview.chromium.org/160246/diff/42/46#newcode170
> > Line 170: const gfx::Rect& other_bounds, const gfx::Rect& saved_work_area,
> > Nit: Please place each arg on its own line.

[Peter Kasting, 2009-07-29 03:48:55]

On Tue, Jul 28, 2009 at 6:56 PM, <yuzo@chromium.org> wrote:

> As for saving monitor geometry, I think saving the geometry of the
> monitor
> closest to the bounds should be enough. We don't care about other
> monitor(s), do we?


I misunderstood how the patch worked -- I thought it was saving one work
area total for all the windows, but it's saving one for each window.

It seems like in the majority of cases (where the user has one monitor) this
will save a lot of duplicate info.

In cases where a window is split across multiple monitors, I'm not sure
whether or not it's OK.  It seems like we could leave a window dangling
half-offscreen.  Maybe that's OK though.

I revise my comments to be that this approach probably works well enough to
fix the bug Brian was experiencing, but that it still seems like enumerating
all the monitors, then enumerating all the windows (my original idea) might
have fewer edge cases.

I guess it's up to you whether you think the benefit of my proposal is large
enough to be worth doing.  Ben's or Brian's comments also welcome.

PK

http://codereview.chromium.org/160246
>

[Yuzo, 2009-07-29 05:32:47]

Hi, Peter,

Thank you for your comments.

I noticed that we should guarantee that the title bar is
not above the work area. I changed the code as such.

As for whether to save configuration for one monitor
or for all, I'd propose to save for one monitor
because enough cases are covered (in my opinion).

As for a-window-spanning-multiple-monitors case:
Suppose 51% of a window is in Monitor A and 49%, B,
the window position is not adjusted as far as Monitor A
is there unchanged. If A is gone, the window is repositioned
to fit in Monitor B.

The behavior becomes questionable as the number of monitors
increases. For example, suppose we have 3 monitors and the
distribution is 34%:A, 33%:B, 33%:C. The window will not be
adjusted as far as Monitor A is there.
But I'd say these are rather rare cases.

I'll be out of office on Thu and Fri.
Please consider patching and submitting this change
if the bug needs to be fixed sooner.

Yuzo

http://codereview.chromium.org/160246/diff/42/44
File chrome/browser/window_sizer.cc (right):

http://codereview.chromium.org/160246/diff/42/44#newcode25
Line 25: virtual bool GetPersistentState(gfx::Rect* bounds, bool* maximized,
On 2009/07/28 18:19:34, Peter Kasting wrote:
> Nit: Please place each arg on its own line.

Done.

http://codereview.chromium.org/160246/diff/42/44#newcode254
Line 254: const gfx::Rect& other_bounds, const gfx::Rect& saved_work_area,
On 2009/07/28 18:19:34, Peter Kasting wrote:
> Nit: Please place each arg on its own line.

Done.

http://codereview.chromium.org/160246/diff/42/44#newcode264
Line 264: // If the saved_work_area is different from the current work area and
On 2009/07/28 18:19:34, Peter Kasting wrote:
> Instead of this algorithm, you need something like:
> 
> for each saved work area {
>   if (work area not found in current monitors) && (window rect intersects work
> area) {
>     adjust window to be completely visible on current monitor;
>     return;
>   }
> }
> 
> You probably also want a less aggressive "adjust" algorithm than "set origin
to
> the work area origin".  For example:
> 
> bounds->set_width(std::min(bounds->width(), work_area.width()));
> bounds->set_height(std::min(bounds->height(), work_area.height()));
> if (bounds->left < work_area.left)
>   bounds->left = work_area.left;
> else
>   bounds->left = std::min(bounds->left, work_area.right - bounds->width());
> if (bounds->top < work_area.top)
>   bounds->top = work_area.top;
> else
>   bounds->top = std::min(bounds->top, work_area.bottom - bounds->height());

Done.

http://codereview.chromium.org/160246/diff/42/46
File chrome/browser/window_sizer.h (right):

http://codereview.chromium.org/160246/diff/42/46#newcode90
Line 90: virtual bool GetPersistentState(gfx::Rect* bounds, bool* maximized,
On 2009/07/28 18:19:34, Peter Kasting wrote:
> Nit: Please place each arg on its own line.

Done.

http://codereview.chromium.org/160246/diff/42/46#newcode165
Line 165: // If |saved_work_area| is non-empty, it is used to determine whethter
the
On 2009/07/28 18:19:34, Peter Kasting wrote:
> Nit: whethter -> whether

Done.

http://codereview.chromium.org/160246/diff/42/46#newcode170
Line 170: const gfx::Rect& other_bounds, const gfx::Rect& saved_work_area,
On 2009/07/28 18:19:34, Peter Kasting wrote:
> Nit: Please place each arg on its own line.

Done.

[Peter Kasting, 2009-07-29 05:52:52]

LGTM

http://codereview.chromium.org/160246/diff/55/57
File chrome/browser/window_sizer.cc (right):

http://codereview.chromium.org/160246/diff/55/57#newcode262
Line 262: // Find the size of the work area of the monitor that intersects the
bounds
Nit: This is subtle, but I think this block and all your added code should move
below the "Ensure the minimum height and width" block.  Enlarging the size of
tiny windows should probably happen before we shift their origins to be more
visible, rather than after.

http://codereview.chromium.org/160246/diff/55/57#newcode268
Line 268: if (bounds->y() < work_area.y()) {
Nit: no {}.

[Yuzo, 2009-07-29 06:25:25]

Thank you very much for reviewing this.

I'll submit it after confirming try bots are still happy with it.

Yuzo

http://codereview.chromium.org/160246/diff/55/57
File chrome/browser/window_sizer.cc (right):

http://codereview.chromium.org/160246/diff/55/57#newcode262
Line 262: // Find the size of the work area of the monitor that intersects the
bounds
On 2009/07/29 05:52:52, Peter Kasting wrote:
> Nit: This is subtle, but I think this block and all your added code should
move
> below the "Ensure the minimum height and width" block.  Enlarging the size of
> tiny windows should probably happen before we shift their origins to be more
> visible, rather than after.

Good point. Done.

http://codereview.chromium.org/160246/diff/55/57#newcode268
Line 268: if (bounds->y() < work_area.y()) {
On 2009/07/29 05:52:52, Peter Kasting wrote:
> Nit: no {}.

Oops. Done.