[Athena] Fix switching activities by swiping from the right bezel

[Athena] Fix switching activities by swiping from the right bezel

BUG=414936
TEST=WindowManagertest.BezelGestureToSwitchBetweenWindows

Committed: https://crrev.com/b166746fc2820f27cecfaa80cd97edaaf00296af
Cr-Commit-Position: refs/heads/master@{#296548}

[pkotwicz, 2014-09-17 00:55:22]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2014-09-17 01:03:09]

Patchset #1 (id:20001) has been deleted

[pkotwicz, 2014-09-17 01:04:06]

Patchset #1 (id:40001) has been deleted

[pkotwicz, 2014-09-17 01:10:04]

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

[pkotwicz, 2014-09-17 01:10:05]

mfomitchev@ PTAL

The change in bezel_controller.cc is to make the unit test not crash due to
BezelController::SetState() being reentrant

I do not mind rebasing my CL on top of your CL if that makes things easier for
you

[pkotwicz, 2014-09-17 01:11:14]

Patchset #1 (id:60001) has been deleted

[mfomitchev, 2014-09-18 02:36:30]

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

https://codereview.chromium.org/574113004/diff/80001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:125:
window_list_provider_->MoveToFront(left_window_);
ActivateWindow already moves the window to front with our window list provider
implementation, so this seems a bit excessive. We should be able to get rid of
one of the MoveToFront calls. Also, |replace_with| should probably be on top of
the other split view window, since |replace_with| is the active one. So the
Activate call should be after the MoveToFront call.

[pkotwicz, 2014-09-18 13:53:01]

Patchset #2 (id:100001) has been deleted

[pkotwicz, 2014-09-18 13:54:45]

mfomitchev@ can you please take another look?

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

https://codereview.chromium.org/574113004/diff/80001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:125:
window_list_provider_->MoveToFront(left_window_);
I have removed the call to wm::ActivateWindow(). This is consistent with the
remainder of SplitViewController which does not do anything with activation.

We do not explicitly call wm::ActivateWindow() prior to calling ReplaceWindow()
in WindowManagerImpl::OnWindowAddedToLayout(). In practice, |child|, being a
views::Widget is activated when views::Widget::Show() is called. Ideally we
would wait till the new window is activated prior to replacing the split view
window.

w.r.t. to the relative z-order of the left and right windows, we do not care
about it in the case of ActivateSplitMode() so I do not think that we should
care about it in the ReplaceWindow() case either. In patch set #1 of
https://codereview.chromium.org/550043002/#ps1 I attempted to change
ActivateSplitMode() such that one of the passed in windows had to be active but
sadrul@ did not like that.

[mfomitchev, 2014-09-18 18:11:52]

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

https://codereview.chromium.org/574113004/diff/80001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:125:
window_list_provider_->MoveToFront(left_window_);
We still use ActivateWindow in ScrollEnd - that's how we ensure that the window
that ends up displayed when is on top in the window list.

I think making sure the invariant "active window is the top one in the stack"
always holds would be a good thing.

In this particular case, the call is typically done when you swipe on the header
of one of the visible windows. If I do this on the left window, I'd expect the
left window to remain active. E.g. if I start typing, I'd expect keyboard input
to go to that window.

Maybe Sadrul, you, and I could have a quick chat on this.

[pkotwicz, 2014-09-19 14:01:04]

I think that chatting with Sadrul is a good idea. I am going to wait till Sadrul
gets in the office to proceed with this CL

[pkotwicz, 2014-09-19 21:49:25]

Patchset #3 (id:140001) has been deleted

[pkotwicz, 2014-09-19 21:58:00]

Mikhail, can you please take another look? I have updated the API as discussed

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

https://codereview.chromium.org/574113004/diff/160001/athena/wm/split_view_co...
athena/wm/split_view_controller.cc:114: if (to_activate)
I am activating the window after calling UpdateLayout() because a window must be
visible in order to be activatable

https://codereview.chromium.org/574113004/diff/160001/athena/wm/window_list_p...
File athena/wm/window_list_provider_impl.cc (right):

