Last Comment Bug 497498 - drag-leave signals are not received due to processing X events before returning from the drag-motion handler
: drag-leave signals are not received due to processing X events before returni...
Status: RESOLVED FIXED
[softblocker]
: regression
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: Trunk
: x86 Linux
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Karl Tomlinson (:karlt)
:
Mentors:
Depends on: 497494 753118 809601
Blocks: 625745 645285 595789 626797
  Show dependency treegraph
 
Reported: 2009-06-10 18:56 PDT by Karl Tomlinson (:karlt)
Modified: 2012-11-07 13:35 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
part 1: Only dispatch drop events when canDrop has been set via dragover and only indicate successful drop when the drop is sent (5.53 KB, patch)
2011-02-14 17:13 PST, Karl Tomlinson (:karlt)
enndeakin: review+
Details | Diff | Splinter Review
part 2: no need to explicitly dispatch NS_DRAGDROP_ENTER as this happens during NS_DRAGDROP_OVER (6.54 KB, patch)
2011-02-14 17:20 PST, Karl Tomlinson (:karlt)
enndeakin: review+
Details | Diff | Splinter Review
part 3: don't set a drag context with the new widget for a leave event on the old widget (3.03 KB, patch)
2011-02-14 17:34 PST, Karl Tomlinson (:karlt)
enndeakin: review+
Details | Diff | Splinter Review
part 4: fire NS_DRAGDROP_DRAG at source even before synthetic NS_DRAGDROP_OVER events (1.00 KB, patch)
2011-02-14 17:40 PST, Karl Tomlinson (:karlt)
enndeakin: review+
Details | Diff | Splinter Review
part 5: move gdk_drag_status code from nsDragService to drag-motion signal handler (5.35 KB, patch)
2011-02-14 17:47 PST, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review
part 6: use event and widget coordinates for drop point instead of new cursor position (1.82 KB, patch)
2011-02-14 17:52 PST, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review
part 6b: use event coordinates for source drag end point instead of new cursor position (3.92 KB, patch)
2011-02-14 17:59 PST, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review
part 7: use a helper function for dispatching destination drag events (5.42 KB, patch)
2011-02-14 18:07 PST, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review
part 8: split drag event dispatch code from GTK signal handling methods (6.13 KB, patch)
2011-02-14 18:10 PST, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review
part 9: rename UpdateDragStatus to UpdateDragAction (4.67 KB, patch)
2011-02-14 18:14 PST, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
part 10: use nsIDragSession::getCurrentSession to remove sIsDraggingOutOf (5.18 KB, patch)
2011-02-14 18:29 PST, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review
part 11: move early drag end code to drag_motion_event_cb and correct return value (4.52 KB, patch)
2011-02-14 18:32 PST, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review
part 12: schedule event dispatch in response to GTK drag signals to avoid running the event loop at unexpected times (35.91 KB, patch)
2011-02-14 19:16 PST, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
part 12: schedule event dispatch in response to GTK drag signals to avoid running the event loop at unexpected times v1.1 (37.05 KB, patch)
2011-02-14 21:10 PST, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
dropless dragleave/dragend testcase (2.51 KB, text/html)
2011-02-16 00:33 PST, Karl Tomlinson (:karlt)
no flags Details
part 12: schedule event dispatch in response to GTK drag signals to avoid running the event loop at unexpected times v1.2 (36.89 KB, patch)
2011-02-16 15:03 PST, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
part 5: use nsDragService::SetCanDrop instead of nsIDragSessionGTK::StartDragMotion to reset before NS_DRAGDROP_OVER (3.23 KB, patch)
2012-03-19 19:23 PDT, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
part 12a: prevent GetTargetDragData (and IsTargetContextList) from being called without a target GtkWidget (3.59 KB, patch)
2012-03-19 19:39 PDT, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review
part 12b: schedule event dispatch in response to GTK drag target signals to avoid running the event loop at unexpected times (42.56 KB, patch)
2012-03-19 19:57 PDT, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review
part 13: move drag event dispatch logic from nsWindow to nsDragService (9.84 KB, patch)
2012-03-19 20:07 PDT, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review
part 14: move nsWindow::UpdateDragStatus to nsDragService::UpdateDragAction (9.84 KB, patch)
2012-03-19 20:12 PDT, Karl Tomlinson (:karlt)
no flags Details | Diff | Splinter Review
part 14: move nsWindow::UpdateDragStatus to nsDragService::UpdateDragAction (5.87 KB, patch)
2012-03-19 20:15 PDT, Karl Tomlinson (:karlt)
enndeakin: review+
Details | Diff | Splinter Review
part 15: remove nsIDragSessionGTK (13.88 KB, patch)
2012-03-19 20:21 PDT, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review
part 12a2: Add a helper to get the nsDragService (2.08 KB, patch)
2012-03-20 19:06 PDT, Karl Tomlinson (:karlt)
roc: review+
Details | Diff | Splinter Review

