Support Drag and Drop across displays.

Support Drag and Drop across displays.

This CL introduces DragDropTracker, which
- Makes a dummy window capture events while dragging an item, and
- Dispatches mouse events to the window at the pointer location in DragDropController::PreHandleMouseEvent.

BUG=136817
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153401

[mazda, 2012-08-14 23:14:44]

Hi Scott,

Could you check the change is the right approach?
I'll add a unittest for DragDropTracker if it looks good.

The previous CLs (http://codereview.chromium.org/10829185/,
http://codereview.chromium.org/10852020/) rely on the function that can retrieve
another root window in aura, but Ben said that root window should not know
anything about other root windows, so I needed to take another approach.

The tricky part is the event handling while dragging.
While dragging, mouse events are dispatched to the window located at the pointer
position, but once the pointer goes out of the display where drag is started,
mouse events are not dispatched to any windows. This is because the root window
associated to the display where drag is started keeps processing mouse events
and there is no window outside the root window's bound.
When mouse events are not dispatched to any windows, DragDropController cannot
process mouse events since it is implemented as an event filter for windows.

I solved this issue by creating a dummy window that captures mouse events, then
dispatching the events to the window located at the event position with
appropriate coordinate conversions.

Please let me know your thoughts on this approach.

[sky, 2012-08-15 13:45:10]

Why don't the events go to the other rootwindow when you cross over to another
display? Is it because we aren't releasing the grab at the native level so X
targets the wrong window?

http://codereview.chromium.org/10855159/diff/2001/ash/drag_drop/drag_drop_con...
File ash/drag_drop/drag_drop_controller.cc (left):

http://codereview.chromium.org/10855159/diff/2001/ash/drag_drop/drag_drop_con...
ash/drag_drop/drag_drop_controller.cc:62: // TODO(oshima): Add CaptureClient
client API.
Why do you need to remove this code?

[mazda, 2012-08-15 23:54:15]

> Why don't the events go to the other rootwindow when you cross over to another
> display? Is it because we aren't releasing the grab at the native level so X
> targets the wrong window?

X keeps reporting the first root window as the event window while dragging.
DispatcherLinux dispatches events to the root window corresponding to the event
window, so the first root window keep receiving the events.
I'm searching the way to change this behavior but I'm not sure if it's possible
yet.

http://codereview.chromium.org/10855159/diff/2001/ash/drag_drop/drag_drop_con...
File ash/drag_drop/drag_drop_controller.cc (left):

http://codereview.chromium.org/10855159/diff/2001/ash/drag_drop/drag_drop_con...
ash/drag_drop/drag_drop_controller.cc:62: // TODO(oshima): Add CaptureClient
client API.
On 2012/08/15 13:45:11, sky wrote:
> Why do you need to remove this code?

Because DragDropTracker::StartTracking will set capture to
DragDropTracker::capture_window_ and the code is now redundant.

[sky, 2012-08-16 00:43:36]

On Wed, Aug 15, 2012 at 4:54 PM,  <mazda@chromium.org> wrote:
>> Why don't the events go to the other rootwindow when you cross over to
>> another
>> display? Is it because we aren't releasing the grab at the native level so
>> X
>> targets the wrong window?
>
>
> X keeps reporting the first root window as the event window while dragging.
> DispatcherLinux dispatches events to the root window corresponding to the
> event
> window, so the first root window keep receiving the events.
> I'm searching the way to change this behavior but I'm not sure if it's
> possible
> yet.

Is this because X is doing an implicit grab? Is it possible to release
that grab and transfer to the other window? Dan might know more here.

  -Scott
>
>
>
>
http://codereview.chromium.org/10855159/diff/2001/ash/drag_drop/drag_drop_con...
> File ash/drag_drop/drag_drop_controller.cc (left):
>
>
http://codereview.chromium.org/10855159/diff/2001/ash/drag_drop/drag_drop_con...
> ash/drag_drop/drag_drop_controller.cc:62: // TODO(oshima): Add
> CaptureClient client API.
> On 2012/08/15 13:45:11, sky wrote:
>>
>> Why do you need to remove this code?
>
>
> Because DragDropTracker::StartTracking will set capture to
> DragDropTracker::capture_window_ and the code is now redundant.
>
> http://codereview.chromium.org/10855159/

[sky, 2012-08-16 00:44:03]

Added Dan to the review since I cc'd him on the latest email.

[Daniel Erat, 2012-08-16 13:23:53]

On 2012/08/16 00:43:36, sky wrote:
> On Wed, Aug 15, 2012 at 4:54 PM, 
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=mazda@chromium.org> wrote:
> >> Why don't the events go to the other rootwindow when you cross over to
> >> another
> >> display? Is it because we aren't releasing the grab at the native level so
> >> X
> >> targets the wrong window?
> >
> >
> > X keeps reporting the first root window as the event window while dragging.
> > DispatcherLinux dispatches events to the root window corresponding to the
> > event
> > window, so the first root window keep receiving the events.
> > I'm searching the way to change this behavior but I'm not sure if it's
> > possible
> > yet.
> 
> Is this because X is doing an implicit grab? Is it possible to release
> that grab and transfer to the other window? Dan might know more here.

Yes, this is probably the implicit grab.  I think but am not certain that you
can release it explicitly and re-grab on behalf of another window, but it's
race-prone and you're probably better off avoiding it if possible (see
http://codereview.chromium.org/10828133/).

>   -Scott
> >
> >
> >
> >
>
http://codereview.chromium.org/10855159/diff/2001/ash/drag_drop/drag_drop_con...
> > File ash/drag_drop/drag_drop_controller.cc (left):
> >
> >
>
http://codereview.chromium.org/10855159/diff/2001/ash/drag_drop/drag_drop_con...
> > ash/drag_drop/drag_drop_controller.cc:62: // TODO(oshima): Add
> > CaptureClient client API.
> > On 2012/08/15 13:45:11, sky wrote:
> >>
> >> Why do you need to remove this code?
> >
> >
> > Because DragDropTracker::StartTracking will set capture to
> > DragDropTracker::capture_window_ and the code is now redundant.
> >
> > http://codereview.chromium.org/10855159/

[mazda, 2012-08-16 21:26:31]

Daniel,
Thank you for your comment.
The approach in this CL does not use native explicit grab/ungrab, so I don't
believe it causes race issues.

Scott,
Do you think it's OK to go with this the approach in this CL?

In the latest patch, I changed RootWindow a little bit to deferr the null check
of |target->delegate()| in RootWindow::DispatchMouseEventToTarget to
RootWindow::ProcessMouseEvent.
Thus we can allow event filters associated with a window that has no delegate to
prehandle mouse events, and I can remove DragDropTrackerWindowDelegate.

[sky, 2012-08-16 22:38:32]

I think getting the right events to route to the right window would be
a lot more straightforward (meaning release grab then regrab). That
said, if such an approach is going to be racy I'm against it. Dan,
what do you think?

  -Scott

On Thu, Aug 16, 2012 at 2:26 PM,  <mazda@chromium.org> wrote:
> Daniel,
> Thank you for your comment.
> The approach in this CL does not use native explicit grab/ungrab, so I don't
> believe it causes race issues.
>
> Scott,
> Do you think it's OK to go with this the approach in this CL?
>
> In the latest patch, I changed RootWindow a little bit to deferr the null
> check
> of |target->delegate()| in RootWindow::DispatchMouseEventToTarget to
> RootWindow::ProcessMouseEvent.
> Thus we can allow event filters associated with a window that has no
> delegate to
> prehandle mouse events, and I can remove DragDropTrackerWindowDelegate.
>
> http://codereview.chromium.org/10855159/

[Daniel Erat, 2012-08-16 23:27:06]

I think it'll be tricky to get right.  I'd suggest avoiding trying to regrab, as
long as it doesn't make things super-ugly.

On 2012/08/16 22:38:32, sky wrote:
> I think getting the right events to route to the right window would be
> a lot more straightforward (meaning release grab then regrab). That
> said, if such an approach is going to be racy I'm against it. Dan,
> what do you think?
> 
>   -Scott
> 
> On Thu, Aug 16, 2012 at 2:26 PM,  <mailto:mazda@chromium.org> wrote:
> > Daniel,
> > Thank you for your comment.
> > The approach in this CL does not use native explicit grab/ungrab, so I don't
> > believe it causes race issues.
> >
> > Scott,
> > Do you think it's OK to go with this the approach in this CL?
> >
> > In the latest patch, I changed RootWindow a little bit to deferr the null
> > check
> > of |target->delegate()| in RootWindow::DispatchMouseEventToTarget to
> > RootWindow::ProcessMouseEvent.
> > Thus we can allow event filters associated with a window that has no
> > delegate to
> > prehandle mouse events, and I can remove DragDropTrackerWindowDelegate.
> >
> > http://codereview.chromium.org/10855159/

[mazda, 2012-08-17 23:08:04]

Scott, Daniel

Could you start the review of this CL and judge between this approach and the
one using native ungrab/regrab (or another approach if any)?
I added a unittest of DragDropTracker, too.

Thanks,

[Daniel Erat, 2012-08-17 23:39:22]

It's more Scott's call than mine,since I'm not very familiar with this code, but
the approach that you're currently using seems fine to me as long as you add
some comments describing why you need to do it.

[sky, 2012-08-20 15:35:18]

Ok, we go with this approach.

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
File ash/drag_drop/drag_drop_controller.cc (right):

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
ash/drag_drop/drag_drop_controller.cc:237: ui::TouchStatus
DragDropController::PreHandleTouchEvent(
Do you need to update touch similarly?

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_tr...
File ash/drag_drop/drag_drop_tracker.cc (right):

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_tr...
ash/drag_drop/drag_drop_tracker.cc:22: default_container->AddChild(window);
Doesn't SetParent(NULL) do what you want instead of getting the default
container?

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_tr...
File ash/drag_drop/drag_drop_tracker.h (right):

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_tr...
ash/drag_drop/drag_drop_tracker.h:22: DragDropTracker();
Can we create this with a specific RootWindow and it tracks until the destructor
is called?

[varunjain, 2012-08-20 15:53:19]

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
File ash/drag_drop/drag_drop_controller.cc (right):

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
ash/drag_drop/drag_drop_controller.cc:237: ui::TouchStatus
DragDropController::PreHandleTouchEvent(
On 2012/08/20 15:35:18, sky wrote:
> Do you need to update touch similarly?

Touch dnd does not currently work. I have a bug for it for m24. I can take care
of this then.

[varunjain, 2012-08-20 16:21:22]

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
File ash/drag_drop/drag_drop_controller.cc (right):

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
ash/drag_drop/drag_drop_controller.cc:218: return false;
when can this happen? Drag drop is in progress, so what happens if you let this
drag event fall through? My concern here is the user seeing some windows, where
the drag image and cursor are not updated.

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
ash/drag_drop/drag_drop_controller.cc:223: DragUpdate(translated_target,
*translated_event.get());
would you also require re-parenting the drag_image_ so the image shows up in the
right root window?

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_tr...
File ash/drag_drop/drag_drop_tracker.h (right):

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_tr...
ash/drag_drop/drag_drop_tracker.h:20: class ASH_EXPORT DragDropTracker {
some comments about what this is for would be nice.

[mazda, 2012-08-21 02:10:58]

Thank you for the review. Please take another look.

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
File ash/drag_drop/drag_drop_controller.cc (right):

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
ash/drag_drop/drag_drop_controller.cc:218: return false;
On 2012/08/20 16:21:22, varunjain wrote:
> when can this happen? Drag drop is in progress, so what happens if you let
this
> drag event fall through? My concern here is the user seeing some windows,
where
> the drag image and cursor are not updated.

This happens when the cursor goes out of root window when testing on desktop.
I don't think it happens on the real machine because the cursor cannot go out of
desktop background view.
Is it better to call Cleanup?

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
ash/drag_drop/drag_drop_controller.cc:223: DragUpdate(translated_target,
*translated_event.get());
On 2012/08/20 16:21:22, varunjain wrote:
> would you also require re-parenting the drag_image_ so the image shows up in
the
> right root window?

DragImageView::SetScreenPosition does it internally.
There was a bug that shows drag image always on the primary display
(http://crbug.com/141192), but it was fixed in another CL
(http://crrev.com/152339).

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
ash/drag_drop/drag_drop_controller.cc:237: ui::TouchStatus
DragDropController::PreHandleTouchEvent(
On 2012/08/20 15:53:20, varunjain wrote:
> On 2012/08/20 15:35:18, sky wrote:
> > Do you need to update touch similarly?
> 
> Touch dnd does not currently work. I have a bug for it for m24. I can take
care
> of this then.

Could you tell me the issue ID? I'll add TODO comment.

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_tr...
File ash/drag_drop/drag_drop_tracker.cc (right):

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_tr...
ash/drag_drop/drag_drop_tracker.cc:22: default_container->AddChild(window);
On 2012/08/20 15:35:18, sky wrote:
> Doesn't SetParent(NULL) do what you want instead of getting the default
> container?

Yes, it does with some setups. I changed the code to use it. Thanks.

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_tr...
File ash/drag_drop/drag_drop_tracker.h (right):

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_tr...
ash/drag_drop/drag_drop_tracker.h:20: class ASH_EXPORT DragDropTracker {
On 2012/08/20 16:21:22, varunjain wrote:
> some comments about what this is for would be nice.

Done.

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_tr...
ash/drag_drop/drag_drop_tracker.h:22: DragDropTracker();
On 2012/08/20 15:35:18, sky wrote:
> Can we create this with a specific RootWindow and it tracks until the
destructor
> is called?

Done.

[varunjain, 2012-08-21 04:37:25]

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
File ash/drag_drop/drag_drop_controller.cc (right):

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
ash/drag_drop/drag_drop_controller.cc:218: return false;
On 2012/08/21 02:10:59, mazda wrote:
> On 2012/08/20 16:21:22, varunjain wrote:
> > when can this happen? Drag drop is in progress, so what happens if you let
> this
> > drag event fall through? My concern here is the user seeing some windows,
> where
> > the drag image and cursor are not updated.
> 
> This happens when the cursor goes out of root window when testing on desktop.
> I don't think it happens on the real machine because the cursor cannot go out
of
> desktop background view.
> Is it better to call Cleanup?

How about calling DragCancel() instead. It will do the necessary cleanup and
also give the user a visual cue that the drag session is over.

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
ash/drag_drop/drag_drop_controller.cc:223: DragUpdate(translated_target,
*translated_event.get());
On 2012/08/21 02:10:59, mazda wrote:
> On 2012/08/20 16:21:22, varunjain wrote:
> > would you also require re-parenting the drag_image_ so the image shows up in
> the
> > right root window?
> 
> DragImageView::SetScreenPosition does it internally.
> There was a bug that shows drag image always on the primary display
> (http://crbug.com/141192), but it was fixed in another CL
> (http://crrev.com/152339).

cool! thanks for fixing!

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
ash/drag_drop/drag_drop_controller.cc:237: ui::TouchStatus
DragDropController::PreHandleTouchEvent(
On 2012/08/21 02:10:59, mazda wrote:
> On 2012/08/20 15:53:20, varunjain wrote:
> > On 2012/08/20 15:35:18, sky wrote:
> > > Do you need to update touch similarly?
> > 
> > Touch dnd does not currently work. I have a bug for it for m24. I can take
> care
> > of this then.
> 
> Could you tell me the issue ID? I'll add TODO comment.

crbug.com/114755

http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
File ash/drag_drop/drag_drop_tracker.cc (right):

http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
ash/drag_drop/drag_drop_tracker.cc:35: capture_window_->SetCapture();
Do you also need to listen to OnWindowDestroying on the capture_window_?

http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
File ash/drag_drop/drag_drop_tracker_unittest.cc (right):

http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
ash/drag_drop/drag_drop_tracker_unittest.cc:18: class DragDropTrackerTest :
public test::AshTestBase {
Suggestion for a test case (should perhaps go in drag_drop_controller_unittest):
Start drag/drop in secondary root window, then destroy that root window (for
example, when the user starts drag/drop in secondary monitor and then switches
that monitor off).

[mazda, 2012-08-24 08:29:00]

Sorry for the belated reply.
It took some time to handle the case where the secondary display is disconnected
while dragging.

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
File ash/drag_drop/drag_drop_controller.cc (right):

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
ash/drag_drop/drag_drop_controller.cc:218: return false;
On 2012/08/21 04:37:25, varunjain wrote:
> On 2012/08/21 02:10:59, mazda wrote:
> > On 2012/08/20 16:21:22, varunjain wrote:
> > > when can this happen? Drag drop is in progress, so what happens if you let
> > this
> > > drag event fall through? My concern here is the user seeing some windows,
> > where
> > > the drag image and cursor are not updated.
> > 
> > This happens when the cursor goes out of root window when testing on
desktop.
> > I don't think it happens on the real machine because the cursor cannot go
out
> of
> > desktop background view.
> > Is it better to call Cleanup?
> 
> How about calling DragCancel() instead. It will do the necessary cleanup and
> also give the user a visual cue that the drag session is over.

Done.

http://codereview.chromium.org/10855159/diff/13001/ash/drag_drop/drag_drop_co...
ash/drag_drop/drag_drop_controller.cc:237: ui::TouchStatus
DragDropController::PreHandleTouchEvent(
On 2012/08/21 04:37:25, varunjain wrote:
> On 2012/08/21 02:10:59, mazda wrote:
> > On 2012/08/20 15:53:20, varunjain wrote:
> > > On 2012/08/20 15:35:18, sky wrote:
> > > > Do you need to update touch similarly?
> > > 
> > > Touch dnd does not currently work. I have a bug for it for m24. I can take
> > care
> > > of this then.
> > 
> > Could you tell me the issue ID? I'll add TODO comment.
> 
> crbug.com/114755

Thanks. I added a TODO comment.

http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
File ash/drag_drop/drag_drop_tracker.cc (right):

http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
ash/drag_drop/drag_drop_tracker.cc:35: capture_window_->SetCapture();
On 2012/08/21 04:37:26, varunjain wrote:
> Do you also need to listen to OnWindowDestroying on the capture_window_?

I don't think it's necessary to listen to OnWindowDestroying. 
When secondary display is disconnected, |capture_window_| is not deleted but
moved to primary root window, so I do not need it for this case.
The primary root window is deleted after DragDropController in Shell, so
OnWindowDestroying is not called for |capture_window_|.

http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
File ash/drag_drop/drag_drop_tracker_unittest.cc (right):

http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
ash/drag_drop/drag_drop_tracker_unittest.cc:18: class DragDropTrackerTest :
public test::AshTestBase {
On 2012/08/21 04:37:26, varunjain wrote:
> Suggestion for a test case (should perhaps go in
drag_drop_controller_unittest):
> Start drag/drop in secondary root window, then destroy that root window (for
> example, when the user starts drag/drop in secondary monitor and then switches
> that monitor off).

Thank you for the suggestion.
I tried EventGenerator for a while and it doesn't seem to work well with multi
display. It does not work when testing even within 2nd display.
Let me add this test case later with the fix of EventGenerator.
As far as I tested the latest patch on the real machine, drag was canceled
without causing problems.

http://codereview.chromium.org/10855159/diff/17001/ash/drag_drop/drag_image_v...
File ash/drag_drop/drag_image_view.cc (right):

http://codereview.chromium.org/10855159/diff/17001/ash/drag_drop/drag_image_v...
ash/drag_drop/drag_image_view.cc:27:
drag_widget->GetNativeWindow()->set_owned_by_parent(false);
This is needed to make the aura window not deleted by RootWindow dtor.
The aura window of this widget is added to DragImageAndTooltipContainer and is
not moved to primary window when the secondary display is disconnected (see
RootWindowController::ReparentAllWindows).

[varunjain, 2012-08-24 13:00:53]

LGTM for drag_drop_* changes

http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
File ash/drag_drop/drag_drop_tracker_unittest.cc (right):

http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
ash/drag_drop/drag_drop_tracker_unittest.cc:18: class DragDropTrackerTest :
public test::AshTestBase {
On 2012/08/24 08:29:01, mazda wrote:
> On 2012/08/21 04:37:26, varunjain wrote:
> > Suggestion for a test case (should perhaps go in
> drag_drop_controller_unittest):
> > Start drag/drop in secondary root window, then destroy that root window (for
> > example, when the user starts drag/drop in secondary monitor and then
switches
> > that monitor off).
> 
> Thank you for the suggestion.
> I tried EventGenerator for a while and it doesn't seem to work well with multi
> display. It does not work when testing even within 2nd display.
> Let me add this test case later with the fix of EventGenerator.
> As far as I tested the latest patch on the real machine, drag was canceled
> without causing problems.

I used EventGenerator in
tooltip_controller_unittest.cc::TooltipsOnMultiDisplayShouldNotCrash for similar
stuff and it seemed to work. You might want to look there (or, that test is also
doing the wrong thing!, in which case let me know! :) )

http://codereview.chromium.org/10855159/diff/17001/ash/drag_drop/drag_drop_tr...
File ash/drag_drop/drag_drop_tracker.h (right):

http://codereview.chromium.org/10855159/diff/17001/ash/drag_drop/drag_drop_tr...
ash/drag_drop/drag_drop_tracker.h:26: DragDropTracker(aura::RootWindow*
root_window);
explicit

[sky, 2012-08-24 18:06:08]

LGTM

[mazda, 2012-08-24 18:52:37]

Thank you for the review.

http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
File ash/drag_drop/drag_drop_tracker_unittest.cc (right):

http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
ash/drag_drop/drag_drop_tracker_unittest.cc:18: class DragDropTrackerTest :
public test::AshTestBase {
On 2012/08/24 13:00:53, varunjain wrote:
> On 2012/08/24 08:29:01, mazda wrote:
> > On 2012/08/21 04:37:26, varunjain wrote:
> > > Suggestion for a test case (should perhaps go in
> > drag_drop_controller_unittest):
> > > Start drag/drop in secondary root window, then destroy that root window
(for
> > > example, when the user starts drag/drop in secondary monitor and then
> switches
> > > that monitor off).
> > 
> > Thank you for the suggestion.
> > I tried EventGenerator for a while and it doesn't seem to work well with
multi
> > display. It does not work when testing even within 2nd display.
> > Let me add this test case later with the fix of EventGenerator.
> > As far as I tested the latest patch on the real machine, drag was canceled
> > without causing problems.
> 
> I used EventGenerator in
> tooltip_controller_unittest.cc::TooltipsOnMultiDisplayShouldNotCrash for
similar
> stuff and it seemed to work. You might want to look there (or, that test is
also
> doing the wrong thing!, in which case let me know! :) )

Thank you for the pointer! I'll take a look.
The problem I encountered was that the test gets stuck when drag started
probably because nested loop does not work well on 2nd display.

http://codereview.chromium.org/10855159/diff/17001/ash/drag_drop/drag_drop_tr...
File ash/drag_drop/drag_drop_tracker.h (right):

http://codereview.chromium.org/10855159/diff/17001/ash/drag_drop/drag_drop_tr...
ash/drag_drop/drag_drop_tracker.h:26: DragDropTracker(aura::RootWindow*
root_window);
On 2012/08/24 13:00:54, varunjain wrote:
> explicit

Done.

[sky, 2012-08-24 19:16:36]

Make sure you update the description of the cl too.

On Fri, Aug 24, 2012 at 11:52 AM,  <mazda@chromium.org> wrote:
> Thank you for the review.
>
>
>
http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
> File ash/drag_drop/drag_drop_tracker_unittest.cc (right):
>
>
http://codereview.chromium.org/10855159/diff/40/ash/drag_drop/drag_drop_track...
> ash/drag_drop/drag_drop_tracker_unittest.cc:18: class
> DragDropTrackerTest : public test::AshTestBase {
> On 2012/08/24 13:00:53, varunjain wrote:
>>
>> On 2012/08/24 08:29:01, mazda wrote:
>> > On 2012/08/21 04:37:26, varunjain wrote:
>> > > Suggestion for a test case (should perhaps go in
>> > drag_drop_controller_unittest):
>> > > Start drag/drop in secondary root window, then destroy that root
>
> window (for
>>
>> > > example, when the user starts drag/drop in secondary monitor and
>
> then
>>
>> switches
>> > > that monitor off).
>> >
>> > Thank you for the suggestion.
>> > I tried EventGenerator for a while and it doesn't seem to work well
>
> with multi
>>
>> > display. It does not work when testing even within 2nd display.
>> > Let me add this test case later with the fix of EventGenerator.
>> > As far as I tested the latest patch on the real machine, drag was
>
> canceled
>>
>> > without causing problems.
>
>
>> I used EventGenerator in
>> tooltip_controller_unittest.cc::TooltipsOnMultiDisplayShouldNotCrash
>
> for similar
>>
>> stuff and it seemed to work. You might want to look there (or, that
>
> test is also
>>
>> doing the wrong thing!, in which case let me know! :) )
>
>
> Thank you for the pointer! I'll take a look.
> The problem I encountered was that the test gets stuck when drag started
> probably because nested loop does not work well on 2nd display.
>
>
>
http://codereview.chromium.org/10855159/diff/17001/ash/drag_drop/drag_drop_tr...
> File ash/drag_drop/drag_drop_tracker.h (right):
>
>
http://codereview.chromium.org/10855159/diff/17001/ash/drag_drop/drag_drop_tr...
> ash/drag_drop/drag_drop_tracker.h:26: DragDropTracker(aura::RootWindow*
> root_window);
> On 2012/08/24 13:00:54, varunjain wrote:
>>
>> explicit
>
>
> Done.
>
> http://codereview.chromium.org/10855159/

[commit-bot: I haz the power, 2012-08-25 00:43:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/10855159/27003

[mazda, 2012-08-25 00:48:49]

On 2012/08/24 19:16:36, sky wrote:
> Make sure you update the description of the cl too.

It seems the title of the mail thread doesn't change though I changed the
description a while ago.
I think commit bot fetches the right description.

[commit-bot: I haz the power, 2012-08-26 11:59:06]

Change committed as 153401

[danakj, 2014-09-27 01:15:04]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2014-09-27 01:15:05]

https://chromiumcodereview.appspot.com/10855159/diff/27003/ash/drag_drop/drag...
File ash/drag_drop/drag_drop_controller.cc (right):

https://chromiumcodereview.appspot.com/10855159/diff/27003/ash/drag_drop/drag...
ash/drag_drop/drag_drop_controller.cc:311: drag_drop_tracker_.Pass();
This is not going to work with unique_ptr, we should break the link to
DragDropController before calling Cleanup() rather than depending on this.