Bug 28460 – Need to update JS bindings and IDL files to support multiple message ports in postMessage()

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

[Andrew Wilson]

We're updating the internal WebCore APIs to support passing multiple MessagePorts to postMessage() as part of bug 26902.

Once that's in, we need to update the JS bindings and IDL files to reflect the new APIs.

[Andrew Wilson]

Created attachment 38775 [details]
proposed patch

This is the patch for the IDL files and JS bindings - I'm making another patch for the V8 bindings.

[David Levin]

Comment on attachment 38775 [details]
proposed patch

r+ from me with some minor changes except for WebCore/bindings/js/*.

It would be great if you got someone to look over those changes. I tried but my naive eyes probably missed things.  If this totally clogs up, I'll be willing to give this one last on those as well as I can to finish this off.

> diff --git a/LayoutTests/fast/events/message-port-multi.html b/LayoutTests/fast/events/message-port-multi.html
> \ No newline at end of file

Please fix.

>
> diff --git a/LayoutTests/fast/workers/resources/worker-context-multi-port.js b/LayoutTests/fast/workers/resources/worker-context-multi-port.js

> +    } else if (event.data.indexOf("PASS") >= 0)

Why is this >= 0 instead of == 0?

> +        testPassed(event.data.substring(4));
> +    else
> +        testFailed(event.data.substring(4));

Why is this substring(4) when the event.data wasn't "PASS"?  
I think you are expecting FAIL.  It may be good to check that it is fail and if not print the whole message.


> diff --git a/LayoutTests/fast/workers/resources/worker-multi-port.js b/LayoutTests/fast/workers/resources/worker-multi-port.js

> +    else if (event.data.indexOf("PASS") >= 0)
> +        testPassed(event.data.substring(4));

Why is this >= 0 instead of == 0?

> +    else
> +        testFailed(event.data.substring(4));

Why is this substring(4) when the event.data wasn't "PASS"?  
I think you are expecting FAIL.  It may be good to check that it is fail and if not print the whole message.

> diff --git a/LayoutTests/fast/workers/worker-context-multi-port.html b/LayoutTests/fast/workers/worker-context-multi-port.html
> \ No newline at end of file

Please fix.


> diff --git a/LayoutTests/fast/workers/worker-multi-port.html b/LayoutTests/fast/workers/worker-multi-port.html
> \ No newline at end of file

Please fix.


> diff --git a/WebCore/bindings/js/JSMessageEventCustom.cpp b/WebCore/bindings/js/JSMessageEventCustom.cpp
> +
> +    MessageEvent* impl = static_cast<MessageEvent*>(this->impl());

Prefer whole words: "impl"  Perhaps "messageEvent" or just "event".


> diff --git a/WebCore/bindings/js/JSMessagePortCustom.cpp b/WebCore/bindings/js/JSMessagePortCustom.cpp
> +JSC::JSValue JSMessagePort::postMessage(JSC::ExecState* exec, const JSC::ArgList& args)
> +{
> +    const UString& message = args.at(0).toString(exec);
> +    MessagePortArray portArray;
> +    fillMessagePortArray(exec, args.at(1), portArray);
> +    if (exec->hadException())
> +        return jsUndefined();
> +
> +    ExceptionCode ec = 0;
> +    impl()->postMessage(message, &portArray, ec);
> +    setDOMException(exec, ec);
> +    return jsUndefined();
> +}

I think I saw two copies of this code.  Is it possible to consolidate them?

[Andrew Wilson]

Created attachment 38896 [details]
Patch addressing Levin's comments

Sam, can you take a look at the JS bindings changes? dave_levin already looked at the rest of the patch.

[Andrew Wilson]

Committed as r48025.