[webkit-reviews] review granted: [Bug 66841] Let MessageEvent.data hold SerializedScriptValue or String selectively : [Attachment 104973] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Yuta Kitamura <yutak at chromium.org>'s
request for review:
Bug 66841: Let MessageEvent.data hold SerializedScriptValue or String
selectively
https://bugs.webkit.org/show_bug.cgi?id=66841

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104973&action=review


> Source/WebCore/bindings/v8/custom/V8MessageEventCustom.cpp:74
> +    // Overwrite the data attribute so it returns the cached result in
future invocations.
> +    v8::PropertyAttribute dataAttr =
static_cast<v8::PropertyAttribute>(v8::DontDelete | v8::ReadOnly);
> +    info.Holder()->ForceSet(name, result, dataAttr);
> +    return result;

I trust the underlying C++ data is immutable.  :)

> Source/WebCore/dom/MessageEvent.cpp:58
> +    , m_origin("")
> +    , m_lastEventId("")

Do you mean for these to be empty strings as opposed to null strings?

> Source/WebCore/dom/MessageEvent.h:76
> +	   enum DataType {
> +	       DataTypeSerializedScriptValue,
> +	       DataTypeString
> +	   };

The switch statements that use this enum shouldn't have a default case.  We
rely upon the compiler to complain when we change the enum and forget to update
the switch statements.

> Source/WebCore/websockets/WebSocket.cpp:-397
> -    evt->initMessageEvent(eventNames().messageEvent, false, false,
SerializedScriptValue::create(msg), "", "", 0, 0);

I see that the empty strings were used here, so you're probably doing the right
thing above.