The default bug view has changed. See this FAQ.

drag-leave signals are not received due to processing X events before returning from the drag-motion handler

RESOLVED FIXED in mozilla15

Status

()

Core
Widget: Gtk
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {regression})

Trunk
mozilla15
x86
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [softblocker])

Attachments

(18 attachments, 6 obsolete attachments)

5.53 KB, patch
Neil Deakin
: review+
Details | Diff | Splinter Review
6.54 KB, patch
Neil Deakin
: review+
Details | Diff | Splinter Review
3.03 KB, patch
Neil Deakin
: review+
Details | Diff | Splinter Review
1.00 KB, patch
Neil Deakin
: review+
Details | Diff | Splinter Review
1.82 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.92 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.42 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.13 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.18 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.52 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.51 KB, text/html
Details
3.23 KB, patch
Details | Diff | Splinter Review
3.59 KB, patch
roc
: review+
Details | Diff | Splinter Review
42.56 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.84 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.87 KB, patch
Neil Deakin
: review+
Details | Diff | Splinter Review
13.88 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.08 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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?
(Assignee)

Comment 1

7 years ago
As well as the increased CPU usage, this is also noticeable in that once this happens no drags (tabs, text selections, ...) can be initiated.
(Assignee)

Comment 2

7 years ago
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.
(Assignee)

Comment 3

7 years ago
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.
blocking2.0: --- → ?
blocking2.0: ? → final+
Assignee: nobody → karlt
(Assignee)

Updated

7 years ago
Blocks: 595789
blocking2.0: final+ → -
Keywords: regression
blocking2.0: - → final+
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
Whiteboard: [softblocker]
(Assignee)

Updated

6 years ago
Blocks: 625745
(Assignee)

Updated

6 years ago
No longer blocks: 625745
(Assignee)

Updated

6 years ago
Blocks: 625745
(Assignee)

Updated

6 years ago
Blocks: 626797
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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.)
Attachment #512352 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #512352 - Flags: review? → review?(enndeakin)
(Assignee)

Comment 6

6 years ago
Created attachment 512354 [details] [diff] [review]
part 2: no need to explicitly dispatch NS_DRAGDROP_ENTER as this happens during NS_DRAGDROP_OVER
Attachment #512354 - Flags: review?(enndeakin)
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
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().
Attachment #512360 - Flags: review?(enndeakin)
(Assignee)

Comment 9

6 years ago
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.
Attachment #512361 - Flags: review?(enndeakin)
(Assignee)

Comment 10

6 years ago
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.
Attachment #512363 - Flags: review?(roc)
(Assignee)

Comment 11

6 years ago
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.
Attachment #512364 - Flags: review?(roc)
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?
Attachment #512363 - Flags: review?(roc) → review+
(Assignee)

Comment 13

6 years ago
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.
Attachment #512366 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #512366 - Attachment description: part 6b: use event and widget coordinates for source drag end point instead of new cursor position → part 6b: use event coordinates for source drag end point instead of new cursor position
Attachment #512364 - Flags: review?(roc) → review+
Attachment #512366 - Flags: review?(roc) → review+
(Assignee)

Comment 14

6 years ago
Created attachment 512368 [details] [diff] [review]
part 7: use a helper function for dispatching destination drag events
Attachment #512368 - Flags: review?(roc)
(Assignee)

Comment 15

6 years ago
Created attachment 512370 [details] [diff] [review]
part 8: split drag event dispatch code from GTK signal handling methods
Attachment #512370 - Flags: review?(roc)
(Assignee)

Comment 16

6 years ago
Created attachment 512371 [details] [diff] [review]
part 9: rename UpdateDragStatus to UpdateDragAction
Attachment #512371 - Flags: review?(enndeakin)
Attachment #512368 - Flags: review?(roc) → review+
Attachment #512370 - Flags: review?(roc) → review+
(Assignee)

Comment 17

6 years ago
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.
Attachment #512375 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #512375 - Attachment description: use nsIDragSession::getCurrentSession to remove sIsDraggingOutOf → part 8: use nsIDragSession::getCurrentSession to remove sIsDraggingOutOf
(Assignee)

Updated

6 years ago
Attachment #512375 - Attachment description: part 8: use nsIDragSession::getCurrentSession to remove sIsDraggingOutOf → part 10: use nsIDragSession::getCurrentSession to remove sIsDraggingOutOf
(Assignee)

Comment 18

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #512377 - Flags: review?(roc)
Attachment #512375 - Flags: review?(roc) → review+
Attachment #512377 - Flags: review?(roc) → review+
(Assignee)

Comment 19

6 years ago
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.
Attachment #512382 - Flags: review?(roc)
(Assignee)

Comment 20

6 years ago
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.)
+    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.
(Assignee)

Comment 22

6 years ago
(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.
(Assignee)

Comment 23

6 years ago
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.
Attachment #512382 - Attachment is obsolete: true
Attachment #512397 - Flags: review?(roc)
Attachment #512382 - Flags: review?(roc)

Comment 24

6 years ago
With these patches, and the drop doesn't occur, I get a dragleave event occurring after the dragend event.
(Assignee)

Comment 25

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #512397 - Flags: review?(roc)
(Assignee)

Comment 26

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #512731 - Attachment is patch: false
Attachment #512731 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 27

6 years ago
I filed Bug 634719 on the order of the dragleave event as the cause is not related to patches here.
(Assignee)

Comment 28

6 years ago
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.
Attachment #512397 - Attachment is obsolete: true
Attachment #512940 - Flags: review?(roc)
(Assignee)

Comment 29

6 years ago
However, fixing Bug 634719 involves co-ordinating source and destination events, and moving DragEventDispatcher onto nsDragService could make that easier.
(Assignee)

Updated

6 years ago
Attachment #512940 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #512371 - Flags: review?(enndeakin)

Comment 30

6 years ago
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.
(Assignee)

Comment 31

6 years ago
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.
blocking2.0: final+ → -
(Assignee)

Updated

6 years ago
Blocks: 645285

Comment 32

6 years ago
Karl, are you still plans here or can I clear the review requests for now?
(Assignee)

Comment 33

6 years ago
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.
(Assignee)

Comment 34

5 years ago
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).
(Assignee)

