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

[bugzilla-daemon at webkit.org]

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





--- Comment #10 from Fumitoshi Ukai <ukai at chromium.org>  2009-12-15 21:17:00 PST ---
(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.

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