Bug 21956 – DragController needs to be made aware of async drags

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

DragController needs to be made aware of async drags

In Chromium drags happen asynchronously, so this preventative dragEnded() call in Chromium is called too early.  AFAICT this code is only hit for WebKit clients which fail to correctly implement UIDelegate anyway, so disabling it for Chromium is a non-issue.

[Eric Seidel (no email)]

Created attachment 24754 [details]
Make DragController async-drag aware

 WebCore/ChangeLog               |   11 +++++++++++
 WebCore/page/DragController.cpp |    5 ++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

[Oliver Hunt]

Comment on attachment 24754 [details]
Make DragController async-drag aware

You can't just not call dragEnded, it will fail to reset appropriate logic.

More over this ifdef is awful, and chromium is the only platform for which it is "necessary" -- i have never encountered any platform on which drag and drop is not a synchronous action, and would actually consider this a bug in chromium.

[Eric Seidel (no email)]

This was the original change in our tree:
https://svn/v/chrome?view=rev&revision=17910

[Eric Seidel (no email)]

(In reply to comment #3)
> This was the original change in our tree:
> https://svn/v/chrome?view=rev&revision=17910

Sorry, I didn't realize that was an internal URL. I don't think there is an external equivalent because the change is so old.  The log was:

Author:	tc
Date:	Mon Dec 17 18:17:59 2007 UTC (10 months, 1 week ago)
Log Message:	
Fork DragController because it makes an assumption that the
OS drag&drop command is synchronous.

[Tony Chang]

The drag operation is triggered in the browser process.  In the browser process, it is a synchronous action.  However, we need to forward the drag events from the browser process to the renderer process.  To the renderer, these appear as distinct messages that do not block the renderer loop.  That is, to the renderer, this appears async.

we do eventually call dragEnd, but it is called asynchronously compared to dragStart.

Does that make sense?

(In reply to comment #2)
> (From update of attachment 24754 [details] [edit])
> You can't just not call dragEnded, it will fail to reset appropriate logic.
> 
> More over this ifdef is awful, and chromium is the only platform for which it
> is "necessary" -- i have never encountered any platform on which drag and drop
> is not a synchronous action, and would actually consider this a bug in
> chromium.
> 

[Oliver Hunt]

Righto, if that is the entire change i believe that the change is wrong.  DRT can get away with async DnD because of a number of hoops it jumps to, and because it guarantees certain things are safe.

Just making DnD asynchronous may lead to lots of edge cases crashes -  when entering drag and drop logic various views and frames are protected to handle a page load or similar occuring during the drag handling.  If you allow dnd to be async you allow those to be released which in turn can lead to crashes.

DnD is assumed to be synchronous throughout the dnd handling in webkit, if it is not currently in chromium that is a chromium platform bug that needs to be fixed.

[Tony Chang]

(In reply to comment #6)
> Righto, if that is the entire change i believe that the change is wrong.  DRT
> can get away with async DnD because of a number of hoops it jumps to, and
> because it guarantees certain things are safe.

Can you elaborate more on the hoops that DRT has to jump through to guarantee that this is safe?

Chrome works like DRT in that once a drag has started, the only messages sent are drag moves or a drag end.

[Oliver Hunt]

I can't remember the specifics of what i had to make DRT do, but one of the most major aspects is that no drag and drop tests in drt trigger page navigation of the kind that resulted in crashes -- IIRC i was unable to get them to be testable in many/most cases.

I would strongly recommend you look into drag and drop occuring when a load is in process.

[Tony Chang]

Ah, I should clarify.  If the drag originates from webkit (ie., web content), then it (In reply to comment #8)
> I would strongly recommend you look into drag and drop occuring when a load is
> in process.

This seems to work fine for me in Chrome and Safari Mac.  Here's a test page:
http://ponderer.org/tests/redir.html

In both Chrome and Safari Mac, I can grab text on the page to start a drag, wait a few seconds for the redirect to happen, then drop the object.  In both browsers, the page load still happens.  Maybe I'm misunderstanding?

[Oliver Hunt]

Okay, after discussing with erici reread patch and realised it was not the call i thought was being removed, however I'm still not convinced this patch is correct.

In fact i will say very simply: this should not be #if !PLATFORM(CHROMIUM)

However I am actually entertaining the idea that it should be #if PLATFORM(MAC), as on re-reading i remember fixing this bug and it is specific to embedders on the mac platform that already depended on this magically working.

So many apologies for thinking it was something completely different from what it was.

[Tony Chang]

(In reply to comment #10)
> In fact i will say very simply: this should not be #if !PLATFORM(CHROMIUM)
> 
> However I am actually entertaining the idea that it should be #if
> PLATFORM(MAC), as on re-reading i remember fixing this bug and it is specific
> to embedders on the mac platform that already depended on this magically
> working.

My guess is that this is helpful to the non-apple ports because it resets the drag state during layout tests if the eventSendingController has not been fully implemented.  I'm not sure if this impacts the qt port, but when we were implementing eventSendingController this caused some tests to fail if run in a sequence.  Alternately, maybe there should just be a way for DRT to reset the drag state.

[Tony Chang]

Created attachment 24781 [details]
[1/1] only call dragEnded() on PLATFORM(MAC)

 WebCore/ChangeLog               |   14 ++++++++++++++
 WebCore/page/DragController.cpp |    6 ++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

[Oliver Hunt]

Comment on attachment 24781 [details]
[1/1] only call dragEnded() on PLATFORM(MAC)

I was talking to Sam about this, and he suggested that instead of putting an icky ifdef here, we add a new method to DragController, say DragController::cleanupAfterDrag or something (I'm really no good with function names), which on mac would just call dragEnded and on everything else would be a no-op

[Tony Chang]

Created attachment 24810 [details]
[1/1] Add a method to be called when the system drag is finished.  This

allows each platform to handle cleaning up drag data.
---
 WebCore/ChangeLog                      |   26 ++++++++++++++++++++++++++
 WebCore/page/DragController.cpp        |    6 +-----
 WebCore/page/DragController.h          |    1 +
 WebCore/page/gtk/DragControllerGtk.cpp |    4 ++++
 WebCore/page/mac/DragControllerMac.mm  |    9 +++++++++
 WebCore/page/qt/DragControllerQt.cpp   |    4 ++++
 WebCore/page/win/DragControllerWin.cpp |    4 ++++
 WebCore/page/wx/DragControllerWx.cpp   |    4 ++++
 8 files changed, 53 insertions(+), 5 deletions(-)

[Oliver Hunt]

Comment on attachment 24810 [details]
[1/1] Add a method to be called when the system drag is finished.  This

r=me

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/38805