[Webkit-unassigned] [Bug 14994] No support for MessageEvent interface

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=14994


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #18200|review?                     |review-
               Flag|                            |




------- Comment #6 from darin at apple.com  2007-12-30 23:41 PDT -------
(From update of attachment 18200)
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.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.