Comment 35

5 years ago
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.
Attachment #512363 - Attachment is obsolete: true
(Assignee)

Comment 36

5 years ago
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.
Attachment #512371 - Attachment is obsolete: true
Attachment #512940 - Attachment is obsolete: true
Attachment #607416 - Flags: review?(roc)
(Assignee)

Comment 37

5 years ago
Additional comment on part 12a:
mTargetDragContext will still always be non-null when mTargetWidget is non-null.
(Assignee)

Comment 38

5 years ago
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.
Attachment #607420 - Flags: review?(roc)
(Assignee)

Comment 39

5 years ago
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.
Attachment #607421 - Flags: review?(roc)
(Assignee)

Comment 40

5 years ago
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.
Attachment #607424 - Flags: review?(enndeakin)
(Assignee)

Updated

5 years ago
Attachment #607424 - Attachment description: move nsWindow::UpdateDragStatus to nsDragService::UpdateDragAction → part 14: move nsWindow::UpdateDragStatus to nsDragService::UpdateDragAction
(Assignee)

Comment 41

5 years ago
Created attachment 607425 [details] [diff] [review]
part 14: move nsWindow::UpdateDragStatus to nsDragService::UpdateDragAction

(right patch this time)
Attachment #607424 - Attachment is obsolete: true
Attachment #607425 - Flags: review?(enndeakin)
Attachment #607424 - Flags: review?(enndeakin)
(Assignee)

Comment 42

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #607426 - Flags: review?(roc)

Comment 43

5 years ago
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.
Attachment #607425 - Flags: review?(enndeakin) → review+

Updated

5 years ago
Attachment #512352 - Flags: review?(enndeakin) → review+

Updated

5 years ago
Attachment #512361 - Flags: review?(enndeakin) → review+

Comment 44

5 years ago
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.
Attachment #512360 - Flags: review?(enndeakin) → review+

Comment 45

5 years ago
(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.
(Assignee)

Comment 46

5 years ago
(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.
Attachment #607416 - Flags: review?(roc) → review+
(Assignee)

Comment 47

5 years ago
(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 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()?
Attachment #607421 - Flags: review?(roc) → review+
Attachment #607426 - Flags: review?(roc) → review+
(Assignee)

Comment 49

5 years ago
(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.
(Assignee)

Comment 50

5 years ago
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?
Attachment #607833 - Flags: review?(roc)
Attachment #607833 - Flags: review?(roc) → review+
(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.
(Assignee)

Comment 52

5 years ago
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 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.
Attachment #607420 - Flags: review?(roc) → review+

Comment 54

5 years ago
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.
Attachment #512354 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 55

5 years ago
I pushed the small parts 1 to 12a (with no part 9).
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9d4bca6460
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd66f15b1f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d31d468778b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a9491869898
https://hg.mozilla.org/integration/mozilla-inbound/rev/46b2bf54dd1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fadcc45f8fe8
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bea9978c445
https://hg.mozilla.org/integration/mozilla-inbound/rev/93a7a0655014
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f5e0ef6ad58
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf998e485300
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e72b79e7cee
https://hg.mozilla.org/integration/mozilla-inbound/rev/b73de501ea5e

I'll leave part 12a2 and subsequent (including the major part 12b) til after mozilla14 branches.
Whiteboard: [softblocker] → [softblocker][Leave open after merge]
https://hg.mozilla.org/mozilla-central/rev/9e9d4bca6460
https://hg.mozilla.org/mozilla-central/rev/5bd66f15b1f2
https://hg.mozilla.org/mozilla-central/rev/0d31d468778b
https://hg.mozilla.org/mozilla-central/rev/3a9491869898
https://hg.mozilla.org/mozilla-central/rev/46b2bf54dd1e
https://hg.mozilla.org/mozilla-central/rev/fadcc45f8fe8
https://hg.mozilla.org/mozilla-central/rev/8bea9978c445
https://hg.mozilla.org/mozilla-central/rev/93a7a0655014
https://hg.mozilla.org/mozilla-central/rev/7f5e0ef6ad58
https://hg.mozilla.org/mozilla-central/rev/cf998e485300
https://hg.mozilla.org/mozilla-central/rev/4e72b79e7cee
https://hg.mozilla.org/mozilla-central/rev/b73de501ea5e
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

5 years ago
> >      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.
(Assignee)

Comment 59

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc7ea0e82d83
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6358d51fa5
https://hg.mozilla.org/integration/mozilla-inbound/rev/509d2f42fc7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce526785dc49
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac5c8077e353
Whiteboard: [softblocker][Leave open after merge] → [softblocker]
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/3c6358d51fa5
https://hg.mozilla.org/mozilla-central/rev/509d2f42fc7f
https://hg.mozilla.org/mozilla-central/rev/ce526785dc49
https://hg.mozilla.org/mozilla-central/rev/ac5c8077e353
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Depends on: 753118
(Assignee)

Updated

4 years ago
Depends on: 809601
You need to log in before you can comment on or make changes to this bug.