CrOS Tablet Window management - Split Screen part I

CrOS Tablet Window management - Split Screen part I

Implement the split view. The behavior is similar to what we have on
Android today. See PRD: go/cros-splitview-prd.

Changes in this CL:
1. Introduce a command switch (--enable-tablet-splitview) to enable
split view in tablet mode (maximized mode).
2. The split view mode can be initiated by dragging the window in
overview mode to the side of the screen
3. After one window is snapped to one side of the screen, all the other
overview windows will display in the other side of the screen, allowing
the user to select another window to snap. When two windows are snapped
to both sides of the screen, the overview mode will be ended.
4. The first snapped window will remain snapped until the user
explicitly exit the split view mode. All the other windows (incluing
newly created window) will be open in the other side of the screen.
Clicking/Tapping on the overview button also open the overview windows
in the other side of the screen.

Not covered in this CL:
1. The highlighted region showing where split will occur is not 
   implemented in this CL.
2. The split divider is not implemented in this CL.

BUG=725683

[xdai1, 2017-06-07 22:56:32]

The CQ bit was checked by xdai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 22:57:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xdai1, 2017-06-07 23:02:36]

Description was changed from

==========
CrOS Tablet Window management - Split Screen part I

Implement the split view. The behavior is similar to what we have on
Android today. See PRD: go/cros-splitview-prd.

Changes in this CL:
1. Introduce a command switch (--enable-tablet-splitview) to enable
split view in tablet mode (maximized mode).
2. The split view mode can be initiated by dragging the window in
overview mode to the side of the screen
3. After one window is snapped to one side of the screen, all the other
overview windows will display in the other side of the screen, allowing
the user to select another window to snap. When two windows are snapped
to both sides of the screen, the overview mode will be ended.
4. The first snapped window will remain snapped until the user
explicitly exit the split view mode. All the other windows (incluing
newly created window) will be open in the other side of the screen.
Clicking/Tapping on the overview button also open the overview windows
in the other side of the screen.

Not covered in this CL:
1. The split divider is not implemented in this CL.

BUG=725683
==========

to

==========
CrOS Tablet Window management - Split Screen part I

Implement the split view. The behavior is similar to what we have on
Android today. See PRD: go/cros-splitview-prd.

Changes in this CL:
1. Introduce a command switch (--enable-tablet-splitview) to enable
split view in tablet mode (maximized mode).
2. The split view mode can be initiated by dragging the window in
overview mode to the side of the screen
3. After one window is snapped to one side of the screen, all the other
overview windows will display in the other side of the screen, allowing
the user to select another window to snap. When two windows are snapped
to both sides of the screen, the overview mode will be ended.
4. The first snapped window will remain snapped until the user
explicitly exit the split view mode. All the other windows (incluing
newly created window) will be open in the other side of the screen.
Clicking/Tapping on the overview button also open the overview windows
in the other side of the screen.

Not covered in this CL:
1. The highlighted region showing where split will occur is not 
   implemented in this CL.
2. The split divider is not implemented in this CL.

BUG=725683
==========

[xdai1, 2017-06-07 23:03:48]

xdai@chromium.org changed reviewers:
+ oshima@chromium.org

[xdai1, 2017-06-07 23:03:48]

oshima@, could you help review this CL please? Thanks!

[oshima, 2017-06-12 21:32:31]

On 2017/06/07 23:03:48, xdai1 wrote:
> oshima@, could you help review this CL please? Thanks!

Sorry for delay. I'll review it today.

[oshima, 2017-06-14 00:19:28]

will send the rest a bit later.