Description Karl Tomlinson (:karlt) 2009-06-10 18:56:05 PDT
On receiving an XdndPosition event (or similar for another protocol) GTK sends
a drag-motion signal to the appropriate widget and waits for the signal
handler to return indicating whether the widget will handle drag events.

Mozilla runs arbitrary JavaScript during handling the drag-motion signal which
often starts a nested event loop.

During the nested event loop, GTK often processes the subsequent XdndLeave
event, but as the widget has not yet indicated interest in drag events
(through the drag-motion handler), GTK does not send a drag-leave signal to
the widget.

A common cause of nested event loops could be addressed through bug 497494,
but I assume the JavaScript can start other event loops through an alert or
what else?
Comment 1 Karl Tomlinson (:karlt) 2010-05-11 17:19:43 PDT
As well as the increased CPU usage, this is also noticeable in that once this happens no drags (tabs, text selections, ...) can be initiated.
Comment 2 Karl Tomlinson (:karlt) 2010-05-11 18:33:00 PDT
The right thing to do here is probably to reply to the drag-motion signal immediately, after scheduling an event to dispatch the NS_DRAGDROP_OVER and NS_DRAGDROP_ENTER.  That may need some accounting to cancel the scheduled event if the drag-leave signal is received on the same widget.
Comment 3 Karl Tomlinson (:karlt) 2010-05-28 13:35:11 PDT
Since bug 545714 landed, the nested event loop of bug 497494 is always being run on the first drag over, so this is much easier to trigger.

One set of STR is:

1. Run Firefox with NSPR_LOG_MODULES=nsDragService:5,WidgetDrag:5 in the
   environment.

2. Drag something from another app quickly across the corner of about:blank.

3. Repeat 2 until the log spew continues without drag motion.

The impact on the user is that, after this starts, no drags can be initiated, and the app fires a timer every 100ms, increasing CPU use.
Comment 4 Karl Tomlinson (:karlt) 2011-02-14 16:59:23 PST
I'll attach a series of patches.  Most are moving things around or simplifying things to minimize the size of the final patch in the series.  Some are corrections to behaviour that seemed appropriate to do here in this bug because otherwise I would have been designing new code to implement the old wrong behaviour.  For parts that alter interaction with Gecko drag code or the DOM, I'll request review from enndeakin.  For parts that are specific to nsWindow and GTK interaction, I'll request review from roc.
Comment 5 Karl Tomlinson (:karlt) 2011-02-14 17:13:11 PST
Created attachment 512352 [details] [diff] [review]
part 1: Only dispatch drop events when canDrop has been set via dragover and only indicate successful drop when the drop is sent

Only dispatch "drop" events when canDrop has been set via "dragover" and only indicate successful drop when the drop is sent.

Dispatching "dragleave" instead of "drop" when canDrop is false is consistent
with http://hg.mozilla.org/mozilla-central/annotate/a05e91710adb/widget/src/cocoa/nsChildView.mm#l5795
and, while canDrop is consistent with dragAction, this is consistent with 
http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#drag-and-drop-processing-mode

(I can't see that we actually track whether the data was successfully fetched
during drop so the "success" value for gdk_drop_finish can still sometimes be
true when it should not be.)
Comment 6 Karl Tomlinson (:karlt) 2011-02-14 17:20:18 PST
Created attachment 512354 [details] [diff] [review]
part 2: no need to explicitly dispatch NS_DRAGDROP_ENTER as this happens during NS_DRAGDROP_OVER
Comment 7 Karl Tomlinson (:karlt) 2011-02-14 17:21:55 PST
In part 2, we can simplify things a bit by leaving NS_DRAGDROP_ENTER handling to the event state manager.

