Bug 66841 – Let MessageEvent.data hold SerializedScriptValue or String selectively

• Component: UI Events
• Status: RESOLVED FIXED

[Yuta Kitamura]

WebSocket generates a MessageEvent when a WebSocket message is received. MessageEvent's data attribute contains String, ArrayBuffer or Blob (ArrayBuffer and Blob messages are not implemented yet).

In current WebCore implementation, MessageEvent.data is defined to be SerializedScriptValue. WebSocket class can directly construct the object representing an incoming message (as a String, an ArrayBuffer or a Blob), and it does not have to be serialized at all.

This bug is dedicated to MessageEvent refactoring making MessageEvent.data hold a variant value (SerializedScriptValue or String).

[Yuta Kitamura]

Created attachment 104973 [details]
Patch

[Adam Barth]

Comment on attachment 104973 [details]
Patch

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.

[Yuta Kitamura]

Comment on attachment 104973 [details]
Patch

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

Thank you for your review!

>> Source/WebCore/bindings/v8/custom/V8MessageEventCustom.cpp:74
>> +    return result;
> 
> I trust the underlying C++ data is immutable.  :)

If we didn't cache the result, .data accessor would generate a different JS object (which is semantically same) every time .data value is obtained. This is not desirable and actually makes some tests (fast/dom/Window/window-postmessage-clone*.html) fail.

ForceSet() is used in SerializedScriptValue::deserializeAndSetProperty(), which stub code for SerializedScriptValue generated by CodeGeneratorV8 uses (http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm?rev=92890#L2592).

>> Source/WebCore/dom/MessageEvent.cpp:58
>> +    , m_lastEventId("")
> 
> Do you mean for these to be empty strings as opposed to null strings?

Yes, this is intentional, as you mentioned below.

>> Source/WebCore/dom/MessageEvent.h:76
>> +        };
> 
> 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.

Default cases will be removed.

[Yuta Kitamura]

Created attachment 105143 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 105143 [details]
Patch for landing

Clearing flags on attachment: 105143

Committed r93766: <http://trac.webkit.org/changeset/93766>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.