[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
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.
More information about the webkit-reviews
mailing list