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

[bugzilla-daemon at webkit.org]

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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #45866|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #22 from David Levin <levin at chromium.org>  2010-01-06 14:13:02 PST ---
(From update of attachment 45866)
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.

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