athena: A simpler implementation of WindowListProvider.

athena: A simpler implementation of WindowListProvider.

Introduce WindowListProviderImpl that simply works with the stacking order of
child windows of the container. This changes the behaviour of repeated bezel
swipes from the left edge to swipe between the last two active windows, which
is the desired behaviour.

BUG=403444
R=mfomitchev@chromium.org, oshima@chromium.org

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

[mfomitchev, 2014-08-14 21:58:54]

I think we should also take this opportunity to simply the SplitViewController:
currently it only calls MoveWIndowToFront when overview mode is activated. With
the new logic it can simply call MoveWIndowToFront() in ScrollEnd() every time a
new window is slid in. Then I think we can get rid of current_activity_winodw_,
getCurrentActivityWIndow() and just take the top window from the list every time
we need the "current window". If you'd rather not make these changes now - feel
free to add a TODO and I can make them when I come back from vacation.

https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
File athena/wm/window_list_provider_impl_unittest.cc (right):

https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
athena/wm/window_list_provider_impl_unittest.cc:36:
first->SetType(ui::wm::WINDOW_TYPE_NORMAL);
Would be good to add a non-normal window to the mix as well

[oshima, 2014-08-14 22:07:05]

https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
File athena/wm/window_list_provider_impl.cc (right):

https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
athena/wm/window_list_provider_impl.cc:33: container_->StackChildAtTop(window);
window moves to top when activation changes. Would it be possible to eliminate
some of MoveToFront in SplitViewController where makes sense?

[mfomitchev, 2014-08-14 22:19:08]

https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
File athena/wm/window_list_provider_impl.cc (right):

https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
athena/wm/window_list_provider_impl.cc:33: container_->StackChildAtTop(window);
In practice I think we could get rid of all of those that exist now in TOT - the
window that you are dragging from the bezel gets the focus and thus becomes
activated. That said, if it's a no-op, is there harm in doing explicitly rather
than relying on focus changes to activate the appropriate windows? Or were you
suggesting to use wm::ActivateWindow instead of  MoveToFront()?

[oshima, 2014-08-14 22:45:50]

On 2014/08/14 22:19:08, mfomitchev wrote:
>
https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
> File athena/wm/window_list_provider_impl.cc (right):
> 
>
https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
> athena/wm/window_list_provider_impl.cc:33:
container_->StackChildAtTop(window);
> In practice I think we could get rid of all of those that exist now in TOT -
the
> window that you are dragging from the bezel gets the focus and thus becomes
> activated. That said, if it's a no-op, is there harm in doing explicitly
rather
> than relying on focus changes to activate the appropriate windows? Or were you
> suggesting to use wm::ActivateWindow instead of  MoveToFront()?

If it's not necessary, I'd prefer removing it because it's confusing.

Ideally, the implementation of WindowListProvider should be able to takes care
of the order by
itself by listening to various events such as activation, visibility change etc,
as
these signals are already available in aura.

[mfomitchev1, 2014-08-14 23:23:15]

On 2014/08/14 22:45:50, oshima wrote:
> On 2014/08/14 22:19:08, mfomitchev wrote:
> >
>
https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
> > File athena/wm/window_list_provider_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
> > athena/wm/window_list_provider_impl.cc:33:
> container_->StackChildAtTop(window);
> > In practice I think we could get rid of all of those that exist now in TOT -
> the
> > window that you are dragging from the bezel gets the focus and thus becomes
> > activated. That said, if it's a no-op, is there harm in doing explicitly
> rather
> > than relying on focus changes to activate the appropriate windows? Or were
you
> > suggesting to use wm::ActivateWindow instead of  MoveToFront()?
> 
> If it's not necessary, I'd prefer removing it because it's confusing.
> 
> Ideally, the implementation of WindowListProvider should be able to takes care
> of the order by
> itself by listening to various events such as activation, visibility change
etc,
> as
> these signals are already available in aura.

Actually I think some action from split view controller is necessary. It could
be a call to wm::ActivateWindow or some event that the window list provider will
react to instead of a direct call to MoveWindowToFront, but after thinking about
it more, I don't believe the new window will always get activated by the drag
automatically. The old window will get the scroll events, so if you go into
overview right after sliding in a new window, the order will be wrong unless we
update it.

