Enables docked windows by default

Enables docked windows by default

BUG=227223
TEST=ash_unittests --gtest_filter=*DockedWindow*

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

[varkha, 2013-11-01 04:24:12]

flackr@, can you please take a look? Some tests required adjustments, more so to
enable sticky dragging than docked windows per se. I hope to land it after the
branch point for M33. Thanks!

[flackr, 2013-11-01 16:52:27]

Please move code changes not related to enabling by default into separate CL(s)
ideally with bugs describing what they're for. Also, I don't see why sticky
edges needs to be enabled given the mouse cursor position is what drives docking
now. That being said, if it does need to be enabled, consider also enabling that
by default rather than having it's value tied to the docking value.

https://codereview.chromium.org/55363004/diff/60001/ash/display/screen_positi...
File ash/display/screen_position_controller.cc (right):

https://codereview.chromium.org/55363004/diff/60001/ash/display/screen_positi...
ash/display/screen_position_controller.cc:206: }
This doesn't look related to simply enabling docked windows, separate bug?

https://codereview.chromium.org/55363004/diff/60001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/55363004/diff/60001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:382: if
(!switches::UseDockedWindows()) {
nit: no curlies { }

https://codereview.chromium.org/55363004/diff/60001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/55363004/diff/60001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_resizer.cc:149: dock_layout_->UndockDraggedWindow();
separate cl

https://codereview.chromium.org/55363004/diff/60001/ash/wm/drag_window_resize...
File ash/wm/drag_window_resizer_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/60001/ash/wm/drag_window_resize...
ash/wm/drag_window_resizer_unittest.cc:168:
window_->SetBoundsInScreen(gfx::Rect(0, 0, 100, 60),
Maybe add a comment w.r.t. why you need the bounds to be > x.

https://codereview.chromium.org/55363004/diff/60001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/55363004/diff/60001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:814:
switches::kAshEnableStickyEdges) || switches::UseDockedWindows()) {
The stick to edges being tied to docking seems unnecessary now that docking is
triggered by the mouse location rather than the window bounds.

[varkha, 2013-11-05 20:34:23]

I have moved the separate bugs to individual CLs, they are still merged in
(waiting for them to land before landing this).

https://codereview.chromium.org/55363004/diff/60001/ash/display/screen_positi...
File ash/display/screen_position_controller.cc (right):

https://codereview.chromium.org/55363004/diff/60001/ash/display/screen_positi...
ash/display/screen_position_controller.cc:206: }
On 2013/11/01 16:52:28, flackr wrote:
> This doesn't look related to simply enabling docked windows, separate bug?

Done. https://codereview.chromium.org/57963003/

https://codereview.chromium.org/55363004/diff/60001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/55363004/diff/60001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:382: if
(!switches::UseDockedWindows()) {
On 2013/11/01 16:52:28, flackr wrote:
> nit: no curlies { }

Done.

https://codereview.chromium.org/55363004/diff/60001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/55363004/diff/60001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_resizer.cc:149: dock_layout_->UndockDraggedWindow();
On 2013/11/01 16:52:28, flackr wrote:
> separate cl

Done. https://codereview.chromium.org/46893003/.

https://codereview.chromium.org/55363004/diff/60001/ash/wm/drag_window_resize...
File ash/wm/drag_window_resizer_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/60001/ash/wm/drag_window_resize...
ash/wm/drag_window_resizer_unittest.cc:168:
window_->SetBoundsInScreen(gfx::Rect(0, 0, 100, 60),
On 2013/11/01 16:52:28, flackr wrote:
> Maybe add a comment w.r.t. why you need the bounds to be > x.

Moved to https://codereview.chromium.org/57963003/.

https://codereview.chromium.org/55363004/diff/60001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/55363004/diff/60001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:814:
switches::kAshEnableStickyEdges) || switches::UseDockedWindows()) {
On 2013/11/01 16:52:28, flackr wrote:
> The stick to edges being tied to docking seems unnecessary now that docking is
> triggered by the mouse location rather than the window bounds.

Done.

[varkha, 2013-11-05 20:47:21]

https://codereview.chromium.org/55363004/diff/340001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer.cc (right):

https://codereview.chromium.org/55363004/diff/340001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.cc:126:
window_state->SetTrackedByWorkspace(false);
This change is from https://codereview.chromium.org/57963003/

https://codereview.chromium.org/55363004/diff/340001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer.cc:139:
window_state->SetTrackedByWorkspace(false);
This change is from https://codereview.chromium.org/57963003/

https://codereview.chromium.org/55363004/diff/340001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

https://codereview.chromium.org/55363004/diff/340001/ash/wm/window_resizer.cc...
ash/wm/window_resizer.cc:270: // calculate the target display after the drag.
This change is from https://codereview.chromium.org/57963003/ (it will disappear
in rebase).

