[webkit-reviews] review granted: [Bug 36095] REGRESSION(r53287): drop event is not fired if dataTransfer.dropEffect is not explicitly set : [Attachment 50675] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 36095: REGRESSION(r53287): drop event is not fired if
dataTransfer.dropEffect is not explicitly set
https://bugs.webkit.org/show_bug.cgi?id=36095

Attachment 50675: Patch
https://bugs.webkit.org/attachment.cgi?id=50675&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    , 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.