[oshima, 2014-08-14 23:27:01]

On 2014/08/14 23:23:15, mfomitchev1 wrote:
> On 2014/08/14 22:45:50, oshima wrote:
> > On 2014/08/14 22:19:08, mfomitchev wrote:
> > >
> >
>
https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
> > > File athena/wm/window_list_provider_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
> > > athena/wm/window_list_provider_impl.cc:33:
> > container_->StackChildAtTop(window);
> > > In practice I think we could get rid of all of those that exist now in TOT
-
> > the
> > > window that you are dragging from the bezel gets the focus and thus
becomes
> > > activated. That said, if it's a no-op, is there harm in doing explicitly
> > rather
> > > than relying on focus changes to activate the appropriate windows? Or were
> you
> > > suggesting to use wm::ActivateWindow instead of  MoveToFront()?
> > 
> > If it's not necessary, I'd prefer removing it because it's confusing.
> > 
> > Ideally, the implementation of WindowListProvider should be able to takes
care
> > of the order by
> > itself by listening to various events such as activation, visibility change
> etc,
> > as
> > these signals are already available in aura.
> 
> Actually I think some action from split view controller is necessary. It could
> be a call to wm::ActivateWindow or some event that the window list provider
will
> react to instead of a direct call to MoveWindowToFront, but after thinking
about
> it more, I don't believe the new window will always get activated by the drag
> automatically. The old window will get the scroll events, so if you go into
> overview right after sliding in a new window, the order will be wrong unless
we
> update it.

There are still events you can listen to such as visibity (not now, but when
minimized state is implemented),
and hierarchy change.

[mfomitchev1, 2014-08-14 23:36:27]

On 2014/08/14 23:23:15, mfomitchev1 wrote:
> On 2014/08/14 22:45:50, oshima wrote:
> > On 2014/08/14 22:19:08, mfomitchev wrote:
> > >
> >
>
https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
> > > File athena/wm/window_list_provider_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
> > > athena/wm/window_list_provider_impl.cc:33:
> > container_->StackChildAtTop(window);
> > > In practice I think we could get rid of all of those that exist now in TOT
-
> > the
> > > window that you are dragging from the bezel gets the focus and thus
becomes
> > > activated. That said, if it's a no-op, is there harm in doing explicitly
> > rather
> > > than relying on focus changes to activate the appropriate windows? Or were
> you
> > > suggesting to use wm::ActivateWindow instead of  MoveToFront()?
> > 
> > If it's not necessary, I'd prefer removing it because it's confusing.
> > 
> > Ideally, the implementation of WindowListProvider should be able to takes
care
> > of the order by
> > itself by listening to various events such as activation, visibility change
> etc,
> > as
> > these signals are already available in aura.
> 
> Actually I think some action from split view controller is necessary. It could
> be a call to wm::ActivateWindow or some event that the window list provider
will
> react to instead of a direct call to MoveWindowToFront, but after thinking
about
> it more, I don't believe the new window will always get activated by the drag
> automatically. The old window will get the scroll events, so if you go into
> overview right after sliding in a new window, the order will be wrong unless
we
> update it.

BTW, it is not that different from handling an alt-tab key press, except that we
are making a gesture be the trigger instead of a hotkey. We'd need to call
sometuing like MoveToFront or ActivateWindow for alt tab, so we need to do it
here as well.

[sadrul, 2014-08-14 23:47:22]

I have made a few more changes in this CL:

 * The SplitViewController (SVC) is no longer a WindowManagerObserver (WMO). One
of the reasons this was necessary is to reset the transform when getting into
overview mode. I believe this was done when we used to set a transform to
position the window on the right. Now that we set the bounds instead, this is
longer be necessary. (going from split-view to overview is pretty broken even
with the SVC as an WMO, and it remains the same amount of broken when SVC is no
longer an WMO).

 * Disable the BezelController (BC) when going into overview mode by resetting
the left/right handler. So the SVC no longer needs to check the state of the WM
before it decides to start the scroll, because the BC won't trigger if the WM is
in overview mode.

 * Remove the only call to MoveToFront() because it is immediately followed by
