Bug 33697 – dragOpFromIEOp("move") returns DragOperationGeneric instead of DragOperationMove

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brian Weinstein]

dragOpFromIEOp("move") returns DragOperationGeneric instead of DragOperationMove. This doesn't make any sense.

[Tony Chang]

Created attachment 51012 [details]
Patch

[Tony Chang]

(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.

[Tony Chang]

Also, as far as I can tell, this code hasn't changed since the initial commit:
http://trac.webkit.org/changeset/6779

[Adam Roben (:aroben)]

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.

[Tony Chang]

(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.

[Adam Roben (:aroben)]

(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>?

[Darin Adler]

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?

[Tony Chang]

(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.

[Tony Chang]

(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/.

[Eric Seidel (no email)]

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?

[Tony Chang]

(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.

[Tony Chang]

Comment on attachment 51012 [details]
Patch

Clearing flags on attachment: 51012

Committed r56513: <http://trac.webkit.org/changeset/56513>

[Tony Chang]

All reviewed patches have been landed.  Closing bug.