Add WebSocket feature in Worker

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Fumitoshi Ukai]

The Web Socket API spec says WebSocket constructor is visible when the script's global object is an object implementing the WorkerUtils interface.

[Fumitoshi Ukai]

Created attachment 44393 [details]
Add WebSocket feature in Worker.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44393 [details] without any errors.

[Alexey Proskuryakov]

Comment on attachment 44393 [details]
Add WebSocket feature in Worker.

+var timeoutID = setTimeout(endTest, 3000);

A general note - I'm not sure if these timeouts are really helpful. They can make the tests fail when the machine is busy, or when running under GuardMalloc. run-webkit-tests has timeout built in already anyway.

+    debug(evt.data);

This test takes extremely little from js-tests. I'd just make it a fully custom one for simplicity, see e.g. websocket/tests/simple-stress.html. 

+#if ENABLE(WEB_SOCKETS)
+#include "JSWebSocketConstructor.h"
+#endif

The #if should be inside JSWebSocketConstructor.h, not at every place we include it. It's also a mistake that JSEventSourceConstructor.h include is guarded below.

+    virtual ~ProcessWebSocketEventTask() {}

We normally put a space between braces here.

+    ProcessWebSocketMessageEventTask(PassRefPtr<WebSocket> websocket, const String& msg)
+        : ProcessWebSocketEventTask(websocket)
+        , m_msg(msg) {}

And these braces typically go each on its own line.

     RefPtr<WebSocket> m_webSocket;
-    RefPtr<Event> m_event;
+};

An event like this cannot be passed between threads, because WebSocket refcounting is not thread safe. But I'm not sure if that's what is happening here. What is the reason for splitting ProcessWebSocketEventTask into multiple classes? This is something ChangeLog per-function comments would serve best for.

+        // FIXME: origin, lastEventId, source, messagePort.

What needs to be fixed here? I think that the current arguments are correct. Per their definition in HTML5, they only apply to server-sent events and cross-document messaging. While origin could theoretically apply to WebSocket messages, too, the spec doesn't make it so.

It looks like the design here is that all cross-platform code is executed inside worker thread/process, with SocketStreamChannel being responsible for communication with loader process/main thread as necessary. The current version of SocketStreamHandleCFNet doesn't do that, so I don't see how the included test can pass with it now. Also, this design conflicts with what you suggested in bug 32246 comment 2, or any other case where we may want to share data between all WebSocketChannels.

It would likely be best to document the design, and to discuss it on webkit-dev before updating the patch.

[Fumitoshi Ukai]

(In reply to comment #3)
> (From update of attachment 44393 [details])
> +var timeoutID = setTimeout(endTest, 3000);
> 
> A general note - I'm not sure if these timeouts are really helpful. They can
> make the tests fail when the machine is busy, or when running under
> GuardMalloc. run-webkit-tests has timeout built in already anyway.
> 
> +    debug(evt.data);
> 
> This test takes extremely little from js-tests. I'd just make it a fully custom
> one for simplicity, see e.g. websocket/tests/simple-stress.html. 
> 
> +#if ENABLE(WEB_SOCKETS)
> +#include "JSWebSocketConstructor.h"
> +#endif
> 
> The #if should be inside JSWebSocketConstructor.h, not at every place we
> include it. It's also a mistake that JSEventSourceConstructor.h include is
> guarded below.
> 
> +    virtual ~ProcessWebSocketEventTask() {}
> 
> We normally put a space between braces here.
> 
> +    ProcessWebSocketMessageEventTask(PassRefPtr<WebSocket> websocket, const
> String& msg)
> +        : ProcessWebSocketEventTask(websocket)
> +        , m_msg(msg) {}
> 
> And these braces typically go each on its own line.
> 
>      RefPtr<WebSocket> m_webSocket;
> -    RefPtr<Event> m_event;
> +};
> 
> An event like this cannot be passed between threads, because WebSocket
> refcounting is not thread safe. But I'm not sure if that's what is happening
> here. What is the reason for splitting ProcessWebSocketEventTask into multiple
> classes? This is something ChangeLog per-function comments would serve best
> for.
> 
> +        // FIXME: origin, lastEventId, source, messagePort.
> 
> What needs to be fixed here? I think that the current arguments are correct.
> Per their definition in HTML5, they only apply to server-sent events and
> cross-document messaging. While origin could theoretically apply to WebSocket
> messages, too, the spec doesn't make it so.
> 
> It looks like the design here is that all cross-platform code is executed
> inside worker thread/process, with SocketStreamChannel being responsible for
> communication with loader process/main thread as necessary. The current version
> of SocketStreamHandleCFNet doesn't do that, so I don't see how the included
> test can pass with it now. Also, this design conflicts with what you suggested
> in bug 32246 comment 2, or any other case where we may want to share data
> between all WebSocketChannels.
> 
> It would likely be best to document the design, and to discuss it on webkit-dev
> before updating the patch.

