Keep the split view activities topmost in z-order in Athena

Keep the split view activities topmost in z-order in Athena

BUG=409888
TEST=WindowManagerTest.* SplitViewController.*

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

[pkotwicz, 2014-09-08 02:27:46]

Sadrul, PTAL

This CL does NOT depend on https://codereview.chromium.org/546123002/

https://codereview.chromium.org/550043002/diff/1/athena/wm/window_manager_imp...
File athena/wm/window_manager_impl.cc (right):

https://codereview.chromium.org/550043002/diff/1/athena/wm/window_manager_imp...
athena/wm/window_manager_impl.cc:286: wm::ActivateWindow(left);
I will add a parameter to indicate which window should be activated in
https://codereview.chromium.org/546123002/

https://codereview.chromium.org/550043002/diff/1/athena/wm/window_manager_uni...
File athena/wm/window_manager_unittest.cc (right):

https://codereview.chromium.org/550043002/diff/1/athena/wm/window_manager_uni...
athena/wm/window_manager_unittest.cc:299: wm_api.wm()->ToggleSplitView();
In order for accelerators to work when an activity is active, there must a
views::FocusManager. (i.e. we need to use views::Widgets in tests instead of
aura::Windows)
I do not mind changing these tests to use views::Widgets. Though it would be
nice to clean up the "AthenaContainerLayoutManager::SetChildBounds() empty
requested bounds hack" first

[pkotwicz, 2014-09-08 02:27:57]

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

[sadrul, 2014-09-09 14:27:28]

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

https://codereview.chromium.org/550043002/diff/20001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:67: 
We allow both to be NULL.

https://codereview.chromium.org/550043002/diff/20001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:178:
container_->StackChildBelow(left_window_, right_window_);
Can the z-ordering issue described in the bug be fixed by just this block of
change?

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

https://codereview.chromium.org/550043002/diff/20001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:112: child->SetBounds(gfx::Rect(size));
Where do we activate this new window in normal mode?

https://codereview.chromium.org/550043002/diff/20001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:286: wm::ActivateWindow(left);
Both left and right can be NULL here.

https://codereview.chromium.org/550043002/diff/20001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:369:
split_view_controller_->ReplaceWindow(window, next_window);
Can you explain why this order is important?

[pkotwicz, 2014-09-09 17:06:28]

Sadrul, I think I have addressed all of your comments

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

https://codereview.chromium.org/550043002/diff/20001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:178:
container_->StackChildBelow(left_window_, right_window_);
The remainder of the change is to ensure that either |left_window_| or
|right_window_| is active. That's the reason why I have changed
ActivateSplitMode() such that one of its arguments must be the active window
(and ActivateSplitMode() cannot be called with two NULL arguments)

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

https://codereview.chromium.org/550043002/diff/20001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:112: child->SetBounds(gfx::Rect(size));
In practice we activate this window in normal mode because we do not show any
activities with views::Widget::ShowInactive(). I do not think there are any
cases where we would want to (I do not think we want to support pop-under
popups)

I activate the window explicitly when in split view mode, because the window is
added to the container prior to it being shown or activated.
SplitViewController::ReplaceWindow() would yield the incorrect stack order if
|child| is not activated.

I have thought about replacing a window in split view only once a window is
activated. The problem is that we would need to figure out an algorithm for when
activating a window will replace the current window in split view. (i.e. what
will activating the window in WindowManagerImpl::OnSelectWindow() do?)

https://codereview.chromium.org/550043002/diff/20001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:286: wm::ActivateWindow(left);
In practice, either |left| or |right| must be activated.
We do not want to get into a state where split view is active but an activity
not in split view is active and has keyboard focus.

In https://codereview.chromium.org/546123002/ I have added a third argument to
this function which indicates whether |left| or |right| should be activated. I
can move that change to this CL

https://codereview.chromium.org/550043002/diff/20001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:369:
split_view_controller_->ReplaceWindow(window, next_window);
Order is important because I changed SplitViewController::ReplaceWindow() to
require |replace_with| to be active. I should have updated the documentation in
split_view_controller.h as well.

[sadrul, 2014-09-11 05:45:48]

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

