Attach panel while dragging to bring it in front of other panels.

Attach panel while dragging to bring it in front of other panels.

BUG=181249
TEST=PanelWindowResizerTest.DragMovesToPanelLayer


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

[flackr, 2013-03-13 19:20:26]

Hey, please take a look. I added a few extra checks to the panel detach reattach
checks just to make sure panels end up attached to the correct layers as well.
Let me know what you think. Thanks.

https://codereview.chromium.org/12441010/diff/1/ash/wm/panels/panel_window_re...
File ash/wm/panels/panel_window_resizer.cc (right):

https://codereview.chromium.org/12441010/diff/1/ash/wm/panels/panel_window_re...
ash/wm/panels/panel_window_resizer.cc:163:
root_window_controller->panel_container_handler();
Right now there's no way to get the drag/resize handler for a layer so I reach
into the root window controller. If this isn't a one off behavior maybe we want
to know what the drag/resize handler for a container is?

[stevenjb, 2013-03-13 19:31:39]

+sky@
I haven't looked too deeply at the resizer or drag handler code yet and will
need to stare at it a bit before I fell confident about reviewing this.

https://codereview.chromium.org/12441010/diff/4002/ash/wm/drag_window_resizer...
File ash/wm/drag_window_resizer_unittest.cc (right):

https://codereview.chromium.org/12441010/diff/4002/ash/wm/drag_window_resizer...
ash/wm/drag_window_resizer_unittest.cc:109: NULL, window, point_in_parent,
window_component).release());
NULL /* owner */

https://codereview.chromium.org/12441010/diff/4002/ash/wm/gestures/two_finger...
File ash/wm/gestures/two_finger_drag_handler.cc (right):

