athena: Fix switching windows with title-drag in split-view.

athena: Fix switching windows with title-drag in split-view.

When split-view mode, make sure dragging the tiel to switch between
windows works as expected.

BUG=397272
R=mfomitchev@chromium.org, mukai@chromium.org

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

[sadrul, 2014-08-13 05:29:45]

This goes on top of https://codereview.chromium.org/459613008/

[Jun Mukai, 2014-08-13 23:38:59]

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

https://codereview.chromium.org/468763002/diff/40001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:248: const aura::Window::Windows& windows =
container_->children();
Possibly you should use mru_window_tracker_ here.

[mfomitchev, 2014-08-14 14:38:13]

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

https://codereview.chromium.org/468763002/diff/40001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:111: CHECK_EQ(container_, window->parent());
might make more sense to check that windows belongs to
window_list_provider_->GetWindowList() instead.

https://codereview.chromium.org/468763002/diff/40001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:115:
replace_with->SetTransform(gfx::Transform());
Any reason we wouldn't want to reset the bounds on window to container bounds
and reset the transform to gfx::Transform()? Typically we leave the windows in
untransformed state after we are done with them in SplitViewController.

https://codereview.chromium.org/468763002/diff/40001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:120:
container_->StackChildAbove(replace_with, window);
I think we should use window_list_provider_->MoveToFront() here. Or add
StackAbove() method to WindowListProvider interface if we need to.  Or use
ActivateWindow() and have MruWindowTracker start listening to activations.
Otherwise we'd have the order of the windows in WindowListProvider get out of
sync with the stack-order of the windows in the container.

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

https://codereview.chromium.org/468763002/diff/40001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:248: const aura::Window::Windows& windows =
container_->children();
+1

https://codereview.chromium.org/468763002/diff/40001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:263: behind = *iter;
We need to set behind to NULL if iter == windows.rend() and the other two
conditions hold. We can also simplify the code:

if (behind != right && behind != left)
  return behind
if (iter == windows.rend())
  return NULL;
// Optional: CHECK(iter != right && iter != left);
return iter;

[sadrul, 2014-08-15 19:00:20]

