[webkit-reviews] review granted: [Bug 31441] Implement SocketStreamHandleCFNet : [Attachment 43105] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 31441: Implement SocketStreamHandleCFNet
https://bugs.webkit.org/show_bug.cgi?id=31441

Attachment 43105: proposed patch
https://bugs.webkit.org/attachment.cgi?id=43105&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   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