Ensure that an activity is activated when overview mode is exited

Ensure that an activity is activated when overview mode is exited

BUG=409724
TEST=WindowManagerTest.*

Committed: https://crrev.com/712b9c49fcd36f05514525815434b81577fc7cd4
Cr-Commit-Position: refs/heads/master@{#295376}

[pkotwicz, 2014-09-05 22:02:06]

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

[pkotwicz, 2014-09-05 22:02:07]

Sadrul PTAL

I ended up resolving my dilemna about the animations started in
RestoreWindowState() being synchronous in unit tests.
I decided to:
- Require OnSelectWindow() to set the opacity of the selected windows
- Start an opacity animation in RestoreWindowState() and hide the window unless
the opacity animation is aborted.

The unit tests pass but it is rather confusing as to why they pass. In a
separate CL (which would land either prior or after this CL), I can move the
class definition of WindowOverviewModeImpl to window_overview_mode.h This will
allow tests to call WindowOverviewMode::SelectWindow()

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

https://codereview.chromium.org/546123002/diff/20001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:247:
split_view_controller_->DeactivateSplitMode();
This ends up hiding the windows in split view immediately which is wrong. I will
fix that as part of a different CL. I have filed crbug.com/410303

[pkotwicz, 2014-09-07 01:57:24]

This CL is still ready for review :) In this latest patch, I have made opening a
new activity while in overview mode make more sense

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

https://codereview.chromium.org/546123002/diff/80001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:106: !instance->IsOverviewModeActive()) {
Opening up new activities as full size when in overview mode makes more sense I
think. Calling SplitViewController::ReplaceWindow() when opening a new window
while in overview mode is most definitely wrong

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

https://codereview.chromium.org/546123002/diff/80001/athena/wm/window_overvie...
athena/wm/window_overview_mode.cc:622: SelectWindow(windows.empty() ? NULL :
*windows.rbegin());
This CL introduces a crash when a user exits overview mode using an accelerator
key while the dragged window is animating closed. That's for a different CL to
fix though

[pkotwicz, 2014-09-07 02:51:40]

It may be worth doing the same hack that we do in order to keep the web contents
visible while animating minimizing a window on CrOS (creating new layers when
the window is hidden - See ScopedHidingAnimationSettings in
ui/wm/core/window_animations.cc). This may be worth doing because we are
planning on limiting the number of overview windows which are visible at a time.

[sadrul, 2014-09-07 05:26:50]

A few comments about the overall approach in the patch.

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

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:265: overview_->SelectDefaultWindow();
If the WOM did not explicitly request a selection change (or split-view mode
change), then the WMI itself should make the selection, and not ask the WOM to
do it.

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_manage...
File athena/wm/window_manager_impl.h (right):

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_manage...
athena/wm/window_manager_impl.h:64: void ExitOverview();
The two functions do things differently enough that we should not use
overloading here.

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_overvi...
File athena/wm/window_overview_mode.cc (right):

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_overvi...
athena/wm/window_overview_mode.cc:111: }
Can you explain this change in RestoreWindowState() a bit more?

We currently cannot animate the hide of the window because at the time
RestoreWindowState() is called, we may not know which windows will be part of
the split, or which window will be active. So we make that decision at the end
of the animation, at which point we are guaranteed to have made that decision. I
think that is much more easier to explain, and cleaner than this code here,
which looks more like a hack.

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_overvi...
athena/wm/window_overview_mode.cc:622: SelectWindow(windows.empty() ? NULL :
*windows.rbegin());
You can use windows.back() instead.

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_overvi...
File athena/wm/window_overview_mode.h (right):

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_overvi...
athena/wm/window_overview_mode.h:30: virtual void OnSelectWindow(aura::Window*
window, SplitType split_type) = 0;
We are going to support changing the selection on both splits from overview
mode. So we should retain OnSplitViewMode().

[pkotwicz, 2014-09-08 19:39:24]

Sadrul, can you please take a look?

This CL is now based off of https://codereview.chromium.org/550043002/

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

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:265: overview_->SelectDefaultWindow();
WOM is best positioned to select the default window, especially once the
algorithm for picking the default window becomes more complex.

