Bug 14994 – No support for MessageEvent interface

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Henry Mason]

As currently specified in HTML 5's section 6.1, WebKit should support the MessageEvent interface for cross-document messaging and server-sent DOM events.

[Henry Mason]

Created attachment 17813 [details]
MessageEvent implementation

Implements section 6.1 of the current draft of HTML5.

[Darin Adler]

Comment on attachment 17813 [details]
MessageEvent implementation

I'm unclear on the value of having the MessageEvent class without any of the specific uses of message events. We might not want to add the class until we have at least one capability that uses these events.

For example, we didn't add TextEvent until we had to code to emit text events.

The code looks good.

Please make a new patch that includes the new files. You'll need to svn add them and use the svn-create-patch function to make the patch.

We'd also want a test for this. Something simple that creates one of these.

[Darin Adler]

By the way, thanks very much for the contribution and your interest in WebKit!

[Darin Adler]

(In reply to comment #2)
> (From update of attachment 17813 [details] [edit])
> I'm unclear on the value of having the MessageEvent class without any of the
> specific uses of message events. We might not want to add the class until we
> have at least one capability that uses these events.

I guess if we implement server-sent events soon after this, then we'll have a use for the class. I had overlooked the bug this bug blocks. But we do still have to resolve the other issues I mentioned in the patch, and we may indeed want to wait to land this until we have a use for it.

[Henry Mason]

Created attachment 18200 [details]
MessageEvent and cross-domain messaging implementation

Here's a slightly updated version of the patch. I've now added support for cross-domain messaging (section 6.4 of HTML5) and a test. Now MessageEvent has a real use.

[Darin Adler]

Comment on attachment 18200 [details]
MessageEvent and cross-domain messaging implementation

Looks great. Thanks for tackling this!

I have quite a few comments.

+    if (args.size() != 1)
+        return throwError(exec, KJS::SyntaxError);

Normally JavaScript bindings do not check the number of parameters. So you should remove this. Instead, extra parameters are treated as undefined, and there's no need to check. Since the HTML 5 specification doesn't say anything specific about what to do here, I think we should do the usual, which will result in sending a message with a data string of "undefined". If we did want to check this I think we would want a custom error message string.

+    if (!args[0]->isString())
+        return throwError(exec, KJS::SyntaxError);

JavaScript functions that take strings don't normally reject non-string arguments. Instead they just call toString. So you should remove this. Again, if we did want to check I'd want a custom error string to explain what's wrong.

+    window->postMessage(args[0]->toString(exec), domain, uri, source);

The toString operation can raise an exception. You need to check if it did by calling exec->hadException() after it returns, and if an exception is present return without calling DOMWindow::postMessage. It's critical that the domain and uri are computed *before* calling toString, so please don't change that.

+        MessageEvent(const AtomicString& type, bool canBubble, bool cancelable, const AtomicString& data, const AtomicString& domain, const AtomicString& uri, DOMWindow* source);

Also, I suggest leaving the type, canBubble, and cancelable arguments out of this constructor's argument list. The constructor can just hard-code these to messageEvent, true, and true. There's no need for the constructor to match the init function parameters; it's only for use in postMessage and in fact I think it's clearer if it does not. Another alternative would be to include only the empty constructor and use initMessageEvent in the postMessage function.

+        MessageEvent(const AtomicString& namespaceURI, const AtomicString& type, bool canBubble, bool cancelable, const AtomicString& data, const AtomicString& domain, const AtomicString& uri, DOMWindow* source);

I suggest omitting this constructor namespaceURI, since there's no client that needs it. It's just dead untested code.

Since I don't know much about server-side events, I don't know how much of what I am saying here is wrong for the server-side event case.

+        void setData(const AtomicString& data) { m_data = data; }

Why is this included? MessageEvent.idl does not allow writing the data attribute, and none of the code seems to use this. Is this something used in the server-side event code?

+   ExceptionCode ec = 0;
+   document()->dispatchEvent(new MessageEvent("message", true, true, name, domain, uri, source), ec, true);

No need to initialize the exception code to 0 since you're not inspecting it. I know this isn't consistent in the existing code but it's good to not do the tiny bit of unnecessary work.

The event name "message" should be an AtomicString to avoid the overhead of computing its hash every time. The right way to do this is to add message to the list of event names in EventNames.h and then use messageEvent instead of "message".

+        void postMessage(const String& name, const String& domain, const String& uri, DOMWindow* source) const;

Why "name" for the first argument? This seems to be called "message" in DOMWindow.idl and "data" in MessageEvent.idl. I'd prefer to use the single term "data" for this everywhere unless there's a good reason not to. It's particularly confusing to use the word "name" here since the event's name is "message".

-			compatibilityVersion = "Xcode 2.4";

This WebKit change should not be checked in. It's just something Xcode adds and removes and there's no reason to include it in your patch.

The regression test should call layoutTestController.dumpAsText() so that its results don't include platform dependent layout details.

Calling layoutTestController.notifyDone() doesn't do any good unless you first call layoutTestController.waitUntilDone(). I'm not sure why the test works without the waitUntilDone() call; please add it.

Do you need to use queueScript instead of just calling the postIt() function directly? I think the right way to do this test is to put an onload handler on the <iframe> and run the function from that. We should avoid using layout test controller functions unless they are absolutely necessary.

The cross-domain-message-receive.html file should be put into a subdirectory named "resources"; that way it won't be treated as a test and run separately.

Some details of the test seem strange. Why does cross-domain-message-receive.html have a body with multiple paragraphs, since all it does is post the message back to the source? Also, it's confusing that the data posted back to the source includes multiple line content in the same format as the format used to dump the fields when the sender gets called back. This detail makes the test result a little bit hard to interpret.

[Henry Mason]

Created attachment 18213 [details]
Updated patched

Ok, I tried to address the concerns raised above. Thanks for being so patient.

[Darin Adler]

Comment on attachment 18213 [details]
Updated patched

 41         MessageEvent(const AtomicString& data, const AtomicString& domain, const AtomicString& uri, DOMWindow* source);

 44         void initMessageEvent(const AtomicString& type, bool canBubble, bool cancelable, const AtomicString& data, const AtomicString& domain, const AtomicString& uri, DOMWindow* source);

 46         void initMessageEventNS(const AtomicString& namespaceURI, const AtomicString& type, bool canBubble, bool cancelable, const AtomicString& data, const AtomicString& domain, const AtomicString& uri, DOMWindow* source);

 48         const AtomicString& data() const { return m_data; }

 50         const AtomicString& domain() const { return m_domain; }

 52         const AtomicString& uri() const { return m_uri; }

 60         AtomicString m_data;
 61         AtomicString m_domain;
 62         AtomicString m_uri;

Except for the namespace and type, these should be plain old String, not AtomicString. AtomicString are kept in a global hash table. They're used for strings where you want high speed comparison. There's no point in using them for strings that are just going to be fetched and manipulated in JavaScript code.

Everything else looks great! Thanks for addressing all my earlier comments.

Since the result goes into a <div>, the newlines don't end up making things go on new lines. Maybe you should use commas instead so the output looks more sensible.

review- only because of the AtomicString thing.

[Darin Adler]

I'll land this for you.

I had to take out initMessageEventNS, because we don't have namespaced types yet on events. That needs to be added at the Event.h level, not down here in MessageEvent.h, so for now I left it out.

[Darin Adler]

Committed revision 29051.

[Henry Mason]

Thanks very much!