Update shelf when overlapping dragged windows exist

Update shelf when overlapping dragged windows exist

BUG=321704
TEST=ash_unittests --gtest_filter=*WorkspaceControllerTestDragging.DragWindowOverlapShelf*

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

[varkha, 2013-11-28 02:03:47]

oshima@, could you please take a look? The problem here is that during drag
windows are parented to docked container and the WorkspaceController needs to be
aware that docked children can also overlap the shelf. Thanks!

[oshima, 2013-12-02 19:34:41]

I believe this question must have been asked and already answered before, but
let me ask for my understanding (and future review)

Why do we need to move to docked_container_ during drag?

https://codereview.chromium.org/93103003/diff/20001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/93103003/diff/20001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:486:
dock_container_->GetRootWindow());
ForLauncher takes a window, so you can just pass dock_container_ (or child)

[varkha, 2013-12-02 20:26:19]

> Why do we need to move to docked_container_ during drag?

We need to have already docked windows (and any windows in fact) to float on top
of the docked windows when dragged. We also wanted to keep all docked windows
above desktop windows at all times (but below attached panels). Those two
conditions combined with the way windows stacking order is maintained suggested
this reparenting during the drag. As a replacement we could implement some way
to elevate a window and its transient children while it is being dragged
regardless of their parent container but this presently would be broken in many
places I would think.

https://codereview.chromium.org/93103003/diff/20001/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/93103003/diff/20001/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:486:
dock_container_->GetRootWindow());
On 2013/12/02 19:34:42, oshima wrote:
> ForLauncher takes a window, so you can just pass dock_container_ (or child)

Done.

[oshima, 2013-12-02 21:00:44]

change lg. can you add test for this scenario?


On Mon, Dec 2, 2013 at 12:26 PM, <varkha@chromium.org> wrote:

> Why do we need to move to docked_container_ during drag?
>>
>
> We need to have already docked windows (and any windows in fact) to float
> on top
> of the docked windows when dragged. We also wanted to keep all docked
> windows
> above desktop windows at all times (but below attached panels).Those two

conditions combined with the way windows stacking order is maintained
> suggested
> this reparenting during the drag. As a replacement we could implement some
> way
> to elevate a window and its transient children while it is being dragged
> regardless of their parent container but this presently would be broken in
> many
> places I would think.
>
>
I see, thank you for the expplanatin. That makes me feel that a having
single container for
docked/undocked window may be better (although I do understand it has its
own cons),
but that's separate discussion.


>
>
> https://codereview.chromium.org/93103003/diff/20001/ash/
> wm/dock/docked_window_layout_manager.cc
> File ash/wm/dock/docked_window_layout_manager.cc (right):
>
> https://codereview.chromium.org/93103003/diff/20001/ash/
> wm/dock/docked_window_layout_manager.cc#newcode486
> ash/wm/dock/docked_window_layout_manager.cc:486:
> dock_container_->GetRootWindow());
> On 2013/12/02 19:34:42, oshima wrote:
>
>> ForLauncher takes a window, so you can just pass dock_container_ (or
>>
> child)
>
> Done.
>
> https://codereview.chromium.org/93103003/
>

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

[varkha, 2013-12-03 17:07:03]

On 2013/12/02 21:00:44, oshima wrote:
> change lg. can you add test for this scenario?

Done. PTAL.

[oshima, 2013-12-03 17:31:27]

https://codereview.chromium.org/93103003/diff/60005/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/93103003/diff/60005/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:488: if (shelf_layout)
I forgot to ask.

Can this be NULL?

https://codereview.chromium.org/93103003/diff/60005/ash/wm/workspace_controll...
File ash/wm/workspace_controller_unittest.cc (right):

