Bug 31441 - Implement SocketStreamHandleCFNet
Summary: Implement SocketStreamHandleCFNet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-12 15:01 PST by Alexey Proskuryakov
Modified: 2009-12-04 11:17 PST (History)
0 users

See Also:


Attachments
proposed patch (16.26 KB, patch)
2009-11-12 15:14 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2009-11-12 15:01:57 PST
Adding a basic implementation of WebSockets for Mac and Windows Safari. Many parts still WIP.
Comment 1 Alexey Proskuryakov 2009-11-12 15:14:14 PST
Created attachment 43105 [details]
proposed patch
Comment 2 Darin Adler 2009-11-13 09:09:03 PST
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
Comment 3 Alexey Proskuryakov 2009-11-13 13:38:15 PST
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.