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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri May 22 07:18:03 PDT 2009

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

Attachment 29947: proposed patch

------- 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...

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);
 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>
   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,
 84	    void postMessage(const String& message, ExceptionCode& ec);

 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.

More information about the webkit-reviews mailing list