Avoids updating dock alignment when drag is cancelled

Avoids updating dock alignment when drag is cancelled

BUG=334313
TEST=ash_unittests --gtest_filter=*RevertDockedDragRevertsAttachment*
TEST=Manual on Chromebook

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245435

[varkha, 2014-01-16 18:23:05]

pkotwicz@, could you please take a look? This restricts updating alignment to
only happen when drag starts or finishes and only when it affects windows that
are still docked when the drag ends or which have been docked when the drag
started. I think this is also easier to understand.

[pkotwicz, 2014-01-16 21:43:26]

https://codereview.chromium.org/141053002/diff/140001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/141053002/diff/140001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:526:
WindowResizer::kBoundsChange_Resizes)) {
I wonder whether it is possible to have a new function
DockedWindowLayoutManager::GetClosestAlignment(aura::Window* window) which
returns DOCKED_ALIGNMENT_LEFT or DOCKED_ALIGNMENT_RIGHT based on which edge is
closer. I think we should be able to use
DockedWindowLayoutManager::GetClosestAlignment() everywhere that
DockedWindowLayoutManager::GetAlignmentOfWindow() is used with the exception of
WorkspaceWindowResizer::SetDraggedWindowDocked().
I think that doing this will allow you to remove the check for
DragDetails::bounds_change

Nit: You can move the if statement above MaybeMinimizeChildrenExcept()

https://codereview.chromium.org/141053002/diff/140001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_resizer_unittest.cc (right):

https://codereview.chromium.org/141053002/diff/140001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_resizer_unittest.cc:650:
EXPECT_EQ(DOCKED_ALIGNMENT_NONE, docked_alignment(manager));
Nit: It may be useful to test that the window is actually partially off screen.

[varkha, 2014-01-16 23:34:14]

This is simpler, thanks!

https://codereview.chromium.org/141053002/diff/140001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/141053002/diff/140001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:526:
WindowResizer::kBoundsChange_Resizes)) {
On 2014/01/16 21:43:27, pkotwicz wrote:
> I wonder whether it is possible to have a new function
> DockedWindowLayoutManager::GetClosestAlignment(aura::Window* window) which
> returns DOCKED_ALIGNMENT_LEFT or DOCKED_ALIGNMENT_RIGHT based on which edge is
> closer. I think we should be able to use
> DockedWindowLayoutManager::GetClosestAlignment() everywhere that
> DockedWindowLayoutManager::GetAlignmentOfWindow() is used with the exception
of
> WorkspaceWindowResizer::SetDraggedWindowDocked().
> I think that doing this will allow you to remove the check for
> DragDetails::bounds_change
> 
> Nit: You can move the if statement above MaybeMinimizeChildrenExcept()

I will try to use the "closest to edge" logic when I am dealing with the
"TODO(oshima|varkha)" in OnWindowAddedToLayout (soon). I have simplified the
logic here to match what we have in OnWindowAddedToLayout - you are right
checking alignment_ is enough here - no need for IsAnyWindowDocked or checking
if this is a resize drag since we would keep alignment in StartDragging for
resizing drags and not clear it.

https://codereview.chromium.org/141053002/diff/140001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_resizer_unittest.cc (right):

https://codereview.chromium.org/141053002/diff/140001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_resizer_unittest.cc:650:
EXPECT_EQ(DOCKED_ALIGNMENT_NONE, docked_alignment(manager));
On 2014/01/16 21:43:27, pkotwicz wrote:
> Nit: It may be useful to test that the window is actually partially off
screen.

Done.

[pkotwicz, 2014-01-17 00:12:18]

LGTM

https://codereview.chromium.org/141053002/diff/200001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/141053002/diff/200001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:467: } else if (!IsAnyWindowDocked()
&&
The reason that we check |DragDetails::bounds_change| is not obvious to me. Can
you please add a comment?

https://codereview.chromium.org/141053002/diff/200001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:519: wm::WindowState* dragged_state
= wm::GetWindowState(dragged_window_);
Nit: Move initialization of |dragged_state| to line 522.

https://codereview.chromium.org/141053002/diff/200001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:529: // If this is the first window
that got docked by a move - update alignment.
Nit: No hyphen

https://codereview.chromium.org/141053002/diff/200001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:532: DCHECK(alignment_ !=
DOCKED_ALIGNMENT_NONE);
Nit: DCHECK_NE

[pkotwicz, 2014-01-17 00:50:45]

https://codereview.chromium.org/141053002/diff/200001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/141053002/diff/200001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:532: DCHECK(alignment_ !=
DOCKED_ALIGNMENT_NONE);
The DCHECK() can fail when FinishDragging() is called from OnWindowDestroying().
(That seems to be the source of unit test failures).

[varkha, 2014-01-17 01:05:28]

https://codereview.chromium.org/141053002/diff/200001/ash/wm/dock/docked_wind...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/141053002/diff/200001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:467: } else if (!IsAnyWindowDocked()
&&
On 2014/01/17 00:12:18, pkotwicz wrote:
> The reason that we check |DragDetails::bounds_change| is not obvious to me.
Can
> you please add a comment?

Done.

https://codereview.chromium.org/141053002/diff/200001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:519: wm::WindowState* dragged_state
= wm::GetWindowState(dragged_window_);
On 2014/01/17 00:12:18, pkotwicz wrote:
> Nit: Move initialization of |dragged_state| to line 522.

Done. No need to change this since it is only used once.

https://codereview.chromium.org/141053002/diff/200001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:529: // If this is the first window
that got docked by a move - update alignment.
On 2014/01/17 00:12:18, pkotwicz wrote:
> Nit: No hyphen

Done.

https://codereview.chromium.org/141053002/diff/200001/ash/wm/dock/docked_wind...
ash/wm/dock/docked_window_layout_manager.cc:532: DCHECK(alignment_ !=
DOCKED_ALIGNMENT_NONE);
On 2014/01/17 00:12:18, pkotwicz wrote:
> Nit: DCHECK_NE

Done. When window is destroyed while it is being dragged the action should be
set to NONE.

[commit-bot: I haz the power, 2014-01-17 01:14:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/141053002/350001

[commit-bot: I haz the power, 2014-01-17 06:25:25]

Change committed as 245435