[Webkit-unassigned] [Bug 32214] Add WebSocket feature in Worker

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32214





--- Comment #4 from Fumitoshi Ukai <ukai at chromium.org>  2009-12-08 20:25:26 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.