https://codereview.chromium.org/574113004/diff/160001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl.cc:80: aura::Window* reference_window) {
I did not add a DCHECK here because it would break AppActivityProxy::Init()

In that case a views::Widget gets activated by mistake. In the long term
ActivityManager::AddActivity() should take a parameter to indicate whether the
activity should be activated

https://codereview.chromium.org/574113004/diff/160001/athena/wm/window_manage...
File athena/wm/window_manager_unittest.cc (left):

https://codereview.chromium.org/574113004/diff/160001/athena/wm/window_manage...
athena/wm/window_manager_unittest.cc:308: generator.PressKey(ui::VKEY_F6,
ui::EF_CONTROL_DOWN);
The accelerator does not work anymore because the |w1| is activated. 
Making |w1| a views::Widget (which has a views::FocusManager) instead of an
aura::Window makes the accelerator work.

Calling WindowManagerImpl::ToggleSplitView() makes most sense in my opinion

[mfomitchev, 2014-09-22 18:04:24]

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

https://codereview.chromium.org/574113004/diff/160001/athena/wm/split_view_co...
athena/wm/split_view_controller.cc:124: DCHECK_EQ(active_window, right_window_);
I'd actually make it into a CHECK for now so that we hit it easily if it fails.
It's a pretty important, and probably a pretty fragile check. There's a bunch of
CHECKs in Athena which could be DCHECKs. At some point when we are close to
releasing we can do a sweeping CHECK->DCHECK switch.

https://codereview.chromium.org/574113004/diff/160001/athena/wm/window_list_p...
File athena/wm/window_list_provider_impl.cc (right):

https://codereview.chromium.org/574113004/diff/160001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl.cc:80: aura::Window* reference_window) {
Perhaps we should create a bug for this? 
Also, can we add a check in StackWindowFrontOf which ensures that
reference_window is not an active window?

[pkotwicz, 2014-09-22 18:30:25]

Mikhail, can you please take another look?

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

https://codereview.chromium.org/574113004/diff/160001/athena/wm/split_view_co...
athena/wm/split_view_controller.cc:124: DCHECK_EQ(active_window, right_window_);
On 2014/09/22 18:04:24, mfomitchev wrote:
> I'd actually make it into a CHECK for now so that we hit it easily if it
fails.
> It's a pretty important, and probably a pretty fragile check. There's a bunch
of
> CHECKs in Athena which could be DCHECKs. At some point when we are close to
> releasing we can do a sweeping CHECK->DCHECK switch.

Done.

https://codereview.chromium.org/574113004/diff/160001/athena/wm/window_list_p...
File athena/wm/window_list_provider_impl.cc (right):

https://codereview.chromium.org/574113004/diff/160001/athena/wm/window_list_p...
athena/wm/window_list_provider_impl.cc:80: aura::Window* reference_window) {
I have filed a bug at crbug.com/416549

[mfomitchev, 2014-09-22 18:31:33]

LGTM

[pkotwicz, 2014-09-22 18:32:45]

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

[pkotwicz, 2014-09-22 18:32:46]

Sadrul, can you please take a look?

[sadrul, 2014-09-23 16:18:33]

LGTM

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

https://codereview.chromium.org/574113004/diff/180001/athena/wm/split_view_co...
athena/wm/split_view_controller.cc:115: wm::ActivateWindow(to_activate);
Add a CHECK here that to_activate is one of left or right (you have an
equivalent check below in lines 121:125, but having the CHECK here would make it
more obvious when reading the code)

I think you should also have an else block here, so the rest of the function
could look like:

  if (to_activate) {
    CHECK(to_activate == left_window_ || to_active == right_window_);
    stack_behind = to_activate == left_window_ ? right_window_ : left_window_;
  } else {
    // If either of the left/right windows is active, then keep it active, and
    // stack the other window behind it. Otherwise, activate the left window,
    // and stack the window on the right behind it.
    aura::Window* active = GetActiveWindow();
    if (active == left_window_) {
      stack_behind = right_window_;
    } else if (active == right_window_) {
      stack_behind = left_window_;
    } else {
      to_activate = left_window_;
      stack_behind = right_window_;
    }
  }

  if (to_activate)
    wm::ActivateWindow(to_activate);
  CHECK(stack_behind);
  window_list_provider_->StackWindowBehindTo(stack_behind, GetActiveWindow());

https://codereview.chromium.org/574113004/diff/180001/athena/wm/split_view_co...
File athena/wm/split_view_controller.h (right):

https://codereview.chromium.org/574113004/diff/180001/athena/wm/split_view_co...
athena/wm/split_view_controller.h:37: void ActivateSplitMode(aura::Window* left,
Add comments about |to_activate|

[pkotwicz, 2014-09-24 14:59:22]

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

[pkotwicz, 2014-09-24 14:59:23]

Oshima for OWNERS

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

https://codereview.chromium.org/574113004/diff/180001/athena/wm/split_view_co...
athena/wm/split_view_controller.cc:115: wm::ActivateWindow(to_activate);
I have changed things around to make them clearer. I have kept initializing
|stack_behind| out of the if/else block though.

[oshima, 2014-09-24 19:42:33]

lgtm

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

https://codereview.chromium.org/574113004/diff/220001/athena/wm/split_view_co...
athena/wm/split_view_controller.cc:230: active_window != right_window_) {
can you document when this can happen?

[pkotwicz, 2014-09-24 20:47:08]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2014-09-24 20:47:13]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2014-09-24 21:05:02]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2014-09-24 21:05:04]

