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

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.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 44393: Add WebSocket feature in Worker.
https://bugs.webkit.org/attachment.cgi?id=44393&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+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.