Addressed the comment (the latest patchset is on top of
https://codereview.chromium.org/470083004/)

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

https://codereview.chromium.org/468763002/diff/40001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:111: CHECK_EQ(container_, window->parent());
On 2014/08/14 14:38:13, mfomitchev wrote:
> might make more sense to check that windows belongs to
> window_list_provider_->GetWindowList() instead.

Done.

https://codereview.chromium.org/468763002/diff/40001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:115:
replace_with->SetTransform(gfx::Transform());
On 2014/08/14 14:38:13, mfomitchev wrote:
> Any reason we wouldn't want to reset the bounds on window to container bounds
> and reset the transform to gfx::Transform()? Typically we leave the windows in
> untransformed state after we are done with them in SplitViewController.

I added the change to reset the transform on |window|. We shouldn't reset the
bounds, because the user may switch back to the window, and we don't want the
unnecessary re-layouts (e.g. we don't resize all the tabs in chrome when the
window resizes, until the user switches to that window).

https://codereview.chromium.org/468763002/diff/40001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:120:
container_->StackChildAbove(replace_with, window);
On 2014/08/14 14:38:13, mfomitchev wrote:
> I think we should use window_list_provider_->MoveToFront() here. Or add
> StackAbove() method to WindowListProvider interface if we need to.  Or use
> ActivateWindow() and have MruWindowTracker start listening to activations.
> Otherwise we'd have the order of the windows in WindowListProvider get out of
> sync with the stack-order of the windows in the container.

Done (calling wm::ActivateWindow()).

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

https://codereview.chromium.org/468763002/diff/40001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:248: const aura::Window::Windows& windows =
container_->children();
On 2014/08/14 14:38:13, mfomitchev wrote:
> +1

Done.

https://codereview.chromium.org/468763002/diff/40001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:263: behind = *iter;
On 2014/08/14 14:38:13, mfomitchev wrote:
> We need to set behind to NULL if iter == windows.rend() and the other two
> conditions hold.

Good catch. Done.

> We can also simplify the code:
> 
> if (behind != right && behind != left)
>   return behind
> if (iter == windows.rend())
>   return NULL;
> // Optional: CHECK(iter != right && iter != left);
> return iter;

[mfomitchev, 2014-08-15 19:15:04]

lgtm with nits

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

https://codereview.chromium.org/468763002/diff/40001/athena/wm/split_view_con...
athena/wm/split_view_controller.cc:115:
replace_with->SetTransform(gfx::Transform());
On 2014/08/15 19:00:20, sadrul wrote:
> On 2014/08/14 14:38:13, mfomitchev wrote:
> > Any reason we wouldn't want to reset the bounds on window to container
bounds
> > and reset the transform to gfx::Transform()? Typically we leave the windows
in
> > untransformed state after we are done with them in SplitViewController.
> 
> I added the change to reset the transform on |window|. We shouldn't reset the
> bounds, because the user may switch back to the window, and we don't want the
> unnecessary re-layouts (e.g. we don't resize all the tabs in chrome when the
> window resizes, until the user switches to that window).

Acknowledged.

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

https://codereview.chromium.org/468763002/diff/40001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:263: behind = *iter;
Nit: I still prefer the code I suggested above - less lines, less nested ifs,
and no unnecessary if checks after you've already set |behind|.
Also: would be good to have a unit test for GetWindowBehind()

[sadrul, 2014-08-15 19:34:54]

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

https://codereview.chromium.org/468763002/diff/40001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:263: behind = *iter;
On 2014/08/15 19:15:03, mfomitchev_OOO_Aug16-24 wrote:
> Nit: I still prefer the code I suggested above - less lines, less nested ifs,
> and no unnecessary if checks after you've already set |behind|.

> if (behind != right && behind != left)
>   return behind
> if (iter == windows.rend())
>   return NULL;
> // Optional: CHECK(iter != right && iter != left);
> return iter;

This doesn't work right because the CHECK would trigger often (e.g. you switch
to split mode and the left window is active. You start title-dragging the
left-window. It's going to return the right-window)

> Also: would be good to have a unit test for GetWindowBehind()

Indeed. I am working on a separate patch to expose WindowManagerImpl out of
window_manager_impl.cc, so that it can be tested. I will add the tests in that
CL.

[sadrul, 2014-08-15 19:46:43]

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

https://codereview.chromium.org/468763002/diff/40001/athena/wm/window_manager...
athena/wm/window_manager_impl.cc:263: behind = *iter;
On 2014/08/15 19:34:54, sadrul wrote:
> On 2014/08/15 19:15:03, mfomitchev_OOO_Aug16-24 wrote:
> > Nit: I still prefer the code I suggested above - less lines, less nested
ifs,
> > and no unnecessary if checks after you've already set |behind|.
> 
> > if (behind != right && behind != left)
> >   return behind
> > if (iter == windows.rend())
> >   return NULL;
> > // Optional: CHECK(iter != right && iter != left);
> > return iter;
> 
> This doesn't work right because the CHECK would trigger often (e.g. you switch
> to split mode and the left window is active. You start title-dragging the
> left-window. It's going to return the right-window)

Mikhail pointed out that this actually does do the right thing. Thanks! Made
this change.

> 
> > Also: would be good to have a unit test for GetWindowBehind()
> 
> Indeed. I am working on a separate patch to expose WindowManagerImpl out of
> window_manager_impl.cc, so that it can be tested. I will add the tests in that
> CL.

[Jun Mukai, 2014-08-15 19:50:35]

lgtm

[mfomitchev, 2014-08-15 19:51:13]

Cool! LGTM

[sadrul, 2014-08-15 21:11:01]

Committed patchset #6 manually as 290008 (presubmit successful).