https://codereview.chromium.org/550043002/diff/20001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:178:
container_->StackChildBelow(left_window_, right_window_);
On 2014/09/09 17:06:28, pkotwicz wrote:
> The remainder of the change is to ensure that either |left_window_| or
> |right_window_| is active. That's the reason why I have changed
> ActivateSplitMode() such that one of its arguments must be the active window
> (and ActivateSplitMode() cannot be called with two NULL arguments)

We are still activating windows from various places after this change. I do not
think this is an improvement over the current state. Can we just do
StackChildAtTop(left), StackChildAtTop(right) here?

[pkotwicz, 2014-09-11 17:26:15]

Sadrul, can you please take another look?

https://codereview.chromium.org/550043002/diff/40001/athena/wm/split_view_con...
File athena/wm/split_view_controller_unittest.cc (right):

https://codereview.chromium.org/550043002/diff/40001/athena/wm/split_view_con...
athena/wm/split_view_controller_unittest.cc:111:
EXPECT_TRUE(SplitViewWindowsTopmost());
The tests happen to pass without the changes in this CL in
split_view_controller.cc
However, SplitViewController::ActivateSplitMode() did not guarantee that the
split view windows were the topmost windows.

e.g. Calling controller->ActivateSplitMode(windows[0], NULL) after line 111
would have not resulted in the split view windows being topmost.

[sadrul, 2014-09-12 04:32:29]

LGTM

https://codereview.chromium.org/550043002/diff/40001/athena/wm/split_view_con...
File athena/wm/split_view_controller_unittest.cc (right):

https://codereview.chromium.org/550043002/diff/40001/athena/wm/split_view_con...
athena/wm/split_view_controller_unittest.cc:111:
EXPECT_TRUE(SplitViewWindowsTopmost());
On 2014/09/11 17:26:15, pkotwicz wrote:
> The tests happen to pass without the changes in this CL in
> split_view_controller.cc
> However, SplitViewController::ActivateSplitMode() did not guarantee that the
> split view windows were the topmost windows.
> 
> e.g. Calling controller->ActivateSplitMode(windows[0], NULL) after line 111
> would have not resulted in the split view windows being topmost.

Can you add that test-case here?

[sadrul, 2014-09-12 04:32:44]

Oh, you should also update the CL description.

[pkotwicz, 2014-09-12 14:27:17]

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

[pkotwicz, 2014-09-12 14:27:26]

Oshima for OWNERS

[oshima, 2014-09-12 18:48:41]

https://codereview.chromium.org/550043002/diff/40001/athena/wm/split_view_con...
File athena/wm/split_view_controller_unittest.cc (right):

https://codereview.chromium.org/550043002/diff/40001/athena/wm/split_view_con...
athena/wm/split_view_controller_unittest.cc:111:
EXPECT_TRUE(SplitViewWindowsTopmost());
On 2014/09/12 04:32:29, sadrul wrote:
> On 2014/09/11 17:26:15, pkotwicz wrote:
> > The tests happen to pass without the changes in this CL in
> > split_view_controller.cc
> > However, SplitViewController::ActivateSplitMode() did not guarantee that the
> > split view windows were the topmost windows.
> > 
> > e.g. Calling controller->ActivateSplitMode(windows[0], NULL) after line 111
> > would have not resulted in the split view windows being topmost.
> 
> Can you add that test-case here?

You probably missed this?

[pkotwicz, 2014-09-12 20:11:30]

Oshima, can you please take another look? I have added the extra test case

[oshima, 2014-09-12 20:19:49]

lgtm with nit

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

https://codereview.chromium.org/550043002/diff/60001/athena/wm/split_view_con...
athena/wm/split_view_controller_unittest.cc:36: bool SplitViewWindowsTopmost()
const {
AreSplitViewWindowsTopmost()

[pkotwicz, 2014-09-12 20:24:54]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-12 20:32:54]

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

[commit-bot: I haz the power, 2014-09-12 21:34:18]

Committed patchset #4 (id:60001) as 1151723cc986467f8cbbfd5725daf06f7d299096

[commit-bot: I haz the power, 2014-09-12 21:38:30]

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