[webkit-reviews] review granted: [Bug 121390] Set MessageEvent.source to the newly created port for shared workers' connect events : [Attachment 211708] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Christophe Dumez <dchris at gmail.com>'s
request for review:
Bug 121390: Set MessageEvent.source to the newly created port for shared
workers' connect events
https://bugs.webkit.org/show_bug.cgi?id=121390

Attachment 211708: Patch
https://bugs.webkit.org/attachment.cgi?id=211708&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.