Fix: New window appears to the left/up if the taskbar is on left/top....

Fix: New window appears to the left/up if the taskbar is on left/top.

Currently, the Windows taskbar (put differently, work area) is not considered
in getting the last window bounds (while it is for the saved window bounds).
This causes new windows (CTRL-N) appear to the left/up of the existing window
instead of right/down, if the taskbar is on left or top.

TESTED=gcl try, manually

TEST=Place the taskbar on left or top of the screen and create new Chrome windows by CTRL-N and observe.

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

[Yuzo, 2009-06-19 04:28:29]

Ping?

[Peter Kasting, 2009-06-19 05:08:16]

Should there be a unittest for this case?

LGTM otherwise

http://codereview.chromium.org/125179/diff/1/4
File chrome/browser/window_sizer.cc (right):

http://codereview.chromium.org/125179/diff/1/4#newcode152
Line 152: bounds->Offset(taskbar_offset.x(), taskbar_offset.y());
Nit: This whole function can become:

bounds->Offset(monitor_info_provider_->GetBoundsOffsetMatching(*bounds));

At that point, you can just inline it into the two callers instead of having a
helper function.

[Yuzo, 2009-06-19 05:39:32]

Peter,

Thank you for your review.

I've added two tests for this to window_sizer_unittest.cc.

Yuzo

http://codereview.chromium.org/125179/diff/1/4
File chrome/browser/window_sizer.cc (right):

http://codereview.chromium.org/125179/diff/1/4#newcode152
Line 152: bounds->Offset(taskbar_offset.x(), taskbar_offset.y());
On 2009/06/19 05:08:16, pkasting wrote:
> Nit: This whole function can become:
> 
> bounds->Offset(monitor_info_provider_->GetBoundsOffsetMatching(*bounds));
> 
> At that point, you can just inline it into the two callers instead of having a
> helper function.

Done.

[Peter Kasting, 2009-06-19 06:21:14]

http://codereview.chromium.org/125179/diff/3001/2005
File chrome/browser/window_sizer.cc (right):

http://codereview.chromium.org/125179/diff/3001/2005#newcode154
Line 154:
bounds->Offset(monitor_info_provider_->GetBoundsOffsetMatching(*bounds));
So wait a minute.  I'm thinking about these more and they feel weird.

Why are we offsetting by the (work area - monitor rect) difference anyway?  Are
the previous bounds we retrieve offset opposite to this and we have to correct
for it?  I assume so... but then the values you get in your unittests kinda
scare me since we go from e.g. (10, 10) to (127, 10).  The only way this can be
right is if the first coordinate is relative to the work area and the second
relative to the screen.  Is that the case?

[Yuzo, 2009-06-19 06:28:16]

Oops, I've just submitted this.
Do you want me to roll it back?

Yes, the coordinate looked weird to me too.

The saved / last window coordinate is relative to work area,
while the "adjusted" coordinate is relative to screen.

An alternative approach I thought was to make
the all coordinates relative to work are in adjusting them,
and converting coordinate relative to the screen in calling
setting window position API.

But I was not confident the approach won't affect other
parts of code. So I took seemingly safer approach.

Yuzo


On 2009/06/19 06:21:14, pkasting wrote:
> http://codereview.chromium.org/125179/diff/3001/2005
> File chrome/browser/window_sizer.cc (right):
> 
> http://codereview.chromium.org/125179/diff/3001/2005#newcode154
> Line 154:
> bounds->Offset(monitor_info_provider_->GetBoundsOffsetMatching(*bounds));
> So wait a minute.  I'm thinking about these more and they feel weird.
> 
> Why are we offsetting by the (work area - monitor rect) difference anyway? 
Are
> the previous bounds we retrieve offset opposite to this and we have to correct
> for it?  I assume so... but then the values you get in your unittests kinda
> scare me since we go from e.g. (10, 10) to (127, 10).  The only way this can
be
> right is if the first coordinate is relative to the work area and the second
> relative to the screen.  Is that the case?

