Adding proper dragging behavior for L/R maximized windows.

Adding proper dragging behavior for L/R maximized windows.


BUG=141750
TEST=unit tests & visual


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

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

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

I added the dragging functionality for L/R maximized windows. Please have a
look!

[sky, 2012-09-05 17:03:24]

And you need some tests.

http://codereview.chromium.org/10918077/diff/2001/ash/wm/frame_painter.cc
File ash/wm/frame_painter.cc (right):

http://codereview.chromium.org/10918077/diff/2001/ash/wm/frame_painter.cc#new...
ash/wm/frame_painter.cc:601: if (hit_code != HTNOWHERE) {
Why does this logic depend upon having restore bounds? Isn't the important thing
that the window fills the width or height and is normal? Also, shouldn't this do
the same thing if the window fills the horizontal space? You can get in the
later state if you double click the left/right edge.

http://codereview.chromium.org/10918077/diff/2001/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_window_resizer.cc (right):

http://codereview.chromium.org/10918077/diff/2001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:103: window->SetBounds(bounds);
This is the wrong place for this. Doing it here means if we revert the drag it
won't go to the right place.

[Mr4D (OOO till 08-26), 2012-09-06 16:06:49]

I will write a few more unit tests, but need to help Matt first with some other
stuff. 

Please have a look!

http://codereview.chromium.org/10918077/diff/2001/ash/wm/frame_painter.cc
File ash/wm/frame_painter.cc (right):

http://codereview.chromium.org/10918077/diff/2001/ash/wm/frame_painter.cc#new...
ash/wm/frame_painter.cc:601: if (hit_code != HTNOWHERE) {
Due to our snapping: I don't think so. You can get very easy into a state where
the window is placed to trigger this behavior in an incorrect situation.
Checking for the restore bounds avoids this problem (and is secure & correct).

Filling horizontal space: A. another "undiscoverable" feature... (thanks for
pointing that out!). B. Was not requested, but makes sense. Done.

http://codereview.chromium.org/10918077/diff/2001/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_window_resizer.cc (right):

http://codereview.chromium.org/10918077/diff/2001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:103: window->SetBounds(bounds);
On 2012/09/05 17:03:24, sky wrote:
> This is the wrong place for this. Doing it here means if we revert the drag it
> won't go to the right place.

Done.

[sky, 2012-09-06 19:45:11]

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter.cc
File ash/wm/frame_painter.cc (right):

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter.cc#new...
ash/wm/frame_painter.cc:605: gfx::Rect bounds = window_->bounds();
const gfx::Rect& here and next line.

And you need to convert coordinates, or use frame_->GetWindowBoundsInScreen().

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter.cc#new...
ash/wm/frame_painter.cc:624: if (bounds.right() == screen.right()) {
Shouldn't this check y coordinates (620 checks x coordinates).

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
File ash/wm/frame_painter_unittest.cc (right):

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
ash/wm/frame_painter_unittest.cc:35: virtual views::Widget* GetWidget() {return
widget_; }
OVERRIDE on these two lines, and space after {

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
ash/wm/frame_painter_unittest.cc:37: private:
newline bewteen 36/67.

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
ash/wm/frame_painter_unittest.cc:39: };
DISALLOW_...

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
ash/wm/frame_painter_unittest.cc:206: frame->SetBounds(0, 0, 100, 100);
Why do you need this? w1->SetBounds should take care of it for you.

http://codereview.chromium.org/10918077/diff/6001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/10918077/diff/6001/ash/wm/window_resizer.cc#ne...
ash/wm/window_resizer.cc:280: size = details.restore_bounds.size();
You may need to adjust the origin too. Consider the following: create a smallish
window, say 100x100, snap it to the left. Say it ends up with a width of 1024.
If you start dragging close the right, say 1000, the dragged window is going to
snap back to 100x100 and the window is no longer going to be under the mouse.

http://codereview.chromium.org/10918077/diff/6001/ash/wm/window_resizer.h
File ash/wm/window_resizer.h (right):

http://codereview.chromium.org/10918077/diff/6001/ash/wm/window_resizer.h#new...
ash/wm/window_resizer.h:74: // This are the restore bounds of the window if it
is a normal window and
Is there any reason to special case this? Can't you always set it? Additionally
I think you only need the size.

http://codereview.chromium.org/10918077/diff/6001/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_window_resizer.cc (right):

http://codereview.chromium.org/10918077/diff/6001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:129: ClearRestoreBounds(window());
Wait until the drag is complete before clearing the restore bounds. That way you
don't have to reset it below. Also, you need thet same thing for the
defaultWindowSizer.

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

Addressed. Please have another look!

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter.cc
File ash/wm/frame_painter.cc (right):

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter.cc#new...
ash/wm/frame_painter.cc:605: gfx::Rect bounds = window_->bounds();
On 2012/09/06 19:45:11, sky wrote:
> const gfx::Rect& here and next line.
> 
> And you need to convert coordinates, or use frame_->GetWindowBoundsInScreen().

Done.

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter.cc#new...
ash/wm/frame_painter.cc:624: if (bounds.right() == screen.right()) {
Actually - I don't think that any additional check is needed here. Done.

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
File ash/wm/frame_painter_unittest.cc (right):

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
ash/wm/frame_painter_unittest.cc:35: virtual views::Widget* GetWidget() {return
widget_; }
On 2012/09/06 19:45:11, sky wrote:
> OVERRIDE on these two lines, and space after {

Done.

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
ash/wm/frame_painter_unittest.cc:37: private:
On 2012/09/06 19:45:11, sky wrote:
> newline bewteen 36/67.

Done.

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
ash/wm/frame_painter_unittest.cc:39: };
On 2012/09/06 19:45:11, sky wrote:
> DISALLOW_...

Done.

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
ash/wm/frame_painter_unittest.cc:206: frame->SetBounds(0, 0, 100, 100);
You need both. I was puzzled by this myself when writing the test. While
debugging I have seen that some hit test related tests in the code were looking
for the widgets bounds and some for the frame bounds. So I assumed that that
must be either a.) intentionally because the two can be handled separately from
each other (which makes a lot of sense since it's our 'special' frame which
might hang over the widget) or b.) because the unit test framework is missing
some glue somewhere.

http://codereview.chromium.org/10918077/diff/6001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/10918077/diff/6001/ash/wm/window_resizer.cc#ne...
ash/wm/window_resizer.cc:280: size = details.restore_bounds.size();
I was first tempted to say that this was taken care off, but then realized that
you are right... Addressed.

http://codereview.chromium.org/10918077/diff/6001/ash/wm/window_resizer.h
File ash/wm/window_resizer.h (right):

http://codereview.chromium.org/10918077/diff/6001/ash/wm/window_resizer.h#new...
ash/wm/window_resizer.h:74: // This are the restore bounds of the window if it
is a normal window and
Yes, because in case of a revert I need to know if there was a restore bound or
not. And note that this can be different (but does not have to be) from the
initial_bounds as well.

Additionally: Once I removed the old restore state, I need size and location -
otherwise we cannot perform an abort from the resize.

http://codereview.chromium.org/10918077/diff/6001/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_window_resizer.cc (right):

http://codereview.chromium.org/10918077/diff/6001/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer.cc:129: ClearRestoreBounds(window());
No. There is code which has special behavior when the restore bounds are set.
Keeping them set and resizing the window to an arbitrary size is asking for
trouble and I don't see any benefit for that. I leave this as it is. 

I will however add the code also to the default resizer. I have seen that code
but was not exactly sure if it was there also needed or not.

[sky, 2012-09-07 02:43:29]

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
File ash/wm/frame_painter_unittest.cc (right):

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
ash/wm/frame_painter_unittest.cc:206: frame->SetBounds(0, 0, 100, 100);
On 2012/09/06 23:16:13, Mr4D wrote:
> You need both. I was puzzled by this myself when writing the test. While
> debugging I have seen that some hit test related tests in the code were
looking
> for the widgets bounds and some for the frame bounds. So I assumed that that
> must be either a.) intentionally because the two can be handled separately
from
> each other (which makes a lot of sense since it's our 'special' frame which
> might hang over the widget) or b.) because the unit test framework is missing
> some glue somewhere.

