Change the window bounds adjusting algorithm to restore...

Change the window bounds adjusting algorithm to restore
the window size as far as possible. Currently, the
window size can shrunk by 2*kWindowTilePixels=20
even when the shrinkage can be avoided by adjusting
the location.

BUG=9587

[yuzo, 2009-05-11 11:32:55]

Hi, Peter,

Can you review this?

Yuzo

[Peter Kasting, 2009-05-11 13:26:52]

The browser.cc change is fine.

I don't understand why the window_sizer change should fix the original bug.  I
thought the window was supposed to be created fully-onscreen, yet the code
you're adding ought to only affect offscreen windows.

I do think the usage of kWindowTilePixels in this function might be wrong, but I
bet just removing it from where it's used is better than adding more logic.  And
I'm not sure whether that's really necessary anyway.

[yuzo, 2009-05-12 02:33:23]

Hi, Peter,

Thank you for the review.

On 2009/05/11 13:26:52, pkasting wrote:
> The browser.cc change is fine.

I've moved browser.cc change to http://codereview.chromium.org/113245
to simplify this change a little bit.

> 
> I don't understand why the window_sizer change should fix the original bug.  I
> thought the window was supposed to be created fully-onscreen, yet the code
> you're adding ought to only affect offscreen windows.
> 
> I do think the usage of kWindowTilePixels in this function might be wrong, but
I
> bet just removing it from where it's used is better than adding more logic. 
And
> I'm not sure whether that's really necessary anyway.

OK, let me explain with an example.

Assume the user:
0. has a 1024x768 screen,
1. placed a Chrome window at (100, 0)-(399, 767)(maximum height, non-maximum
width),
2. closed Chrome, and
3. restarted Chrome.

The current (i.e. before this change) Chrome tries to place
10 (=kwindowTilePixels) pixel gaps between window border and
the screen border. So Chrome first shifts the window to
(100, *10*)-(399, *777*) to insert a gap at the top.
Notice that the window is now offscreen.
Then Chrome shrinks the height such that there is a gap also 
at the bottom: (100, 10)-(399, *758*) 

My change makes Chrome give up the gaps if keeping them results
in shrinking window. Hence the new location is the same as the
original one.

I agree that the necessity of the gaps is questionable.
My intention was just to maintain the current behavior
as far as feasible.

I have no objection against always removing the gaps
regardless of the window size. Do you prefer this?

[yuzo, 2009-05-12 06:11:04]

On 2009/05/12 02:33:23, yuzo wrote:
> Hi, Peter,
> 
> Thank you for the review.
> 
> On 2009/05/11 13:26:52, pkasting wrote:
> > The browser.cc change is fine.
> 
> I've moved browser.cc change to http://codereview.chromium.org/113245
> to simplify this change a little bit.
> 
> > 
> > I don't understand why the window_sizer change should fix the original bug. 
I
> > thought the window was supposed to be created fully-onscreen, yet the code
> > you're adding ought to only affect offscreen windows.
> > 
> > I do think the usage of kWindowTilePixels in this function might be wrong,
but
> I
> > bet just removing it from where it's used is better than adding more logic. 
> And
> > I'm not sure whether that's really necessary anyway.
> 
> OK, let me explain with an example.
> 
> Assume the user:
> 0. has a 1024x768 screen,
> 1. placed a Chrome window at (100, 0)-(399, 767)(maximum height, non-maximum
> width),
> 2. closed Chrome, and
> 3. restarted Chrome.
> 
> The current (i.e. before this change) Chrome tries to place
> 10 (=kwindowTilePixels) pixel gaps between window border and
> the screen border. So Chrome first shifts the window to
> (100, *10*)-(399, *777*) to insert a gap at the top.
> Notice that the window is now offscreen.
> Then Chrome shrinks the height such that there is a gap also 
> at the bottom: (100, 10)-(399, *758*) 
> 
> My change makes Chrome give up the gaps if keeping them results
> in shrinking window. Hence the new location is the same as the
> original one.
> 
> I agree that the necessity of the gaps is questionable.
> My intention was just to maintain the current behavior
> as far as feasible.
> 
> I have no objection against always removing the gaps
> regardless of the window size. Do you prefer this?

gcl try succeeds for all the three platforms.