The CQ bit was unchecked by pkotwicz@chromium.org

[pkotwicz, 2014-09-24 21:05:12]

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

https://codereview.chromium.org/574113004/diff/220001/athena/wm/split_view_co...
athena/wm/split_view_controller.cc:230: active_window != right_window_) {
I've made this happen by pressing Ctrl+F6 while touching the minimized home card

[pkotwicz, 2014-09-24 21:05:16]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-24 21:06:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/574113004/240001

[commit-bot: I haz the power, 2014-09-24 21:10:21]

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

[commit-bot: I haz the power, 2014-09-24 21:10:22]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pkotwicz, 2014-09-24 22:10:52]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-24 22:12:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/574113004/260001

[commit-bot: I haz the power, 2014-09-24 22:57:28]

Committed patchset #8 (id:260001) as 1336694d5430fd7d6f90b20a0fcb9b144c1a5d8e

[commit-bot: I haz the power, 2014-09-24 22:58:02]

Patchset 8 (id:??) landed as
https://crrev.com/b166746fc2820f27cecfaa80cd97edaaf00296af
Cr-Commit-Position: refs/heads/master@{#296548}

[Mr4D (OOO till 08-26), 2014-09-26 22:12:25]

I do not quite understand why WindowListProvider::MoveToFront got removed. This
was supposed to be a general accessor to the order of activities.

[pkotwicz, 2014-09-27 19:55:59]

I removed WindowListProvider::MoveToFront() because activating an activity
produces the same effect as WindowListProvider::MoveToFront()

I think that "activating an activity causes it to move to the front" is better
than "moving an activity to the front causes the activity to be activated".
Sorry for not CCing you on the CL. I will CC you on future CLs which touch code
that you have written
We can bring back WindowListProvider::MoveToFront() if we find out that it is
needed. If we do that we would need to change code which assumes that the
topmost activity is the active activity.

[oshima, 2014-09-29 19:56:12]

On 2014/09/27 19:55:59, pkotwicz wrote:
> I removed WindowListProvider::MoveToFront() because activating an activity
> produces the same effect as WindowListProvider::MoveToFront()
> 
> I think that "activating an activity causes it to move to the front" is better
> than "moving an activity to the front causes the activity to be activated".
> Sorry for not CCing you on the CL. I will CC you on future CLs which touch
code
> that you have written
> We can bring back WindowListProvider::MoveToFront() if we find out that it is
> needed. If we do that we would need to change code which assumes that the
> topmost activity is the active activity.

I agree with Peter here. athena/wm is aura level API, and we already have API to
activate, so we should uses that. If we need a method to move a window w/o
activation,
we can add MoveToFront back.