[webkit-reviews] review granted: [Bug 18725] Make postMessage asynchronous : [Attachment 20978] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Collin Jackson
<collinj-webkit at collinjackson.com>'s request for review:
Bug 18725: Make postMessage asynchronous
http://bugs.webkit.org/show_bug.cgi?id=18725

Attachment 20978: Patch
http://bugs.webkit.org/attachment.cgi?id=20978&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>
I assume this is due to a change in the spec.  Could you note this in the
changelog.
-    : Event(messageEvent, true, true)
+    : Event(messageEvent, false, true)
I don't think these need to take PassRefPtrs.  You are not transferring
ownership, you just want the timer to ref the objects.
+    PostMessageTimer(PassRefPtr<DOMWindow> window, PassRefPtr<MessageEvent>
event, PassRefPtr<SecurityOrigin> targetOrigin)


The leading { should be on its own line.  I am not sure about the 'delete
this'.	In the DOMWindowTimer case, we have the the postMessageTimerFired
equivalent in JSDOMWindowBase delete the timer.  I think either is fine though.

+    virtual void fired() {
+	 m_window->postMessageTimerFired(this);
+	 delete this;
+    }

I think this would look better on one line.
+    PostMessageTimer* timer = new PostMessageTimer(
+	     this, new MessageEvent(message, sourceOrigin, source), target);

Is the actual security check done in the postMessageTimerFired because of a
possibility that the document has changed since the timer was fired?

This looks great.

r=me