https://codereview.chromium.org/2918403006/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/shell.cc#newcode731
ash/shell.cc:731: split_view_controller_.reset();
can you keep the reverse order of init process?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_window_state.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_state.cc:76: if
(state_object->CanSnap() && SplitViewController::ShouldAllowSplitView() &&
do you need CanSnap check? I believe it shouldn't be snapped if CanSnap returns
false.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_state.cc:78: return
Shell::Get()->split_view_controller()->GetLeftWindowBoundsInParent(
You may want to call it first, second as you most likely need to support
splitting to top and bottom.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_state.cc:330: (target_state ==
wm::WINDOW_STATE_TYPE_LEFT_SNAPPED)
it's probably simple to have two ifs. (just a guess, so you may leave it if you
think this is simpler)

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
File ash/wm/overview/overview_window_drag_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.cc:50: DCHECK(item_ == item);
DCHECK_EQ

If it should be always same, why we're passing it?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.cc:64:
previous_event_location_.x());
Rect::Offset()

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.cc:132: new
PhantomWindowController(target_window));
base::MakeUnique

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
File ash/wm/overview/overview_window_drag_controller.h (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.h:23: // the position of the
corresponding window item while dragging and shows/hides
update the position using transform?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.h:27: enum SnapType { SNAP_LEFT,
SNAP_RIGHT, SNAP_NONE };
does this have to be public?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.h:29:
OverviewWindowDragController(WindowSelector* window_selector);
explicit

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/scoped_...
File ash/wm/overview/scoped_transform_overview_window.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/scoped_...
ash/wm/overview/scoped_transform_overview_window.cc:468: }
nuke {}

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_grid.cc:529: return
item->Contains(selector_item->GetWindow());
why this isn't simple == ?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_grid.h (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_grid.h:107: void SetBoundsInScreen(const gfx::Rect
bounds);
SetTotalBoundsInScreen/SetTotalGridBoundsInScreen?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:154:
Shell::Get()->split_view_controller()->GetLeftWindowBoundsInScreen(
it's probably better to have

GetSnappedWindowBoundsInScreen(root_window, snap_state);

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:509: window_drag_controller_.reset(new
OverviewWindowDragController(this));
MakeUnique

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:515: DCHECK(window_drag_controller_.get());
it'll crash immediately, so you don't need this.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:520: DCHECK(window_drag_controller_.get());
same here

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:658: return;
not clear to me why you need this. Can you elaborate?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:723: gfx::Rect bounds_in_screen =
nit: const

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:730: gfx::Rect bounds_in_screen =
const

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:735: DCHECK(state ==
SplitViewController::NOSNAP);
DCHECK_EQ

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector_controller.cc:75: DCHECK(iter != windows.end());
DCHECK_NE

[oshima, 2017-06-14 05:45:47]

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector_controller.cc:70: // occupy the other side of
the screen in overview mode.
can you also mention where the default window will be laid out?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector_item.cc:104: static const float kPreDragScale =
0.04f;
The scale will be applied during drag, correct? How about DragWindowScale?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector_item.cc:846:
widget_window->parent()->StackChildBelow(GetWindow(), widget_window);
This will not work if there is always on top window. You don't have to fix this
soon, but please file a bug + add TODO.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
File ash/wm/splitview/split_view_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
ash/wm/splitview/split_view_controller.cc:205: EndSplitView();
you probably should skip restroing the window being destroyed?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
File ash/wm/splitview/split_view_controller.h (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
ash/wm/splitview/split_view_controller.h:83: int separator_position() { return
separator_position_; }
const

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
ash/wm/splitview/split_view_controller.h:84: State state() { return state_; }
const

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/workspace/backdr...
File ash/wm/workspace/backdrop_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/workspace/backdr...
ash/wm/workspace/backdrop_controller.cc:157: force_hidden_ = false;
what happen if I close the snapped window while the other side is in overview
mode?

[xdai1, 2017-06-15 22:11:42]

oshima@, I've addressed your comments. Please take another look, thanks for the
review!

https://codereview.chromium.org/2918403006/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/shell.cc#newcode731
ash/shell.cc:731: split_view_controller_.reset();
On 2017/06/14 00:19:26, oshima wrote:
> can you keep the reverse order of init process?

Done. However, in line 639 in this patch the comment says "// Please keep in
same order as in Init() because it's easy to miss one", and I noticed that most
of the variables were not reset in the same order or reverse order they
initialized. Do we need to remove or modify the comment?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_window_state.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_state.cc:76: if
(state_object->CanSnap() && SplitViewController::ShouldAllowSplitView() &&
On 2017/06/14 00:19:26, oshima wrote:
> do you need CanSnap check? I believe it shouldn't be snapped if CanSnap
returns
> false.

Yes. Theoretically the state type won't be wm::WINDOW_STATE_TYPE_LEFT_SNAPPED or
wm::WINDOW_STATE_TYPE_RIGHT_SNAPPED if CanSnap() is false.

Do you mind I keep this check? Because I don't want
SplitViewController::ShouldAllowSplitView() to be called in multiple places in
this file. (currently it's moved to CanSnap() function).

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_state.cc:78: return
Shell::Get()->split_view_controller()->GetLeftWindowBoundsInParent(
On 2017/06/14 00:19:26, oshima wrote:
> You may want to call it first, second as you most likely need to support
> splitting to top and bottom.

Do you mean put the two if-clause at top of this function? If so, Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_state.cc:330: (target_state ==
wm::WINDOW_STATE_TYPE_LEFT_SNAPPED)
On 2017/06/14 00:19:26, oshima wrote:
> it's probably simple to have two ifs. (just a guess, so you may leave it if
you
> think this is simpler)

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
File ash/wm/overview/overview_window_drag_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.cc:50: DCHECK(item_ == item);
On 2017/06/14 00:19:26, oshima wrote:
> DCHECK_EQ
> 
> If it should be always same, why we're passing it?

Theoretically it should be always same. But I was hoping if something goes wrong
(for example, the mouse was frozen?), we could get a DCHECK crash stack here.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.cc:64:
previous_event_location_.x());
On 2017/06/14 00:19:26, oshima wrote:
> Rect::Offset()

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.cc:132: new
PhantomWindowController(target_window));
On 2017/06/14 00:19:27, oshima wrote:
> base::MakeUnique

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
File ash/wm/overview/overview_window_drag_controller.h (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.h:23: // the position of the
corresponding window item while dragging and shows/hides
On 2017/06/14 00:19:27, oshima wrote:
> update the position using transform?

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.h:27: enum SnapType { SNAP_LEFT,
SNAP_RIGHT, SNAP_NONE };
On 2017/06/14 00:19:27, oshima wrote:
> does this have to be public?

You're right. Not necessarily. Moved to private.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.h:29:
OverviewWindowDragController(WindowSelector* window_selector);
On 2017/06/14 00:19:27, oshima wrote:
> explicit

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/scoped_...
File ash/wm/overview/scoped_transform_overview_window.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/scoped_...
ash/wm/overview/scoped_transform_overview_window.cc:468: }
On 2017/06/14 00:19:27, oshima wrote:
> nuke {}

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_grid.cc:529: return
item->Contains(selector_item->GetWindow());
On 2017/06/14 00:19:27, oshima wrote:
> why this isn't simple == ?

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_grid.h (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_grid.h:107: void SetBoundsInScreen(const gfx::Rect
bounds);
On 2017/06/14 00:19:27, oshima wrote:
> SetTotalBoundsInScreen/SetTotalGridBoundsInScreen?

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:154:
Shell::Get()->split_view_controller()->GetLeftWindowBoundsInScreen(
On 2017/06/14 00:19:27, oshima wrote:
> it's probably better to have
> 
> GetSnappedWindowBoundsInScreen(root_window, snap_state);

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:509: window_drag_controller_.reset(new
OverviewWindowDragController(this));
On 2017/06/14 00:19:27, oshima wrote:
> MakeUnique

reset is better here since |window_drag_controller_| might not be nullptr?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:515: DCHECK(window_drag_controller_.get());
On 2017/06/14 00:19:27, oshima wrote:
> it'll crash immediately, so you don't need this.

I don't understand. Why will it crash immediately?

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:520: DCHECK(window_drag_controller_.get());
On 2017/06/14 00:19:27, oshima wrote:
> same here

ditto

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:658: return;
On 2017/06/14 00:19:27, oshima wrote:
> not clear to me why you need this. Can you elaborate?

The window activation might be caused by window snap. In this case we should not
cancel overview mode. 

Only when two windows are both snapped to both sides of the screen, the overview
mode should be cancelled, and this logic is addressed in line 718-719 in this
patch.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:723: gfx::Rect bounds_in_screen =
On 2017/06/14 00:19:27, oshima wrote:
> nit: const

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:730: gfx::Rect bounds_in_screen =
On 2017/06/14 00:19:27, oshima wrote:
> const

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector.cc:735: DCHECK(state ==
SplitViewController::NOSNAP);
On 2017/06/14 00:19:27, oshima wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector_controller.cc:70: // occupy the other side of
the screen in overview mode.
On 2017/06/14 05:45:47, oshima wrote:
> can you also mention where the default window will be laid out?

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector_controller.cc:75: DCHECK(iter != windows.end());
On 2017/06/14 00:19:27, oshima wrote:
> DCHECK_NE

DCHECK_NE could not be used to compare two iterators.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector_item.cc:104: static const float kPreDragScale =
0.04f;
On 2017/06/14 05:45:47, oshima wrote:
> The scale will be applied during drag, correct? How about DragWindowScale?

Yes. It will be applied during drag. Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_selector_item.cc:846:
widget_window->parent()->StackChildBelow(GetWindow(), widget_window);
On 2017/06/14 05:45:47, oshima wrote:
> This will not work if there is always on top window. You don't have to fix
this
> soon, but please file a bug + add TODO.

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
File ash/wm/splitview/split_view_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
ash/wm/splitview/split_view_controller.cc:205: EndSplitView();
On 2017/06/14 05:45:47, oshima wrote:
> you probably should skip restroing the window being destroyed?

Currently StopObservingAndRestore() function only removes SplitViewController as
an observer of Window and WindowState. So if the window is being destroyed, we
still should do these.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
File ash/wm/splitview/split_view_controller.h (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
ash/wm/splitview/split_view_controller.h:83: int separator_position() { return
separator_position_; }
On 2017/06/14 05:45:47, oshima wrote:
> const

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
ash/wm/splitview/split_view_controller.h:84: State state() { return state_; }
On 2017/06/14 05:45:47, oshima wrote:
> const

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/workspace/backdr...
File ash/wm/workspace/backdrop_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/workspace/backdr...
ash/wm/workspace/backdrop_controller.cc:157: force_hidden_ = false;
On 2017/06/14 05:45:47, oshima wrote:
> what happen if I close the snapped window while the other side is in overview
> mode?

I didn't handle it in the previous patch. Handled it in the new patch. If one of
the snapped window was closed while the other side is in overview mode, end
split view mode and overview mode at the same time.

[xdai1, 2017-06-15 22:11:47]

The CQ bit was checked by xdai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 22:12:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 23:01:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 23:01:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xdai1, 2017-06-15 23:15:52]

The CQ bit was checked by xdai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 23:16:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 23:24:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 23:24:40]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[oshima, 2017-06-16 01:20:55]

oshima@chromium.org changed reviewers:
+ varkha@chromium.org

[oshima, 2017-06-16 01:20:57]

https://codereview.chromium.org/2918403006/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/shell.cc#newcode731
ash/shell.cc:731: split_view_controller_.reset();
On 2017/06/15 22:11:40, xdai1 wrote:
> On 2017/06/14 00:19:26, oshima wrote:
> > can you keep the reverse order of init process?
> 
> Done. However, in line 639 in this patch the comment says "// Please keep in
> same order as in Init() because it's easy to miss one", and I noticed that
most
> of the variables were not reset in the same order or reverse order they
> initialized. Do we need to remove or modify the comment?

That's added after we had issues with mixed order. Certain controllers are
placed at current point due to complicated dependency, but we should try to keep
the reversed order in general.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_window_state.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_state.cc:76: if
(state_object->CanSnap() && SplitViewController::ShouldAllowSplitView() &&
On 2017/06/15 22:11:41, xdai1 wrote:
> On 2017/06/14 00:19:26, oshima wrote:
> > do you need CanSnap check? I believe it shouldn't be snapped if CanSnap
> returns
> > false.
> 
> Yes. Theoretically the state type won't be wm::WINDOW_STATE_TYPE_LEFT_SNAPPED
or
> wm::WINDOW_STATE_TYPE_RIGHT_SNAPPED if CanSnap() is false.
> 
> Do you mind I keep this check? Because I don't want
> SplitViewController::ShouldAllowSplitView() to be called in multiple places in
> this file. (currently it's moved to CanSnap() function).

Please use DCHECK if that condition shouldn't happen but want to catch it if it
ever happens.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_state.cc:78: return
Shell::Get()->split_view_controller()->GetLeftWindowBoundsInParent(
On 2017/06/15 22:11:41, xdai1 wrote:
> On 2017/06/14 00:19:26, oshima wrote:
> > You may want to call it first, second as you most likely need to support
> > splitting to top and bottom.
> 
> Do you mean put the two if-clause at top of this function? If so, Done.

I was going to remove this comment, sorry. You may put it back here or top is
fine too.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
File ash/wm/overview/overview_window_drag_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.cc:50: DCHECK(item_ == item);
On 2017/06/15 22:11:41, xdai1 wrote:
> On 2017/06/14 00:19:26, oshima wrote:
> > DCHECK_EQ
> > 
> > If it should be always same, why we're passing it?
> 
> Theoretically it should be always same. But I was hoping if something goes
wrong
> (for example, the mouse was frozen?), we could get a DCHECK crash stack here. 

If that's the case, can you move the DCHECK to caller side? You'll need
accessor, but it's better than adding an argument used only for DCHECK.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_grid.cc:529: return
item->Contains(selector_item->GetWindow());
Actually, we should double check with +varkha@.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
File ash/wm/splitview/split_view_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
ash/wm/splitview/split_view_controller.cc:205: EndSplitView();
On 2017/06/15 22:11:42, xdai1 wrote:
> On 2017/06/14 05:45:47, oshima wrote:
> > you probably should skip restroing the window being destroyed?
> 
> Currently StopObservingAndRestore() function only removes SplitViewController
as
> an observer of Window and WindowState. So if the window is being destroyed, we
> still should do these. 

Sorry, I misunderstood what Restore means here.

https://codereview.chromium.org/2918403006/diff/100001/ash/wm/overview/overvi...
File ash/wm/overview/overview_window_drag_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/100001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:130: target_window,
SplitViewController::RIGHT_SNAPPED);
snap_type == LEFT ? SplitViewController::LEFT_SNAPPED : ...

If'd be nice if you can use the same enum though.

[xdai1, 2017-06-16 18:48:52]

Patchset #7 (id:120001) has been deleted

[xdai1, 2017-06-16 18:49:52]

oshima@, I've addressed your comments. Please take another look, thanks for your
review!

https://codereview.chromium.org/2918403006/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/shell.cc#newcode731
ash/shell.cc:731: split_view_controller_.reset();
On 2017/06/16 01:20:56, oshima wrote:
> On 2017/06/15 22:11:40, xdai1 wrote:
> > On 2017/06/14 00:19:26, oshima wrote:
> > > can you keep the reverse order of init process?
> > 
> > Done. However, in line 639 in this patch the comment says "// Please keep in
> > same order as in Init() because it's easy to miss one", and I noticed that
> most
> > of the variables were not reset in the same order or reverse order they
> > initialized. Do we need to remove or modify the comment?
> 
> That's added after we had issues with mixed order. Certain controllers are
> placed at current point due to complicated dependency, but we should try to
keep
> the reversed order in general.

I see. Thanks!

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_window_state.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_state.cc:76: if
(state_object->CanSnap() && SplitViewController::ShouldAllowSplitView() &&
On 2017/06/16 01:20:56, oshima wrote:
> On 2017/06/15 22:11:41, xdai1 wrote:
> > On 2017/06/14 00:19:26, oshima wrote:
> > > do you need CanSnap check? I believe it shouldn't be snapped if CanSnap
> > returns
> > > false.
> > 
> > Yes. Theoretically the state type won't be
wm::WINDOW_STATE_TYPE_LEFT_SNAPPED
> or
> > wm::WINDOW_STATE_TYPE_RIGHT_SNAPPED if CanSnap() is false.
> > 
> > Do you mind I keep this check? Because I don't want
> > SplitViewController::ShouldAllowSplitView() to be called in multiple places
in
> > this file. (currently it's moved to CanSnap() function).
> 
> Please use DCHECK if that condition shouldn't happen but want to catch it if
it
> ever happens.

Moved the CanSnap() to DCHECK.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_state.cc:78: return
Shell::Get()->split_view_controller()->GetLeftWindowBoundsInParent(
On 2017/06/16 01:20:56, oshima wrote:
> On 2017/06/15 22:11:41, xdai1 wrote:
> > On 2017/06/14 00:19:26, oshima wrote:
> > > You may want to call it first, second as you most likely need to support
> > > splitting to top and bottom.
> > 
> > Do you mean put the two if-clause at top of this function? If so, Done.
> 
> I was going to remove this comment, sorry. You may put it back here or top is
> fine too.

Put it back here.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
File ash/wm/overview/overview_window_drag_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/overvie...
ash/wm/overview/overview_window_drag_controller.cc:50: DCHECK(item_ == item);
On 2017/06/16 01:20:57, oshima wrote:
> On 2017/06/15 22:11:41, xdai1 wrote:
> > On 2017/06/14 00:19:26, oshima wrote:
> > > DCHECK_EQ
> > > 
> > > If it should be always same, why we're passing it?
> > 
> > Theoretically it should be always same. But I was hoping if something goes
> wrong
> > (for example, the mouse was frozen?), we could get a DCHECK crash stack
here. 
> 
> If that's the case, can you move the DCHECK to caller side? You'll need
> accessor, but it's better than adding an argument used only for DCHECK.

Done.

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
File ash/wm/splitview/split_view_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/splitview/split_...
ash/wm/splitview/split_view_controller.cc:205: EndSplitView();
On 2017/06/16 01:20:57, oshima wrote:
> On 2017/06/15 22:11:42, xdai1 wrote:
> > On 2017/06/14 05:45:47, oshima wrote:
> > > you probably should skip restroing the window being destroyed?
> > 
> > Currently StopObservingAndRestore() function only removes
SplitViewController
> as
> > an observer of Window and WindowState. So if the window is being destroyed,
we
> > still should do these. 
> 
> Sorry, I misunderstood what Restore means here.

I rename the function to StopObserving() to better describe its function usage.

https://codereview.chromium.org/2918403006/diff/100001/ash/wm/overview/overvi...
File ash/wm/overview/overview_window_drag_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/100001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:130: target_window,
SplitViewController::RIGHT_SNAPPED);
On 2017/06/16 01:20:57, oshima wrote:
> snap_type == LEFT ? SplitViewController::LEFT_SNAPPED : ...
> 
> If'd be nice if you can use the same enum though.

Done. I think use two enum might be better here since
OverviewWindowDragController::SnapType describes the snap type while
SplitViewController::State describes the snap state. For a snap type, it's
impossible to have a BOTH_SNAPPED type. What do you think?

[oshima, 2017-06-16 19:39:15]

lgtm

https://codereview.chromium.org/2918403006/diff/140001/ash/wm/overview/window...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/2918403006/diff/140001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:145: // opposite side of the default snap
window.
left is the default correct?

https://codereview.chromium.org/2918403006/diff/140001/ash/wm/overview/window...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/2918403006/diff/140001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:176: }
nit: nuke {}

[xdai1, 2017-06-16 20:01:46]

oshima@, I've addressed your comments. Thanks for the review!

https://codereview.chromium.org/2918403006/diff/140001/ash/wm/overview/window...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/2918403006/diff/140001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:145: // opposite side of the default snap
window.
On 2017/06/16 19:39:15, oshima wrote:
> left is the default correct?

It depends. The default position is decided by the first snapped window. If the
first window was snapped to left, then left is the default position. Otherwise,
right is the default position.

https://codereview.chromium.org/2918403006/diff/140001/ash/wm/overview/window...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/2918403006/diff/140001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:176: }
On 2017/06/16 19:39:15, oshima wrote:
> nit: nuke {}

Done.

[xdai1, 2017-06-16 22:11:08]

The CQ bit was checked by xdai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 22:11:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 23:47:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-16 23:47:44]

Dry run: This issue passed the CQ dry run.

[xdai1, 2017-06-20 22:10:00]

varkha@, could you help answer the following question? Thanks!

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_grid.cc:529: return
item->Contains(selector_item->GetWindow());
On 2017/06/16 01:20:57, oshima wrote:
> Actually, we should double check with +varkha@. 

varkha@, as suggested by oshima@, could you help take a look at this function to
see if it's fine to modify to use == for the pointer comparison? See the latest
patch for the modification.

[varkha, 2017-06-21 01:51:54]

I've only looks at the overview mode files so far. Looks really interesting!

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/2918403006/diff/40001/ash/wm/overview/window_...
ash/wm/overview/window_grid.cc:529: return
item->Contains(selector_item->GetWindow());
On 2017/06/20 22:10:00, xdai1 wrote:
> On 2017/06/16 01:20:57, oshima wrote:
> > Actually, we should double check with +varkha@. 
> 
> varkha@, as suggested by oshima@, could you help take a look at this function
to
> see if it's fine to modify to use == for the pointer comparison? See the
latest
> patch for the modification.

I am not sure why this has Contains in the first place. Did you have a reason
for it? I think looking at how this gets invoked the selector_item you are
passing is the top level window and so you should not need Contains.
If this is correct then you could also merge the code in OnWindowDestroying by
changing the parameter to be an aura::Window*, passing
selector_item->GetWindow() instead of the selector_item to RemoveItem() and
calling RemoveItem() from OnWindowDestroying() to collapse what seems to be
largely the same code.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
File ash/wm/overview/overview_animation_type.h (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_animation_type.h:31: // Used to animate scaling up of a
window that is about the get dragged while
nit: ... about to get dragged ...

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
File ash/wm/overview/overview_window_drag_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:58: // Updates the dragged
|item_|'s board accordingly.
nit: Updates -> Update
nit: border? bounds?

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:65: // Then updates the
phantom window if applicable.
nit: updates -> update or consider removing this comment, the method name is
chosen well enough!

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:82: else if (split_state ==
SplitViewController::RIGHT_SNAPPED)
nit: maybe just else and DCHECK_EQ(split_state,
SplitViewController::RIGHT_SNAPPED)
Or alternatively calculate snap type from split_state_ and did_move_ and merge
the paths to SnapLeft / SnapRight.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:88:
window_selector_->PositionWindows(true /* animate */);
Could you make sure that this scenario does not change the stacking order? See
my comment in WindowSelectorItem::EndDrag(). I suggest slowing all animations to
make sure that the windows return from overview in the same stacking order as
they were when entering overview.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:120:
Shell::Get()->split_view_controller()->GetSnappedWindowBoundsInScreen(
Can you use |split_view_controller_| here?

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:136:
area.Inset(kScreenEdgeInsetForDrag, 0, kScreenEdgeInsetForDrag, 0);
I think it would be much easier to snap when dragging just past the divider as
opposed to requiring a drag almost all the way to the edge - at least when there
is a divider already. This can be a separate CL of course.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:136:
area.Inset(kScreenEdgeInsetForDrag, 0, kScreenEdgeInsetForDrag, 0);
nit: area.Inset(kScreenEdgeInsetForDrag, 0) should do the same thing.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:156: void
OverviewWindowDragController::SnapToRight() {
Seems like those 2 methods could be merged since they are 80% similar.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
File ash/wm/overview/overview_window_drag_controller.h (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.h:57: gfx::Point
initial_event_location_;
I suspect you could use previous_event_location_ for the same purpose with no
loss of clarity.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_grid.cc:567: PositionWindows(true);
nit: true /* animate */

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_grid.h (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_grid.h:93: // Remove window selector iten |selector_item|
from the grid.
nit: How about
// Removes |selector_item| from the grid.
Simpler and the type can be inferred from the method parameter.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_grid.h:107: // Set bounds for the window grid and
position all windows in the grid.
nits:
 Set -> Sets
 position -> positions

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_grid.h:108: void SetTotalBoundsInScreen(const gfx::Rect
bounds);
nit: Consider SetBoundsAndUpdatePositions(const gfx::Rect& bounds_in_screen)
since "total" qualifier is used in .cc file for just the portion of the screen
that accommodates the window items. This terminology goes back to the UX design
mocks so maybe I am just partial but it is more consistent since you are really
just setting |bounds_| and then calculating a local |total_bounds| variable in
code.
Note const& as well.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_grid.h:201: // The bound in screen coordinates for the
window grid.
nit: // This WindowGrid's total bounds in screen coordinates.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:28: #include
"ash/wm/splitview/split_view_controller.h"
nit: Not necessary since you have to bring it in this .cc file's header file?

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:146: gfx::Rect
GetGridsBoundsInScreen(aura::Window* root_window) {
nit: I have a mild preference for simpler GetGridBoundsInScreen() since you are
only getting bounds for a single grid and cannot use "'" in the name.
nit: Caching Shell::Get()->split_view_controller() in a local variable could
simplify this a bit since you use it in every branch.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:146: gfx::Rect
GetGridsBoundsInScreen(aura::Window* root_window) {
Could root_window be const* ? That would help remove the const_cast in line 588.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:165: gfx::Rect total_bounds =
GetGridsBoundsInScreen(root_window);
nit: const.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:169: total_bounds.x(),
nit: x + offset rather than offset + x to make it consistent with how y is
spelled out.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:356: // that will call into this function
again.
Subtle! Thanks for commenting this!

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:653: // snapping window to one side of the
screen.
<random_thought> This just made me think of scenario where you start dragging
with touch and then use the mouse while in gesture drag or vice versa. So just
make sure that this scenario doesn't cause some unexpected activation or other
side effects. </random_thought>

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_selector.h (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.h:130: // Position all of the windows in the
overview.
nit: Position -> Positions

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_controller.cc:69: // snapped window occupy one
side of the screen, while the other windows
nit: occupies

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_controller.cc:71: // position is the position
where the window was firstly snapped. See
nit: first

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:119: // visible part of an overview
window. Note the drag events are only handled in
nit: "Note:" or "Note that"

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:136: // views::CustomButton overrides:
nit: "// views::CustomButton:" for consistency.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:192: return
static_cast<WindowSelectorItem*>(listener_);
Since this is a local class, I think it would look better if we simply store a
pointer to WindowSlsectorItem instead of ButtonListener and rename the variable
to selector_item_. This way you would not need the cast and using it as a
ButtonListener should still work.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:603: if
(!SplitViewController::ShouldAllowSplitView()) {
nit: no {}

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:840: SetBounds(scaled_bounds,
animation_type);
Don't you need to revert this somewhere?

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:854:
widget_window->parent()->StackChildAtBottom(widget_window);
I don't think we should be changing the order relative to what it was at the
beginning of the drag. This seems to be changing that order so entering
overview, dragging an item and releasing could be changing state. Not sure if
the window would also get activated as a result of a drag but this will probably
change the stacking of windows while they are animated back to their original
positions.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.h:123: // Handle the mouse/gesture event on
the window selector item.
nit: How about "// Handle mouse/gesture events and facilitate dragging the
item." ?

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.h:174: // Called before dragging. Scale up
the window a little bit to indicate its
nit: Scale -> Scales

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.h:177: void PrepareDrag();
How about StartDrag() for symmetry?

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.h:179: // Called after dragging. Stack the
window at the bottom of the Z order.
nit: Stack -> Stacks.

[xdai1, 2017-06-22 21:46:34]

varkha@, I've addressed your comments. Please take another look, thanks for the
review!

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
File ash/wm/overview/overview_animation_type.h (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_animation_type.h:31: // Used to animate scaling up of a
window that is about the get dragged while
On 2017/06/21 01:51:52, varkha wrote:
> nit: ... about to get dragged ...

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
File ash/wm/overview/overview_window_drag_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:58: // Updates the dragged
|item_|'s board accordingly.
On 2017/06/21 01:51:52, varkha wrote:
> nit: Updates -> Update
> nit: border? bounds?

oops. Done

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:65: // Then updates the
phantom window if applicable.
On 2017/06/21 01:51:52, varkha wrote:
> nit: updates -> update or consider removing this comment, the method name is
> chosen well enough!

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:82: else if (split_state ==
SplitViewController::RIGHT_SNAPPED)
On 2017/06/21 01:51:52, varkha wrote:
> nit: maybe just else and DCHECK_EQ(split_state,
> SplitViewController::RIGHT_SNAPPED)
> Or alternatively calculate snap type from split_state_ and did_move_ and merge
> the paths to SnapLeft / SnapRight.

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:88:
window_selector_->PositionWindows(true /* animate */);
On 2017/06/21 01:51:52, varkha wrote:
> Could you make sure that this scenario does not change the stacking order? See
> my comment in WindowSelectorItem::EndDrag(). I suggest slowing all animations
to
> make sure that the windows return from overview in the same stacking order as
> they were when entering overview.

Comfirmed it does change the stacking order. I modifed EndDrag() function to
address this issue.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:120:
Shell::Get()->split_view_controller()->GetSnappedWindowBoundsInScreen(
On 2017/06/21 01:51:52, varkha wrote:
> Can you use |split_view_controller_| here?

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:136:
area.Inset(kScreenEdgeInsetForDrag, 0, kScreenEdgeInsetForDrag, 0);
On 2017/06/21 01:51:52, varkha wrote:
> nit: area.Inset(kScreenEdgeInsetForDrag, 0) should do the same thing.

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:136:
area.Inset(kScreenEdgeInsetForDrag, 0, kScreenEdgeInsetForDrag, 0);
On 2017/06/21 01:51:52, varkha wrote:
> I think it would be much easier to snap when dragging just past the divider as
> opposed to requiring a drag almost all the way to the edge - at least when
there
> is a divider already. This can be a separate CL of course.

For the first window, it requires the user to drag all the way to the edge, but
after that, user can simply click one of the overview window items to select the
other window to snap. But yes, if it's necessary to snap the window after
padding the divider, I can address it in a separate CL.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.cc:156: void
OverviewWindowDragController::SnapToRight() {
On 2017/06/21 01:51:52, varkha wrote:
> Seems like those 2 methods could be merged since they are 80% similar.

Merged them together to SnapWindow(..) function.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
File ash/wm/overview/overview_window_drag_controller.h (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/overvi...
ash/wm/overview/overview_window_drag_controller.h:57: gfx::Point
initial_event_location_;
On 2017/06/21 01:51:52, varkha wrote:
> I suspect you could use previous_event_location_ for the same purpose with no
> loss of clarity.

You're right. Removed |initial_event_location_|. Thanks!

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_grid.cc (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_grid.cc:567: PositionWindows(true);
On 2017/06/21 01:51:52, varkha wrote:
> nit: true /* animate */

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_grid.h (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_grid.h:93: // Remove window selector iten |selector_item|
from the grid.
On 2017/06/21 01:51:53, varkha wrote:
> nit: How about
> // Removes |selector_item| from the grid.
> Simpler and the type can be inferred from the method parameter.

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_grid.h:107: // Set bounds for the window grid and
position all windows in the grid.
On 2017/06/21 01:51:52, varkha wrote:
> nits:
>  Set -> Sets
>  position -> positions

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_grid.h:108: void SetTotalBoundsInScreen(const gfx::Rect
bounds);
On 2017/06/21 01:51:53, varkha wrote:
> nit: Consider SetBoundsAndUpdatePositions(const gfx::Rect& bounds_in_screen)
> since "total" qualifier is used in .cc file for just the portion of the screen
> that accommodates the window items. This terminology goes back to the UX
design
> mocks so maybe I am just partial but it is more consistent since you are
really
> just setting |bounds_| and then calculating a local |total_bounds| variable in
> code.
> Note const& as well.

Good suggestion. Thanks. Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_grid.h:201: // The bound in screen coordinates for the
window grid.
On 2017/06/21 01:51:52, varkha wrote:
> nit: // This WindowGrid's total bounds in screen coordinates.

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:28: #include
"ash/wm/splitview/split_view_controller.h"
On 2017/06/21 01:51:53, varkha wrote:
> nit: Not necessary since you have to bring it in this .cc file's header file?

Removed. Thanks!

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:146: gfx::Rect
GetGridsBoundsInScreen(aura::Window* root_window) {
On 2017/06/21 01:51:53, varkha wrote:
> nit: I have a mild preference for simpler GetGridBoundsInScreen() since you
are
> only getting bounds for a single grid and cannot use "'" in the name.
> nit: Caching Shell::Get()->split_view_controller() in a local variable could
> simplify this a bit since you use it in every branch.

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:146: gfx::Rect
GetGridsBoundsInScreen(aura::Window* root_window) {
On 2017/06/21 01:51:53, varkha wrote:
> Could root_window be const* ? That would help remove the const_cast in line
588.

This const_cast is inevitable, either in line 588, or in
split_view_controller.cc when calculating GetDisplayWorkAreaBoundsInScreen() if
I modify this function to accept const*.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:165: gfx::Rect total_bounds =
GetGridsBoundsInScreen(root_window);
On 2017/06/21 01:51:53, varkha wrote:
> nit: const.

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:169: total_bounds.x(),
On 2017/06/21 01:51:53, varkha wrote:
> nit: x + offset rather than offset + x to make it consistent with how y is
> spelled out.

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:653: // snapping window to one side of the
screen.
On 2017/06/21 01:51:53, varkha wrote:
> <random_thought> This just made me think of scenario where you start dragging
> with touch and then use the mouse while in gesture drag or vice versa. So just
> make sure that this scenario doesn't cause some unexpected activation or other
> side effects. </random_thought> 

Plugged a mouse and check the scenarios you described. If there is no snapped
window yet when dragging a window around with touch, the mouse event on another
window item will cancel the overview mode and activate the window. If there is
one snapped window, when dragging another window around with touch, the mouse
event on a third window will cancel the overview mode and snap the third window
to the other side of the screen. and vice versa.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_selector.h (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.h:130: // Position all of the windows in the
overview.
On 2017/06/21 01:51:53, varkha wrote:
> nit: Position -> Positions

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_controller.cc:69: // snapped window occupy one
side of the screen, while the other windows
On 2017/06/21 01:51:53, varkha wrote:
> nit: occupies

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_controller.cc:71: // position is the position
where the window was firstly snapped. See
On 2017/06/21 01:51:53, varkha wrote:
> nit: first

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:119: // visible part of an overview
window. Note the drag events are only handled in
On 2017/06/21 01:51:53, varkha wrote:
> nit: "Note:" or "Note that"

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:136: // views::CustomButton overrides:
On 2017/06/21 01:51:53, varkha wrote:
> nit: "// views::CustomButton:" for consistency.

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:192: return
static_cast<WindowSelectorItem*>(listener_);
On 2017/06/21 01:51:53, varkha wrote:
> Since this is a local class, I think it would look better if we simply store a
> pointer to WindowSlsectorItem instead of ButtonListener and rename the
variable
> to selector_item_. This way you would not need the cast and using it as a
> ButtonListener should still work.

|listener_| is a class member variable from the base class Button. So we still
need the cast here.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:603: if
(!SplitViewController::ShouldAllowSplitView()) {
On 2017/06/21 01:51:53, varkha wrote:
> nit: no {}

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:840: SetBounds(scaled_bounds,
animation_type);
On 2017/06/21 01:51:53, varkha wrote:
> Don't you need to revert this somewhere?

I don't think it's necessary since if the window was snapped, the selector item
will be deleted, and if the window was moved back to the overview grid, we
called WindowSelector::PositionWindows() in
OverviewWindowDragController::CompleteDrag() to recalculate all window selector
items' bounds.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:854:
widget_window->parent()->StackChildAtBottom(widget_window);
On 2017/06/21 01:51:53, varkha wrote:
> I don't think we should be changing the order relative to what it was at the
> beginning of the drag. This seems to be changing that order so entering
> overview, dragging an item and releasing could be changing state. Not sure if
> the window would also get activated as a result of a drag but this will
probably
> change the stacking of windows while they are animated back to their original
> positions.

I changed the animation time and confirmed that the windows stack does change. I
modified the function so that the windows stack remains the same after dragging.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.h:123: // Handle the mouse/gesture event on
the window selector item.
On 2017/06/21 01:51:53, varkha wrote:
> nit: How about "// Handle mouse/gesture events and facilitate dragging the
> item." ?

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.h:174: // Called before dragging. Scale up
the window a little bit to indicate its
On 2017/06/21 01:51:53, varkha wrote:
> nit: Scale -> Scales

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.h:177: void PrepareDrag();
On 2017/06/21 01:51:54, varkha wrote:
> How about StartDrag() for symmetry?

Done.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.h:179: // Called after dragging. Stack the
window at the bottom of the Z order.
On 2017/06/21 01:51:54, varkha wrote:
> nit: Stack -> Stacks.

Done.

[xdai1, 2017-06-22 21:46:45]

The CQ bit was checked by xdai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 21:47:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-22 22:56:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 22:56:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xdai1, 2017-06-23 23:35:26]

varkha@, kindly ping?

[sadrul, 2017-06-23 23:58:48]

sadrul@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2017-06-23 23:58:49]

Hi! A couple of drive by comments:
 . This CL seems to do a fair amount of things. Can you split into smaller CLs?
 . It doesn't look like there are any tests. Can you add tests? If you split
into smaller CLs, that will make it much easier to make sure the new code is
tested correctly.

[xdai1, 2017-06-26 15:49:26]

Thanks for the drive by comments!

On 2017/06/23 23:58:49, sadrul wrote:
> Hi! A couple of drive by comments:
>  . This CL seems to do a fair amount of things. Can you split into smaller
CLs?

This CL only contains the minimum functionality of split view (i.e., drag a
window in overview mode and snap it to one side of the screen). If I split it
into small CLs, the smaller CLs won't be functional. And currently this CL is
behind a flag (command switch: --enable-tablet-splitview) and won't be enabled
until M62.

>  . It doesn't look like there are any tests. Can you add tests? If you split
> into smaller CLs, that will make it much easier to make sure the new code is
> tested correctly.

I was planning to add tests in a following CL. But I can also add tests here in
the same CL if it's desired.

[varkha, 2017-06-26 16:40:08]

> If I split it into small CLs, the smaller CLs won't be functional.

Perhaps if you split it into a CL that adds a split_view_controller
and its unit tests (even though the controller is not used yet by
the code outside of the tests) and then add another CL that wires
the controller to the overview mode (maybe with some additional
tests), maybe that could work?

Other than that a few remaining nits and a question about stacking
with the transient children present.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_selector.cc (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:146: gfx::Rect
GetGridsBoundsInScreen(aura::Window* root_window) {
On 2017/06/22 21:46:33, xdai1 wrote:
> On 2017/06/21 01:51:53, varkha wrote:
> > Could root_window be const* ? That would help remove the const_cast in line
> 588.
> 
> This const_cast is inevitable, either in line 588, or in
> split_view_controller.cc when calculating GetDisplayWorkAreaBoundsInScreen()
if
> I modify this function to accept const*. 

I thought GetDisplayWorkAreaBoundsInScreen() could be made to accept const
Window*, maybe with some additional plumbing in ScreenUtil.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector.cc:653: // snapping window to one side of the
screen.
On 2017/06/22 21:46:33, xdai1 wrote:
> On 2017/06/21 01:51:53, varkha wrote:
> > <random_thought> This just made me think of scenario where you start
dragging
> > with touch and then use the mouse while in gesture drag or vice versa. So
just
> > make sure that this scenario doesn't cause some unexpected activation or
other
> > side effects. </random_thought> 
> 
> Plugged a mouse and check the scenarios you described. If there is no snapped
> window yet when dragging a window around with touch, the mouse event on
another
> window item will cancel the overview mode and activate the window. If there is
> one snapped window, when dragging another window around with touch, the mouse
> event on a third window will cancel the overview mode and snap the third
window
> to the other side of the screen. and vice versa.

OK, thanks for checking this. I don't think those behaviors are truly very
important, as long as they do not cause some unexpected behaviour.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:192: return
static_cast<WindowSelectorItem*>(listener_);
On 2017/06/22 21:46:34, xdai1 wrote:
> On 2017/06/21 01:51:53, varkha wrote:
> > Since this is a local class, I think it would look better if we simply store
a
> > pointer to WindowSlsectorItem instead of ButtonListener and rename the
> variable
> > to selector_item_. This way you would not need the cast and using it as a
> > ButtonListener should still work.
> 
> |listener_| is a class member variable from the base class Button. So we still
> need the cast here.

Acknowledged.

https://codereview.chromium.org/2918403006/diff/160001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:840: SetBounds(scaled_bounds,
animation_type);
On 2017/06/22 21:46:34, xdai1 wrote:
> On 2017/06/21 01:51:53, varkha wrote:
> > Don't you need to revert this somewhere?
> 
> I don't think it's necessary since if the window was snapped, the selector
item
> will be deleted, and if the window was moved back to the overview grid, we
> called WindowSelector::PositionWindows() in
> OverviewWindowDragController::CompleteDrag() to recalculate all window
selector
> items' bounds.

Acknowledged.

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
File ash/wm/overview/window_grid.h (right):

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
ash/wm/overview/window_grid.h:93: // Remove |selector_item| from the grid.
nit: Removes

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:868:
Shell::Get()->split_view_controller()->GetDefaultSnappedWindow();
nit: you could cache the parent here (e.g. dragged_widget_window->parent()) to
simplify.

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:878: // Then find the window which is
stacked right above this selector item's
nit: is -> was

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:888: }
See the comment below, but just a concern here. Have you checked that this logic
works when you have transient children (e.g. open modal dialogs) where there
might be more than one window in the stacking order between the overview items?

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:889: for (int i = position_in_grid - 1;
i >= 0; i--) {
Since you are breaking out of the for loop first time you enter it, could you
replace it with a condition?

Not sure - is it something like this?

if (position_in_grid < 2)
  return;
aura::Window* stacking_target =
    selector_items[position_in_grid - 2].get()->GetWindow();
... stack ...
And you could use the cached parent here as well from above.

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.h:183: // so that the windows orders are
the same as when they entering overview.
nit (I think): the windows are ordered in the same order
   or
     the window order is the same
   or
     the order of windows is the same

[xdai1, 2017-06-26 21:44:00]

varkha@, I've addressed your comments. Please take another look at this CL,
thanks!

> Perhaps if you split it into a CL that adds a 
> split_view_controller and its unit tests (even though the
> controller is not used yet by the code outside of the tests) 
> and then add another CL that wires the controller to the 
> overview mode (maybe with some additional tests), maybe that 
> could work?

Sure. I'll do what you suggested to split this CL into two CLs.

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
File ash/wm/overview/window_grid.h (right):

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
ash/wm/overview/window_grid.h:93: // Remove |selector_item| from the grid.
On 2017/06/26 16:40:07, varkha wrote:
> nit: Removes

Done.

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
File ash/wm/overview/window_selector_item.cc (right):

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:868:
Shell::Get()->split_view_controller()->GetDefaultSnappedWindow();
On 2017/06/26 16:40:07, varkha wrote:
> nit: you could cache the parent here (e.g. dragged_widget_window->parent()) to
> simplify.

Done.

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:878: // Then find the window which is
stacked right above this selector item's
On 2017/06/26 16:40:07, varkha wrote:
> nit: is -> was

Done.

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:888: }
On 2017/06/26 16:40:07, varkha wrote:
> See the comment below, but just a concern here. Have you checked that this
logic
> works when you have transient children (e.g. open modal dialogs) where there
> might be more than one window in the stacking order between the overview
items?

I checked the scenario you described here (ctrl-o to open the File browser in a
browser window) and there is no problem with the stack logic. The reason is
WindowStackingClient::AdjustStacking() will do the right thing for the transient
windows.

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.cc:889: for (int i = position_in_grid - 1;
i >= 0; i--) {
On 2017/06/26 16:40:07, varkha wrote:
> Since you are breaking out of the for loop first time you enter it, could you
> replace it with a condition?
> 
> Not sure - is it something like this?
> 
> if (position_in_grid < 2)
>   return;
> aura::Window* stacking_target =
>     selector_items[position_in_grid - 2].get()->GetWindow();
> ... stack ...
> And you could use the cached parent here as well from above.

Sorry, I intended to put "break;" in the if-clause.

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
File ash/wm/overview/window_selector_item.h (right):

https://codereview.chromium.org/2918403006/diff/200001/ash/wm/overview/window...
ash/wm/overview/window_selector_item.h:183: // so that the windows orders are
the same as when they entering overview.
On 2017/06/26 16:40:08, varkha wrote:
> nit (I think): the windows are ordered in the same order
>    or
>      the window order is the same
>    or
>      the order of windows is the same

Done.

[xdai1, 2017-06-27 23:47:17]

This CL has split into two sub-CLs as suggested:
https://codereview.chromium.org/2960843004/ and
https://codereview.chromium.org/2955203002/.

I'll close this CL as a result.