[Peter Kasting, 2009-05-12 18:41:49]

On 2009/05/12 06:11:04, yuzo wrote:
> > The current (i.e. before this change) Chrome tries to place
> > 10 (=kwindowTilePixels) pixel gaps between window border and
> > the screen border. So Chrome first shifts the window to
> > (100, *10*)-(399, *777*) to insert a gap at the top.

I understand that you're claiming that's what happens.  But I don't understand
why it would happen.  We don't shift the window position at all unless an edge
is *already* offscreen.

[yuzo, 2009-05-14 08:32:05]

Yes, you are right. My explanation and understanding was wrong.

Actually, the current code works fine if the window is not offscreen.

The issue occurs when it becomes offscreen. When a user manually
"maximize" a window, s/he easily makes it too large and offscreen.

This explains why Chrome works fine when the window is maximized
by the maximize icon but not if the user manually maximizes it,
as described in the bug description.

This change is needed to handle offscreen cases properly.

Yuzo

On 2009/05/12 18:41:49, pkasting wrote:
> On 2009/05/12 06:11:04, yuzo wrote:
> > > The current (i.e. before this change) Chrome tries to place
> > > 10 (=kwindowTilePixels) pixel gaps between window border and
> > > the screen border. So Chrome first shifts the window to
> > > (100, *10*)-(399, *777*) to insert a gap at the top.
> 
> I understand that you're claiming that's what happens.  But I don't understand
> why it would happen.  We don't shift the window position at all unless an edge
> is *already* offscreen.

[Peter Kasting, 2009-05-14 17:08:25]

On 2009/05/14 08:32:05, yuzo wrote:
> The issue occurs when it becomes offscreen. When a user manually
> "maximize" a window, s/he easily makes it too large and offscreen.

So then isn't this working as intended?  The window is offscreen, so we resize
it.

Perhaps you should check with Ben, who originally wrote this code, on what the
desired behavior and use cases are.  It seems like either doing nothing,
removing the 10 pixel offset entirely, or saving the screen resolution with the
saved window info and then only resizing if the current resolution differs would
all be better fixes than the one in this patch.

[yuzo, 2009-05-15 01:24:11]

Ben,

Can you tell me your opinion on the following?

Yuzo

On 2009/05/14 17:08:25, pkasting wrote:
> On 2009/05/14 08:32:05, yuzo wrote:
> > The issue occurs when it becomes offscreen. When a user manually
> > "maximize" a window, s/he easily makes it too large and offscreen.
> 
> So then isn't this working as intended?  The window is offscreen, so we resize
> it.
> 
> Perhaps you should check with Ben, who originally wrote this code, on what the
> desired behavior and use cases are.  It seems like either doing nothing,
> removing the 10 pixel offset entirely, or saving the screen resolution with
the
> saved window info and then only resizing if the current resolution differs
would
> all be better fixes than the one in this patch.

[yuzo, 2009-05-19 06:39:37]

Ping, Ben?

[yuzo, 2009-05-25 06:34:06]

Peter, Ben,

I've simplified the adjusting algorithm as follows:
- Window is moved if necessary to make the top left
  kMinVisibleWidth * kMinVisibleHeight of the window
  visible.
- Window is enlarged if necessary to make the window at
  least as large as kMinVisibleWidth * kMinVisibleHeight.
- Otherwise, the widow is not moved or resized.

Do you like this better?

Yuzo

[Peter Kasting, 2009-05-25 06:39:26]

I'm about to go to bed, but I would say that having the top left be visible, as
opposed to any arbitrary section, seems unnecessary to me.  Also, the minimum
sizes you have seem too large by an order of magnitude -- I would think 30x30 is
fine (this is the minimum size we constrain popups to at the moment).

So, in short, I would think ensuring at least 30 px of the window is visible in
each dimension is good enough.

[yuzo, 2009-05-25 07:48:59]

Thank you for your comment.

I've modified the code such that:
- The code doesn't care about showing the top left corner.
- The minimum visible size is 30 x 30 pixels.

Yuzo

[Peter Kasting, 2009-05-27 19:56:46]

Code looks OK; testcases need some work.  Ben should comment on whether he's OK
with this algorithm.

