Bug 66841 - Let MessageEvent.data hold SerializedScriptValue or String selectively
Summary: Let MessageEvent.data hold SerializedScriptValue or String selectively
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yuta Kitamura
URL:
Keywords:
Depends on:
Blocks: 65249
  Show dependency treegraph
 
Reported: 2011-08-24 01:09 PDT by Yuta Kitamura
Modified: 2011-08-25 02:25 PDT (History)
5 users (show)

See Also:


Attachments
Patch (13.48 KB, patch)
2011-08-24 02:16 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch for landing (14.18 KB, patch)
2011-08-25 01:25 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuta Kitamura 2011-08-24 01:09:09 PDT
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).
Comment 1 Yuta Kitamura 2011-08-24 02:16:17 PDT
Created attachment 104973 [details]
Patch
Comment 2 Adam Barth 2011-08-24 11:59:51 PDT
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 3 Yuta Kitamura 2011-08-25 01:23:07 PDT
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.
Comment 4 Yuta Kitamura 2011-08-25 01:25:42 PDT
Created attachment 105143 [details]
Patch for landing
Comment 5 WebKit Review Bot 2011-08-25 02:25:30 PDT
Comment on attachment 105143 [details]
Patch for landing

Clearing flags on attachment: 105143

Committed r93766: <http://trac.webkit.org/changeset/93766>
Comment 6 WebKit Review Bot 2011-08-25 02:25:35 PDT
All reviewed patches have been landed.  Closing bug.