Bug 28839 – Need to update v8 bindings to support passing multiple ports to postMessage()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Andrew Wilson]

The HTML5 spec not supports passing multiple ports to postMessage() - we need to update the v8 bindings accordingly.

[Andrew Wilson]

Created attachment 38801 [details]
proposed patch

v8 bindings portion of bug 28460

[Eric Seidel (no email)]

Comment on attachment 38801 [details]
proposed patch

Would be even better to list what tests now pass after this change:
 10         No new tests (tests for JSC bindings are sufficient)

I looked briefly, but it seems you'll need a v8 expert here.  Geez v8 bindings are so wordy/ugly!

[Dimitri Glazkov (Google)]

Comment on attachment 38801 [details]
proposed patch

These are all style nits:

> +        if (!fillWebCoreMessagePortArray(args[1], portArray))
> +            return v8::Undefined();

if (!getMessagePortArray(.. is the common convention in WebKit. No need to use WebCore in prefix. It's in String conv methods only because there are two ambiguous string types.

> +        if (!fillWebCoreMessagePortArray(args[1], portArray))

ditto.

> +    if (ec)
> +        V8Proxy::setDOMException(ec);

return throwError(ec);

> +
> +CALLBACK_FUNC_DECL(MessageEventInitMessageEvent)
> +{
> +    INC_STATS("DOM.MessageEvent.initMessageEvent");
> +    MessageEvent* event = V8DOMWrapper::convertToNativeObject<MessageEvent>(V8ClassIndex::MESSAGEEVENT, args.Holder());
> +    String typeArg = v8ValueToWebCoreString(args[0]);
> +    bool canBubbleArg = args[1]->BooleanValue();
> +    bool cancelableArg = args[2]->BooleanValue();
> +    String dataArg = v8ValueToWebCoreString(args[3]);
> +    String originArg = v8ValueToWebCoreString(args[4]);
> +    String lastEventIdArg = v8ValueToWebCoreString(args[5]);
> +    DOMWindow* sourceArg = V8DOMWindow::HasInstance(args[6]) ? V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, v8::Handle<v8::Object>::Cast(args[6])) : 0;

I am surprised there's no arg checking here. Is this correct?

> +    if (ec)
> +        V8Proxy::setDOMException(ec);

return throwError(ec);

> +bool fillWebCoreMessagePortArray(v8::Local<v8::Value> value, MessagePortArray& portArray)

bool getMessagePortArray(v8::Local<v8::Value> value, MessagePortArray& portArray)

> +    if (!value->IsObject()) {
> +        V8Proxy::throwError(V8Proxy::TypeError, "MessagePortArray argument must be an object");
> +        return false;

throwError("MessagePortArray argument must be an object");
return false;

> +        v8::Local<v8::Value> sequenceLength = ports->Get(v8::String::New("length"));
> +        if (!sequenceLength->IsNumber()) {
> +            V8Proxy::throwError(V8Proxy::TypeError, "MessagePortArray argument has no length attribute");
> +            return false;

throwError("MessagePortArray argument has no length attribute");
return false;

> +            V8Proxy::setDOMException(INVALID_STATE_ERR);
> +            return false;


throwError(ec);
return false;

> +            V8Proxy::throwError(V8Proxy::TypeError, "MessagePortArray argument must contain only MessagePorts");
> +            return false;

ditto.

> +    if (ec)
> +        V8Proxy::setDOMException(ec);

return throwError(ec);

[Andrew Wilson]

(In reply to comment #3)

> 
> > +
> > +CALLBACK_FUNC_DECL(MessageEventInitMessageEvent)
> > +{
> > +    INC_STATS("DOM.MessageEvent.initMessageEvent");
> > +    MessageEvent* event = V8DOMWrapper::convertToNativeObject<MessageEvent>(V8ClassIndex::MESSAGEEVENT, args.Holder());
> > +    String typeArg = v8ValueToWebCoreString(args[0]);
> > +    bool canBubbleArg = args[1]->BooleanValue();
> > +    bool cancelableArg = args[2]->BooleanValue();
> > +    String dataArg = v8ValueToWebCoreString(args[3]);
> > +    String originArg = v8ValueToWebCoreString(args[4]);
> > +    String lastEventIdArg = v8ValueToWebCoreString(args[5]);
> > +    DOMWindow* sourceArg = V8DOMWindow::HasInstance(args[6]) ? V8DOMWrapper::convertToNativeObject<DOMWindow>(V8ClassIndex::DOMWINDOW, v8::Handle<v8::Object>::Cast(args[6])) : 0;
> 
> I am surprised there's no arg checking here. Is this correct?

This is basically the verbatim code from the generated bindings (before I made this a custom binding).

I guess it's because pretty much anything (including an undefined argument) can be cast to a string/boolean, so the only arg checking that needs to be done is for the DOMWindow argument.

I'll make those other changes as suggested - thanks for the quick turnaround!

[Andrew Wilson]

Created attachment 38901 [details]
patch addressing review comments

[Dimitri Glazkov (Google)]

Comment on attachment 38901 [details]
patch addressing review comments

r=me.

[Andrew Wilson]

Committed as r48026