https://codereview.chromium.org/12441010/diff/4002/ash/wm/gestures/two_finger...
ash/wm/gestures/two_finger_drag_handler.cc:103: window_resizer_ =
CreateWindowResizer(/* owner */ NULL, target,
NULL /* owner */

[flackr, 2013-03-13 19:41:22]

Alternately, we could try having a single TopLevelWindowEventHandler for the
various containers and then we wouldn't have to pass the resizer to another
handler.

[sky, 2013-03-13 21:21:34]

https://codereview.chromium.org/12441010/diff/4002/ash/wm/window_resizer_owner.h
File ash/wm/window_resizer_owner.h (right):

https://codereview.chromium.org/12441010/diff/4002/ash/wm/window_resizer_owne...
ash/wm/window_resizer_owner.h:17: virtual void PassResizer(WindowResizerOwner*
new_owner) = 0;
This seems specific to panels complicates the implementation. Can this be kept
specific to the panels size? In particular I don't think we should change
WindowResizer.

[flackr, 2013-03-14 20:02:15]

https://codereview.chromium.org/12441010/diff/4002/ash/wm/window_resizer_owner.h
File ash/wm/window_resizer_owner.h (right):

https://codereview.chromium.org/12441010/diff/4002/ash/wm/window_resizer_owne...
ash/wm/window_resizer_owner.h:17: virtual void PassResizer(WindowResizerOwner*
new_owner) = 0;
On 2013/03/13 21:21:34, sky wrote:
> This seems specific to panels complicates the implementation. Can this be kept
> specific to the panels size? In particular I don't think we should change
> WindowResizer.

Okay, I've made this more specific. Rather than passing the resize owner along
with WindowResizer the PanelWindowResizer now gets it from the
WorkspaceController. It does still require a PassResizer method on
ToplevelWindowEventHandler though.

https://codereview.chromium.org/12441010/diff/11001/ash/wm/toplevel_window_ev...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/12441010/diff/11001/ash/wm/toplevel_window_ev...
ash/wm/toplevel_window_event_handler.cc:336: if (!window_resizer_.get())
Needed because in panel_window_resizer_unittest.cc the test owns the resizer and
this is NULL.

[sky, 2013-03-14 20:34:46]

I don't think I understand this. Why doesn't the EventHandler for panels create
the WindowResizer in the first place?

https://codereview.chromium.org/12441010/diff/11001/ash/wm/workspace_controll...
File ash/wm/workspace_controller.h (right):

https://codereview.chromium.org/12441010/diff/11001/ash/wm/workspace_controll...
ash/wm/workspace_controller.h:45: WorkspaceEventHandler*
GetEventHandlerForWindow(aura::Window* window);
I have tried to keep the surface area of API exposed by the workspace code very
narrow. I don't want to start openning this up.

[flackr, 2013-03-14 20:42:04]

When the panel is detached it's reparented to the workspace container like a
regular window that can be stacked below / above other windows. However, this
means that the workspace event handler gets the drag events and creates the
resizer for the panel.

I initially tried to add a pretargeteventhandler to the panel before moving it
to the workspace container however the workspace container's pretarget handler
gets the events first, before the added handler.

https://codereview.chromium.org/12441010/diff/11001/ash/wm/workspace_controll...
File ash/wm/workspace_controller.h (right):

https://codereview.chromium.org/12441010/diff/11001/ash/wm/workspace_controll...
ash/wm/workspace_controller.h:45: WorkspaceEventHandler*
GetEventHandlerForWindow(aura::Window* window);
On 2013/03/14 20:34:46, sky wrote:
> I have tried to keep the surface area of API exposed by the workspace code
very
> narrow. I don't want to start openning this up.

I could cast it to a ToplevelWindowEventHandler* before returning it if that
would help.

[flackr, 2013-03-16 13:33:32]

Hi, as discussed I've changed the code such that the
ToplevelWindowEventHandler::ScopedWindowResizer temporarily adds handlers on the
dragged window's new parent, only if the property kContinueDragAfterReparent is
set. Please take a look. Also, I don't think we currently cancel drags when the
window is reparented. We seem to only reset the resize handler when the window
is destroyed. I can add this to
ToplevelWindowEventHandler::ScopedWindowResizer::OnWindowHierarchyChanging if we
should be.

[sky, 2013-03-18 16:17:47]

I get that we don't cancel drags now if a window is reparented now, but
shouldn't we? Otherwise won't we leave the toplevelwindoweventfilter in a bad
state?

https://codereview.chromium.org/12441010/diff/22001/ash/wm/toplevel_window_ev...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/12441010/diff/22001/ash/wm/toplevel_window_ev...
ash/wm/toplevel_window_event_handler.cc:82: aura::Window*
extra_handler_container_;
Can you name this target_contaienr_ ?

https://codereview.chromium.org/12441010/diff/22001/ash/wm/toplevel_window_ev...
File ash/wm/toplevel_window_event_handler.h (right):

https://codereview.chromium.org/12441010/diff/22001/ash/wm/toplevel_window_ev...
ash/wm/toplevel_window_event_handler.h:40: aura::Window* owner() const { return
owner_; }
const aura::Window* owner() const
If you need a non-const version add that too, but it should then be non-const on
both, eg:
aura::Window* owner() { ...

[flackr, 2013-03-20 01:59:32]

Definitely agree, just wanted to be sure there wasn't any reason we didn't. It
does leave the ToplevelWindowEventHandler in an inconsistent state. Done.

https://codereview.chromium.org/12441010/diff/22001/ash/wm/toplevel_window_ev...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/12441010/diff/22001/ash/wm/toplevel_window_ev...
ash/wm/toplevel_window_event_handler.cc:82: aura::Window*
extra_handler_container_;
On 2013/03/18 16:17:47, sky wrote:
> Can you name this target_contaienr_ ?

Done.

https://codereview.chromium.org/12441010/diff/22001/ash/wm/toplevel_window_ev...
File ash/wm/toplevel_window_event_handler.h (right):

https://codereview.chromium.org/12441010/diff/22001/ash/wm/toplevel_window_ev...
ash/wm/toplevel_window_event_handler.h:40: aura::Window* owner() const { return
owner_; }
On 2013/03/18 16:17:47, sky wrote:
> const aura::Window* owner() const
> If you need a non-const version add that too, but it should then be non-const
on
> both, eg:
> aura::Window* owner() { ...

Done.

[flackr, 2013-03-20 10:01:40]

https://codereview.chromium.org/12441010/diff/15006/ash/wm/toplevel_window_ev...
File ash/wm/toplevel_window_event_handler.cc (right):

https://codereview.chromium.org/12441010/diff/15006/ash/wm/toplevel_window_ev...
ash/wm/toplevel_window_event_handler.cc:112:
handler_->CompleteDrag(DRAG_COMPLETE, 0);
If the new parent is also contained by handler_->owner() we could continue the
drag, but I don't think we rely on this for any current drag operations and
allowing that might just add confusion.

[sky, 2013-03-20 14:31:50]

LGTM

[commit-bot: I haz the power, 2013-03-20 20:53:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/12441010/15006

[commit-bot: I haz the power, 2013-03-20 23:10:36]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-03-20 23:48:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/12441010/15006

[commit-bot: I haz the power, 2013-03-21 06:41:27]

Change committed as 189536