wm::ActiveWindow() which does the same. (I see the value of making an explicit
call here, but I think it's better to have the WLP be aware of the
activation/stacking order changes through existing aura/wm APIs)

 * Remove activity-window tracking from SVC.

PTAL (I will update the CL description accordingly).

https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
File athena/wm/window_list_provider_impl.cc (right):

https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
athena/wm/window_list_provider_impl.cc:33: container_->StackChildAtTop(window);
On 2014/08/14 22:19:08, mfomitchev wrote:
> In practice I think we could get rid of all of those that exist now in TOT -
the
> window that you are dragging from the bezel gets the focus and thus becomes
> activated. That said, if it's a no-op, is there harm in doing explicitly
rather
> than relying on focus changes to activate the appropriate windows? Or were you
> suggesting to use wm::ActivateWindow instead of  MoveToFront()?

Removed MoveToFront() from the interface

https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
File athena/wm/window_list_provider_impl_unittest.cc (right):

https://codereview.chromium.org/470083004/diff/20001/athena/wm/window_list_pr...
athena/wm/window_list_provider_impl_unittest.cc:36:
first->SetType(ui::wm::WINDOW_TYPE_NORMAL);
On 2014/08/14 21:58:54, mfomitchev wrote:
> Would be good to add a non-normal window to the mix as well

I have added a separate test that makes sure that abNORMAL windows don't show up
in the returned list instead (to make it easier in this test to test the two
lists directly).

[oshima, 2014-08-15 01:00:41]

lgtm

[mfomitchev, 2014-08-15 14:42:29]

https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
File athena/wm/split_view_controller.cc (left):

https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:304: state_ = INACTIVE;
Until split-overview is implemented, going to overview means resetting the state
to splitview INACTIVE. Somebody needs to do that...

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

https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:235: separator_position_ = 0;
I think we need to call wm::ActiveWindow(right_window_) here. If you've just
dragged right_window_ all the way from the right bezel, at this point
left_window is still the active window and at the top of the window stack. If
you go into the overview mode right away without activating right_window_ by
touching it, I don't think it will get activated, and left_window will show up
at the bottom of the stack incorrectly.

https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:238: separator_position_ = container_width;
Ditto for wm::ActiveWindow(left_window_)

[sadrul, 2014-08-15 16:07:13]

https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
File athena/wm/split_view_controller.cc (left):

https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:304: state_ = INACTIVE;
On 2014/08/15 14:42:28, mfomitchev wrote:
> Until split-overview is implemented, going to overview means resetting the
state
> to splitview INACTIVE. Somebody needs to do that...

A good fix for this for now would be to explicitly turn off the split-view mode
when going into overview mode (using oshima@'s ToggleSplitView()) (like we reset
the bezel-controller). The proper fix would be to not turn it off at all
(crbug.com/404119)

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

https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:235: separator_position_ = 0;
On 2014/08/15 14:42:28, mfomitchev_OOO_Aug16-24 wrote:
> I think we need to call wm::ActiveWindow(right_window_) here. If you've just
> dragged right_window_ all the way from the right bezel, at this point
> left_window is still the active window and at the top of the window stack. If
> you go into the overview mode right away without activating right_window_ by
> touching it, I don't think it will get activated, and left_window will show up
> at the bottom of the stack incorrectly.

Done.

https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:238: separator_position_ = container_width;
On 2014/08/15 14:42:28, mfomitchev_OOO_Aug16-24 wrote:
> Ditto for wm::ActiveWindow(left_window_)

Done.

[mfomitchev, 2014-08-15 16:43:26]

https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
File athena/wm/split_view_controller.cc (left):

https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:304: state_ = INACTIVE;
On 2014/08/15 16:07:12, sadrul wrote:
> On 2014/08/15 14:42:28, mfomitchev wrote:
> > Until split-overview is implemented, going to overview means resetting the
> state
> > to splitview INACTIVE. Somebody needs to do that...
> 
> A good fix for this for now would be to explicitly turn off the split-view
mode
> when going into overview mode (using oshima@'s ToggleSplitView()) (like we
reset
> the bezel-controller). The proper fix would be to not turn it off at all
> (crbug.com/404119)

