Adding new window management

Adding new window management

Windows get placed according to the new algorithm:

The first window restores as the previous window was created (or if none were, the default will be chosen).
When the second window gets created, the first one will get pushed towards the left or right side (dependent on which side it was 'more') and the new window will get pushed to the other side and made fully visible.

Everything else remains as is.

Update: The bug 127283 seems also to be fixed with this change. I therefore add this into the bug section.

BUG=146528, 127283
TEST=unit test

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

[Mr4D (OOO till 08-26), 2012-09-13 02:24:11]

Please have a look!

[sky, 2012-09-13 15:44:51]

http://codereview.chromium.org/10911274/diff/1/chrome/browser/ui/window_sizer...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right):

http://codereview.chromium.org/10911274/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:42: aura::Window*
GetTopWindow(const gfx::Rect& bounds) {
The coordinates you pass in here are in screen coordinates, but you use
window->bounds() which are not necessarily screen coordinates. Same thing for
IsValidToplevelWindow above.

http://codereview.chromium.org/10911274/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:79: int
GetNumberOfValidTopLevelWindows(const gfx::Rect& bounds) {
This method is named as though it wants all top levels, but it only looks at
browsers. Please name accordingly.

http://codereview.chromium.org/10911274/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:86: if (browser &&
browser->window() &&
Can this and IsToplevelWindow share code?

[Mr4D (OOO till 08-26), 2012-09-13 16:45:08]

Addressed! Please have another look!

http://codereview.chromium.org/10911274/diff/1/chrome/browser/ui/window_sizer...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right):

http://codereview.chromium.org/10911274/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:42: aura::Window*
GetTopWindow(const gfx::Rect& bounds) {
Right! Done.

http://codereview.chromium.org/10911274/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:79: int
GetNumberOfValidTopLevelWindows(const gfx::Rect& bounds) {
On 2012/09/13 15:44:51, sky wrote:
> This method is named as though it wants all top levels, but it only looks at
> browsers. Please name accordingly.

Done.

http://codereview.chromium.org/10911274/diff/1/chrome/browser/ui/window_sizer...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:86: if (browser &&
browser->window() &&
On 2012/09/13 15:44:51, sky wrote:
> Can this and IsToplevelWindow share code?

Done.

[sky, 2012-09-13 17:32:02]

http://codereview.chromium.org/10911274/diff/12/chrome/browser/ui/window_size...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right):

http://codereview.chromium.org/10911274/diff/12/chrome/browser/ui/window_size...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:30: 
remove this newline.

http://codereview.chromium.org/10911274/diff/12/chrome/browser/ui/window_size...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:145: if (MoveRect(work_area,
other_bounds, !move_right))
You're comparing screen bounds and non-screen bounds here.

I suggest you name all bounds that are screen based such to make it easier (say
screen_bounds) to know what's going on. That would also make folks looking at
the code know the expectations.

http://codereview.chromium.org/10911274/diff/12/chrome/browser/ui/window_size...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:146:
top_window->SetBounds(other_bounds);
Moving a window makes me nervous. Consider the case of a modal dialog being on
top of the window, this could potentially move the window out from underneath
the modal dialog. I've no doubt there are other weird cases.

[Mr4D (OOO till 08-26), 2012-09-13 18:56:59]

Addressed! Please have another look!

http://codereview.chromium.org/10911274/diff/12/chrome/browser/ui/window_size...
File chrome/browser/ui/window_sizer/window_sizer_ash.cc (right):

http://codereview.chromium.org/10911274/diff/12/chrome/browser/ui/window_size...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:30: 
On 2012/09/13 17:32:02, sky wrote:
> remove this newline.

Done.

http://codereview.chromium.org/10911274/diff/12/chrome/browser/ui/window_size...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:145: if (MoveRect(work_area,
other_bounds, !move_right))
Thanks for your explanations of the term 'screen' (especially in regards of
multiple screens). We should name it 'desktop' or something else and maybe even
have a special logic in the gfx::Rect structure which does these things
automatically after setting a flag (or create a special 'absolute' coordinate
object).

Done.

http://codereview.chromium.org/10911274/diff/12/chrome/browser/ui/window_size...
chrome/browser/ui/window_sizer/window_sizer_ash.cc:146:
top_window->SetBounds(other_bounds);
Well - this automatic movement was requested by Alex and I agree that this is a
useful operation. to make better use of the available desktop space. Also note
that this is only a first stab of a better window management to follow (post
23).

[sky, 2012-09-13 19:50:36]

LGTM when you fix the coordinate issue.

[commit-bot: I haz the power, 2012-09-13 20:16:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/10911274/12003

[Mr4D (OOO till 08-26), 2012-09-13 20:16:38]

Done! Thanks!

[commit-bot: I haz the power, 2012-09-13 22:24:59]

Change committed as 156655