For consistency, stop using the workarea coordinate in...

For consistency, stop using the workarea coordinate in
saving/retrieving/adjusting window positions.

TEST=Open Chrome windows and see if they are placed properly.
TESTED=gcl try, manually
BUG=none



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

[Yuzo, 2009-06-22 08:33:26]

Hi, Peter,

Can you review this?

Yuzo

[Peter Kasting, 2009-06-22 19:13:54]

http://codereview.chromium.org/141039/diff/1/4
File views/window/window_win.cc (right):

http://codereview.chromium.org/141039/diff/1/4#newcode118
Line 118: void WindowWin::GetWindowPlacementInScreen(gfx::Rect* bounds, bool*
maximized)
Nit: This function should be in the same place in this file as it is in the
header.

http://codereview.chromium.org/141039/diff/1/4#newcode122
Line 122: BOOL r0 = GetWindowPlacement(GetNativeView(), &wp);
Nit: Use "const bool ret = !!GetWindowPlacement(...)" like the old code (i.e.
avoid BOOL)

http://codereview.chromium.org/141039/diff/1/4#newcode125
Line 125: if (maximized != NULL)
Nit: This is totally minor, but I'd put this block after the |bounds| block
since that's the order it's declared in the argument list.

http://codereview.chromium.org/141039/diff/1/4#newcode129
Line 129: // Convert normal position from the workarea coordinate to the screen
Nit: "the ____ coordinate" -> "____ coordinates"