Eventually, only a subset of windows will be visible in overview mode.
SelectDefaultWindow() should pick a window from that subset. (as opposed to
selecting the topmost, possibly hidden, window)

I have also changed SelectDefaultWindow() such that it is no-op if there a no
windows in overview mode. This prevents exiting overview mode when no windows
are open

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_overvi...
File athena/wm/window_overview_mode.cc (right):

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_overvi...
athena/wm/window_overview_mode.cc:111: }
I ended up keeping the old logic and making
SplitViewController::ActivateSplitMode() hide the windows which were replaced.

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_overvi...
athena/wm/window_overview_mode.cc:622: SelectWindow(windows.empty() ? NULL :
*windows.rbegin());
On 2014/09/07 05:26:49, sadrul wrote:
> You can use windows.back() instead.

Done.

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_overvi...
File athena/wm/window_overview_mode.h (right):

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_overvi...
athena/wm/window_overview_mode.h:30: virtual void OnSelectWindow(aura::Window*
window, SplitType split_type) = 0;
I renamed OnSplitViewMode() to SelectSplitViewWindow() and have added a third
argument indicating whether the left or right window should be activated

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

https://codereview.chromium.org/546123002/diff/120001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:266:
window->layer()->GetAnimator()->AbortAllAnimations();
We do not want to abort the opacity animation started in RestoreWindowState()

[sadrul, 2014-09-11 05:43:37]

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

https://codereview.chromium.org/546123002/diff/100001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:265: overview_->SelectDefaultWindow();
On 2014/09/08 19:39:23, pkotwicz wrote:
> WOM is best positioned to select the default window, especially once the
> algorithm for picking the default window becomes more complex.
> 
> Eventually, only a subset of windows will be visible in overview mode.
> SelectDefaultWindow() should pick a window from that subset. (as opposed to
> selecting the topmost, possibly hidden, window)

If the user dismisses the overview mode without actually selecting a window,
then we should restore the window that was active before getting into overview
mode, even if it is currently out of view in overview mode. That's how the
tab-selector in chrome-android also works.

> 
> I have also changed SelectDefaultWindow() such that it is no-op if there a no
> windows in overview mode. This prevents exiting overview mode when no windows
> are open
>

[pkotwicz, 2014-09-11 20:01:29]

Sadrul, can you please take another look?

You're right, the tab-selector on chrome-android activates the window which was
active prior to going into overview mode if no window was selected. Removed
WindowOverviewMode::SelectDefaultWindow()

[sadrul, 2014-09-12 04:42:06]

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

https://codereview.chromium.org/546123002/diff/160001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:193: window_list_provider_->GetWindowList();
Just const Windows windows = ...;

https://codereview.chromium.org/546123002/diff/160001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:204: return;
I think |overview_|, bezel-controller's delegate etc. gets updated indirectly
through calls to SetInOverview() from
OnSelectSplitViewWindow()/OnSelectWindow()? But that's not obvious from reading
the code. And it doesn't sound like a great thing to depend on indirectly
recursing into SetInOverview() for all this. Can this move into ToggleOverview()
instead?

[pkotwicz, 2014-09-12 20:28:20]

Sadrul, can you please take another look?

[sadrul, 2014-09-15 15:38:06]

I don't like/understand the changes in split_view_controller. Can you explain
why those are necessary? Are those necessary for this CL?

[pkotwicz, 2014-09-15 16:07:26]

The unit tests fail if the changes in split_view_controller.cc are not there.
(Because animations are instantaneous during tests) I think that the changes in
split_view_controller.cc are good changes. It makes sense for
SplitViewController::ActivateSplitMode() to hide the windows which are no longer
in split view.

[sadrul, 2014-09-15 16:13:04]

On 2014/09/15 16:07:26, pkotwicz wrote:
> The unit tests fail if the changes in split_view_controller.cc are not there.
> (Because animations are instantaneous during tests) I think that the changes
in
> split_view_controller.cc are good changes. It makes sense for
> SplitViewController::ActivateSplitMode() to hide the windows which are no
longer
> in split view.

