Bug 36095 - REGRESSION(r53287): drop event is not fired if dataTransfer.dropEffect is not explicitly set
Summary: REGRESSION(r53287): drop event is not fired if dataTransfer.dropEffect is not...
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-14 04:29 PDT by Oliver Hunt
Modified: 2010-03-15 17:10 PDT (History)
1 user (show)

See Also:


Attachments
Patch (11.10 KB, patch)
2010-03-14 04:39 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (11.18 KB, patch)
2010-03-14 14:20 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (10.94 KB, patch)
2010-03-14 16:13 PDT, Oliver Hunt
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>