Bug 33635 - [DnD] effectAllowed and dropEffect can be set to bogus values
: [DnD] effectAllowed and dropEffect can be set to bogus values
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Brian Weinstein
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-13 16:04 PST by Brian Weinstein
Modified: 2010-02-24 07:23 PST (History)
4 users (show)

See Also:


Attachments
[PATCH] Fix (13.59 KB, patch)
2010-01-13 16:26 PST, Brian Weinstein
bweinstein: commit‑queue-
Details | Formatted Diff | Diff
[PATCH] Fix without breaking Mac (13.60 KB, patch)
2010-01-13 16:34 PST, Brian Weinstein
no flags Details | Formatted Diff | Diff
[PATCH] Fix + Fixed Layout Tests (19.92 KB, patch)
2010-01-14 11:49 PST, Brian Weinstein
no flags Details | Formatted Diff | Diff
Patch (1.44 KB, patch)
2010-01-14 13:06 PST, Brian Weinstein
no flags Details | Formatted Diff | Diff
Patch (1.59 KB, patch)
2010-01-14 13:12 PST, Brian Weinstein
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2010-01-13 16:04:16 PST
event.dataTransfer.dropEffect = "bogus"

and

event.dataTransfer.effectAllowed = "bogus"

Both work, and can cause strange behavior in drag and drop behavior.
Comment 1 Brian Weinstein 2010-01-13 16:26:00 PST
Created attachment 46524 [details]
[PATCH] Fix
Comment 2 Eric Seidel 2010-01-13 16:30:37 PST
Attachment 46524 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/185881
Comment 3 Brian Weinstein 2010-01-13 16:34:09 PST
Created attachment 46526 [details]
[PATCH] Fix without breaking Mac
Comment 4 Oliver Hunt 2010-01-13 22:56:36 PST
Comment on attachment 46526 [details]
[PATCH] Fix without breaking Mac

r=me
Comment 5 Eric Seidel 2010-01-14 10:39:05 PST
Looks like this broke 5 tests on Snow Leopard:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r53270%20(4299)/results.html
Comment 6 Eric Seidel 2010-01-14 10:40:06 PST
And one test on Qt:
http://build.webkit.org/results/Qt%20Linux%20Release/r53270%20(5993)/fast/events/bogus-dropEffect-effectAllowed-pretty-diff.html

I expect Brian is fully aware of the issues. :)  Just noticed and thought I should record them on the off chance he missed them.
Comment 7 Brian Weinstein 2010-01-14 10:49:34 PST
Comment on attachment 46526 [details]
[PATCH] Fix without breaking Mac

This broke 5 layout tests on Mac, it was landed in r53270, but rolled out in r53272. I will fix the tests, and upload new results for Mac + GTK + Qt.
Comment 8 Brian Weinstein 2010-01-14 11:49:09 PST
Created attachment 46591 [details]
[PATCH] Fix + Fixed Layout Tests
Comment 9 Oliver Hunt 2010-01-14 12:33:49 PST
Comment on attachment 46591 [details]
[PATCH] Fix + Fixed Layout Tests

r=me
Comment 10 Brian Weinstein 2010-01-14 12:48:30 PST
Landed in r53287.
Comment 11 Brian Weinstein 2010-01-14 13:06:31 PST
Created attachment 46601 [details]
Patch
Comment 12 Adam Roben (:aroben) 2010-01-14 13:08:17 PST
Comment on attachment 46601 [details]
Patch

r=me

Can we make an even stronger assertion: that m_effectAllowed and m_dropEffect are one of the allowed values?
Comment 13 Brian Weinstein 2010-01-14 13:12:47 PST
Created attachment 46602 [details]
Patch
Comment 14 Adam Roben (:aroben) 2010-01-14 13:15:15 PST
Comment on attachment 46602 [details]
Patch

This looks OK. r=me. But now these functions always return true. Let's get rid of the boolean return value and just return a DragOperation!
Comment 15 Eric Seidel 2010-01-14 16:21:25 PST
This broke Leopard Debug due to a bad ASSERT:
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r53288%20(9281)/fast/events/drag-and-drop-stderr.txt

ASSERTION FAILED: op == DragOperationCopy || op == DragOperationNone || op == DragOperationLink || op == DragOperationMove
(/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebCore/dom/Clipboard.cpp:113 bool WebCore::Clipboard::destinationOperation(WebCore::DragOperation&) const)

Perhaps this was fixed in http://trac.webkit.org/changeset/53296?
Comment 16 Eric Seidel 2010-01-14 16:21:53 PST
Actually, this broke all debug builds. :(
Comment 17 Brian Weinstein 2010-01-14 16:23:52 PST
(In reply to comment #16)
> Actually, this broke all debug builds. :(

I have a fix, but am trying to figure out why dragOpFromIEOp returns DragOperationGeneric instead of DragOperationMove.
Comment 18 noel gordon 2010-02-24 07:23:46 PST
(In reply to comment #16)
> Actually, this broke all debug builds. :(

Is it still in that state?  And I noticed your reference to a related
change http://trac.webkit.org/changeset/53296, which removed some old
IE compat code.  So does 

  http://www.whatwg.org/demos/2008-sept/dnd/dnd.html

work for you on a webkit nightly?  Broken on chrome-dev for me, works
fine in Chrome4, Firefox 3.5.7/3.6, and IE6.  Other examples:

  http://html5demos.com/drag#