https://codereview.chromium.org/93103003/diff/60005/ash/wm/workspace_controll...
ash/wm/workspace_controller_unittest.cc:1452: ::testing::Values("mouse", "mouse
docked"));
why not just boolean? Is this recommended pattern?

[varkha, 2013-12-03 18:01:27]

https://codereview.chromium.org/93103003/diff/60005/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/93103003/diff/60005/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:488: if (shelf_layout)
On 2013/12/03 17:31:28, oshima wrote:
> I forgot to ask.
> 
> Can this be NULL?

In Ash it can only be NULL during startup or shutdown, and docked layout manager
lifetime seems to be enclosed in shelf lifetime (see
RootWindowController::InitLayoutManagers). I just didn't want to assume this
dependency following what I see elsewhere in the code such as here
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/workspace/w....

https://codereview.chromium.org/93103003/diff/60005/ash/wm/workspace_controll...
File ash/wm/workspace_controller_unittest.cc (right):

https://codereview.chromium.org/93103003/diff/60005/ash/wm/workspace_controll...
ash/wm/workspace_controller_unittest.cc:1452: ::testing::Values("mouse", "mouse
docked"));
On 2013/12/03 17:31:28, oshima wrote:
> why not just boolean? Is this recommended pattern?

I used this elsewhere where there were more cases and I liked how this gives you
an informative string in the test output rather than 0/1. I've changed it to
testing::Bool - let me know if you like this better.

[oshima, 2013-12-03 18:08:49]

lgtm

https://codereview.chromium.org/93103003/diff/60005/ash/wm/dock/docked_window...
File ash/wm/dock/docked_window_layout_manager.cc (right):

https://codereview.chromium.org/93103003/diff/60005/ash/wm/dock/docked_window...
ash/wm/dock/docked_window_layout_manager.cc:488: if (shelf_layout)
On 2013/12/03 18:01:27, varkha wrote:
> On 2013/12/03 17:31:28, oshima wrote:
> > I forgot to ask.
> > 
> > Can this be NULL?
> 
> In Ash it can only be NULL during startup or shutdown, and docked layout
manager
> lifetime seems to be enclosed in shelf lifetime (see
> RootWindowController::InitLayoutManagers). I just didn't want to assume this
> dependency following what I see elsewhere in the code such as here
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/workspace/w....

I believe this is from ancient spec where shelf was only on primary display.
This is fine for now, but we probably should revisit this later.

https://codereview.chromium.org/93103003/diff/60005/ash/wm/workspace_controll...
File ash/wm/workspace_controller_unittest.cc (right):

https://codereview.chromium.org/93103003/diff/60005/ash/wm/workspace_controll...
ash/wm/workspace_controller_unittest.cc:1452: ::testing::Values("mouse", "mouse
docked"));
On 2013/12/03 18:01:27, varkha wrote:
> On 2013/12/03 17:31:28, oshima wrote:
> > why not just boolean? Is this recommended pattern?
> 
> I used this elsewhere where there were more cases and I liked how this gives
you
> an informative string in the test output rather than 0/1. I've changed it to
> testing::Bool - let me know if you like this better.

I'd just use local enum rather than string. Bool is ok too. I'll leave it to
you.

[varkha, 2013-12-03 18:14:45]

https://codereview.chromium.org/93103003/diff/60005/ash/wm/workspace_controll...
File ash/wm/workspace_controller_unittest.cc (right):

https://codereview.chromium.org/93103003/diff/60005/ash/wm/workspace_controll...
ash/wm/workspace_controller_unittest.cc:1452: ::testing::Values("mouse", "mouse
docked"));
On 2013/12/03 18:08:49, oshima wrote:
> On 2013/12/03 18:01:27, varkha wrote:
> > On 2013/12/03 17:31:28, oshima wrote:
> > > why not just boolean? Is this recommended pattern?
> > 
> > I used this elsewhere where there were more cases and I liked how this gives
> you
> > an informative string in the test output rather than 0/1. I've changed it to
> > testing::Bool - let me know if you like this better.
> 
> I'd just use local enum rather than string. Bool is ok too. I'll leave it to
> you.

There (tab_drag_controller_interactive_uitest.cc) there were simply too many
cases and their combinations were not really informative). In this test however
I agree, Bool seems better suited for the task.

[commit-bot: I haz the power, 2013-12-03 18:15:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/93103003/80001

[commit-bot: I haz the power, 2013-12-03 19:15:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/93103003/80001

[commit-bot: I haz the power, 2013-12-03 19:44:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/93103003/100001

[commit-bot: I haz the power, 2013-12-03 23:44:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varkha@chromium.org/93103003/100001

[commit-bot: I haz the power, 2013-12-04 00:56:24]

Change committed as 238507