athena: Fix overview mode for split-view mode.

Committed: https://crrev.com/35e2f0861aa93d71fe5b59626f63088100635a40
Cr-Commit-Position: refs/heads/master@{#292468}

[sadrul, 2014-08-21 21:19:52]

[ Please ignore the change in split_view_controller.cc. It will go away with
pkotwicz@'s https://codereview.chromium.org/488153003/ ]

[oshima, 2014-08-21 22:41:00]

can you add test?

https://codereview.chromium.org/495193003/diff/1/athena/wm/window_overview_mo...
File athena/wm/window_overview_mode.cc (right):

https://codereview.chromium.org/495193003/diff/1/athena/wm/window_overview_mo...
athena/wm/window_overview_mode.cc:123: split_.enabled =
delegate_->IsInSplitViewMode(&split_.left, &split_.right);
can't you just pass SplitViewController and get state from there?

[sadrul, 2014-08-22 02:02:49]

On 2014/08/21 22:41:00, oshima wrote:
> can you add test?

This implementation isn't necessarily the actual behaviour we want. There is a
relatively good chance we will iterate a few times to decide the exact
behaviour. I am thinking we could wait until then before we add tests for this?
However, this particular CL changes the interaction between WMO and its
delegate: (1) getting into overview mode no longer deactivates split-view mode,
and (2) overview mode calling explicitly OnSelectWindow() should deactivate the
split-view mode. I will add tests for these two cases in this CL.

> 
>
https://codereview.chromium.org/495193003/diff/1/athena/wm/window_overview_mo...
> File athena/wm/window_overview_mode.cc (right):
> 
>
https://codereview.chromium.org/495193003/diff/1/athena/wm/window_overview_mo...
> athena/wm/window_overview_mode.cc:123: split_.enabled =
> delegate_->IsInSplitViewMode(&split_.left, &split_.right);
> can't you just pass SplitViewController and get state from there?

The SVC and the WOM are currently pretty self contained and independent of each
other. If you don't feel strongly against it, I would like to keep them separate
for now until they are more mature, and communicate with each other via the
delegate interface in the WindowManagerImpl. (It would be easy to remove the
delegate interface later on, but it may be difficult to untangle the two if they
start communicating with each other now) WDYT?

[oshima, 2014-08-22 17:28:25]

On 2014/08/22 02:02:49, sadrul wrote:
> On 2014/08/21 22:41:00, oshima wrote:
> > can you add test?
> 
> This implementation isn't necessarily the actual behaviour we want. There is a
> relatively good chance we will iterate a few times to decide the exact
> behaviour. I am thinking we could wait until then before we add tests for
this?
> However, this particular CL changes the interaction between WMO and its
> delegate: (1) getting into overview mode no longer deactivates split-view
mode,
> and (2) overview mode calling explicitly OnSelectWindow() should deactivate
the
> split-view mode. I will add tests for these two cases in this CL.

Acknowledged.

> 
> > 
> >
>
https://codereview.chromium.org/495193003/diff/1/athena/wm/window_overview_mo...
> > File athena/wm/window_overview_mode.cc (right):
> > 
> >
>
https://codereview.chromium.org/495193003/diff/1/athena/wm/window_overview_mo...
> > athena/wm/window_overview_mode.cc:123: split_.enabled =
> > delegate_->IsInSplitViewMode(&split_.left, &split_.right);
> > can't you just pass SplitViewController and get state from there?
> 
> The SVC and the WOM are currently pretty self contained and independent of
each
> other. If you don't feel strongly against it, I would like to keep them
separate
> for now until they are more mature, and communicate with each other via the
> delegate interface in the WindowManagerImpl. (It would be easy to remove the
> delegate interface later on, but it may be difficult to untangle the two if
they
> start communicating with each other now) WDYT?

Hmm, the way I see is that that is just an illusion and they do depend each
other in reality :)

I personally don't like to duplicate the state as you need to keep them in sync
one way or anther
(what if the right window gets closed during overview mode?)

I understand that that isn't an issue right now, but I still think delegate
interface is overkill.

Can't you just pass Details instead?

[sadrul, 2014-08-28 19:10:59]

https://codereview.chromium.org/495193003/diff/1/athena/wm/window_overview_mo...
File athena/wm/window_overview_mode.cc (right):

https://codereview.chromium.org/495193003/diff/1/athena/wm/window_overview_mo...
athena/wm/window_overview_mode.cc:123: split_.enabled =
delegate_->IsInSplitViewMode(&split_.left, &split_.right);
On 2014/08/21 22:41:00, oshima wrote:
> can't you just pass SplitViewController and get state from there?

Done.

[oshima, 2014-08-28 19:59:46]

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

https://codereview.chromium.org/495193003/diff/20001/athena/wm/window_overvie...
athena/wm/window_overview_mode.cc:129: split_.right =
split_view_controller_->right_window();
do you still need to copy these state?

If you copy it, then split_view_controller_ isn't necessary?

[sadrul, 2014-08-28 20:28:29]

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

https://codereview.chromium.org/495193003/diff/20001/athena/wm/window_overvie...
athena/wm/window_overview_mode.cc:129: split_.right =
split_view_controller_->right_window();
On 2014/08/28 19:59:46, oshima wrote:
> do you still need to copy these state?
> 
> If you copy it, then split_view_controller_ isn't necessary?

Done (removed split_view_controller_).

[oshima, 2014-08-28 20:32:42]

lgtm

[sadrul, 2014-08-28 21:41:15]

Committed patchset #3 (id:40001) to pending queue manually as 3c3aa9a (presubmit
successful).

[commit-bot: I haz the power, 2014-09-10 03:02:26]

Patchset 3 (id:??) landed as
https://crrev.com/35e2f0861aa93d71fe5b59626f63088100635a40
Cr-Commit-Position: refs/heads/master@{#292468}