Ok, I'll write up a document about WebSocket threading design.
Thanks.

[Fumitoshi Ukai]

Created attachment 44602 [details]
Add WebSocket feature in Worker.

[David Levin]

I'm working on this but I won't quite finish until Monday.

[David Levin]

Comment on attachment 44602 [details]
Add WebSocket feature in Worker.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

This change log lacks any useful content describing why any changes were done.

> 
> diff --git a/WebCore/websockets/ThreadableWebSocketChannel.cpp b/WebCore/websockets/ThreadableWebSocketChannel.cpp
> +#endif  // ENABLE(WORKERS)

One space before end of line comments.

> diff --git a/WebCore/websockets/ThreadableWebSocketChannel.h b/WebCore/websockets/ThreadableWebSocketChannel.h
> +}  // namespace WebCore

One space before end of line comments. (Feel free to improve check-webkit-style :) ).

> +#endif  // ENABLE(WEB_SOCKETS)

One space before end of line comments.

> +#endif  // ThreadableWebSocketChannel_h

One space before end of line comments.

> diff --git a/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.h b/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.h
> +class ThreadableWebSocketChannelClientWrapper : public ThreadSafeShared<ThreadableWebSocketChannelClientWrapper> {
> +public:
> +    static PassRefPtr<ThreadableWebSocketChannelClientWrapper> create(WebSocketChannelClient* client)
> +     {

Indented { too far.

> +         return adoptRef(new ThreadableWebSocketChannelClientWrapper(client));
> +     }

Indented } too far.

> +    void callSyncMethod()
> +    {
> +        m_didProcessMethod = false;

How about "m_syncMethodDone" which mirrors the method names more closely?

> +}  // namespace WebCore

One space before end of line comments.

> +#endif  // ENABLE(WEB_SOCKETS)

One space before end of line comments.

> +#endif  // ThreadableWebSocketChannelClientWrapper_h

One space before end of line comments.


> diff --git a/WebCore/websockets/WebSocket.cpp b/WebCore/websockets/WebSocket.cpp
> -    // FIXME: origin, lastEventId, source, messagePort.

Why are you removing this FIXME? (How was it fixed?)

> diff --git a/WebCore/websockets/WebSocket.h b/WebCore/websockets/WebSocket.h

> +    // ActiveDOMObject
> +    //  virtual bool hasPendingActivity() const;
> +    // virtual void contextDestroyed();
> +    // virtual bool canSuspend() const;
> +    // virtual void suspend();
> +    // virtual void resume();
> +    // virtual void stop();

All of this commented out code should be removed.

>  }  // namespace WebCore

One space before end of line comments.

> diff --git a/WebCore/websockets/WebSocketChannel.h b/WebCore/websockets/WebSocketChannel.h

I can't tell what changed in here easily and the diff tool in bugzilla isn't smart enough to notice that many of these line are probably just re-indenting. Please do the reindenting separately since it really makes it much harder to tell what is actually changing.