http://codereview.chromium.org/115180/diff/16/18
File chrome/browser/window_sizer_unittest.cc (right):

http://codereview.chromium.org/115180/diff/16/18#newcode275
Line 275: { // offset would put the new window offscreen at the bottom
You should add testcases where the new window would be so much offscreen that
its position needs adjustment.  Perhaps remove one or two of the third, fourth,
and fifth testcases here and replace them.

http://codereview.chromium.org/115180/diff/16/18#newcode352
Line 352: EXPECT_EQ(gfx::Rect(-1024, 0, 1024, 768), window_bounds);
Nit: Change this back to initial_bounds

http://codereview.chromium.org/115180/diff/16/18#newcode355
Line 355: { // a little off the left
Much like above, these testcases are now all basically no-ops.  Most should be
removed and replaced with cases that actually do something.  (There are a few at
function bottom, maybe we just need to remove nearly all of these, and perhaps
add a few more.)

[yuzo, 2009-05-28 07:09:35]

Thank you for the review.

Can you take yet another look?

Yuzo

http://codereview.chromium.org/115180/diff/16/18
File chrome/browser/window_sizer_unittest.cc (right):

http://codereview.chromium.org/115180/diff/16/18#newcode275
Line 275: { // offset would put the new window offscreen at the bottom
On 2009/05/27 19:56:46, pkasting wrote:
> You should add testcases where the new window would be so much offscreen that
> its position needs adjustment.  Perhaps remove one or two of the third,
fourth,
> and fifth testcases here and replace them.

Removed unnecessary tests and added ones for:
- offscreen but the minimum visibility condition is
  satisfied without relocation
- offscreen and the minimum visibility condition is
  satisfied by relocation
- onscreen but the minimum visibility condition is
  satisfied by enlargement

http://codereview.chromium.org/115180/diff/16/18#newcode352
Line 352: EXPECT_EQ(gfx::Rect(-1024, 0, 1024, 768), window_bounds);
On 2009/05/27 19:56:46, pkasting wrote:
> Nit: Change this back to initial_bounds

Done.

http://codereview.chromium.org/115180/diff/16/18#newcode355
Line 355: { // a little off the left
On 2009/05/27 19:56:46, pkasting wrote:
> Much like above, these testcases are now all basically no-ops.  Most should be
> removed and replaced with cases that actually do something.  (There are a few
at
> function bottom, maybe we just need to remove nearly all of these, and perhaps
> add a few more.)

Did the same as above.

[Peter Kasting, 2009-05-28 18:10:34]

LGTM with nits

http://codereview.chromium.org/115180/diff/1025/1027
File chrome/browser/window_sizer_unittest.cc (right):

http://codereview.chromium.org/115180/diff/1025/1027#newcode398
Line 398: gfx::Rect(-470, 50, 500, 400), false, PERSISTED,
Nit: For all the tests where the final bounds should match the initial bounds,
use an "initial_bounds" temp like the cases above.

http://codereview.chromium.org/115180/diff/1025/1027#newcode412
Line 412: EXPECT_EQ(gfx::Rect(-470 /* not -470 */, 50, 500, 400),
window_bounds);
Nit: You mean "not -471"

[yuzo, 2009-05-29 02:21:24]

Thank you for the review.

Addressed the comments and confirmed that gcl try succeeds.

Can you take yet another look and submit this if it looks good?

Yuzo

http://codereview.chromium.org/115180/diff/1025/1027
File chrome/browser/window_sizer_unittest.cc (right):

http://codereview.chromium.org/115180/diff/1025/1027#newcode398
Line 398: gfx::Rect(-470, 50, 500, 400), false, PERSISTED,
On 2009/05/28 18:10:34, pkasting wrote:
> Nit: For all the tests where the final bounds should match the initial bounds,
> use an "initial_bounds" temp like the cases above.

Done.

http://codereview.chromium.org/115180/diff/1025/1027#newcode412
Line 412: EXPECT_EQ(gfx::Rect(-470 /* not -470 */, 50, 500, 400),
window_bounds);
On 2009/05/28 18:10:34, pkasting wrote:
> Nit: You mean "not -471"

Thank you for the catch. Done.

[Peter Kasting, 2009-05-29 20:48:17]

Landed in r17233.