[webkit-reviews] review granted: [Bug 79173] dragover's default action should prevent drop when appropriate : [Attachment 169788] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 79173: dragover's default action should prevent drop when appropriate
https://bugs.webkit.org/show_bug.cgi?id=79173

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=169788&action=review


> Source/WebCore/ChangeLog:11
> +	   action of the dragover event. Otherwise, if the cursor is over a
text field, editing host,
> +	   or an editable element, we also consider the drag to be accepted.

Can you include information about browser compat here? E.g., This makes us
match the spec, IE and Opera, but differs from Gecko.  You might also want to
include the spec text that describes this behavior (maybe also link to the msdn
article about behavior in IE?).

> Source/WebCore/page/DragController.h:124
> +	   bool m_documentIsHandlingDrag;

Should we initialize this in the constructor?  I guess it'll always be set by
dragEnteredOrUpdated before we read it in performDrag.

> LayoutTests/fast/events/only-valid-drop-targets-receive-drop-expected.txt:8
> +Dropping here should NOT result in a drop event.
> +Dropping here should result in a drop event.
> +Starting drag...
> +Drop not received
> +Starting drag...
> +Drop received

Nit: It would be nice if the text output was clear about which div is which. 
E.g., 
Dropping on drop1 should NOT...
Dropping on drop2 should result...
Starting drag to drop1...
Drop not received on drop1.
Starting drag to drop2...
Drop received on drop2.

> LayoutTests/fast/events/only-valid-drop-targets-receive-drop.html:64
> +

Nit: Extra blank line.