Use SocketProvider to create SocketStreamHandles

[Alex Christensen]

Use SocketProvider to create SocketStreamHandles

[Alex Christensen]

Created attachment 283663 [details]
Patch

[Alex Christensen]

Created attachment 283664 [details]
Patch

[Brady Eidson]

Comment on attachment 283664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283664&action=review

> Source/WebCore/Modules/websockets/ThreadableWebSocketChannel.cpp:45
> +#include "ThreadableWebSocketChannel.h"
> +
> +#include "Document.h"
> +#include "ScriptExecutionContext.h"
> +#include "ThreadableWebSocketChannelClientWrapper.h"
> +#include "WebSocketChannel.h"
> +#include "WebSocketChannelClient.h"
> +#include "WorkerGlobalScope.h"
> +#include "WorkerRunLoop.h"
> +#include "WorkerThread.h"
> +#include "WorkerThreadableWebSocketChannel.h"
> +#include <wtf/text/StringBuilder.h>

Really needs all of these?

> Source/WebCore/Modules/websockets/ThreadableWebSocketChannel.cpp:49
> +static const char webSocketChannelMode[] = "webSocketChannelMode";

ASCIILiteral?

> Source/WebCore/Modules/websockets/ThreadableWebSocketChannel.cpp:57
> +        mode.appendLiteral(webSocketChannelMode);

And move the ASCIILiteral here, the only place it's actually used?

> Source/WebKit/mac/Misc/WebSocketProvider.mm:51
> +RefPtr<SocketStreamHandle> WebSocketProvider::createSocketStreamHandle(const URL& url, SocketStreamHandleClient& client, NetworkingContext& context, SessionID sessionID)
>  {
> -    if (is<WorkerGlobalScope>(context)) {
> -        WorkerGlobalScope& workerGlobalScope = downcast<WorkerGlobalScope>(context);
> -        WorkerRunLoop& runLoop = workerGlobalScope.thread().runLoop();
> -        StringBuilder mode;
> -        mode.appendLiteral(webSocketChannelMode);
> -        mode.appendNumber(runLoop.createUniqueId());
> -        return WorkerThreadableWebSocketChannel::create(workerGlobalScope, client, mode.toString());
> -    }
> -
> -    return WebSocketChannel::create(downcast<Document>(context), client);
> +    return SocketStreamHandle::create(url, client, context, sessionID);

Default WebCore::SocketProvider should just do this, and the WebKit2 one should override.

[Alex Christensen]

Created attachment 283665 [details]
Patch

[Alex Christensen]

(In reply to comment #3)
> Comment on attachment 283664 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283664&action=review
> 
> > Source/WebCore/Modules/websockets/ThreadableWebSocketChannel.cpp:45
> > +#include "ThreadableWebSocketChannel.h"
> > +
> > +#include "Document.h"
> > +#include "ScriptExecutionContext.h"
> > +#include "ThreadableWebSocketChannelClientWrapper.h"
> > +#include "WebSocketChannel.h"
> > +#include "WebSocketChannelClient.h"
> > +#include "WorkerGlobalScope.h"
> > +#include "WorkerRunLoop.h"
> > +#include "WorkerThread.h"
> > +#include "WorkerThreadableWebSocketChannel.h"
> > +#include <wtf/text/StringBuilder.h>
> 
> Really needs all of these?
Yes.
> 
> > Source/WebCore/Modules/websockets/ThreadableWebSocketChannel.cpp:49
> > +static const char webSocketChannelMode[] = "webSocketChannelMode";
> 
> ASCIILiteral?
> 
> > Source/WebCore/Modules/websockets/ThreadableWebSocketChannel.cpp:57
> > +        mode.appendLiteral(webSocketChannelMode);
> 
> And move the ASCIILiteral here, the only place it's actually used?
StringBuilder has a nice appendLiteral(const char (&characters)[charactersCount]) which counts the characters at compile time.
> 
> > Source/WebKit/mac/Misc/WebSocketProvider.mm:51
> > +RefPtr<SocketStreamHandle> WebSocketProvider::createSocketStreamHandle(const URL& url, SocketStreamHandleClient& client, NetworkingContext& context, SessionID sessionID)
> >  {
> > -    if (is<WorkerGlobalScope>(context)) {
> > -        WorkerGlobalScope& workerGlobalScope = downcast<WorkerGlobalScope>(context);
> > -        WorkerRunLoop& runLoop = workerGlobalScope.thread().runLoop();
> > -        StringBuilder mode;
> > -        mode.appendLiteral(webSocketChannelMode);
> > -        mode.appendNumber(runLoop.createUniqueId());
> > -        return WorkerThreadableWebSocketChannel::create(workerGlobalScope, client, mode.toString());
> > -    }
> > -
> > -    return WebSocketChannel::create(downcast<Document>(context), client);
> > +    return SocketStreamHandle::create(url, client, context, sessionID);
> 
> Default WebCore::SocketProvider should just do this, and the WebKit2 one
> should override.
Done.

[Alex Christensen]

Created attachment 283672 [details]
Patch

[Alex Christensen]

Created attachment 283678 [details]
Patch

[Alex Christensen]

Created attachment 283683 [details]
Patch

[Alex Christensen]

https://trac.webkit.org/changeset/203244

[Csaba Osztrogonác]

(In reply to comment #9)
> https://trac.webkit.org/changeset/203244

It broke the !ENABLE(WEB_SOCKETS) build, because WebCore::SocketProvider
is defined inside guards, but this change uses it without guard:

../../Source/WebCore/inspector/InspectorOverlay.cpp: In member function 'WebCore::Page* WebCore::InspectorOverlay::overlayPage()':
../../Source/WebCore/inspector/InspectorOverlay.cpp:867:77: error: 'create' is not a member of 'WebCore::SocketProvider'

[Csaba Osztrogonác]

This snippet in Source/WebCore/page/SocketProvider.h fixes the build,
but I'm not sure if it is the proper fix here. I don't have any time
to investigate and/or prepare patch, feel free to pick it up or fix
it however you want.

-#if ENABLE(WEB_SOCKETS)
     static Ref<SocketProvider> create() { return adoptRef(*new SocketProvider); }
+#if ENABLE(WEB_SOCKETS)