Bug 36095 – REGRESSION(r53287): drop event is not fired if dataTransfer.dropEffect is not explicitly set

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

REGRESSION(r53287): drop event is not fired if dataTransfer.dropEffect is not explicitly set

[Oliver Hunt]

Created attachment 50671 [details]
Patch

[Oliver Hunt]

Comment on attachment 50671 [details]
Patch

Gah! despite fixing every difference i could obviously see the testcase still fails :-/

[Oliver Hunt]

Created attachment 50675 [details]
Patch

[Darin Adler]

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.

[Oliver Hunt]

Comment on attachment 50675 [details]
Patch

Looking at addressing darin's comments lead to some obvious improvements in the patch.  New one coming up

[Oliver Hunt]

Created attachment 50679 [details]
Patch

[Darin Adler]

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

[Oliver Hunt]

(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

[Oliver Hunt]

Committed r55977: <http://trac.webkit.org/changeset/55977>