OK. Sounds like this should be a separate change that needs to land first.

[pkotwicz, 2014-09-17 14:52:15]

Patchset #12 (id:220001) has been deleted

[pkotwicz, 2014-09-17 14:54:30]

Sadrul, can you please take another look?
https://codereview.chromium.org/573803003/ has landed and I have rebased this CL

[sadrul, 2014-09-17 18:04:44]

One question, otherwise lgtm

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

https://codereview.chromium.org/546123002/diff/240001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:117: // mode if the new window is activated.
Or the window should position itself correctly in the overview stack? Mind
filing a bug and reference it here so this gets tracked?

https://codereview.chromium.org/546123002/diff/240001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:185: windows.back()->Show();
Why do we need to explicitly Show() here?

https://codereview.chromium.org/546123002/diff/240001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:249: wm::ActivateWindow(window);
ditto

[pkotwicz, 2014-09-17 20:13:02]

Patchset #14 (id:280001) has been deleted

[pkotwicz, 2014-09-17 20:14:36]

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

[pkotwicz, 2014-09-17 20:14:38]

Oshima for OWNERS

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

https://codereview.chromium.org/546123002/diff/240001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:117: // mode if the new window is activated.
On 2014/09/17 18:04:44, sadrul wrote:
> Or the window should position itself correctly in the overview stack? Mind
> filing a bug and reference it here so this gets tracked?

Done.

https://codereview.chromium.org/546123002/diff/240001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:185: windows.back()->Show();
During unit tests, SetInOverview(false) immediately ends all of the animations
so windows.back() is hidden by the time we get here. I have added a comment to
explain this.

Non-visible windows are not activatable

[oshima, 2014-09-17 22:07:30]

lgtm with one q and nit

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

https://codereview.chromium.org/546123002/diff/240001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:179: SetInOverview(false);
It's probably cleaner if we can have two methods

EnterOverview
ExitOverview(aura::Window* to_activate);

if possible. Is it doable?

https://codereview.chromium.org/546123002/diff/260001/athena/wm/window_manage...
File athena/wm/window_manager_unittest.cc (right):

https://codereview.chromium.org/546123002/diff/260001/athena/wm/window_manage...
athena/wm/window_manager_unittest.cc:29: scoped_ptr<aura::Window>
CreateWindow(aura::WindowDelegate* delegate) {
CreateAndShowWindow?

[pkotwicz, 2014-09-17 22:24:16]

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

https://codereview.chromium.org/546123002/diff/240001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:179: SetInOverview(false);
It is doable. There are several ways of doing what you suggested. I will do it
in a separate CL

https://codereview.chromium.org/546123002/diff/260001/athena/wm/window_manage...
File athena/wm/window_manager_unittest.cc (right):

https://codereview.chromium.org/546123002/diff/260001/athena/wm/window_manage...
athena/wm/window_manager_unittest.cc:29: scoped_ptr<aura::Window>
CreateWindow(aura::WindowDelegate* delegate) {
On 2014/09/17 22:07:30, oshima wrote:
> CreateAndShowWindow?

Done.

[pkotwicz, 2014-09-17 22:24:20]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-17 22:24:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/546123002/300001

[oshima, 2014-09-17 22:32:40]

still lgtm

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

https://codereview.chromium.org/546123002/diff/240001/athena/wm/window_manage...
athena/wm/window_manager_impl.cc:179: SetInOverview(false);
On 2014/09/17 22:24:16, pkotwicz wrote:
> It is doable. There are several ways of doing what you suggested. I will do it
> in a separate CL

Great, thanks!

[commit-bot: I haz the power, 2014-09-17 23:29:54]

Committed patchset #14 (id:300001) as ff49ab54fa41d53bfcc788c103d533ec79bf81fa

[commit-bot: I haz the power, 2014-09-17 23:31:09]

Patchset 14 (id:??) landed as
https://crrev.com/712b9c49fcd36f05514525815434b81577fc7cd4
Cr-Commit-Position: refs/heads/master@{#295376}