Bug 121390 – Set MessageEvent.source to the newly created port for shared workers' connect events

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Consider merging the following patch from Blink:
"""
Set MessageEvent.source to the newly created port for shared workers' connect events
instead of previously null, as per the latest specification:
http://www.whatwg.org/specs/web-apps/current-work/multipage/comms.html#dom-messageevent-source
http://www.whatwg.org/specs/web-apps/current-work/multipage/workers.html#dom-sharedworker
"""

Blink revisions:
https://src.chromium.org/viewvc/blink?revision=156385&view=revision
https://src.chromium.org/viewvc/blink?revision=156420&view=revision

[Chris Dumez]

Created attachment 211708 [details]
Patch

[Darin Adler]

Comment on attachment 211708 [details]
Patch

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

> Source/WebCore/dom/EventTarget.h:114
> +        virtual MessagePort* toMessagePort();

Given that the only client uses this as "isMessagePort", maybe we should add that instead?

> Source/WebCore/dom/MessageEvent.h:59
> +    static PassRefPtr<MessageEvent> create(PassOwnPtr<MessagePortArray> ports, const ScriptValue& data = ScriptValue(), const String& origin = "", const String& lastEventId = "", PassRefPtr<EventTarget> source = 0)

I think the default values should be emptyString() instead of "", although not 100% sure that compiles. Null strings would be even more efficient, but that might require a look at the body of the constructor to see if that will change behavior.

> Source/WebCore/dom/MessageEvent.h:63
> +    static PassRefPtr<MessageEvent> create(PassOwnPtr<MessagePortArray> ports, PassRefPtr<SerializedScriptValue> data, const String& origin = "", const String& lastEventId = "", PassRefPtr<EventTarget> source = 0)

Ditto.

> Source/WebCore/dom/MessagePort.h:78
> +        MessagePort* toMessagePort() OVERRIDE { return this; }

Should mark either this class or this function as FINAL. SHould make this function private as there is no reason to call it on a MessagePort object.

> Source/WebCore/workers/SharedWorkerGlobalScope.cpp:49
> +    RefPtr<MessagePort> sourcePort = port;

Naming here is unclear. Both sourcePort and port sound fine to use. That’s why we normally use the crazy “prp” naming scheme, so it won’t be ambiguous which one to use.

> Source/WebCore/workers/SharedWorkerGlobalScope.cpp:50
> +    RefPtr<MessageEvent> event = MessageEvent::create(adoptPtr(new MessagePortArray(1, sourcePort)), ScriptValue(), "", "", sourcePort);

Should use emptyString() instead of "". No question at a call site like this one.

[Chris Dumez]

Created attachment 211870 [details]
Patch

[Chris Dumez]

I took all the feedback into consideration.

I am requesting review again due to some non-trivial changes (using null Strings instead of empty ones in MessageEvent).

[WebKit Commit Bot]

Comment on attachment 211870 [details]
Patch

Clearing flags on attachment: 211870

Committed r155959: <http://trac.webkit.org/changeset/155959>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.