nsEventStateManager::GenerateDragDropEnterExit (called from
nsEventStateManager::PreHandleEvent for NS_DRAGDROP_OVER events) will dispatch
an NS_DRAGDROP_ENTER event.

This dispatch differs from the NS_DRAGDROP_ENTER processing in that
nsEventStateManager::PostHandleEvent will not be invoked to SetDragAction,
SetCanDrop or SetOnlyChromeDrop on the nsIDragSession.  dragAction and canDrop
get reset before the DRAGDROP_OVER event anyway and I assume onlyChromeDrop is
only needed for the drop (which will not happen until after the DRAGDROP_OVER.
Comment 8 Karl Tomlinson (:karlt) 2011-02-14 17:34:08 PST
Created attachment 512360 [details] [diff] [review]
part 3: don't set a drag context with the new widget for a leave event on the old widget

This code is going to be rearranged in the final patch in the series, but I'm
requesting review on this logic change separately. 

The comment "set this now before any of the drag enter or leave events happen"
was in the initial port from GTK1 in bug 121251.  With this change we still
set the context before enter events (during _OVER) but after leave events.

With this patch, during the leave events there will be no context from which
nsIDragSession::GetData or GetNumDropItems or isDataFlavorSupported can fetch
information during the leave event (for an external drag).  That seems
logically appropriate to me, because once the drag has passed over an element
it is clearly not intended for that element.  This was already the case for
_LEAVE events sent after a native drag-leave signal; this patch simply makes
the behavior consistent for synthetic _LEAVE events generated during
CheckNeedDragLeave().
Comment 9 Karl Tomlinson (:karlt) 2011-02-14 17:40:05 PST
Created attachment 512361 [details] [diff] [review]
part 4: fire NS_DRAGDROP_DRAG at source even before synthetic NS_DRAGDROP_OVER events

Later in the series, this code will be shared with the drag-motion code. 

For consistency with regular dragover events, a drag event should be fired at
the source.
Comment 10 Karl Tomlinson (:karlt) 2011-02-14 17:47:18 PST
Created attachment 512363 [details] [diff] [review]
part 5: move gdk_drag_status code from nsDragService to drag-motion signal handler

gdk_drag_status() is associated with the drag-motion signal, not nsDragService.
I think this makes what is happening more explicit.
I've left the nsIDragSessionGTK interface intact.  Now, we just don't do anything in response to the notifications.
Comment 11 Karl Tomlinson (:karlt) 2011-02-14 17:52:31 PST
Created attachment 512364 [details] [diff] [review]
part 6: use event and widget coordinates for drop point instead of new cursor position

gdk_display_get_pointer synchronously gets the current pointer position, but
we want the position where the drop happened.  With all the inter-process
communication during drags, the pointer may well have moved well away from the
drop location.  In the final patch in this series, the _DROP event is
scheduled off the event loop adding to the issues.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-14 17:55:55 PST
Comment on attachment 512363 [details] [diff] [review]
part 5: move gdk_drag_status code from nsDragService to drag-motion signal handler

I presume we need a followup patch to remove the unused methods from nsIGTKDragSession?
Comment 13 Karl Tomlinson (:karlt) 2011-02-14 17:59:45 PST
Created attachment 512366 [details] [diff] [review]
part 6b: use event coordinates for source drag end point instead of new cursor position

This patch doesn't /need/ to be in this series, but part 6 sets the drag end point for drags ending in the Gecko app.  This is the corresponding fix for drags starting in Gecko but ending outside the Gecko app.
Comment 14 Karl Tomlinson (:karlt) 2011-02-14 18:07:27 PST
Created attachment 512368 [details] [diff] [review]
part 7: use a helper function for dispatching destination drag events
Comment 15 Karl Tomlinson (:karlt) 2011-02-14 18:10:54 PST
Created attachment 512370 [details] [diff] [review]
part 8: split drag event dispatch code from GTK signal handling methods
Comment 16 Karl Tomlinson (:karlt) 2011-02-14 18:14:16 PST
Created attachment 512371 [details] [diff] [review]
part 9: rename UpdateDragStatus to UpdateDragAction
Comment 17 Karl Tomlinson (:karlt) 2011-02-14 18:29:06 PST
Created attachment 512375 [details] [diff] [review]
part 10: use nsIDragSession::getCurrentSession to remove sIsDraggingOutOf

This implementation of DragInProgress, added for bug 179078, is more convenient for this patch series.
Comment 18 Karl Tomlinson (:karlt) 2011-02-14 18:32:13 PST
Created attachment 512377 [details] [diff] [review]
part 11: move early drag end code to drag_motion_event_cb and correct return value

OnDragMotionEvent, etc. won't exist after the next patch.
Comment 19 Karl Tomlinson (:karlt) 2011-02-14 19:16:30 PST
Created attachment 512382 [details] [diff] [review]
part 12: schedule event dispatch in response to GTK drag signals to avoid running the event loop at unexpected times

This is the key fix for this bug.

The DragEventDispatcher comment gives an overview and most of the reasoning.

Two changes included here that didn't make sense to separate out into earlier patches are:

gtk_drag_finish() is what is required after a GTK drag-drop signal.
gdk_drop_finish() was OK for Xdnd but does not send the right events for Motif
drops.

We don't need to send another _OVER event before the _DROP if we have already
sent one exactly the same.
Comment 20 Karl Tomlinson (:karlt) 2011-02-14 19:20:37 PST
http://hg.mozilla.org/try/annotate/8a1e416fa877/widget/src/gtk2/nsWindow.cpp
can be used to see the final state of nsWindow.cpp.
(That version is missing some comment changes though.)
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-14 19:47:24 PST
+    nsRefPtr<nsWindow> mCurrentWindow;
+    nsRefPtr<nsWindow> mPendingWindow;
+    nsCountedRef<GdkDragContext> mDragContext;
+    nsIntPoint mCurrentWindowPoint;
+    nsIntPoint mPendingWindowPoint;
+    nsIntPoint mRootDropPoint;
+    guint mTime;
+    DragStatus mScheduledStatus;

Can we have some comments about what these mean?

I wonder whether we should take this for FF4. It looks very risky.
Comment 22 Karl Tomlinson (:karlt) 2011-02-14 20:25:15 PST
(In reply to comment #3)
> 2. Drag something from another app quickly across the corner of about:blank.

> The impact on the user is that, after this starts, no drags can be initiated,
> and the app fires a timer every 100ms, increasing CPU use.

The fix for bug 495343 means that the timer no longer fires every 100ms.

Without the patches here though, I still sometimes find myself unable to drag anything in Firefox because I've dragged something completely unrelated across the desktop which happened to pass over Firefox.  I can resolve this by dragging something across Firefox again but slowly.
Comment 23 Karl Tomlinson (:karlt) 2011-02-14 21:10:18 PST
Created attachment 512397 [details] [diff] [review]
part 12: schedule event dispatch in response to GTK drag signals to avoid running the event loop at unexpected times v1.1

Added documentation for member variables.
Comment 24 Neil Deakin 2011-02-15 08:41:24 PST
With these patches, and the drop doesn't occur, I get a dragleave event occurring after the dragend event.
Comment 25 Karl Tomlinson (:karlt) 2011-02-15 13:59:07 PST
Thanks Neil.  I assume that's because we get the drag-end signal when we reply to drag-drop before sending the NS_DRAGDROP_EXIT.  I'll need to think about that.
Comment 26 Karl Tomlinson (:karlt) 2011-02-16 00:33:02 PST
Created attachment 512731 [details]
dropless dragleave/dragend testcase

Drag div A and drop onto div B, which doesn't accept the drop.
Expected order of events is dragstart, dragenter, dragleave, dragend.

Without the patches here, the dragleave does not arrive until after a second
drag starts.

43:373 DIV: dragstart client:430,32 screen:983,309
43:699 DIV: dragenter client:436,68 screen:989,345
45:364 DIV: dragend client:0,0 screen:983,388
46:538 DIV: dragstart client:385,43 screen:938,320
46:573 DIV: dragleave client:386,54 screen:939,331
46:635 DIV: dragenter client:388,68 screen:941,345
47:356 DIV: dragend client:0,0 screen:945,391

Which the patches here, the dragleave arrives soon after the dragend.

51:897 DIV: dragstart client:408,51 screen:554,292
51:948 DIV: dragenter client:410,81 screen:556,322
52:547 DIV: dragend client:0,0 screen:558,361
52:552 DIV: dragleave client:412,120 screen:558,361
53:747 DIV: dragstart client:383,45 screen:529,286
53:893 DIV: dragenter client:384,76 screen:530,317
54:493 DIV: dragend client:0,0 screen:533,362
54:499 DIV: dragleave client:387,121 screen:533,362

This is an improvement on the current situation, but still not quite right.
I'll look into what is happening.
Comment 27 Karl Tomlinson (:karlt) 2011-02-16 13:19:59 PST
I filed Bug 634719 on the order of the dragleave event as the cause is not related to patches here.
Comment 28 Karl Tomlinson (:karlt) 2011-02-16 15:03:39 PST
Created attachment 512940 [details] [diff] [review]
part 12: schedule event dispatch in response to GTK drag signals to avoid running the event loop at unexpected times v1.2

Changed some word usage to distinguish "tasks" that are run on the main loop from "events" that are dispatched from widget code.  The signal/task/event distinction is helpful.
Comment 29 Karl Tomlinson (:karlt) 2011-02-16 15:54:38 PST
However, fixing Bug 634719 involves co-ordinating source and destination events, and moving DragEventDispatcher onto nsDragService could make that easier.
Comment 30 Neil Deakin 2011-02-17 08:43:27 PST
The patches here look ok, but the dragleave issue means that my manual dragdrop tests fail, so I'm not absolutely certain there isn't some other issue.
Comment 31 Karl Tomlinson (:karlt) 2011-02-17 13:04:43 PST
Thanks for looking, Neil.
I'm going to look at moving the new code to nsDragService (though I'm not sure I'll being doing that soon).  At that point it should be relatively easy to add code to coordinate dragleave and dragend.  It sounds like that will help with your tests, which would provide reassurance, so that would be the way to go here.
Comment 32 Neil Deakin 2011-08-16 06:10:43 PDT
Karl, are you still plans here or can I clear the review requests for now?
Comment 33 Karl Tomlinson (:karlt) 2011-08-18 19:29:51 PDT
The review requests that I've left here are for patches that are still going to be stages in the new plan as of comment 31.

I think they are mostly correctness or simplification patches, and they can land before the new work is done, but if you'd like to wait to test the whole series when I've written the patches for the new plan, then that is fine too.
Comment 34 Karl Tomlinson (:karlt) 2012-03-19 19:11:39 PDT
Most of the patches here are still relevant apart from minor merge collisions with the PRBool-to-bool and GTK3 changes.  I'll upload patches that have more changes than that.  The easiest way to get the full updated and merged series for this bug and bug 634719 is from the branch with head https://hg.mozilla.org/try/rev/f65485cf4e20 (i.e. hg pull -r f65485cf4e20 https://hg.mozilla.org/try).
Comment 35 Karl Tomlinson (:karlt) 2012-03-19 19:23:34 PDT
Created attachment 607413 [details] [diff] [review]
part 5: use nsDragService::SetCanDrop instead of nsIDragSessionGTK::StartDragMotion to reset before NS_DRAGDROP_OVER

With event dispatch moving to nsDragService, it no longer makes sense to move the gdk_drag_status code to nsWindow so I've dropped that code from the patch.
I've kept the changes beginning migration away from nsIDragSessionGTK as subsequent reviewed patches depend on this.  Carrying forward review.
Comment 36 Karl Tomlinson (:karlt) 2012-03-19 19:39:46 PDT
Created attachment 607416 [details] [diff] [review]
part 12a: prevent GetTargetDragData (and IsTargetContextList) from being called without a target GtkWidget

Part 9 will change and move to after the bulk of the changes.

Part 12 will change to an implementation on nsDragService.

This patch is new.  There is an existing bug in that mTargetWidget and mTargetDragContext can be inappropriately used when null, if some of these public methods get called at the wrong times.  The new code in the next patch will also add the possibility that mTargetWidget is null if a target window is destroyed, so including the null check here.
Comment 37 Karl Tomlinson (:karlt) 2012-03-19 19:41:18 PDT
Additional comment on part 12a:
mTargetDragContext will still always be non-null when mTargetWidget is non-null.
Comment 38 Karl Tomlinson (:karlt) 2012-03-19 19:57:14 PDT
Created attachment 607420 [details] [diff] [review]
part 12b: schedule event dispatch in response to GTK drag target signals to avoid running the event loop at unexpected times

This is the bulk of the old part 12 with DragEventDispatcher replaced with an implementation on nsDragService.

See comment 19 for a description of the more minor behaviour changes introduced here.

There are a couple of things for which I felt I didn't find an elegant solution:

1. The StartDragSession and EndDragSession both happen to work on the same
nsDragService, I assume because the first do_GetService call must hold onto an
extra reference to the nsDragService until shutdown.  Using that knowledge we
could provide a static method on nsDragService to return a pointer to the
singleton, but it would look precarious copying the raw pointer from the
nsCOMPtr and then releasing when the nsCOMPtr goes out of scope.  As this patch is we end up with the do_GetService/static_cast code splattered around.  It's a step in the direction on deCOMtamination, but barely.  I don't know if there's a more elegant way to have a singleton accessible both through XPCOM and directly.

2. We can't have multiple definitions of nsAutoRefTraits template specializations, which makes using nsAutoRef tricky in headers.  While this patch doesn't introduce any conflicts, the potential arises once these specializations are in headers, so the HAVE_NSAUTOREFTRAITS_* preprocessor symbols are trying to establish a convention to resolve these problems.  I guess we could try to put all of these in a global header file.  There aren't many to put in there right now, but let me know if you'd prefer that.
Comment 39 Karl Tomlinson (:karlt) 2012-03-19 20:07:13 PDT
Created attachment 607421 [details] [diff] [review]
part 13: move drag event dispatch logic from nsWindow to nsDragService

This logic is now simpler on nsDragService and with that the inter-file interface is a little smaller.
Comment 40 Karl Tomlinson (:karlt) 2012-03-19 20:12:22 PDT
Created attachment 607424 [details] [diff] [review]
part 14: move nsWindow::UpdateDragStatus to nsDragService::UpdateDragAction

This replaces the old part 9 and now occurs later in the series.

Again things are simpler with this on nsDragService.
Comment 41 Karl Tomlinson (:karlt) 2012-03-19 20:15:37 PDT
Created attachment 607425 [details] [diff] [review]
part 14: move nsWindow::UpdateDragStatus to nsDragService::UpdateDragAction

(right patch this time)
Comment 42 Karl Tomlinson (:karlt) 2012-03-19 20:21:07 PDT
Created attachment 607426 [details] [diff] [review]
part 15: remove nsIDragSessionGTK

This interface is Copyright (C) 1998.  I don't think we need XPCOM for this, but let me know if you prefer the type-safety it offers over static_cast.
Comment 43 Neil Deakin 2012-03-20 06:00:32 PDT
Comment on attachment 607425 [details] [diff] [review]
part 14: move nsWindow::UpdateDragStatus to nsDragService::UpdateDragAction

>+nsDragService::UpdateDragAction()
>+{
>+    // This doesn't look right.  dragSession.dragAction is used by
>+    // nsContentUtils::SetDataTransferInEvent() to set the initial
>+    // dataTransfer.dropEffect, so GdkDragContext::suggested_action would be
>+    // more appropriate.  GdkDragContext::actions should be used to set
>+    // dataTransfer.effectAllowed, which doesn't currently happen with
>+    // external sources.
>+

Yes, it should be the setting the drag action to the one the user is requesting.
Comment 44 Neil Deakin 2012-03-20 08:13:19 PDT
Comment on attachment 512360 [details] [diff] [review]
part 3: don't set a drag context with the new widget for a leave event on the old widget

Not sure if this will regress bug 539862, but later patches seem to remove this code anyway.
Comment 45 Neil Deakin 2012-03-20 08:14:42 PDT
(In reply to Karl Tomlinson (:karlt) from comment #7)
> This dispatch differs from the NS_DRAGDROP_ENTER processing in that
> nsEventStateManager::PostHandleEvent will not be invoked to SetDragAction,
> SetCanDrop or SetOnlyChromeDrop on the nsIDragSession.  dragAction and
> canDrop
> get reset before the DRAGDROP_OVER event anyway and I assume onlyChromeDrop
> is
> only needed for the drop (which will not happen until after the
> DRAGDROP_OVER.

Is dragover guaranteed to be fired after dragenter?

The actions can be changed during dragenter as well.
Comment 46 Karl Tomlinson (:karlt) 2012-03-20 13:09:03 PDT
(In reply to Neil Deakin from comment #44)
> Comment on attachment 512360 [details] [diff] [review]
> part 3: don't set a drag context with the new widget for a leave event on
> the old widget
> 
> Not sure if this will regress bug 539862, but later patches seem to remove
> this code anyway.

I'm assuming bug 539862 was about the enter event.
(Later patches use different code, but implement similar logic.)
I tested with a theme, and got a door hanger saying it couldn't be installed because the theme wasn't compatible with nightly.  That seems to indicate that the drop was successful.
Comment 47 Karl Tomlinson (:karlt) 2012-03-20 14:07:52 PDT
(In reply to Neil Deakin from comment #45)
> (In reply to Karl Tomlinson (:karlt) from comment #7)
> > This dispatch differs from the NS_DRAGDROP_ENTER processing in that
> > nsEventStateManager::PostHandleEvent will not be invoked to SetDragAction,
> > SetCanDrop or SetOnlyChromeDrop on the nsIDragSession.  dragAction and
> > canDrop get reset before the DRAGDROP_OVER event anyway and I assume
> > onlyChromeDrop is only needed for the drop (which will not happen until
> > after the DRAGDROP_OVER.
> 
> Is dragover guaranteed to be fired after dragenter?

GTK has only a single "drag-motion" signal with no separate "drag-enter" or
similar.  The existing code synthesizes a NS_DRAGDROP_ENTER during drag-motion
when appropriate before the NS_DRAGDROP_OVER, which always follows.

This patch removes the synthetic NS_DRAGDROP_ENTER, dispatching only the
NS_DRAGDROP_OVER and depending on the ESM to dispatch appropriate enter events
(as it needs to do this anyway to handle crossing into a new element).

As the ESM dispatches the NS_DRAGDROP_ENTER during PreHandleEvent for
NS_DRAGDROP_OVER, I assume this is before the dispatch of the over event.
I'm not expecting the ESM to cancel the dispatch of the over event.

Please let me know if I'm missing the point of your question.

> The actions can be changed during dragenter as well.

I see my comment was wrong about the dragAction getting reset before
DRAGDROP_OVER.  That actually happened before the DRAGDROP_ENTER.

If a NS_DRAGDROP_ENTER were dispatched from widget PostHandleEvent would
SetDragAction if the dragenter is cancelled.  If a subsequent NS_DRAGDROP_OVER
is dispatched but not cancelled, it looks like PostHandleEvent resets
dropEffect to none, but SetDragAction is not called again.  If widget
dispatches only the NS_DRAGDROP_OVER, it looks like we are better off if anything
because we don't end up with a stale dragAction.

I'm really assuming the ESM gets synthetic enter events right, as otherwise there would be existing problems.

Again, please let me know if I'm missing the point of your comment.
Comment 48 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-20 15:59:43 PDT
Comment on attachment 607420 [details] [diff] [review]
part 12b: schedule event dispatch in response to GTK drag target signals to avoid running the event loop at unexpected times

Review of attachment 607420 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gtk2/nsDragService.cpp
@@ +1802,5 @@
> +        // that a leave or drop is waiting, but managing different priorities
> +        // may not be worth the effort.  Motion tasks shouldn't queue up as
> +        // they should be throttled based on replies.
> +        mTaskSource =
> +            g_idle_add_full(G_PRIORITY_HIGH, TaskDispatchCallback, this, NULL);

What keeps 'this' alive during the TaskDispatchCallback?

Should we addref "this" here and unref it during the callback?

Should we perhaps not even pass the nsDragService here, and just get the singleton during the callback? r+ if you do that.

::: widget/gtk2/nsWindow.cpp
@@ +5820,5 @@
>          return;
>  
> +    nsCOMPtr<nsIDragService> dragService = do_GetService(kCDragServiceCID);
> +    nsDragService *dragServiceGTK =
> +        static_cast<nsDragService*>(dragService.get());

Put this in a helper function in nsDragService --- nsDragService::GetInstance()?
Comment 49 Karl Tomlinson (:karlt) 2012-03-20 19:06:22 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #48)
> > +        mTaskSource =
> > +            g_idle_add_full(G_PRIORITY_HIGH, TaskDispatchCallback, this, NULL);
> 
> What keeps 'this' alive during the TaskDispatchCallback?

It's relying on XPCOM not shutting down before returning to the main event loop.
This is a pre-existing assumption made by the drag-end and drag-failed signal handlers when they call nsBaseDragService::EndDragSession to dispatch dom events and then modify the DragService.
Comment 50 Karl Tomlinson (:karlt) 2012-03-20 19:06:30 PDT
Created attachment 607833 [details] [diff] [review]
part 12a2: Add a helper to get the nsDragService

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #48)
> > +    nsCOMPtr<nsIDragService> dragService = do_GetService(kCDragServiceCID);
> > +    nsDragService *dragServiceGTK =
> > +        static_cast<nsDragService*>(dragService.get());
> 
> Put this in a helper function in nsDragService ---
> nsDragService::GetInstance()?

Is this what you mean, or do you want to return already_AddRefed for callers to use nsRefPtrs?
Comment 51 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-20 19:21:59 PDT
(In reply to Karl Tomlinson (:karlt) from comment #49)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #48)
> > > +        mTaskSource =
> > > +            g_idle_add_full(G_PRIORITY_HIGH, TaskDispatchCallback, this, NULL);
> > 
> > What keeps 'this' alive during the TaskDispatchCallback?
> 
> It's relying on XPCOM not shutting down before returning to the main event
> loop.
> This is a pre-existing assumption made by the drag-end and drag-failed
> signal handlers when they call nsBaseDragService::EndDragSession to dispatch
> dom events and then modify the DragService.

It wasn't obvious to me that XPCOM can't shut down before that glib idle handler runs. But OK.
Comment 52 Karl Tomlinson (:karlt) 2012-03-20 19:27:07 PDT
XPCOM can shut down before the idle handler runs, but ~nsDragService takes care of that.  It's not expected to shut down while the handler is running.
Comment 53 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-20 19:33:09 PDT
Comment on attachment 607420 [details] [diff] [review]
part 12b: schedule event dispatch in response to GTK drag target signals to avoid running the event loop at unexpected times

Review of attachment 607420 [details] [diff] [review]:
-----------------------------------------------------------------

OK, r+ assuming you revise this to call the new GetInstance method.
Comment 54 Neil Deakin 2012-04-09 11:12:19 PDT
Comment on attachment 512354 [details] [diff] [review]
part 2: no need to explicitly dispatch NS_DRAGDROP_ENTER as this happens during NS_DRAGDROP_OVER

Sorry, didn't realize I didn't mark this as reviewed.
Comment 57 :Ms2ger (⌚ UTC+1/+2) 2012-04-19 02:31:07 PDT
Comment on attachment 512352 [details] [diff] [review]
part 1: Only dispatch drop events when canDrop has been set via dragover and only indicate successful drop when the drop is sent

Review of attachment 512352 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/gtk2/nsWindow.cpp
@@ +3520,5 @@
>      LOGDRAG(("nsWindow::OnDragDropSignal\n"));
>  
>      // get our drag context
>      nsCOMPtr<nsIDragService> dragService = do_GetService(kCDragServiceCID);
> +    nsDragService *dragServiceGTK = static_cast<nsDragService*>(dragService.get());

Can't extensions override this service?
Comment 58 Neil Deakin 2012-04-19 07:47:31 PDT
> >      nsCOMPtr<nsIDragService> dragService = do_GetService(kCDragServiceCID);
> > +    nsDragService *dragServiceGTK = static_cast<nsDragService*>(dragService.get());
> 
> Can't extensions override this service?

We already static_cast the drag service on Windows and Mac.

Note You need to log in before you can comment on or make changes to this bug.