Sure. So what are you going to do now? Wait for oshima's change to land first?
If you don't want to do that, I think you can simply reset
split_view_controller_ just like you do with the bezel controller for now.

[sadrul, 2014-08-15 16:58:56]

On 2014/08/15 16:43:26, mfomitchev_OOO_Aug16-24 wrote:
>
https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
> File athena/wm/split_view_controller.cc (left):
> 
>
https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
> athena/wm/split_view_controller.cc:304: state_ = INACTIVE;
> On 2014/08/15 16:07:12, sadrul wrote:
> > On 2014/08/15 14:42:28, mfomitchev wrote:
> > > Until split-overview is implemented, going to overview means resetting the
> > state
> > > to splitview INACTIVE. Somebody needs to do that...
> > 
> > A good fix for this for now would be to explicitly turn off the split-view
> mode
> > when going into overview mode (using oshima@'s ToggleSplitView()) (like we
> reset
> > the bezel-controller). The proper fix would be to not turn it off at all
> > (crbug.com/404119)
> 
> Sure. So what are you going to do now? Wait for oshima's change to land first?

oshima@ What do you think? Should I wait for
https://codereview.chromium.org/465983002/ to land, or add a temporary
SVC::DeactivateSplit() with this CL (which presumably would be removed when
ToggleSplitView() is added)?

> If you don't want to do that, I think you can simply reset
> split_view_controller_ just like you do with the bezel controller for now.

[sadrul, 2014-08-15 18:06:59]

On 2014/08/15 16:58:56, sadrul wrote:
> On 2014/08/15 16:43:26, mfomitchev_OOO_Aug16-24 wrote:
> >
>
https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
> > File athena/wm/split_view_controller.cc (left):
> > 
> >
>
https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
> > athena/wm/split_view_controller.cc:304: state_ = INACTIVE;
> > On 2014/08/15 16:07:12, sadrul wrote:
> > > On 2014/08/15 14:42:28, mfomitchev wrote:
> > > > Until split-overview is implemented, going to overview means resetting
the
> > > state
> > > > to splitview INACTIVE. Somebody needs to do that...
> > > 
> > > A good fix for this for now would be to explicitly turn off the split-view
> > mode
> > > when going into overview mode (using oshima@'s ToggleSplitView()) (like we
> > reset
> > > the bezel-controller). The proper fix would be to not turn it off at all
> > > (crbug.com/404119)
> > 
> > Sure. So what are you going to do now? Wait for oshima's change to land
first?
> 
> oshima@ What do you think? Should I wait for
> https://codereview.chromium.org/465983002/ to land, or add a temporary
> SVC::DeactivateSplit() with this CL (which presumably would be removed when
> ToggleSplitView() is added)?

