Bug 33691 - Drag and Drop source/destination code needs cleanup
Summary: Drag and Drop source/destination code needs cleanup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-14 15:01 PST by Brian Weinstein
Modified: 2010-01-14 16:08 PST (History)
5 users (show)

See Also:


Attachments
Patch (5.34 KB, patch)
2010-01-14 15:01 PST, Brian Weinstein
no flags Details | Formatted Diff | Diff
Patch that works on Qt and Chromium (8.26 KB, patch)
2010-01-14 15:39 PST, Brian Weinstein
aroben: review+
bweinstein: commit-queue-
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 15:01:05 PST
Drag and Drop source/destination code needs cleanup
Comment 1 Brian Weinstein 2010-01-14 15:01:39 PST
Created attachment 46611 [details]
Patch
Comment 2 Adam Roben (:aroben) 2010-01-14 15:10:18 PST
Comment on attachment 46611 [details]
Patch

> -static DragOperation defaultOperationForDrag(DragOperation srcOpMask)
> -{
> -    // This is designed to match IE's operation fallback for the case where
> -    // the page calls preventDefault() in a drag event but doesn't set dropEffect.
> -    if (srcOpMask & DragOperationCopy)
> -         return DragOperationCopy;
> -    if (srcOpMask & DragOperationMove || srcOpMask & DragOperationGeneric)
> -        return DragOperationMove;
> -    if (srcOpMask & DragOperationLink)
> -        return DragOperationLink;
> -
> -    // FIXME: Does IE really return "generic" even if no operations were allowed by the source?
> -    return DragOperationGeneric;
> -}
> -
>  bool DragController::tryDHTMLDrag(DragData* dragData, DragOperation& operation)
>  {
>      ASSERT(dragData);
> @@ -517,10 +502,8 @@ bool DragController::tryDHTMLDrag(DragDa
>          return false;
>      }
>  
> -    if (!clipboard->destinationOperation(operation)) {
> -        // The element accepted but they didn't pick an operation, so we pick one (to match IE).
> -        operation = defaultOperationForDrag(srcOpMask);
> -    } else if (!(srcOpMask & operation)) {
> +    operation = clipboard->destinationOperation();
> +    if (!(srcOpMask & operation)) {
>          // The element picked an operation which is not supported by the source
>          operation = DragOperationNone;
>      }

Is it really OK to make this change? Will we break websites that call preventDefault() but don't set dropEffect?
Comment 3 Brian Weinstein 2010-01-14 15:13:29 PST
(In reply to comment #2)
> (From update of attachment 46611 [details])
> > -static DragOperation defaultOperationForDrag(DragOperation srcOpMask)
> > -{
> > -    // This is designed to match IE's operation fallback for the case where
> > -    // the page calls preventDefault() in a drag event but doesn't set dropEffect.
> > -    if (srcOpMask & DragOperationCopy)
> > -         return DragOperationCopy;
> > -    if (srcOpMask & DragOperationMove || srcOpMask & DragOperationGeneric)
> > -        return DragOperationMove;
> > -    if (srcOpMask & DragOperationLink)
> > -        return DragOperationLink;
> > -
> > -    // FIXME: Does IE really return "generic" even if no operations were allowed by the source?
> > -    return DragOperationGeneric;
> > -}
> > -
> >  bool DragController::tryDHTMLDrag(DragData* dragData, DragOperation& operation)
> >  {
> >      ASSERT(dragData);
> > @@ -517,10 +502,8 @@ bool DragController::tryDHTMLDrag(DragDa
> >          return false;
> >      }
> >  
> > -    if (!clipboard->destinationOperation(operation)) {
> > -        // The element accepted but they didn't pick an operation, so we pick one (to match IE).
> > -        operation = defaultOperationForDrag(srcOpMask);
> > -    } else if (!(srcOpMask & operation)) {
> > +    operation = clipboard->destinationOperation();
> > +    if (!(srcOpMask & operation)) {
> >          // The element picked an operation which is not supported by the source
> >          operation = DragOperationNone;
> >      }
> 
> Is it really OK to make this change? Will we break websites that call
> preventDefault() but don't set dropEffect?

That code will never be hit after r53287, because destinationOperation will never return false. This makes it compliant with the spec, but I haven't found any demos. I did have to make changes to some of our layout tests, that was why I had to roll out the change and roll it back in. I feel like being more compliant with the spec is important, but if there are a lot of websites that use drag and drop, then I could drop this change, and make some changes to r53287.
Comment 4 Adam Roben (:aroben) 2010-01-14 15:16:13 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 46611 [details] [details])
> > Is it really OK to make this change? Will we break websites that call
> > preventDefault() but don't set dropEffect?
> 
> That code will never be hit after r53287, because destinationOperation will
> never return false.

Ah, OK, so we've already made this change. You're just removing dead code now.

> This makes it compliant with the spec, but I haven't found
> any demos. I did have to make changes to some of our layout tests, that was why
> I had to roll out the change and roll it back in. I feel like being more
> compliant with the spec is important, but if there are a lot of websites that
> use drag and drop, then I could drop this change, and make some changes to
> r53287.

We should try to find sites that rely on the old behavior. It might be useful to find out what sites were used to reverse-engineer IE's implementation in order to write WebKit's.
Comment 5 WebKit Review Bot 2010-01-14 15:16:26 PST
Attachment 46611 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/187684
Comment 6 WebKit Review Bot 2010-01-14 15:28:06 PST
Attachment 46611 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/186750
Comment 7 Adam Roben (:aroben) 2010-01-14 15:32:40 PST
The old behavior was added in r6779: <http://trac.webkit.org/changeset/6779#file10>.
Comment 8 Brian Weinstein 2010-01-14 15:39:16 PST
Created attachment 46615 [details]
Patch that works on Qt and Chromium
Comment 9 Adam Roben (:aroben) 2010-01-14 15:54:01 PST
(In reply to comment #4)
> We should try to find sites that rely on the old behavior. It might be useful
> to find out what sites were used to reverse-engineer IE's implementation in
> order to write WebKit's.

Vicki Murley might remember!
Comment 10 Adam Roben (:aroben) 2010-01-14 15:56:19 PST
Comment on attachment 46615 [details]
Patch that works on Qt and Chromium

>  void Clipboard::setSourceOperation(DragOperation op)
>  {
> +    ASSERT(op != DragOperationPrivate);
>      m_effectAllowed = IEOpFromDragOp(op);
>  }
>  
>  void Clipboard::setDestinationOperation(DragOperation op)
>  {
> +    ASSERT(op == DragOperationCopy || op == DragOperationNone || op == DragOperationLink || op == DragOperationMove);
>      m_dropEffect = IEOpFromDragOp(op);
>  }

ASSERT_ARG would be better for these.

This looks fine to land. But I don't want to forget to find out if removing our old IE-compat code is going to bite us in the end. Maybe we should get a bug filed saying that we need to do more testing? (E.g., it would be useful to find out if IE even still has this behavior.)
Comment 11 Brian Weinstein 2010-01-14 16:08:06 PST
Landed in r53296.