Please investigate this more deeply. Setting the frame widget bounds immediately
sets the window bounds which should immediately resize the NonClientFrameView.

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:105: restore_bounds(gfx::Rect()),
not necessary.

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:284: } else {
else if

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.h
File ash/wm/window_resizer.h (right):

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.h#ne...
ash/wm/window_resizer.h:77: // This are the restore bounds of the window if it
is a normal window and
How about: Restore bounds (in screen coordinates) of the window before the drag
started. Only set if the window is normal and is being dragged.

http://codereview.chromium.org/10918077/diff/12001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

http://codereview.chromium.org/10918077/diff/12001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_window_resizer_unittest.cc:908:
TEST_F(WorkspaceWindowResizerTest, RestoreToPreMaximizeCoordinates) {
Add coverage of the revert case

[sky, 2012-09-07 15:46:14]

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:213: origin.set_x(location.x() -
details.restore_bounds.width() / 2);
One more comment here. Centering is going to be equally jarring. For example, if
you start dragging very close to the left/right edge, then the window is going
to snap. Only move if necessary.

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

Changed as requested.

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:213: origin.set_x(location.x() -
details.restore_bounds.width() / 2);
I tried it actually this way and I found it this way much better. But - done.

[Mr4D (OOO till 08-26), 2012-09-08 01:17:41]

Mostly addresed. Will continue on Monday

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
File ash/wm/frame_painter_unittest.cc (right):

http://codereview.chromium.org/10918077/diff/6001/ash/wm/frame_painter_unitte...
ash/wm/frame_painter_unittest.cc:206: frame->SetBounds(0, 0, 100, 100);
I have dumped already several hours on this and will continue to find this. I
hope this is worth it.

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:105: restore_bounds(gfx::Rect()),
On 2012/09/07 02:43:30, sky wrote:
> not necessary.

Done.

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:284: } else {
On 2012/09/07 02:43:30, sky wrote:
> else if

Done.

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.h
File ash/wm/window_resizer.h (right):

http://codereview.chromium.org/10918077/diff/12001/ash/wm/window_resizer.h#ne...
ash/wm/window_resizer.h:77: // This are the restore bounds of the window if it
is a normal window and
On 2012/09/07 02:43:30, sky wrote:
> How about: Restore bounds (in screen coordinates) of the window before the
drag
> started. Only set if the window is normal and is being dragged.

Done.

http://codereview.chromium.org/10918077/diff/12001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

http://codereview.chromium.org/10918077/diff/12001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_window_resizer_unittest.cc:908:
TEST_F(WorkspaceWindowResizerTest, RestoreToPreMaximizeCoordinates) {
On 2012/09/07 02:43:30, sky wrote:
> Add coverage of the revert case

Done.

[Mr4D (OOO till 08-26), 2012-09-10 16:18:30]

Please have another look

[sky, 2012-09-10 17:10:48]

LGTM

http://codereview.chromium.org/10918077/diff/2003/ash/wm/workspace/workspace_...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

http://codereview.chromium.org/10918077/diff/2003/ash/wm/workspace/workspace_...
ash/wm/workspace/workspace_window_resizer_unittest.cc:912: SetGridSize(1);
I'm removing the grid @ http://codereview.chromium.org/10910164/ .

[Mr4D (OOO till 08-26), 2012-09-10 19:38:09]

Done.

[commit-bot: I haz the power, 2012-09-10 19:38:25]

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

[commit-bot: I haz the power, 2012-09-10 21:44:59]

Change committed as 155840

[commit-bot: I haz the power, 2012-09-11 14:44:39]

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

[commit-bot: I haz the power, 2012-09-11 18:05:51]

Change committed as 156058