Bug 66789 – MessageEvent.ports shouldn't ever be null.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[David Levin]

Currently we set it to null in various circumstances and this doesn't follow the spec.

Also, see http://www.w3.org/Bugs/Public/show_bug.cgi?id=13675

[David Levin]

Created attachment 104877 [details]
Patch

[Darin Adler]

Comment on attachment 104877 [details]
Patch

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

> LayoutTests/fast/workers/resources/worker-thread-multi-port.js:4
> +            testPassed("event.ports is non-null and zero- ength when no port sent");

I think this is an editing mistake where you turned the "l" into a space instead of turning the"-" into a space.

[David Levin]

Created attachment 104921 [details]
Patch

[Darin Adler]

Comment on attachment 104877 [details]
Patch

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

> Source/WebCore/bindings/js/JSMessageEventCustom.cpp:49
>      MessagePortArray* ports = static_cast<MessageEvent*>(impl())->ports();
>      if (!ports || ports->isEmpty())
> -        return jsNull();
> +        return constructEmptyArray(exec, globalObject());

It seems there is no need to check ports->isEmpty() any more here, since empty is no longer a special case. Unless we want a performance optimization for the empty case.

It also seems to me we could change things so a MessageEvent always has a MessagePortArray in it, and then we might not need a custom binding at all!

[Darin Adler]

Comment on attachment 104921 [details]
Patch

I made a comment on the old patch that still applies. Seems to me that the “ports is an empty array instead of null” rule could be in the DOM instead of in the binding.

[David Levin]

(In reply to comment #4)
> (From update of attachment 104877 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104877&action=review
> 
> > Source/WebCore/bindings/js/JSMessageEventCustom.cpp:49
> >      MessagePortArray* ports = static_cast<MessageEvent*>(impl())->ports();
> >      if (!ports || ports->isEmpty())
> > -        return jsNull();
> > +        return constructEmptyArray(exec, globalObject());
> 
> It seems there is no need to check ports->isEmpty() any more here, since empty is no longer a special case. Unless we want a performance optimization for the empty case.

Good point. I'll clean that up before check-in.

> 
> It also seems to me we could change things so a MessageEvent always has a MessagePortArray in it, and then we might not need a custom binding at all!

I tried this and failed -- probably something simple. (Along with a few other changes) I tried to express the array as follows
-        readonly attribute [CustomGetter] Array ports;
+        readonly attribute MessagePort[] ports;
but that didn't work and I couldn't quickly figure out how to do the array, so I filed 66829 to do this later.

[David Levin]

(In reply to comment #6)
> I tried this and failed -- probably something simple. (Along with a few other changes) I tried to express the array as follows
> -        readonly attribute [CustomGetter] Array ports;
> +        readonly attribute MessagePort[] ports;
> but that didn't work and I couldn't quickly figure out how to do the array, so I filed 66829 to do this later.

I also tried 
        readonly attribute  Array ports;
and had problems.

[David Levin]

Committed as http://trac.webkit.org/changeset/93709