http://codereview.chromium.org/141039/diff/1/4#newcode133
Line 133: BOOL r1 = GetMonitorInfo(
Nit: See above re: BOOL

http://codereview.chromium.org/141039/diff/1/4#newcode136
Line 136: gfx::Rect rect(wp.rcNormalPosition);
Nit: Why use a temp here?  Why not just use *bounds the whole way?

http://codereview.chromium.org/141039/diff/1/4#newcode1317
Line 1317: GetWindowPlacementInScreen(&bounds, &maximized);
The old code saved the window position without adjusting to screen coordinates. 
Doesn't this mean that after your change goes in, the next time people run
Chrome all their saved coordinates will be wrong?

http://codereview.chromium.org/141039/diff/1/5
File views/window/window_win.h (right):

http://codereview.chromium.org/141039/diff/1/5#newcode204
Line 204: // Returns the normal bounds of the window in the screen coordinate
and
Nit: "the screen coordinate" -> "screen coordinates"

http://codereview.chromium.org/141039/diff/1/5#newcode206
Line 206: void GetWindowPlacementInScreen(gfx::Rect* bounds, bool* maximized)
const;
Nit: GetWindowBoundsAndMaximizedState() perhaps?

[Yuzo, 2009-06-23 02:33:10]

Peter,

Thank you for the review. I've addressed the comments.
Can you take another look?

Yuzo

http://codereview.chromium.org/141039/diff/1/4
File views/window/window_win.cc (right):

http://codereview.chromium.org/141039/diff/1/4#newcode118
Line 118: void WindowWin::GetWindowPlacementInScreen(gfx::Rect* bounds, bool*
maximized)
On 2009/06/22 19:13:54, pkasting wrote:
> Nit: This function should be in the same place in this file as it is in the
> header.

Sorry about this. Fixed.

http://codereview.chromium.org/141039/diff/1/4#newcode122
Line 122: BOOL r0 = GetWindowPlacement(GetNativeView(), &wp);
On 2009/06/22 19:13:54, pkasting wrote:
> Nit: Use "const bool ret = !!GetWindowPlacement(...)" like the old code (i.e.
> avoid BOOL)

Done.

http://codereview.chromium.org/141039/diff/1/4#newcode125
Line 125: if (maximized != NULL)
On 2009/06/22 19:13:54, pkasting wrote:
> Nit: This is totally minor, but I'd put this block after the |bounds| block
> since that's the order it's declared in the argument list.

Done.

http://codereview.chromium.org/141039/diff/1/4#newcode129
Line 129: // Convert normal position from the workarea coordinate to the screen
On 2009/06/22 19:13:54, pkasting wrote:
> Nit: "the ____ coordinate" -> "____ coordinates"

Done.

http://codereview.chromium.org/141039/diff/1/4#newcode133
Line 133: BOOL r1 = GetMonitorInfo(
On 2009/06/22 19:13:54, pkasting wrote:
> Nit: See above re: BOOL

Done.

http://codereview.chromium.org/141039/diff/1/4#newcode136
Line 136: gfx::Rect rect(wp.rcNormalPosition);
On 2009/06/22 19:13:54, pkasting wrote:
> Nit: Why use a temp here?  Why not just use *bounds the whole way?

Done.

http://codereview.chromium.org/141039/diff/1/4#newcode1317
Line 1317: GetWindowPlacementInScreen(&bounds, &maximized);
On 2009/06/22 19:13:54, pkasting wrote:
> The old code saved the window position without adjusting to screen
coordinates. 
> Doesn't this mean that after your change goes in, the next time people run
> Chrome all their saved coordinates will be wrong?

Yes.

If the user places his/her task bar on top/left of the screen, his/her first
window after this change will be
shown shifted toward top/left by the width of the task bar.

My assumption was that this is acceptable, because it is 
one-time thing and not severe. Do you agree?

http://codereview.chromium.org/141039/diff/1/5
File views/window/window_win.h (right):

http://codereview.chromium.org/141039/diff/1/5#newcode204
Line 204: // Returns the normal bounds of the window in the screen coordinate
and
On 2009/06/22 19:13:54, pkasting wrote:
> Nit: "the screen coordinate" -> "screen coordinates"

Done.

http://codereview.chromium.org/141039/diff/1/5#newcode206
Line 206: void GetWindowPlacementInScreen(gfx::Rect* bounds, bool* maximized)
const;
On 2009/06/22 19:13:54, pkasting wrote:
> Nit: GetWindowBoundsAndMaximizedState() perhaps?

Done.

[Peter Kasting, 2009-06-23 02:40:19]

http://codereview.chromium.org/141039/diff/2001/2004
File views/window/window_win.cc (right):

http://codereview.chromium.org/141039/diff/2001/2004#newcode1292
Line 1292: 
Nit: No blank line here

http://codereview.chromium.org/141039/diff/2001/2004#newcode1364
Line 1364: const bool r0 = !!GetWindowPlacement(GetNativeView(), &wp);
Nit: Just name these both "ret" or "succeeded" (the inner declaration will mask
the outer one).  "r0/r1" is meaningless.

http://codereview.chromium.org/141039/diff/2001/2004#newcode1368
Line 1368: // Convert normal position from the workarea coordinates to the
screen
Nit: No "the" before "___ coordinates"

Move this comment down to above the Offset() call.

http://codereview.chromium.org/141039/diff/2001/2005
File views/window/window_win.h (right):

http://codereview.chromium.org/141039/diff/2001/2005#newcode204
Line 204: // Returns the normal bounds of the window in the screen coordinates
and
Nit: No "the" before "screen coordinates"

[Yuzo, 2009-06-23 03:46:05]

Peter,

Thank you. Can you take yet another look?

Yuzo

http://codereview.chromium.org/141039/diff/2001/2004
File views/window/window_win.cc (right):

http://codereview.chromium.org/141039/diff/2001/2004#newcode1292
Line 1292: 
On 2009/06/23 02:40:19, pkasting wrote:
> Nit: No blank line here

Done.

http://codereview.chromium.org/141039/diff/2001/2004#newcode1364
Line 1364: const bool r0 = !!GetWindowPlacement(GetNativeView(), &wp);
On 2009/06/23 02:40:19, pkasting wrote:
> Nit: Just name these both "ret" or "succeeded" (the inner declaration will
mask
> the outer one).  "r0/r1" is meaningless.

Changed to use a local bool variable "succeeded".
(I felt somewhat uneasy about masking.)

http://codereview.chromium.org/141039/diff/2001/2004#newcode1368
Line 1368: // Convert normal position from the workarea coordinates to the
screen
On 2009/06/23 02:40:19, pkasting wrote:
> Nit: No "the" before "___ coordinates"
> 
> Move this comment down to above the Offset() call.

Done.

http://codereview.chromium.org/141039/diff/2001/2005
File views/window/window_win.h (right):

http://codereview.chromium.org/141039/diff/2001/2005#newcode204
Line 204: // Returns the normal bounds of the window in the screen coordinates
and
On 2009/06/23 02:40:19, pkasting wrote:
> Nit: No "the" before "screen coordinates"

Done.

[Peter Kasting, 2009-06-23 03:56:36]

http://codereview.chromium.org/141039/diff/2007/2010
File views/window/window_win.cc (right):

http://codereview.chromium.org/141039/diff/2007/2010#newcode1364
Line 1364: succeeded = !!GetWindowPlacement(GetNativeView(), &wp);
You've dropped the const and made both calls use the same variable.  This
unnecessarily expands the liveness range of the variable.  Please change back,
this way of doing it is slightly misleading.  Only the name was poor before, not
the usage.

[Yuzo, 2009-06-23 04:03:51]

Peter,

Can you take yet another look?

Yuzo

http://codereview.chromium.org/141039/diff/2007/2010
File views/window/window_win.cc (right):

http://codereview.chromium.org/141039/diff/2007/2010#newcode1364
Line 1364: succeeded = !!GetWindowPlacement(GetNativeView(), &wp);
On 2009/06/23 03:56:36, pkasting wrote:
> You've dropped the const and made both calls use the same variable.  This
> unnecessarily expands the liveness range of the variable.  Please change back,
> this way of doing it is slightly misleading.  Only the name was poor before,
not
> the usage.

I see. Changed to const bool.

[Peter Kasting, 2009-06-23 04:22:31]

LGTM