Bug 116304 – Remove custom code for MessageEvent.ports getter

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

The "ports" attribute in MessageEvent.idl is currently using a CustomGetter. The attribute is of type "MessagePortArray", which the JSC bindings generator has support for. Therefore, we should be able to get rid of the custom code with minor generator tweaks.

[Chris Dumez]

Created attachment 202062 [details]
Patch

[Kentaro Hara]

Comment on attachment 202062 [details]
Patch

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

> Source/WebCore/dom/MessageEvent.idl:37
> +    [InitializedByEventConstructor] readonly attribute MessagePortArray ports;

Per the spec, this should be 'MessagePort[]?'. Would you fix it in a follow-up patch?

http://dev.w3.org/html5/postmsg/#event-definitions

[WebKit Commit Bot]

Comment on attachment 202062 [details]
Patch

Clearing flags on attachment: 202062

Committed r150249: <http://trac.webkit.org/changeset/150249>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 202062 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202062&action=review
> 
> > Source/WebCore/dom/MessageEvent.idl:37
> > +    [InitializedByEventConstructor] readonly attribute MessagePortArray ports;
> 
> Per the spec, this should be 'MessagePort[]?'. Would you fix it in a follow-up patch?
> 
> http://dev.w3.org/html5/postmsg/#event-definitions

This is visibly a recent spec change as it wasn't nullable before. I don't know how other browsers behave so this would need to be checked. Do you know the reason behind this change? Having a nullable array makes life harder for JS developers and I'm not sure why having an empty array is not enough.

[Kentaro Hara]

(In reply to comment #5)
> This is visibly a recent spec change as it wasn't nullable before. I don't know how other browsers behave so this would need to be checked. Do you know the reason behind this change? Having a nullable array makes life harder for JS developers and I'm not sure why having an empty array is not enough.

Yeah, needs verification of other browsers. I don't know about nullable, but replacing XXXArray with XXX[] looks like a recent trend.

[Alexey Proskuryakov]

Comment on attachment 202062 [details]
Patch

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

> Source/WebCore/bindings/scripts/test/JS/JSTestSerializedScriptValueInterface.cpp:182
> +    JSValue result = impl->ports() ? jsArray(exec, castedThis->globalObject(), *impl->ports()) : constructEmptyArray(exec, 0, castedThis->globalObject());

It seems unfortunate that we call into DOM twice now. Why can't the result of impl->ports() be cached?

[Chris Dumez]

(In reply to comment #7)
> (From update of attachment 202062 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202062&action=review
> 
> > Source/WebCore/bindings/scripts/test/JS/JSTestSerializedScriptValueInterface.cpp:182
> > +    JSValue result = impl->ports() ? jsArray(exec, castedThis->globalObject(), *impl->ports()) : constructEmptyArray(exec, 0, castedThis->globalObject());
> 
> It seems unfortunate that we call into DOM twice now. Why can't the result of impl->ports() be cached?

I don't think it is a big issue currently because this code is only used for MessageEvent::ports() currently, which is inlined. However, you have a point that we should cache it to be safe. I will look into it, thanks.

[Chris Dumez]

Addressing ap and Kentaro's feedback in Bug 116653.