Bug 36095

Summary: REGRESSION(r53287): drop event is not fired if dataTransfer.dropEffect is not explicitly set
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dbates
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+

Description Oliver Hunt 2010-03-14 04:29:04 PDT
REGRESSION(r53287): drop event is not fired if dataTransfer.dropEffect is not explicitly set
Comment 1 Oliver Hunt 2010-03-14 04:39:11 PDT
Created attachment 50671 [details]
Patch
Comment 2 Oliver Hunt 2010-03-14 04:47:05 PDT
Comment on attachment 50671 [details]
Patch

Gah! despite fixing every difference i could obviously see the testcase still fails :-/
Comment 3 Oliver Hunt 2010-03-14 14:20:10 PDT
Created attachment 50675 [details]
Patch
Comment 4 Darin Adler 2010-03-14 14:35:52 PDT
Comment on attachment 50675 [details]
Patch

> -    , m_dropEffect("none")
> +    , m_dropEffect("uninitialized")

Why not use the null string instead of the string "uninitialized"?

>  static DragOperation dragOpFromIEOp(const String& op)
>  {
>      // yep, it's really just this fixed set
> -    if (op == "uninitialized")
> +    if (op.isEmpty() || op == "uninitialized")
>          return DragOperationEvery;

What does this change accomplish?

> +    if (srcOpMask & DragOperationMove || srcOpMask & DragOperationGeneric)
> +        return (srcOpMask = DragOperationMove);

Could you write this in a more conventional fashion so the assignment statement is more obvious?

    if (srcOpMask & DragOperationMove || srcOpMask & DragOperationGeneric) {
        srcOpMask = DragOperationMove;
        return DragOperationMove;
    }

A function like defaultOperationForDrag, that has a side effect of changing its argument, probably needs a name with a verb rather than sounding just like a clean getter with no side effects.

I'm going to say r=me, but I'm puzzled by the use of a specific string for the uninitialized state and by the unexplained change to dragOpFromIEOp.
Comment 5 Oliver Hunt 2010-03-14 15:38:38 PDT
Comment on attachment 50675 [details]
Patch

Looking at addressing darin's comments lead to some obvious improvements in the patch.  New one coming up
Comment 6 Oliver Hunt 2010-03-14 16:13:52 PDT
Created attachment 50679 [details]
Patch
Comment 7 Darin Adler 2010-03-14 16:18:11 PDT
Comment on attachment 50679 [details]
Patch

> -    , m_dropEffect("none")
> +    , m_dropEffect("uninitialized")

Why not use the null string instead of "uninitialized"?

Both initializing to null and checking for null are more efficient than initializing to "uninitialized" and comparing with "uninitialized". Also, I think null is a pretty clear way to represent a drop effect that has not been explicitly set; arguably clearer than the string "uninitialized".

> +static DragOperation defaultOperationForDrag(const DragOperation& srcOpMask)

Since DragOperation is a scalar, I think typing it just DragOperation is fine; it doesn't have to be const DragOperation&.

r=me
Comment 8 Oliver Hunt 2010-03-14 16:34:13 PDT
(In reply to comment #7)
> (From update of attachment 50679 [details])
> > -    , m_dropEffect("none")
> > +    , m_dropEffect("uninitialized")
> 
> Why not use the null string instead of "uninitialized"?
> 
> Both initializing to null and checking for null are more efficient than
> initializing to "uninitialized" and comparing with "uninitialized". Also, I
> think null is a pretty clear way to represent a drop effect that has not been
> explicitly set; arguably clearer than the string "uninitialized".

The code for converting between these strings and the actual enum is used to distinguish valid from invalid strings, and an empty string is invalid -- i would have to either add an additional empty string check in some places, or make the conversion function take a flag to indicate that should convert an empty string

> 
> > +static DragOperation defaultOperationForDrag(const DragOperation& srcOpMask)
> 
> Since DragOperation is a scalar, I think typing it just DragOperation is fine;
> it doesn't have to be const DragOperation&.
whoops

> 
> r=me
Comment 9 Oliver Hunt 2010-03-14 17:02:08 PDT
Committed r55977: <http://trac.webkit.org/changeset/55977>