>  }  // namespace WebCore

One space before end of line comments.

> diff --git a/WebCore/websockets/WebSocketChannelClient.h b/WebCore/websockets/WebSocketChannelClient.h

I really wish that all of these formatting changes were not part of this change (when they are lines of even files that aren't even part of what you are accomplishing). It makes it harder to get through it and it will make it harder for me to verify the real changes being made.

>  }  // namespace WebCore

One space before end of line comments.

> diff --git a/WebCore/websockets/WebSocketHandshake.h b/WebCore/websockets/WebSocketHandshake.h

This apparently pure formatting change shouldn't be part of this change. Please do a separate change for these indent fixes.
  
>  }  // namespace WebCore

One space before end of line comments.

> diff --git a/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp b/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp


> +bool WorkerThreadableWebSocketChannel::send(const String& msg)

Use message instead of msg (in several places). (WebKit style is to prefer whole words over abbreviations.)

> +{
> +    return m_bridge.send(msg);
> +}


> +void WorkerThreadableWebSocketChannel::MainThreadBridge::mainThreadDestroy(ScriptExecutionContext* context, MainThreadBridge* thisPtr)
> +{
> +    ASSERT(isMainThread());
> +    ASSERT_UNUSED(context, context->isDocument());
> +    delete thisPtr;
> +}

How do we know MainThreadBridge won't be used anymore after this?


> +void WorkerThreadableWebSocketChannel::MainThreadBridge::waitSyncMethod()
> +{
> +    WorkerRunLoop& runLoop = m_workerContext->thread()->runLoop();
> +    MessageQueueWaitResult result = MessageQueueMessageReceived;
> +    ThreadableWebSocketChannelClientWrapper* clientWrapper = static_cast<ThreadableWebSocketChannelClientWrapper*>(m_workerClientWrapper.get());
> +    while (clientWrapper && clientWrapper->syncMethodDone() && result != MessageQueueTerminated) {
> +        result = runLoop.runInMode(m_workerContext.get(), m_taskMode);

You just used m_taskMode on the worker thread.

> +        clientWrapper = static_cast<ThreadableWebSocketChannelClientWrapper*>(m_workerClientWrapper.get());
> +    }
> +    return;

No "return;" needed here.

> +}
> +
> +void WorkerThreadableWebSocketChannel::MainThreadBridge::mainThreadCreateWebSocketChannel(ScriptExecutionContext* context, MainThreadBridge* thisPtr, const KURL& url, const String& protocol)
> +{
> +    ASSERT(isMainThread());
> +    ASSERT(context->isDocument());

Should this assert !thisPtr->m_mainWebSocketChannel here?

> +
> +    thisPtr->m_mainWebSocketChannel = WebSocketChannel::create(context, thisPtr, url, protocol);
> +    ASSERT(thisPtr->m_mainWebSocketChannel);
> +}


> +void WorkerThreadableWebSocketChannel::MainThreadBridge::mainThreadSend(ScriptExecutionContext* context, MainThreadBridge* thisPtr, const String& msg)
> +{
> +    ASSERT(isMainThread());
> +    ASSERT_UNUSED(context, context->isDocument());
> +
> +    if (!thisPtr->m_mainWebSocketChannel)
> +        return;
> +    if (!thisPtr->m_workerClientWrapper)
> +        return;

Why not 
  if (!thisPtr->m_mainWebSocketChannel || !thisPtr->m_workerClientWrapper)
      return;
?

> +    ASSERT(!thisPtr->m_workerClientWrapper->syncMethodDone());
> +    bool sent = thisPtr->m_mainWebSocketChannel->send(msg.crossThreadString());

Why are you doing crossThreadString here? "msg" should already have been copied for this thread.

> +    thisPtr->m_workerContext->thread()->workerLoaderProxy().postTaskForModeToWorkerContext(createCallbackTask(&workerContextDidSend, thisPtr->m_workerClientWrapper, sent), thisPtr->m_taskMode);
> +}
> +


