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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Dec 14 22:40:59 PST 2009


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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #44602|review?                     |review-
               Flag|                            |




--- Comment #7 from David Levin <levin at chromium.org>  2009-12-14 22:40:59 PST ---
(From update of attachment 44602)
> 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?)

> 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.



More information about the webkit-unassigned mailing list