[webkit-reviews] review granted: [Bug 26700] HTML5 Drag and drop, effectAllowed and dragEffect broken : [Attachment 39230] Patch with updated test case

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted 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 39230: Patch with updated test case
https://bugs.webkit.org/attachment.cgi?id=39230&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Sigh.  The game of submitting patches when not a contributer and unknown to the
project is one of making them easy to r+.   This patch is large and changes a
bunch of poorly tested code, which makes reviewing harder... but I think I've
stared at enough times to be ready to pull the trigger.  :(


However staring at your test again I'm finding it somewhat hard to read.  And
this nit (which would be a non-issue if you were a committer), concerns me:
179 Property changes on: LayoutTests/fast/events/drag-dropeffect.html
2180 ___________________________________________________________________
3181 Name: svn:eol-style
4182   + LF

Your spacing choices in the test don't match my recollection for "webkit
style".  Generally I've seen folks put spaces around operators :" as : " for
instance.   and "+test as "  + test.

Functions technically are supposed to have { on a new line according to wk
style too.  Sadly we don't have a jslint yet (nor is our js styling nearly as
strict as our c++ styling or Google's JS styling).

Please file a bug about the DRT issues mentioned above.  Ideally mentioning it
in your LayoutTest ChangeLog.

Staring at it, I think I would have probably abstracted the various event
handlers, since they could share a bunch of assert code and call counts and
preventDefault.  Given the above, I think your and my abstraction thresholds
are a bit different. :)

Anyway, I'm gonna r+ this and pray it doesn't break anything.  I think this
will have to be landed by hand unless you post another patch w/o the line
ending setting.