[Peter Kasting, 2009-06-19 06:51:52]

On 2009/06/19 06:28:16, yuzo wrote:
> Oops, I've just submitted this.
> Do you want me to roll it back?

No need for that.

> The saved / last window coordinate is relative to work area,
> while the "adjusted" coordinate is relative to screen.

At the very least I would add a comment about this.

> An alternative approach I thought was to make
> the all coordinates relative to work are in adjusting them,
> and converting coordinate relative to the screen in calling
> setting window position API.

Or make all coordinates screen-relative.  Or something.  Now that you have a fix
for the issue at hand in place and some unittests, maybe something like this
makes sense?  I don't really care whether coordinates are in work area coords or
monitor coords (though it seems like "work area coords" aren't used almost
anywhere else in the code?) but I would think consistency would help be less
confusing...

[Yuzo, 2009-06-19 06:58:08]

To be more precise:

Currently, WindowWin::SetInitialBounds calls SetWindowPos using
the bounds returned by WindowSizer, unadjusted.

An alternative would be to
- convert the bounds to the screen coordinate, and
- have WindowSizer return bounds in work area coordinate.
- change all other callers of WindowSizer accordingly.

Yuzo
in calling SetWindowPos
  


On 2009/06/19 06:28:16, yuzo wrote:
> Oops, I've just submitted this.
> Do you want me to roll it back?
> 
> Yes, the coordinate looked weird to me too.
> 
> The saved / last window coordinate is relative to work area,
> while the "adjusted" coordinate is relative to screen.
> 
> An alternative approach I thought was to make
> the all coordinates relative to work are in adjusting them,
> and converting coordinate relative to the screen in calling
> setting window position API.
> 
> But I was not confident the approach won't affect other
> parts of code. So I took seemingly safer approach.
> 
> Yuzo
> 
> 
> On 2009/06/19 06:21:14, pkasting wrote:
> > http://codereview.chromium.org/125179/diff/3001/2005
> > File chrome/browser/window_sizer.cc (right):
> > 
> > http://codereview.chromium.org/125179/diff/3001/2005#newcode154
> > Line 154:
> > bounds->Offset(monitor_info_provider_->GetBoundsOffsetMatching(*bounds));
> > So wait a minute.  I'm thinking about these more and they feel weird.
> > 
> > Why are we offsetting by the (work area - monitor rect) difference anyway? 
> Are
> > the previous bounds we retrieve offset opposite to this and we have to
correct
> > for it?  I assume so... but then the values you get in your unittests kinda
> > scare me since we go from e.g. (10, 10) to (127, 10).  The only way this can
> be
> > right is if the first coordinate is relative to the work area and the second
> > relative to the screen.  Is that the case?

[Yuzo, 2009-06-19 07:00:59]

Sorry, the previous one is not complete.

To be more precise:

Currently, WindowWin::SetInitialBounds calls SetWindowPos using
the bounds returned by WindowSizer, unadjusted.

An alternative would be to
- have WindowSizer return bounds in work area coordinate,
- convert the bounds to the screen coordinate in calling SetWindowPos, and
- change all other callers of WindowSizer similarly.

Yuzo
  
 
> On 2009/06/19 06:28:16, yuzo wrote:
> > Oops, I've just submitted this.
> > Do you want me to roll it back?
> > 
> > Yes, the coordinate looked weird to me too.
> > 
> > The saved / last window coordinate is relative to work area,
> > while the "adjusted" coordinate is relative to screen.
> > 
> > An alternative approach I thought was to make
> > the all coordinates relative to work are in adjusting them,
> > and converting coordinate relative to the screen in calling
> > setting window position API.
> > 
> > But I was not confident the approach won't affect other
> > parts of code. So I took seemingly safer approach.
> > 
> > Yuzo
> > 
> > 
> > On 2009/06/19 06:21:14, pkasting wrote:
> > > http://codereview.chromium.org/125179/diff/3001/2005
> > > File chrome/browser/window_sizer.cc (right):
> > > 
> > > http://codereview.chromium.org/125179/diff/3001/2005#newcode154
> > > Line 154:
> > > bounds->Offset(monitor_info_provider_->GetBoundsOffsetMatching(*bounds));
> > > So wait a minute.  I'm thinking about these more and they feel weird.
> > > 
> > > Why are we offsetting by the (work area - monitor rect) difference anyway?

