Summary: | Implement SocketStreamHandleCFNet | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||
Component: | Platform | Assignee: | Alexey Proskuryakov <ap> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Alexey Proskuryakov
2009-11-12 15:01:57 PST
Created attachment 43105 [details]
proposed patch
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 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. |