[webkit-reviews] review denied: [Bug 14994] No support for MessageEvent interface : [Attachment 18213] Updated patched

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Henry Mason <hmason at mac.com>'s request
for review:
Bug 14994: No support for MessageEvent interface
http://bugs.webkit.org/show_bug.cgi?id=14994

Attachment 18213: Updated patched
http://bugs.webkit.org/attachment.cgi?id=18213&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
 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.