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).
Created attachment 104973 [details] Patch
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.
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.
Created attachment 105143 [details] Patch for landing
Comment on attachment 105143 [details] Patch for landing Clearing flags on attachment: 105143 Committed r93766: <http://trac.webkit.org/changeset/93766>
All reviewed patches have been landed. Closing bug.