> +}  // namespace WebCore

One space before end of line comments.

> +#endif  // ENABLE(WEB_SOCKETS)

One space before end of line comments.

> diff --git a/WebCore/websockets/WorkerThreadableWebSocketChannel.h b/WebCore/websockets/WorkerThreadableWebSocketChannel.h

> +class WorkerThreadableWebSocketChannel : public RefCounted<WorkerThreadableWebSocketChannel>, public ThreadableWebSocketChannel {
...
> +    class MainThreadBridge : public WebSocketChannelClient {

I know you're mirroring what I did for workers and xhr but now that I've had a chance to see it again, I don't think it is a good approach because it seems to lead to bugs too easily where things are ref counted on the wrong thread.

MainThreadBridge should be split into two classes:
  LoaderThreadClient
  WorkerThreadProxy?

Perhaps you can think of better names. Then there would be much less chance of things getting ref counted on the wrong thread.


> +    public:
> +        MainThreadBridge(PassRefPtr<ThreadableWebSocketChannelClientWrapper>, PassRefPtr<WorkerContext>, const String& /* taskMode */, const KURL&, const String& /* protocol */);

Why are all of these parameters commented out?

> +        // ThreadableWebSocketClientWrapper is to be used on the worker context thread.
> +        // The ref counting is done on either thread.
> +        RefPtr<ThreadSafeShared<ThreadableWebSocketChannelClientWrapper> > m_workerClientWrapper;
> +
> +        // WorkerLoaderProxy is to be used on either thread.

Why is this talking about WorkerLoaderProxy?

> +        // WorkerRunLoop is to be used on the worker thread.

Why is this talking about WorkerRunLoop?

> +        // The ref counting is done on either thread.
> +        RefPtr<WorkerContext> m_workerContext;

This is incorrect for lots of reasons:
1. This object is deleted on the main thread so m_workerContext will get deref'ed there which is bad because it isn't ThreadSafeShared (only RefCounted).
2. Very few methods can be used on this class on the main thread, so it isn't great to provide access there.
3. It makes it too easy to do unintentional refcounting on the main thread (which is happening already, see #1).

> +        // For use on the main thread.
> +        String m_taskMode;
> +    };
> +
> +    WorkerThreadableWebSocketChannel(WorkerContext*, WebSocketChannelClient*, const String& taskMode, const KURL&, const String&);

What is the const String& parameter for? It needs a name!

> +
> +    RefPtr<WorkerContext> m_workerContext;
> +    RefPtr<ThreadableWebSocketChannelClientWrapper> m_workerClientWrapper;
> +    MainThreadBridge& m_bridge;
> +};
> +
> +}  // namespace WebCore

One space before end of line comments.

> +#endif  // ENABLE(WEB_SOCKETS)

One space before end of line comments.

> +#endif  // WorkerThreadableWebSocketChannel_h

One space before end of line comments.

> diff --git a/WebCore/workers/WorkerContext.idl b/WebCore/workers/WorkerContext.idl
> +        attribute [JSCCustomGetter,EnabledAtRuntime] WebSocketConstructor WebSocket;  // Usable with the new operator

One space before end of line comments.

[Fumitoshi Ukai]

Created attachment 44936 [details]
Add WebSocket feature in Worker

[WebKit Review Bot]

Attachment 44936 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/CrossThreadCopier.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1

[Fumitoshi Ukai]

