[webkit-reviews] review denied: [Bug 26700] HTML5 Drag and drop, effectAllowed and dragEffect broken : [Attachment 38494] patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Jens Alfke <snej at chromium.org>'s
request for review:
Bug 26700: HTML5 Drag and drop, effectAllowed and dragEffect broken
https://bugs.webkit.org/show_bug.cgi?id=26700

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
There is a lot changing in this patch, which makes it difficult/scary to
review. :(

This is sad:
 45	    m_effectAllowed = "uninitialized";
Not your fault, just sad that that's how HTML5 is currently spec'd.

Sadder still:
 105	 if (m_effectAllowed.isNull() || m_effectAllowed == "uninitialized")

Does the spec explain why?
 1510		      // Temporarily set dropEffect to 'none' while calling
dropleave handler (as per HTML5 spec)

Your test could have been an js-test and would have saved you having to
duplicate all the shouldBe/assert logic:
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
Even if you didn't convert it to a full js test, you could import the js test
standard js files (like js-test-pre.js) to get all the fun stuff.

Setting these to 0 is not really needed:
 var dragStartCalled=0, dragCalled=0, dragEndCalled=0;
 41 var dragEnterCalled=0, dragOverCalled=0, dragLeaveCalled=0, dropCalled=0;
Also I think officially we put spaces around =

You do this a few times:
  119	  var dragTarget = document.getElementById(dstId);
 120	 x = dragTarget.offsetLeft + dragTarget.offsetWidth / 2;
 121	 y = dragTarget.offsetTop + dragTarget.offsetHeight / 2;
 122	 eventSender.mouseMoveTo(x, y);

I've written functions in other drag tests which move the mouse to the center
of an element.	Seems you want to do the same here.

Why is this needed?
 132	 layoutTestController.waitUntilDone();

Not needed if you import the js-test-pre.js stuff or make this a real js-test:
 133	 layoutTestController.dumpAsText();

Not needed:
 171	 layoutTestController.notifyDone();

r-, mostly for testing nits.  This would be easier to review if fewer
functional changes were being made at once.