athena: Allow getting into split-view mode from overview-mode.

athena: Allow getting into split-view mode from overview-mode.

When a window is dragged horizontally in the overview-mode, create an
OverviewToolbar object. The toolbar shows a couple of ActionButtons for 'close'
and 'split' options. If the user drags (and drops) the window over the 'split'
button, then the split-view mode is triggered. The buttons are highlighted
(scaled up 50%) to indicate the action that will be performed.

BUG=402598
R=mfomitchev@chromium.org, mukai@chromium.org

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

[sadrul, 2014-08-13 02:18:05]

mfomitchev@ for the changes in split_view_controller

mukai@ for the rest

[mfomitchev, 2014-08-13 15:12:31]

https://codereview.chromium.org/459613008/diff/60001/athena/wm/split_view_con...
File athena/wm/split_view_controller.h (right):

https://codereview.chromium.org/459613008/diff/60001/athena/wm/split_view_con...
athena/wm/split_view_controller.h:36: void ActivateSplitMode(aura::Window* left,
aura::Window* right);
Can you document here how the left and right windows are chosen if left/right
are NULL?

https://codereview.chromium.org/459613008/diff/60001/athena/wm/window_manager...
File athena/wm/window_manager_impl.cc (right):

https://codereview.chromium.org/459613008/diff/60001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:213: SetInOverview(false);
Hmm.. my understanding from the mocks was that you are supposed to stay in
overview-splitview after you drag the window to the side, not exit overview to
splitview right away. Perhaps we should clarify with kuscher.

https://codereview.chromium.org/459613008/diff/60001/athena/wm/window_overvie...
File athena/wm/window_overview_mode.cc (right):

https://codereview.chromium.org/459613008/diff/60001/athena/wm/window_overvie...
athena/wm/window_overview_mode.cc:442: delegate_->OnSplitViewMode(NULL,
dragged_window_);
Why not pass the left window here as well? The bottom window in the stack should
be the left window, and it seems it would be cleaner if the overview mode
ensured that rather than relying on SplitViewController.

[sadrul, 2014-08-13 16:20:12]

https://codereview.chromium.org/459613008/diff/60001/athena/wm/split_view_con...
File athena/wm/split_view_controller.h (right):

https://codereview.chromium.org/459613008/diff/60001/athena/wm/split_view_con...
athena/wm/split_view_controller.h:36: void ActivateSplitMode(aura::Window* left,
aura::Window* right);
On 2014/08/13 15:12:31, mfomitchev wrote:
> Can you document here how the left and right windows are chosen if left/right
> are NULL?

Done.

https://codereview.chromium.org/459613008/diff/60001/athena/wm/window_manager...
File athena/wm/window_manager_impl.cc (right):

https://codereview.chromium.org/459613008/diff/60001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:213: SetInOverview(false);
On 2014/08/13 15:12:31, mfomitchev wrote:
> Hmm.. my understanding from the mocks was that you are supposed to stay in
> overview-splitview after you drag the window to the side, not exit overview to
> splitview right away. Perhaps we should clarify with kuscher.

We are going with this for now. I clarified with kuscher@. Forwarded you the
email.

https://codereview.chromium.org/459613008/diff/60001/athena/wm/window_overvie...
File athena/wm/window_overview_mode.cc (right):

https://codereview.chromium.org/459613008/diff/60001/athena/wm/window_overvie...
athena/wm/window_overview_mode.cc:442: delegate_->OnSplitViewMode(NULL,
dragged_window_);
On 2014/08/13 15:12:31, mfomitchev wrote:
> Why not pass the left window here as well? The bottom window in the stack
should
> be the left window, and it seems it would be cleaner if the overview mode
> ensured that rather than relying on SplitViewController.

The purpose here is to tell the delegate that the user has explicitly selected
the window on the right. The delegate (which is responsible for actually going
into the split-view mode) is responsible for deciding whether to keep the
foremost window on the left, or whether to switch to a different window on the
left (e.g. if the selected window on the right belongs to a multi-window app,
perhaps the left window would switch to a window that belongs to the same app).
That's a detail the overview-mode itself does not need to worry about.

[Jun Mukai, 2014-08-13 16:55:13]

https://codereview.chromium.org/459613008/diff/80001/athena/athena.gyp
File athena/athena.gyp (right):

https://codereview.chromium.org/459613008/diff/80001/athena/athena.gyp#newcode21
athena/athena.gyp:21: 'resources/athena_resources.gyp:athena_resources',
can you remove the resources dependency from athena_main.gyp?

https://codereview.chromium.org/459613008/diff/80001/athena/wm/DEPS
File athena/wm/DEPS (right):

https://codereview.chromium.org/459613008/diff/80001/athena/wm/DEPS#newcode5
athena/wm/DEPS:5: "+grit",
oshima told me that grit path policy has slightly changed recently, and
rewriting is ongoing, it's better to follow the new style now.

In the new style, include path would be:
athena/resources/grit/athena_resources.h

so the DEPS would also be "athena/resources", or "athena/resources/grit"

[Jun Mukai, 2014-08-13 17:00:23]

https://codereview.chromium.org/459613008/diff/80001/athena/wm/overview_toolb...
File athena/wm/overview_toolbar.cc (right):