(In reply to comment #7)
> (From update of attachment 44602 [details])
> > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> 
> This change log lacks any useful content describing why any changes were done.
> 
> > 
> > diff --git a/WebCore/websockets/ThreadableWebSocketChannel.cpp b/WebCore/websockets/ThreadableWebSocketChannel.cpp
> > +#endif  // ENABLE(WORKERS)
> 
> One space before end of line comments.
> 
> > diff --git a/WebCore/websockets/ThreadableWebSocketChannel.h b/WebCore/websockets/ThreadableWebSocketChannel.h
> > +}  // namespace WebCore
> 
> One space before end of line comments. (Feel free to improve check-webkit-style
> :) ).
> 
> > +#endif  // ENABLE(WEB_SOCKETS)
> 
> One space before end of line comments.
> 
> > +#endif  // ThreadableWebSocketChannel_h
> 
> One space before end of line comments.
> 
> > diff --git a/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.h b/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.h
> > +class ThreadableWebSocketChannelClientWrapper : public ThreadSafeShared<ThreadableWebSocketChannelClientWrapper> {
> > +public:
> > +    static PassRefPtr<ThreadableWebSocketChannelClientWrapper> create(WebSocketChannelClient* client)
> > +     {
> 
> Indented { too far.
> 
> > +         return adoptRef(new ThreadableWebSocketChannelClientWrapper(client));
> > +     }
> 
> Indented } too far.
> 
> > +    void callSyncMethod()
> > +    {
> > +        m_didProcessMethod = false;
> 
> How about "m_syncMethodDone" which mirrors the method names more closely?
> 
> > +}  // namespace WebCore
> 
> One space before end of line comments.
> 
> > +#endif  // ENABLE(WEB_SOCKETS)
> 
> One space before end of line comments.
> 
> > +#endif  // ThreadableWebSocketChannelClientWrapper_h
> 
> One space before end of line comments.
> 
> 
> > diff --git a/WebCore/websockets/WebSocket.cpp b/WebCore/websockets/WebSocket.cpp
> > -    // FIXME: origin, lastEventId, source, messagePort.
> 
> Why are you removing this FIXME? (How was it fixed?)

As Alexey mentioned in comment #3, it is wrong comment.

> 
> > diff --git a/WebCore/websockets/WebSocket.h b/WebCore/websockets/WebSocket.h
> 
> > +    // ActiveDOMObject
> > +    //  virtual bool hasPendingActivity() const;
> > +    // virtual void contextDestroyed();
> > +    // virtual bool canSuspend() const;
> > +    // virtual void suspend();
> > +    // virtual void resume();
> > +    // virtual void stop();
> 
> All of this commented out code should be removed.
> 
> >  }  // namespace WebCore
> 
> One space before end of line comments.
> 
> > diff --git a/WebCore/websockets/WebSocketChannel.h b/WebCore/websockets/WebSocketChannel.h
> 
> I can't tell what changed in here easily and the diff tool in bugzilla isn't
> smart enough to notice that many of these line are probably just re-indenting.
> Please do the reindenting separately since it really makes it much harder to
> tell what is actually changing.
> 
> >  }  // namespace WebCore
> 
> One space before end of line comments.
> 
> > diff --git a/WebCore/websockets/WebSocketChannelClient.h b/WebCore/websockets/WebSocketChannelClient.h
> 
> I really wish that all of these formatting changes were not part of this change
> (when they are lines of even files that aren't even part of what you are
> accomplishing). It makes it harder to get through it and it will make it harder
> for me to verify the real changes being made.
> 
> >  }  // namespace WebCore
> 
> One space before end of line comments.
> 
> > diff --git a/WebCore/websockets/WebSocketHandshake.h b/WebCore/websockets/WebSocketHandshake.h
> 
> This apparently pure formatting change shouldn't be part of this change. Please
> do a separate change for these indent fixes.
> 
> >  }  // namespace WebCore
> 
> One space before end of line comments.
> 
> > diff --git a/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp b/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp
> 
> 
> > +bool WorkerThreadableWebSocketChannel::send(const String& msg)
> 
> Use message instead of msg (in several places). (WebKit style is to prefer
> whole words over abbreviations.)
> 
> > +{
> > +    return m_bridge.send(msg);
> > +}
> 
> 
> > +void WorkerThreadableWebSocketChannel::MainThreadBridge::mainThreadDestroy(ScriptExecutionContext* context, MainThreadBridge* thisPtr)
> > +{
> > +    ASSERT(isMainThread());
> > +    ASSERT_UNUSED(context, context->isDocument());
> > +    delete thisPtr;
> > +}
> 
> How do we know MainThreadBridge won't be used anymore after this?
> 
> 
> > +void WorkerThreadableWebSocketChannel::MainThreadBridge::waitSyncMethod()
> > +{
> > +    WorkerRunLoop& runLoop = m_workerContext->thread()->runLoop();
> > +    MessageQueueWaitResult result = MessageQueueMessageReceived;
> > +    ThreadableWebSocketChannelClientWrapper* clientWrapper = static_cast<ThreadableWebSocketChannelClientWrapper*>(m_workerClientWrapper.get());
> > +    while (clientWrapper && clientWrapper->syncMethodDone() && result != MessageQueueTerminated) {
> > +        result = runLoop.runInMode(m_workerContext.get(), m_taskMode);
> 
> You just used m_taskMode on the worker thread.
> 
> > +        clientWrapper = static_cast<ThreadableWebSocketChannelClientWrapper*>(m_workerClientWrapper.get());
> > +    }
> > +    return;
> 
> No "return;" needed here.
> 
> > +}
> > +
> > +void WorkerThreadableWebSocketChannel::MainThreadBridge::mainThreadCreateWebSocketChannel(ScriptExecutionContext* context, MainThreadBridge* thisPtr, const KURL& url, const String& protocol)
> > +{
> > +    ASSERT(isMainThread());
> > +    ASSERT(context->isDocument());
> 
> Should this assert !thisPtr->m_mainWebSocketChannel here?
> 
> > +
> > +    thisPtr->m_mainWebSocketChannel = WebSocketChannel::create(context, thisPtr, url, protocol);
> > +    ASSERT(thisPtr->m_mainWebSocketChannel);
> > +}
> 
> 
> > +void WorkerThreadableWebSocketChannel::MainThreadBridge::mainThreadSend(ScriptExecutionContext* context, MainThreadBridge* thisPtr, const String& msg)
> > +{
> > +    ASSERT(isMainThread());
> > +    ASSERT_UNUSED(context, context->isDocument());
> > +
> > +    if (!thisPtr->m_mainWebSocketChannel)
> > +        return;
> > +    if (!thisPtr->m_workerClientWrapper)
> > +        return;
> 
> Why not 
>   if (!thisPtr->m_mainWebSocketChannel || !thisPtr->m_workerClientWrapper)
>       return;
> ?
> 
> > +    ASSERT(!thisPtr->m_workerClientWrapper->syncMethodDone());
> > +    bool sent = thisPtr->m_mainWebSocketChannel->send(msg.crossThreadString());
> 
> Why are you doing crossThreadString here? "msg" should already have been copied
> for this thread.
> 
> > +    thisPtr->m_workerContext->thread()->workerLoaderProxy().postTaskForModeToWorkerContext(createCallbackTask(&workerContextDidSend, thisPtr->m_workerClientWrapper, sent), thisPtr->m_taskMode);
> > +}
> > +
> 
> 
> > +}  // namespace WebCore
> 
> One space before end of line comments.
> 
> > +#endif  // ENABLE(WEB_SOCKETS)
> 
> One space before end of line comments.
> 
> > diff --git a/WebCore/websockets/WorkerThreadableWebSocketChannel.h b/WebCore/websockets/WorkerThreadableWebSocketChannel.h
> 
> > +class WorkerThreadableWebSocketChannel : public RefCounted<WorkerThreadableWebSocketChannel>, public ThreadableWebSocketChannel {
> ...
> > +    class MainThreadBridge : public WebSocketChannelClient {
> 
> I know you're mirroring what I did for workers and xhr but now that I've had a
> chance to see it again, I don't think it is a good approach because it seems to
> lead to bugs too easily where things are ref counted on the wrong thread.
> 
> MainThreadBridge should be split into two classes:
>   LoaderThreadClient
>   WorkerThreadProxy?
> 
> Perhaps you can think of better names. Then there would be much less chance of
> things getting ref counted on the wrong thread.
> 
> 
> > +    public:
> > +        MainThreadBridge(PassRefPtr<ThreadableWebSocketChannelClientWrapper>, PassRefPtr<WorkerContext>, const String& /* taskMode */, const KURL&, const String& /* protocol */);
> 
> Why are all of these parameters commented out?
> 
> > +        // ThreadableWebSocketClientWrapper is to be used on the worker context thread.
> > +        // The ref counting is done on either thread.
> > +        RefPtr<ThreadSafeShared<ThreadableWebSocketChannelClientWrapper> > m_workerClientWrapper;
> > +
> > +        // WorkerLoaderProxy is to be used on either thread.
> 
> Why is this talking about WorkerLoaderProxy?
> 
> > +        // WorkerRunLoop is to be used on the worker thread.
> 
> Why is this talking about WorkerRunLoop?
> 
> > +        // The ref counting is done on either thread.
> > +        RefPtr<WorkerContext> m_workerContext;
> 
> This is incorrect for lots of reasons:
> 1. This object is deleted on the main thread so m_workerContext will get
> deref'ed there which is bad because it isn't ThreadSafeShared (only
> RefCounted).
> 2. Very few methods can be used on this class on the main thread, so it isn't
> great to provide access there.
> 3. It makes it too easy to do unintentional refcounting on the main thread
> (which is happening already, see #1).
> 
> > +        // For use on the main thread.
> > +        String m_taskMode;
> > +    };
> > +
> > +    WorkerThreadableWebSocketChannel(WorkerContext*, WebSocketChannelClient*, const String& taskMode, const KURL&, const String&);
> 
> What is the const String& parameter for? It needs a name!
> 
> > +
> > +    RefPtr<WorkerContext> m_workerContext;
> > +    RefPtr<ThreadableWebSocketChannelClientWrapper> m_workerClientWrapper;
> > +    MainThreadBridge& m_bridge;
> > +};
> > +
> > +}  // namespace WebCore
> 
> One space before end of line comments.
> 
> > +#endif  // ENABLE(WEB_SOCKETS)
> 
> One space before end of line comments.
> 
> > +#endif  // WorkerThreadableWebSocketChannel_h
> 
> One space before end of line comments.
> 
> > diff --git a/WebCore/workers/WorkerContext.idl b/WebCore/workers/WorkerContext.idl
> > +        attribute [JSCCustomGetter,EnabledAtRuntime] WebSocketConstructor WebSocket;  // Usable with the new operator
> 
> One space before end of line comments.

