Bug 31441 – Implement SocketStreamHandleCFNet

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Alexey Proskuryakov]

Adding a basic implementation of WebSockets for Mac and Windows Safari. Many parts still WIP.

[Alexey Proskuryakov]

Created attachment 43105 [details]
proposed patch

[Darin Adler]

Comment on attachment 43105 [details]
proposed patch

> +        bool shouldUseSSL() const { return m_url.protocolIs("wss"); }

I'd like to see the "ws" and "wss" constants centralized in one place if possible.

> +        enum ConnectionType {
> +            Unknown,
> +            Direct,
> +            SOCKSProxy,
> +            CONNECTProxy
> +        };

Seems this would fit nicely on a single line.

> +        m_url.setPort(m_url.protocolIs("wss") ? 443 : 80);

Could just call shouldUseSSL() here.

> +    KURL httpURL(KURL(), (m_url.protocolIs("wss") ? "https://" : "http://") + m_url.host());

And here.

> +    ASSERT(!m_readStream == !m_writeStream);

I suppose you could put this assertion a few other places too, like at the start of SocketStreamHandle::platformClose.

Or you could put an assertion where the failure would be easier to interpret like this:

    if (!m_readStream) {
        ASSERT(!m_writeStream);
        return;
    }
    ASSERT(m_writeStream);

> +    CFReadStreamRef readStream = 0;
> +    CFWriteStreamRef writeStream = 0;

It seems these are defined too early. I'd define them just before the CFStreamCreatePairWithSocketToHost call; in fact they are really scoped to just that call and then the adoptCF calls just below it.

> +    SocketStreamHandle* obj = static_cast<SocketStreamHandle*>(info);

How about "handle" for this local variable instead?

> +    RetainPtr<CFStringRef> url(AdoptCF, obj->m_url.string().createCFString());
> +    return CFStringCreateWithFormat(0, 0, CFSTR("WebKit socket stream, %@"), url.get());

You could use WebCore::String appending rather than the CFString format operation here.

> +        CFIndex len;

How about "length" for this local variable instead? Or "bufferLength"?

> +        const UInt8* ptr = CFReadStreamGetBuffer(m_readStream.get(), 0, &len);
> +        if (!ptr) {
> +            ASSERT(isMainThread());
> +            static UInt8 buf[1024];
> +            len = CFReadStreamRead(m_readStream.get(), buf, 1024);
> +            ptr = buf;
> +        }
> +
> +        m_client->didReceiveData(this, reinterpret_cast<const char*>(ptr), len);

I don't understand why a global-variable buffer is needed here. Could we use a stack buffer instead?


r=me

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/50951>.

> I don't understand why a global-variable buffer is needed here. Could we use a
> stack buffer instead?

Tried to save some stack space, but I don't think I had a a good reason.