https://codereview.chromium.org/459613008/diff/80001/athena/wm/overview_toolb...
athena/wm/overview_toolbar.cc:30: class ActionButton : public ui::LayerDelegate
{
got confused, why this is a layer delegate rather than a CustomButton?
it seems no one is handling the button, so this would look like a button but
doesn't behave as a button?

[mfomitchev, 2014-08-13 17:42:52]

https://codereview.chromium.org/459613008/diff/60001/athena/wm/window_manager...
File athena/wm/window_manager_impl.cc (right):

https://codereview.chromium.org/459613008/diff/60001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:213: SetInOverview(false);
I thought it was a bit ambiguous, so I responded on the thread to clarify. I
think this is fine for now either way though.

https://codereview.chromium.org/459613008/diff/60001/athena/wm/window_overvie...
File athena/wm/window_overview_mode.cc (right):

https://codereview.chromium.org/459613008/diff/60001/athena/wm/window_overvie...
athena/wm/window_overview_mode.cc:442: delegate_->OnSplitViewMode(NULL,
dragged_window_);
On 2014/08/13 16:20:12, sadrul wrote:
> On 2014/08/13 15:12:31, mfomitchev wrote:
> > Why not pass the left window here as well? The bottom window in the stack
> should
> > be the left window, and it seems it would be cleaner if the overview mode
> > ensured that rather than relying on SplitViewController.
> 
> The purpose here is to tell the delegate that the user has explicitly selected
> the window on the right. The delegate (which is responsible for actually going
> into the split-view mode) is responsible for deciding whether to keep the
> foremost window on the left, or whether to switch to a different window on the
> left (e.g. if the selected window on the right belongs to a multi-window app,
> perhaps the left window would switch to a window that belongs to the same
app).
> That's a detail the overview-mode itself does not need to worry about.

Acknowledged.

[sadrul, 2014-08-13 19:25:29]

Responded to comments in earlier patchsets. PTAL at the following comment about
the change made in split_view_controller.cc in the new patchset.

https://codereview.chromium.org/459613008/diff/80001/athena/athena.gyp
File athena/athena.gyp (right):

https://codereview.chromium.org/459613008/diff/80001/athena/athena.gyp#newcode21
athena/athena.gyp:21: 'resources/athena_resources.gyp:athena_resources',
On 2014/08/13 16:55:13, Jun Mukai wrote:
> can you remove the resources dependency from athena_main.gyp?

I believe we would have to export_dependent_settings for that? But athena_main
itself also depends on the resources (for the wallpaper). So it should retain
that.

https://codereview.chromium.org/459613008/diff/80001/athena/wm/DEPS
File athena/wm/DEPS (right):

https://codereview.chromium.org/459613008/diff/80001/athena/wm/DEPS#newcode5
athena/wm/DEPS:5: "+grit",
On 2014/08/13 16:55:13, Jun Mukai wrote:
> oshima told me that grit path policy has slightly changed recently, and
> rewriting is ongoing, it's better to follow the new style now.
> 
> In the new style, include path would be:
> athena/resources/grit/athena_resources.h
> 
> so the DEPS would also be "athena/resources", or "athena/resources/grit"

Good to know. Thanks! Done (went with athena/resources)

https://codereview.chromium.org/459613008/diff/80001/athena/wm/overview_toolb...
File athena/wm/overview_toolbar.cc (right):

https://codereview.chromium.org/459613008/diff/80001/athena/wm/overview_toolb...
athena/wm/overview_toolbar.cc:30: class ActionButton : public ui::LayerDelegate
{
On 2014/08/13 17:00:23, Jun Mukai wrote:
> got confused, why this is a layer delegate rather than a CustomButton?
> it seems no one is handling the button, so this would look like a button but
> doesn't behave as a button?

Pretty much, yeah. I considered making this a views Button at first. But these
buttons live only during the gesture, and do not get a chance to interact with
any events. So the sole purpose of these is to show something to the user.
Creating a Widget with the views in them felt too heavy-handed for that, and I
chose to use only a Layer since that is sufficient for our purposes (paint
something on the screen, sometimes with transforms).

https://codereview.chromium.org/459613008/diff/120001/athena/wm/split_view_co...
File athena/wm/split_view_controller.cc (right):

https://codereview.chromium.org/459613008/diff/120001/athena/wm/split_view_co...
athena/wm/split_view_controller.cc:103:
container_->StackChildAtTop(left_window_);
This is necessary to make sure that the two selected windows are always above
their siblings. The ordering of the calls make it so |left_window_| is on top of
|right_window_| in the z-order, but because the windows do not overlap, it
doesn't cause issues.

[mfomitchev, 2014-08-13 19:27:45]

LGTM

https://codereview.chromium.org/459613008/diff/120001/athena/wm/split_view_co...
File athena/wm/split_view_controller.cc (right):

https://codereview.chromium.org/459613008/diff/120001/athena/wm/split_view_co...
athena/wm/split_view_controller.cc:103:
container_->StackChildAtTop(left_window_);
On 2014/08/13 19:25:29, sadrul wrote:
> This is necessary to make sure that the two selected windows are always above
> their siblings. The ordering of the calls make it so |left_window_| is on top
of
> |right_window_| in the z-order, but because the windows do not overlap, it
> doesn't cause issues.

Acknowledged.

[Jun Mukai, 2014-08-13 19:36:45]

lgtm

[sadrul, 2014-08-14 03:54:11]

Committed patchset #7 manually as 289454 (presubmit successful).