WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
21956
DragController needs to be made aware of async drags
https://bugs.webkit.org/show_bug.cgi?id=21956
Summary
DragController needs to be made aware of async drags
Eric Seidel (no email)
Reported
2008-10-29 13:55:30 PDT
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.
Attachments
Make DragController async-drag aware
(1.65 KB, patch)
2008-10-29 13:57 PDT
,
Eric Seidel (no email)
oliver
: review-
Details
Formatted Diff
Diff
[1/1] only call dragEnded() on PLATFORM(MAC)
(1.74 KB, patch)
2008-10-30 14:50 PDT
,
Tony Chang
oliver
: review-
Details
Formatted Diff
Diff
[1/1] Add a method to be called when the system drag is finished. This
(4.97 KB, patch)
2008-10-31 13:21 PDT
,
Tony Chang
oliver
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1
2008-10-29 13:57:11 PDT
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 2
2008-10-29 14:11:19 PDT
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)
Comment 3
2008-10-29 14:17:56 PDT
This was the original change in our tree:
https://svn/v/chrome?view=rev&revision=17910
Eric Seidel (no email)
Comment 4
2008-10-29 14:24:18 PDT
(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
Comment 5
2008-10-29 14:27:16 PDT
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
Comment 6
2008-10-29 14:31:23 PDT
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
Comment 7
2008-10-29 14:37:44 PDT
(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
Comment 8
2008-10-29 16:01:32 PDT
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
Comment 9
2008-10-29 17:14:21 PDT
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
Comment 10
2008-10-30 01:02:00 PDT
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
Comment 11
2008-10-30 10:38:31 PDT
(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
Comment 12
2008-10-30 14:50:14 PDT
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 13
2008-10-30 15:12:28 PDT
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
Comment 14
2008-10-31 13:21:30 PDT
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 15
2008-11-04 14:13:23 PST
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)
Comment 16
2008-11-26 17:32:09 PST
http://trac.webkit.org/changeset/38805
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug