Ignoring alignment when it pushes a window out of the screen

Ignoring alignment when it pushes a window out of the screen


BUG=121580, 122841
TEST=None


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

[Mr4D (OOO till 08-26), 2012-04-11 20:29:57]

This change will ignore any alignment requests which pushes a window out of the
screen.

[sky, 2012-04-11 21:10:00]

http://codereview.chromium.org/9969164/diff/1/ash/wm/workspace/workspace_mana...
File ash/wm/workspace/workspace_manager.cc (right):

http://codereview.chromium.org/9969164/diff/1/ash/wm/workspace/workspace_mana...
ash/wm/workspace/workspace_manager.cc:53: gfx::Rect AlignRectToGrid(const
gfx::Rect& rect, int grid_size) {
I believe this is called from a handful of places, and I'm not sure we would
always want to ignore the grid. Who is configuring the bounds that is triggering
this bug?

http://codereview.chromium.org/9969164/diff/1/ash/wm/workspace/workspace_mana...
ash/wm/workspace/workspace_manager.cc:70: x = rect.x();
Why ignore the grid in this case?

[Mr4D (OOO till 08-26), 2012-04-13 18:08:11]

As discussed, I have moved the logic from the WindowManager into the WindowSizer
/ PopupPositioner

[sky, 2012-04-13 19:44:01]

http://codereview.chromium.org/9969164/diff/7001/ash/wm/workspace/workspace_m...
File ash/wm/workspace/workspace_manager.cc (right):

http://codereview.chromium.org/9969164/diff/7001/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:14: #include "ash/wm/window_resizer.h"
Can you remove this include now?

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/window_positioner.cc (right):

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/window_positioner.cc:158: y += (grid - (y + grid -
1) % grid) % grid;
I don't understand this code. I'm sure I missing something, but if you want to
align to the grid how come this isn't:

y = y / grid * grid
?

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/window_positioner.cc:185: if (abs(pos.right() -
work_space.right()) < grid) {
nit: no {} here and 188

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/window_siz...
File chrome/browser/ui/window_sizer_aura.cc (right):

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/window_siz...
chrome/browser/ui/window_sizer_aura.cc:129: // Note: It is highly unlikely but
not impossible that this will bring the
This could happen if the launcher is aligned to the left/right, which is on the
table for 20/21.

[Mr4D (OOO till 08-26), 2012-04-13 20:47:39]

Merged and addressed

http://codereview.chromium.org/9969164/diff/7001/ash/wm/workspace/workspace_m...
File ash/wm/workspace/workspace_manager.cc (right):

http://codereview.chromium.org/9969164/diff/7001/ash/wm/workspace/workspace_m...
ash/wm/workspace/workspace_manager.cc:14: #include "ash/wm/window_resizer.h"
On 2012/04/13 19:44:02, sky wrote:
> Can you remove this include now?

Done.

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/window_positioner.cc (right):

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/window_positioner.cc:158: y += (grid - (y + grid -
1) % grid) % grid;
y / grid * grid: Certainly not. The compiler (windows at least) will optimize
that likely away (seen that in the past). But furthermore we want to gravitate
to the next line. So I might use something else here..

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/window_positioner.cc:185: if (abs(pos.right() -
work_space.right()) < grid) {
On 2012/04/13 19:44:02, sky wrote:
> nit: no {} here and 188

Done.

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/window_siz...
File chrome/browser/ui/window_sizer_aura.cc (right):

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/window_siz...
chrome/browser/ui/window_sizer_aura.cc:129: // Note: It is highly unlikely but
not impossible that this will bring the
Okay, changed the comment.

[sky, 2012-04-13 21:42:03]

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/window_positioner.cc (right):

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/window_positioner.cc:158: y += (grid - (y + grid -
1) % grid) % grid;
On 2012/04/13 20:47:39, Mr4D wrote:
> y / grid * grid: Certainly not. The compiler (windows at least) will optimize
> that likely away (seen that in the past).

That sounds like a compiler bug, we have code like that else where.

> But furthermore we want to gravitate
> to the next line. So I might use something else here..

If you want to gravitate, then use WindowResizer::AlignToGrid.

[Mr4D (OOO till 08-26), 2012-04-13 22:33:48]

There wasn't any change necessary (to my view).

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/window_positioner.cc (right):

http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/window_positioner.cc:158: y += (grid - (y + grid -
1) % grid) % grid;
It is not exactly a compiler bug - it is a behavior. I am not even sure if the
arithmetic has to be performed within the equation in pure integer - or if it
might even use 'higher precision' and floats. I know for sure that the windows
compilers in the past were doing it that way.


"Gravitate" was probably the wrong word - what I meant is: Take the next aligned
and uncovered position. This is doing exactly what we want.

[sky, 2012-04-13 22:51:07]

On 2012/04/13 22:33:48, Mr4D wrote:
> There wasn't any change necessary (to my view).
> 
>
http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/views/ash/...
> File chrome/browser/ui/views/ash/window_positioner.cc (right):
> 
>
http://codereview.chromium.org/9969164/diff/7001/chrome/browser/ui/views/ash/...
> chrome/browser/ui/views/ash/window_positioner.cc:158: y += (grid - (y + grid -
> 1) % grid) % grid;
> It is not exactly a compiler bug - it is a behavior. I am not even sure if the
> arithmetic has to be performed within the equation in pure integer - or if it
> might even use 'higher precision' and floats. I know for sure that the windows
> compilers in the past were doing it that way.
> 
> 
> "Gravitate" was probably the wrong word - what I meant is: Take the next
aligned
> and uncovered position. This is doing exactly what we want.

I believe AlignToGridRoundUp does what you want then.

[Mr4D (OOO till 08-26), 2012-04-13 23:22:56]

I have replaced it as requested. I have to point out here that a real *simple*
mathematical term like this - which will never ever change - shouldn't be packed
in an obscure function call like this. My opinion.

[sky, 2012-04-13 23:37:29]

*simple*, eh? I must be stupid. I had to stare at that code for a long time to
figure out what it was trying to do.

Fix the following, and LGTM

  -Scott

http://codereview.chromium.org/9969164/diff/9016/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/window_positioner.cc (right):

http://codereview.chromium.org/9969164/diff/9016/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/window_positioner.cc:173: const gfx::Rect&
work_space,
nit: work_space -> work_area. If you really want workspace, it should be word.

http://codereview.chromium.org/9969164/diff/9016/chrome/browser/ui/window_siz...
File chrome/browser/ui/window_sizer_ash_unittest.cc (right):

http://codereview.chromium.org/9969164/diff/9016/chrome/browser/ui/window_siz...
chrome/browser/ui/window_sizer_ash_unittest.cc:34: int align(int size, int
alignment_factor) {
Replace this (and the copy in your other test) with AlignToGridRoundDown.

[Mr4D (OOO till 08-26), 2012-04-14 00:19:45]

Okay done as requested. This isn't my code anymore.

http://codereview.chromium.org/9969164/diff/9016/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/window_positioner.cc (right):

http://codereview.chromium.org/9969164/diff/9016/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/window_positioner.cc:173: const gfx::Rect&
work_space,
On 2012/04/13 23:37:29, sky wrote:
> nit: work_space -> work_area. If you really want workspace, it should be word.

Done.

http://codereview.chromium.org/9969164/diff/9016/chrome/browser/ui/window_siz...
File chrome/browser/ui/window_sizer_ash_unittest.cc (right):

http://codereview.chromium.org/9969164/diff/9016/chrome/browser/ui/window_siz...
chrome/browser/ui/window_sizer_ash_unittest.cc:34: int align(int size, int
alignment_factor) {
Same here - This isn't my code anymore.

[commit-bot: I haz the power, 2012-04-14 01:17:48]

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

[commit-bot: I haz the power, 2012-04-14 02:44:24]

Change committed as 132316