[webkit-reviews] review granted: [Bug 32214] Add WebSocket feature in Worker : [Attachment 45866] Add WebSocket feature in Worker

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted 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 45866: Add WebSocket feature in Worker
https://bugs.webkit.org/attachment.cgi?id=45866&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.