Bug 33697 - dragOpFromIEOp("move") returns DragOperationGeneric instead of DragOperationMove
Summary: dragOpFromIEOp("move") returns DragOperationGeneric instead of DragOperationMove
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-14 16:37 PST by Brian Weinstein
Modified: 2010-03-25 01:46 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.52 KB, patch)
2010-03-18 00:46 PDT, Tony Chang
no flags 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-14 16:37:22 PST
dragOpFromIEOp("move") returns DragOperationGeneric instead of DragOperationMove. This doesn't make any sense.
Comment 1 Tony Chang 2010-03-18 00:46:40 PDT
Created attachment 51012 [details]
Patch
Comment 2 Tony Chang 2010-03-18 00:49:35 PDT
(In reply to comment #0)
> dragOpFromIEOp("move") returns DragOperationGeneric instead of
> DragOperationMove. This doesn't make any sense.

This is causing fast/events/drag-and-drop.html to fail on Chromium.  The test tries to set a dropEffect of 'move', but no operation occurs because the source operation mask doesn't include generic.
Comment 3 Tony Chang 2010-03-18 00:50:31 PDT
Also, as far as I can tell, this code hasn't changed since the initial commit:
http://trac.webkit.org/changeset/6779
Comment 4 Adam Roben (:aroben) 2010-03-18 06:22:16 PDT
How do we know this won't cause any regressions in interacting with the rest of the OS? Your patch doesn't mention what effect this will have on platforms that *do* have a concept of a "generic" operation.
Comment 5 Tony Chang 2010-03-18 07:11:51 PDT
(In reply to comment #4)
> How do we know this won't cause any regressions in interacting with the rest of
> the OS? Your patch doesn't mention what effect this will have on platforms that
> *do* have a concept of a "generic" operation.

The layout tests pass on mac and manual testing seems to confirm that nothing has regressed.  Is there some other way for me to test for regressions (manual or automated tests)?  I'm happy to write additional tests if you have some suggestions on areas that might not be well covered.
Comment 6 Adam Roben (:aroben) 2010-03-18 08:05:13 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > How do we know this won't cause any regressions in interacting with the rest of
> > the OS? Your patch doesn't mention what effect this will have on platforms that
> > *do* have a concept of a "generic" operation.
> 
> The layout tests pass on mac and manual testing seems to confirm that nothing
> has regressed.  Is there some other way for me to test for regressions (manual
> or automated tests)?  I'm happy to write additional tests if you have some
> suggestions on areas that might not be well covered.

Have you tried the case mentioned in <http://trac.webkit.org/changeset/3400>?
Comment 7 Darin Adler 2010-03-18 08:29:57 PDT
Comment on attachment 51012 [details]
Patch

> +        Covered by existing tests (fast/events/drag-and-drop.html).

If this change is covered by those tests, why aren't the tests failing?
Comment 8 Tony Chang 2010-03-18 14:51:04 PDT
(In reply to comment #7)
> (From update of attachment 51012 [details])
> > +        Covered by existing tests (fast/events/drag-and-drop.html).
> 
> If this change is covered by those tests, why aren't the tests failing?

The win port is passing because it converts generic into move:
http://trac.webkit.org/browser/trunk/WebKit/win/WebView.cpp#L4861

gkt and qt skip this test and this test is failing on the chromium ports.  If you prefer, I can make the chromium ports convert generic to move, but I imagine qt and gtk will have to do the same.
Comment 9 Tony Chang 2010-03-18 19:23:30 PDT
(In reply to comment #6)
> Have you tried the case mentioned in <http://trac.webkit.org/changeset/3400>?

Yes, I am still able to drag text from Safari to Terminal.  Note that I haven't removed Generic, I just added Move.

Let me know if this isn't a patch you want to take and I'll just work around the problem in WebKit/chromium/ like the code in WebKit/win/.
Comment 10 Eric Seidel (no email) 2010-03-25 00:33:35 PDT
Comment on attachment 51012 [details]
Patch

This looks sane to me.   Doesn't this also require changes to test_expectations now that that's in webkit.org?
Comment 11 Tony Chang 2010-03-25 00:36:48 PDT
(In reply to comment #10)
> (From update of attachment 51012 [details])
> This looks sane to me.   Doesn't this also require changes to test_expectations
> now that that's in webkit.org?

It does.  I will rev the patch in https://bugs.webkit.org/show_bug.cgi?id=36484 which will include the test_expectations change.
Comment 12 Tony Chang 2010-03-25 01:46:44 PDT
Comment on attachment 51012 [details]
Patch

Clearing flags on attachment: 51012

Committed r56513: <http://trac.webkit.org/changeset/56513>
Comment 13 Tony Chang 2010-03-25 01:46:53 PDT
All reviewed patches have been landed.  Closing bug.