[webkit-reviews] review granted: [Bug 205758] Add support for DragEvent : [Attachment 386745] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 205758: Add support for DragEvent
https://bugs.webkit.org/show_bug.cgi?id=205758

Attachment 386745: Patch

https://bugs.webkit.org/attachment.cgi?id=386745&action=review




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 386745
  --> https://bugs.webkit.org/attachment.cgi?id=386745
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=386745&action=review

> Source/WebCore/dom/DragEvent.h:47
> +    static Ref<DragEvent> create(const AtomString& eventType,
DragEventInit&& init)
> +    {
> +	   return adoptRef(*new DragEvent(eventType, WTFMove(init)));
> +    }

There's no good reason for this function body to be in the header; inlining
this at the call site isn’t really valuable since it’s going to call a
constructor, which is not inlined. Instead we can let the constructor get
inlined in the create function. And so the create function can be in the .cpp
file. Nice to have a little less code in the header, I think.

> Source/WebCore/dom/DragEvent.h:52
> +    static Ref<DragEvent> createForBindings()
> +    {
> +	   return adoptRef(*new DragEvent);
> +    }

Ditto.

> Source/WebCore/dom/DragEvent.idl:33
> +    DataTransfer? dataTransfer = null;

Is the "= null" really required?

> Source/WebCore/page/EventHandler.cpp:2255
> +    // FIXME: Use DragEvent::create which takes PlatformMouseEvent.

Is this FIXME valuable? Does the suggestec change need to be made? Would it
improve things?

> LayoutTests/fast/events/constructors/mouse-event-constructor.html:30
> +shouldBe("new MouseEvent('eventType').dataTransfer", "undefined");

More than just "should be undefined". The property shouldn’t even be present!
Might be better to check that. Something ike this:

    shouldBeFalse("dataTransfer in (new MouseEvent('eventType'))")