Bug 28902

Created attachment 40183 [details] Patch No test case attached because it uses the same test case as included in the patch for bug #24731. This patch can be applied before the patch for bug #24731, but there will be no noticeable effect because we use a hard-coded value in the Windows drag-and-drop code anyway: <http://trac.webkit.org/browser/trunk/WebKit/win/WebDropSource.cpp?rev=45515#L112>.

CC'ed Eric as he might be interested in this since he is interested in fixing drag-and-drop support for files in Win DRT (bug #25924).

Created attachment 40184 [details] Updated test case I updated the test case to work under Firefox (for comparison). Will merge this test case into the patch after I get something to eat.

Comment on attachment 40184 [details] Updated test case Oops, attached to the wrong bug (so hungry that I am not thinking straight ;-))

Comment on attachment 40183 [details] Patch > + // Reset |didDragEnter| so that another drag started within the same frame works properly. > + // > + // Notice, WebView::DragEnter (i.e. webViewDropTarget->DragEnter) initializes the field > + // |WebView::m_dragData|, which is checked in WebView::DragOver (i.e. webViewDropTarget->DragOver). > + // > + // If |didDragEnter| == false, then |WebView::m_dragData| is uninitialized, and by > + // <http://trac.webkit.org/browser/trunk/WebKit/win/WebView.cpp?rev=48754#L4657> > + // WebView::DragOver returns the drop effect DROPEFFECT_NONE, which is incorrect. > + // Hence, we set |didDragEnter| = false on a mouse up to reset the DRT drag-and-drop > + // machinery. I don't understand this comment. You say that the behavior of WebView::DragOver "is incorrect". Is there a bug filed about this incorrectness? If so, it would probably be better to reference that bug instead of the Trac link you have now. What is the correct behavior? Maybe this will all become clear when I see the bug.

Notice, Win DRT emulates the drag-and-drop event loop implemented in DoDragDrop <http://msdn.microsoft.com/en-us/library/ms678486%28VS.85%29.aspx>. For any drag-and-drop operation that ends with a successful drop on the target, the correct call flow in Win DRT is: webViewDropTarget->DragEnter, webViewDropTarget->DragOver, webViewDropTarget->Drop (*). Suppose there are two such drag-and-drop operations A, B on the same page. Tracing through the EventSender code, the call flow for A will be: (*) (and will set |didDragEnter| to true). But the call flow for B will be: webViewDropTarget->DragOver, webViewDropTarget->Drop (because |didDragEnter| is still true!; that is, it is never set to false after A completes). This is incorrect. The call flow should be the same for both operations and it should be (*). There is no bug filed about this. A better idea is to file a new bug for this issue with the fix (setting didDragEnter to false), and remove this change from the fix for this bug (#28902). Let me know your thoughts. (In reply to comment #5) > (From update of attachment 40183 [details]) > > + // Reset |didDragEnter| so that another drag started within the same frame works properly. > > + // > > + // Notice, WebView::DragEnter (i.e. webViewDropTarget->DragEnter) initializes the field > > + // |WebView::m_dragData|, which is checked in WebView::DragOver (i.e. webViewDropTarget->DragOver). > > + // > > + // If |didDragEnter| == false, then |WebView::m_dragData| is uninitialized, and by > > + // <http://trac.webkit.org/browser/trunk/WebKit/win/WebView.cpp?rev=48754#L4657> > > + // WebView::DragOver returns the drop effect DROPEFFECT_NONE, which is incorrect. > > + // Hence, we set |didDragEnter| = false on a mouse up to reset the DRT drag-and-drop > > + // machinery. > > I don't understand this comment. You say that the behavior of WebView::DragOver > "is incorrect". Is there a bug filed about this incorrectness? If so, it would > probably be better to reference that bug instead of the Trac link you have now. > What is the correct behavior? Maybe this will all become clear when I see the > bug.

(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 40183 [details] [details]) > > > + // Reset |didDragEnter| so that another drag started within the same frame works properly. > > > + // > > > + // Notice, WebView::DragEnter (i.e. webViewDropTarget->DragEnter) initializes the field > > > + // |WebView::m_dragData|, which is checked in WebView::DragOver (i.e. webViewDropTarget->DragOver). > > > + // > > > + // If |didDragEnter| == false, then |WebView::m_dragData| is uninitialized, and by > > > + // <http://trac.webkit.org/browser/trunk/WebKit/win/WebView.cpp?rev=48754#L4657> > > > + // WebView::DragOver returns the drop effect DROPEFFECT_NONE, which is incorrect. > > > + // Hence, we set |didDragEnter| = false on a mouse up to reset the DRT drag-and-drop > > > + // machinery. > > > > I don't understand this comment. You say that the behavior of WebView::DragOver > > "is incorrect". Is there a bug filed about this incorrectness? If so, it would > > probably be better to reference that bug instead of the Trac link you have now. > > What is the correct behavior? Maybe this will all become clear when I see the > > bug. > > Notice, Win DRT emulates the drag-and-drop event loop implemented in DoDragDrop > <http://msdn.microsoft.com/en-us/library/ms678486%28VS.85%29.aspx>. For any > drag-and-drop operation that ends with a successful drop on the target, the > correct call flow in Win DRT is: webViewDropTarget->DragEnter, > webViewDropTarget->DragOver, webViewDropTarget->Drop (*). > > Suppose there are two such drag-and-drop operations A, B on the same page. > Tracing through the EventSender code, the call flow for A will be: (*) (and > will set |didDragEnter| to true). But the call flow for B will be: > webViewDropTarget->DragOver, webViewDropTarget->Drop (because |didDragEnter| is > still true!; that is, it is never set to false after A completes). This is > incorrect. The call flow should be the same for both operations and it should > be (*). OK, so it sounds like your comment is explaining a bug that currently exists but that will be fixed by this patch. I think a much more concise comment would make this clearer. In fact, I'd recommend just removing everything but the first sentence of the comment you have in your patch. > There is no bug filed about this. A better idea is to file a new bug for this > issue with the fix (setting didDragEnter to false), and remove this change from > the fix for this bug (#28902). Let me know your thoughts. I think it's OK to leave this change as part of this patch. Thanks for clearing this up for me!

Created attachment 40213 [details] Patch Cleaned up comment based on Adam's suggestion.

Comment on attachment 40213 [details] Patch > + * DumpRenderTree/win/DraggingInfo.h: Added field |m_dropEffect| to store performed drop effect. I haven't seen this "|variableName|" style in ChangeLogs/comments in WebKit before. I'm not saying I'm opposed, just pointing out that you're going against the grain here. r=me

I'll change the formatting before I land. (In reply to comment #9) > (From update of attachment 40213 [details]) > > + * DumpRenderTree/win/DraggingInfo.h: Added field |m_dropEffect| to store performed drop effect. > > I haven't seen this "|variableName|" style in ChangeLogs/comments in WebKit > before. I'm not saying I'm opposed, just pointing out that you're going against > the grain here. > > r=me

Committed r48899: <http://trac.webkit.org/changeset/48899>