[webkit-reviews] review denied: [Bug 32214] Add WebSocket feature in Worker : [Attachment 45264] Add WebSocket feature in Worker

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 4 17:20:39 PST 2010


David Levin <levin at chromium.org> has denied Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 32214: Add WebSocket feature in Worker
https://bugs.webkit.org/show_bug.cgi?id=32214

Attachment 45264: Add WebSocket feature in Worker
https://bugs.webkit.org/attachment.cgi?id=45264&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Sorry for the delay (I was reviewing it but it is complicated and then there
was a vacation).

My biggest concern is regarding lifetime and cross thread ownership issues.
Specifically, MainThreadClient is owned by two threads, so it is
ThreadsafeShared which means the final delete may happen on any thread.
However, it contains a String which is RefCounted (and not compatible with
being deleted on any thread). It would be best if it would be owned by one
thread (like it wouldn't delete itself until a "final" message came through. If
that doesn't seem possible, then it shouldn't contain any RefCounted objects).


There are some naming things that we probably should address before this gets
checked in. (There may be other things but the ownership issue is the biggest.)


> diff --git a/WebCore/websockets/WorkerThreadableWebSocketChannel.h
b/WebCore/websockets/WorkerThreadableWebSocketChannel.h
> +class WorkerThreadableWebSocketChannel : public
RefCounted<WorkerThreadableWebSocketChannel>, public ThreadableWebSocketChannel
{
...
> +    class MainThreadClient : public ThreadSafeShared<MainThreadClient>,
public WebSocketChannelClient {

I know MainThreadClient and WorkThreadProxy were my suggestions, but I don't
like them that much now that I see them. Any other ideas?

> +    /* Proxy for MainThreadClient.  Running on the worker thread. */

In general WebKit uses // style comments.

> +    class WorkerThreadProxy {
> +	   void callSyncMethod();

Consider setMethodNotCompleted (right now it sounds like this does the sync
call).

> +	   void waitSyncMethod();

Consider waitForMethodCompletion.


More information about the webkit-reviews mailing list