> > Are
> > > the previous bounds we retrieve offset opposite to this and we have to
> correct
> > > for it?  I assume so... but then the values you get in your unittests
kinda
> > > scare me since we go from e.g. (10, 10) to (127, 10).  The only way this
can
> > be
> > > right is if the first coordinate is relative to the work area and the
second
> > > relative to the screen.  Is that the case?

[Yuzo, 2009-06-19 07:39:56]

I agree that consistency is important.
I'll try to make it consistent next week, in a separate change.

Additional note:

BrowserWindow::GetNormalBounds() is actually EXPECTED
to return the bounds in screen coordinate:

  // TODO(beng): RENAME (GetRestoredBounds)
  // Returns the nonmaximized bounds of the frame (even if the frame is
  // currently maximized or minimized) in terms of the screen coordinates.
  virtual gfx::Rect GetNormalBounds() const = 0;

but

  GetWindowPlacement

called from the function returns the bounds in the work area
coordinate. (The msdn site is unclear as to in which 
coordinate the function returns the position. But as far
as I can tell using a debugger, the returned position is
in the work area coordinate.)

Yuzo

On 2009/06/19 07:00:59, yuzo wrote:
> Sorry, the previous one is not complete.
> 
> To be more precise:
> 
> Currently, WindowWin::SetInitialBounds calls SetWindowPos using
> the bounds returned by WindowSizer, unadjusted.
> 
> An alternative would be to
> - have WindowSizer return bounds in work area coordinate,
> - convert the bounds to the screen coordinate in calling SetWindowPos, and
> - change all other callers of WindowSizer similarly.
> 
> Yuzo
>   
>  
> > On 2009/06/19 06:28:16, yuzo wrote:
> > > Oops, I've just submitted this.
> > > Do you want me to roll it back?
> > > 
> > > Yes, the coordinate looked weird to me too.
> > > 
> > > The saved / last window coordinate is relative to work area,
> > > while the "adjusted" coordinate is relative to screen.
> > > 
> > > An alternative approach I thought was to make
> > > the all coordinates relative to work are in adjusting them,
> > > and converting coordinate relative to the screen in calling
> > > setting window position API.
> > > 
> > > But I was not confident the approach won't affect other
> > > parts of code. So I took seemingly safer approach.
> > > 
> > > Yuzo
> > > 
> > > 
> > > On 2009/06/19 06:21:14, pkasting wrote:
> > > > http://codereview.chromium.org/125179/diff/3001/2005
> > > > File chrome/browser/window_sizer.cc (right):
> > > > 
> > > > http://codereview.chromium.org/125179/diff/3001/2005#newcode154
> > > > Line 154:
> > > >
bounds->Offset(monitor_info_provider_->GetBoundsOffsetMatching(*bounds));
> > > > So wait a minute.  I'm thinking about these more and they feel weird.
> > > > 
> > > > Why are we offsetting by the (work area - monitor rect) difference
anyway?
> 
> > > Are
> > > > the previous bounds we retrieve offset opposite to this and we have to
> > correct
> > > > for it?  I assume so... but then the values you get in your unittests
> kinda
> > > > scare me since we go from e.g. (10, 10) to (127, 10).  The only way this
> can
> > > be
> > > > right is if the first coordinate is relative to the work area and the
> second
> > > > relative to the screen.  Is that the case?