[Shinichiro Hamaji]

> > diff --git a/WebCore/websockets/ThreadableWebSocketChannel.cpp b/WebCore/websockets/ThreadableWebSocketChannel.cpp
> > +#endif  // ENABLE(WORKERS)
> 
> One space before end of line comments.

It seems there are no explicit rule about this but this is the consensus of core WebKit developers. Do you think we should note this rule in style guide explicitly and modify check-webkit-style?

[Fumitoshi Ukai]

(In reply to comment #7)

> One space before end of line comments. (Feel free to improve check-webkit-style
> :) ).

Created a patch in https://bugs.webkit.org/show_bug.cgi?id=32597

[WebKit Review Bot]

Attachment 44936 [details] did not pass chromium-ews:
Full output: http://webkit-commit-queue.appspot.com/results/128550

[Fumitoshi Ukai]

(In reply to comment #13)
> Attachment 44936 [details] did not pass chromium-ews:
> Full output: http://webkit-commit-queue.appspot.com/results/128550

It is chromium-ews system issue.

[Adam Barth]

> It is chromium-ews system issue.

Thanks.  Not sure what went wrong.  I'll investigate.  Sorry for the noise.

[David Levin]

(In reply to comment #11)
> > > diff --git a/WebCore/websockets/ThreadableWebSocketChannel.cpp b/WebCore/websockets/ThreadableWebSocketChannel.cpp
> > > +#endif  // ENABLE(WORKERS)
> > 
> > One space before end of line comments.
> 
> It seems there are no explicit rule about this but this is the consensus of
> core WebKit developers. Do you think we should note this rule in style guide
> explicitly and modify check-webkit-style?

Yes, see this email where I listed a number of things that there is consensus on but nothing in the webkit style guide:
  http://lists.macosforge.org/pipermail/webkit-dev/2009-September/009807.html

Unfortunately, I haven't yet done the next step of adding these items to the style guide, but there is consensus on them (see that thread for a few small modifications to those guidelines).

[Fumitoshi Ukai]

Created attachment 45264 [details]
Add WebSocket feature in Worker

[WebKit Review Bot]

Attachment 45264 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/CrossThreadCopier.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1

[David Levin]

Comment on attachment 45264 [details]
Add WebSocket feature in Worker

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.

[Fumitoshi Ukai]

Created attachment 45866 [details]
Add WebSocket feature in Worker

[WebKit Review Bot]

Attachment 45866 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/CrossThreadCopier.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1

[David Levin]

Comment on attachment 45866 [details]
Add WebSocket feature in Worker

Please consider addressing the nits below and submitting.  Thanks for your patience -- this one was large and complicated to review (due to the threading issues).

> diff --git a/WebCore/websockets/WorkerThreadableWebSocketChannel.h b/WebCore/websockets/WorkerThreadableWebSocketChannel.h
> +private:
> +    // Generated by the bridge.  The Peer and it's bridge should have idential

s/it's/its/
s/idential/identical/

> +    // lifetimes.
> +    class Peer : public WebSocketChannelClient {


> +        RefPtr<ThreadSafeShared<ThreadableWebSocketChannelClientWrapper> > m_workerClientWrapper;
> +
> +        WorkerLoaderProxy& m_loaderProxy;
> +
> +        RefPtr<ThreadableWebSocketChannel> m_mainWebSocketChannel;
> +
> +        String m_taskMode;

I don't understand why the member variables are double spaced (e.g. they have blank lines between all of them), and I find it distracting.


> +    class Bridge : public RefCounted<Bridge> {
> +    private:
> +        static void setWebSocketChannel(ScriptExecutionContext* context, Bridge* thisPtr, Peer* peer, RefPtr<ThreadSafeShared<ThreadableWebSocketChannelClientWrapper> > workerClientWrapper);

param names "context", "peer", and "workerClientWrapper" shouldn't be here.

> +
> +        // Executed on the main thread to create a Peer for this bridge.
> +        static void mainThreadCreateWebSocketChannel(ScriptExecutionContext* context, Bridge* thisPtr, RefPtr<ThreadSafeShared<ThreadableWebSocketChannelClientWrapper> > clientWrapper, const String& taskMode, const KURL& url, const String& protocol);

param names "context", "clientWrapper", and "url" shouldn't be here.


> +        RefPtr<ThreadSafeShared<ThreadableWebSocketChannelClientWrapper> > m_workerClientWrapper;
> +
> +        RefPtr<WorkerContext> m_workerContext;
> +
> +        WorkerLoaderProxy& m_loaderProxy;
> +
> +        String m_taskMode;
> +
> +        Peer* m_peer;

I don't understand why the member variables are double spaced (e.g. they have blank lines between all of them), and I find it distracting.

[Fumitoshi Ukai]

Committed r52892: <http://trac.webkit.org/changeset/52892>

[Andrew Wilson]

Is there a reason why this is only enabled for Workers, not SharedWorkers?

[Andrew Wilson]

Actually, it looks like the API *is* defined for both SharedWorkers and dedicated Workers, but the test only tests dedicated Workers. You should add a test for SharedWorkers also.

[David Levin]

Filed https://bugs.webkit.org/show_bug.cgi?id=33285 re the test.