[varkha, 2013-11-06 18:54:23]

I have rebased it to include my recent CLs, so there should be no changes not
directly related to enabling the flag. Please take a look.

[flackr, 2013-11-06 20:02:31]

https://codereview.chromium.org/55363004/diff/580001/ash/wm/toplevel_window_e...
File ash/wm/toplevel_window_event_handler_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/580001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler_unittest.cc:411:
target->SetBounds(gfx::Rect(700, 0, 100, 100));
I'm not sure I follow why enabling docking necessitates this.

https://codereview.chromium.org/55363004/diff/580001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/580001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer_unittest.cc:482:
scoped_ptr<WorkspaceWindowResizer> resizer(WorkspaceWindowResizer::Create(
You didn't change all of the WorkspaceWindowResizer::Create calls, intentional?

https://codereview.chromium.org/55363004/diff/580001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer_unittest.cc:556:
scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
While this is probably a worthwhile cleanup to these tests to have them
instantiate the real chain of resizers, it seems to me like none of the changes
in this unit test are necessary because of enabling docking by default. Can this
be deferred to a different CL?

https://codereview.chromium.org/55363004/diff/580001/ash/wm/workspace_control...
File ash/wm/workspace_controller_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/580001/ash/wm/workspace_control...
ash/wm/workspace_controller_unittest.cc:1235: // hierarchy changes affecting
specified window.
nit:affecting the specified window.

[varkha, 2013-11-06 21:19:52]

PTAL

https://codereview.chromium.org/55363004/diff/580001/ash/wm/toplevel_window_e...
File ash/wm/toplevel_window_event_handler_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/580001/ash/wm/toplevel_window_e...
ash/wm/toplevel_window_event_handler_unittest.cc:411:
target->SetBounds(gfx::Rect(700, 0, 100, 100));
On 2013/11/06 20:02:31, flackr wrote:
> I'm not sure I follow why enabling docking necessitates this.

This was also due to sticky behavior being enabled. I think this could be a good
change to avoid flakiness in future, but you are correct, it is not directly
related to docking. Removed for now.

https://codereview.chromium.org/55363004/diff/580001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/580001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer_unittest.cc:482:
scoped_ptr<WorkspaceWindowResizer> resizer(WorkspaceWindowResizer::Create(
On 2013/11/06 20:02:31, flackr wrote:
> You didn't change all of the WorkspaceWindowResizer::Create calls,
intentional?

Yes, This is testing the creation path from
MultiWindowResizeController::StartResize which resizes multiple adjacent
windows. This path is orthogonal to docking so we can use WorskpaceWindowResizer
directly here - similar to how it is used in real Chrome.

https://codereview.chromium.org/55363004/diff/580001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer_unittest.cc:556:
scoped_ptr<WindowResizer> resizer(CreateWindowResizer(
On 2013/11/06 20:02:31, flackr wrote:
> While this is probably a worthwhile cleanup to these tests to have them
> instantiate the real chain of resizers, it seems to me like none of the
changes
> in this unit test are necessary because of enabling docking by default. Can
this
> be deferred to a different CL?

Since WorkspaceWindowResizer manipulates the phantom controller and calls into
DockedWindowLayoutManager we need the same resizer chain as in real Chrome to
keep the state in DockedLayoutWindowManager correct. I have changed the tests to
use scoped_ptr<WindowResizer> where possible.

https://codereview.chromium.org/55363004/diff/580001/ash/wm/workspace_control...
File ash/wm/workspace_controller_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/580001/ash/wm/workspace_control...
ash/wm/workspace_controller_unittest.cc:1235: // hierarchy changes affecting
specified window.
On 2013/11/06 20:02:31, flackr wrote:
> nit:affecting the specified window.

Done.

[flackr, 2013-11-06 21:41:41]

https://codereview.chromium.org/55363004/diff/660001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer.h (right):

https://codereview.chromium.org/55363004/diff/660001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.h:138: bool ShouldStickToEdge(int
distance_from_edge, int sticky_size) const;
Making this externally visible doesn't seem to be necessary.

https://codereview.chromium.org/55363004/diff/660001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/660001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer_unittest.cc:653:
WorkspaceWindowResizer::instance_;
Suggestion, rather than reaching directly at instance_ here (and elsewhere), how
about creating a method in the test which instantiates a WindowResizer (i.e.
CreateWindowResizer) and at that time also fetches the current instance pointer
for accessing the WorkspaceWindowResizer?

[varkha, 2013-11-06 22:40:14]

https://codereview.chromium.org/55363004/diff/660001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer.h (right):

https://codereview.chromium.org/55363004/diff/660001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer.h:138: bool ShouldStickToEdge(int
distance_from_edge, int sticky_size) const;
On 2013/11/06 21:41:41, flackr wrote:
> Making this externally visible doesn't seem to be necessary.

This was to maintain snapping when kAshEnableStickyEdges is enabled, but only
for touch drag. I have reverted this part for now since kAshEnableStickyEdges is
not coupled with docking anymore.

https://codereview.chromium.org/55363004/diff/660001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/660001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer_unittest.cc:653:
WorkspaceWindowResizer::instance_;
On 2013/11/06 21:41:41, flackr wrote:
> Suggestion, rather than reaching directly at instance_ here (and elsewhere),
how
> about creating a method in the test which instantiates a WindowResizer (i.e.
> CreateWindowResizer) and at that time also fetches the current instance
pointer
> for accessing the WorkspaceWindowResizer?

Done.

[flackr, 2013-11-07 03:06:05]

lgtm with nits

https://codereview.chromium.org/55363004/diff/740001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/740001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer_unittest.cc:101: }; // namespace
nit: double space after } and no ;
i.e.
}  // namespace

https://codereview.chromium.org/55363004/diff/740001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer_unittest.cc:922: if
(IsStickyEdgesEnabled())
If/when sticky edges is enabled by default, these tests should just be in a
special subclass (i.e. WorkspaceWindowResizerSnapTest) which disables sticky
edges. For now I don't think anything is necessary given sticky edges isn't
enabled by default.

[varkha, 2013-11-07 03:23:19]

https://codereview.chromium.org/55363004/diff/740001/ash/wm/workspace/workspa...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/740001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer_unittest.cc:101: }; // namespace
On 2013/11/07 03:06:05, flackr wrote:
> nit: double space after } and no ;
> i.e.
> }  // namespace

