[webkit-reviews] review denied: [Bug 25435] Dedicated workers do not support sending MessagePorts in messages : [Attachment 29947] proposed patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Andrew Wilson <atwilson at google.com>'s
request for review:
Bug 25435: Dedicated workers do not support sending MessagePorts in messages
https://bugs.webkit.org/show_bug.cgi?id=25435

Attachment 29947: proposed patch
https://bugs.webkit.org/attachment.cgi?id=29947&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Is this safe/correct?  args.size() < 1?
 88	String message = args.at(exec, 0).toString(exec);

Sad that JSWorker and JSWorkerContext can't share more code...

style:
7     if (proxy) {
 58	    context = proxy->workerContext();
 59	} else {


We eventually need to write a throwOrReturn(ec, value) helper function to git
rid of this idiom:
67     if (ec)
 68	    return throwError(ec);
 69 
 70	return v8::Undefined();

return throwOrReturn(ec, v8::Undefined())

Or better yet, a special case for there of:

return throwOrReturnUndefined(ec);

static v8::Handle<Value> throwOrReturn(ExceptionCode ec, v8::Handle<Value>
value)
{
   if (ec)
       return throwError(ec);
   return value;
}

that would clean up many of these bindings think.  Even just using it in these
two new places might be worth the win :)

We only name arguments when the names provide clarity beyond the typename:
 83	    void postMessage(const String& message, MessagePort* port,
ExceptionCode&);
 84	    void postMessage(const String& message, ExceptionCode& ec);

again:
 56	    virtual void postMessageToWorkerContext(const String&,
MessagePort*, ExceptionCode& ec) = 0;

Overall this looks sane to me, but I'm not the right reviewer here.  Alexey is
your man I think.

r- for the style issues.  Re-post and Alexey can review for content.