Looks like oshima@ is OOO today. So I am going to go ahead and make this change
(to unblock https://codereview.chromium.org/468763002/).

> 
> > If you don't want to do that, I think you can simply reset
> > split_view_controller_ just like you do with the bezel controller for now.

[mfomitchev, 2014-08-15 18:11:31]

sgtm

[sadrul, 2014-08-15 18:23:27]

On 2014/08/15 18:06:59, sadrul wrote:
> On 2014/08/15 16:58:56, sadrul wrote:
> > On 2014/08/15 16:43:26, mfomitchev_OOO_Aug16-24 wrote:
> > >
> >
>
https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
> > > File athena/wm/split_view_controller.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_con...
> > > athena/wm/split_view_controller.cc:304: state_ = INACTIVE;
> > > On 2014/08/15 16:07:12, sadrul wrote:
> > > > On 2014/08/15 14:42:28, mfomitchev wrote:
> > > > > Until split-overview is implemented, going to overview means resetting
> the
> > > > state
> > > > > to splitview INACTIVE. Somebody needs to do that...
> > > > 
> > > > A good fix for this for now would be to explicitly turn off the
split-view
> > > mode
> > > > when going into overview mode (using oshima@'s ToggleSplitView()) (like
we
> > > reset
> > > > the bezel-controller). The proper fix would be to not turn it off at all
> > > > (crbug.com/404119)
> > > 
> > > Sure. So what are you going to do now? Wait for oshima's change to land
> first?
> > 
> > oshima@ What do you think? Should I wait for
> > https://codereview.chromium.org/465983002/ to land, or add a temporary
> > SVC::DeactivateSplit() with this CL (which presumably would be removed when
> > ToggleSplitView() is added)?
> 
> Looks like oshima@ is OOO today. So I am going to go ahead and make this
change
> (to unblock https://codereview.chromium.org/468763002/).

Done.

[mfomitchev, 2014-08-15 18:30:51]

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

https://codereview.chromium.org/470083004/diff/120001/athena/wm/split_view_co...
athena/wm/split_view_controller.cc:90: state_ = INACTIVE;
Set left/right window to NULL here to be safe.

https://codereview.chromium.org/470083004/diff/120001/athena/wm/window_manage...
File athena/wm/window_manager_impl.cc (right):

https://codereview.chromium.org/470083004/diff/120001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:176:
split_view_controller_->DeactivateSplitMode();
Can we only do it when active == true? Doing it every time for active == false
is a waste and might interfere with entering split view from overview.

[sadrul, 2014-08-15 18:44:19]

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

https://codereview.chromium.org/470083004/diff/120001/athena/wm/split_view_co...
athena/wm/split_view_controller.cc:90: state_ = INACTIVE;
On 2014/08/15 18:30:51, mfomitchev_OOO_Aug16-24 wrote:
> Set left/right window to NULL here to be safe.

Done.

https://codereview.chromium.org/470083004/diff/120001/athena/wm/window_manage...
File athena/wm/window_manager_impl.cc (right):

https://codereview.chromium.org/470083004/diff/120001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:176:
split_view_controller_->DeactivateSplitMode();
On 2014/08/15 18:30:51, mfomitchev_OOO_Aug16-24 wrote:
> Can we only do it when active == true? Doing it every time for active == false
> is a waste and might interfere with entering split view from overview.

Done.

[mfomitchev, 2014-08-15 18:44:53]

LGTM

[sadrul, 2014-08-15 19:48:16]

Committed patchset #8 manually as 289984 (presubmit successful).

[oshima, 2014-08-18 13:33:58]

On Fri, Aug 15, 2014 at 11:06 AM, <sadrul@chromium.org> wrote:

> On 2014/08/15 16:58:56, sadrul wrote:
>
>> On 2014/08/15 16:43:26, mfomitchev_OOO_Aug16-24 wrote:
>> >
>>
>
> https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_
> controller.cc
>
>> > File athena/wm/split_view_controller.cc (left):
>> >
>> >
>>
>
> https://codereview.chromium.org/470083004/diff/60001/athena/wm/split_view_
> controller.cc#oldcode304
>
>> > athena/wm/split_view_controller.cc:304: state_ = INACTIVE;
>> > On 2014/08/15 16:07:12, sadrul wrote:
>> > > On 2014/08/15 14:42:28, mfomitchev wrote:
>> > > > Until split-overview is implemented, going to overview means
>> resetting
>>
> the
>
>> > > state
>> > > > to splitview INACTIVE. Somebody needs to do that...
>> > >
>> > > A good fix for this for now would be to explicitly turn off the
>> split-view
>> > mode
>> > > when going into overview mode (using oshima@'s ToggleSplitView())
>> (like we
>> > reset
>> > > the bezel-controller). The proper fix would be to not turn it off at
>> all
>> > > (crbug.com/404119)
>> >
>> > Sure. So what are you going to do now? Wait for oshima's change to land
>>
> first?
>
>  oshima@ What do you think? Should I wait for
>> https://codereview.chromium.org/465983002/ to land, or add a temporary
>> SVC::DeactivateSplit() with this CL (which presumably would be removed
>> when
>> ToggleSplitView() is added)?
>>
>
> Looks like oshima@ is OOO today. So I am going to go ahead and make this
> change
> (to unblock https://codereview.chromium.org/468763002/).
>
>
sgtm and lgtm


>
>
>  > If you don't want to do that, I think you can simply reset
>> > split_view_controller_ just like you do with the bezel controller for
>> now.
>>
>
>
> https://codereview.chromium.org/470083004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.