Done.

https://codereview.chromium.org/55363004/diff/740001/ash/wm/workspace/workspa...
ash/wm/workspace/workspace_window_resizer_unittest.cc:922: if
(IsStickyEdgesEnabled())
On 2013/11/07 03:06:05, flackr wrote:
> If/when sticky edges is enabled by default, these tests should just be in a
> special subclass (i.e. WorkspaceWindowResizerSnapTest) which disables sticky
> edges. For now I don't think anything is necessary given sticky edges isn't
> enabled by default.

Done.

[varkha, 2013-11-07 03:24:02]

+sky for OWNERS review.

[sky, 2013-11-07 17:00:12]

LGTM

https://codereview.chromium.org/55363004/diff/850001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/850001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:217: gfx::Point
initial_location_in_parent() const {
const gfx::Point&

[varkha, 2013-11-07 17:35:15]

https://codereview.chromium.org/55363004/diff/850001/ash/wm/dock/docked_windo...
File ash/wm/dock/docked_window_resizer_unittest.cc (right):

https://codereview.chromium.org/55363004/diff/850001/ash/wm/dock/docked_windo...
ash/wm/dock/docked_window_resizer_unittest.cc:217: gfx::Point
initial_location_in_parent() const {
On 2013/11/07 17:00:12, sky wrote:
> const gfx::Point&

Done.

[commit-bot: I haz the power, 2013-11-07 17:58:07]

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

[commit-bot: I haz the power, 2013-11-08 01:04:32]

Change committed as 233738