<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [Curl] WebSocket platform part is not implemented."
href="https://bugs.webkit.org/show_bug.cgi?id=144628">bug 144628</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #252729 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [Curl] WebSocket platform part is not implemented."
href="https://bugs.webkit.org/show_bug.cgi?id=144628#c10">Comment # 10</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [Curl] WebSocket platform part is not implemented."
href="https://bugs.webkit.org/show_bug.cgi?id=144628">bug 144628</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=252729&action=diff" name="attach_252729" title="Patch">attachment 252729</a> <a href="attachment.cgi?id=252729&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=252729&action=review">https://bugs.webkit.org/attachment.cgi?id=252729&action=review</a>
lets not use this unnecessary std::unique_ptr
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandle.h:48
> +#if PLATFORM(WIN)
> +#include <windows.h>
> +#include <winsock2.h>
> +#endif
> +
> +#include <curl/curl.h>
> +
> +#include <mutex>
> +
> +#include <wtf/Deque.h>
> #include <wtf/RefCounted.h>
> +#include <wtf/Threading.h></span >
These aren’t sorted in the normal WebKit project way.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandle.h:88
> + const char* data() const { return m_data.get(); }
> + int size() const { return m_size; }</span >
No need for these.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandle.h:90
> + std::unique_ptr<const char[]> m_data;</span >
Should just name this data.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandle.h:91
> + int m_size;</span >
Should just name this size. I also suggest initializing it to zero instead of leaving it uninitialized.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandle.h:99
> + Deque<std::unique_ptr<SocketData>> m_sendData;
> + Deque<std::unique_ptr<SocketData>> m_receiveData;</span >
These should be Deque<SocketData> instead of Deque<std::unique_ptr<SocketData>). There’s no need for all the extra heap allocation.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:51
> + startThread();</span >
Should probably ASSERT(isMainThread()) here.
But also, why do we need to start the thread? I don’t think we do.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:57
> + stopThread();</span >
Should just be ASSERT(!m_workerThread). The handle is ref'd as long as the thread is going.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:58
> + setClient(nullptr);</span >
I don’t understand the value of doing this. The setClient function has no side effects, and once the handle is deleted the null pointer can be overwritten by other random values. I don’t think this class needs a destructor at all, although perhaps the logging and assertions would be OK.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:70
> + std::unique_ptr<const char[]> copy(new const char[length]);
> + memcpy(const_cast<char*>(copy.get()), data, length);</span >
Should be able to allocate a std::unique_ptr<char[]> and then pass it to a function expecting a std::unique_ptr<const char[]> and have it be converted. We’d avoid a const_cast that way.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:124
> + std::lock_guard<std::mutex> lock(m_mutexSend);</span >
Do we really need to lock during the actual send? This seems like a really long time to hold a lock.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:144
> + std::unique_ptr<const char[]> copy(new const char[restLength]);
> + memcpy(const_cast<char*>(copy.get()), sendData->data() + totalBytesSent, restLength);
> + m_sendData.prepend(std::unique_ptr<SocketData>(new SocketData { WTF::move(copy), restLength } ));</span >
Would be nice to share this code with platformSend.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:158
> + timeout.tv_sec = 0;
> + timeout.tv_usec = std::chrono::duration_cast<std::chrono::microseconds>(selectTimeout).count();</span >
This can do something crazy thing if selectTimeout is 1 second or more or if it’s negative.
<a href="http://stackoverflow.com/questions/17402657/how-to-convert-stdchronosystem-clockduration-into-struct-timeval">http://stackoverflow.com/questions/17402657/how-to-convert-stdchronosystem-clockduration-into-struct-timeval</a>
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:188
> + CString host = m_url.host().utf8();
> + curl_easy_setopt(curlHandle, CURLOPT_URL, host.data());</span >
No need for the local variable here.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:200
> + if (refCount() > 1)
> + didOpenSocket();</span >
This check of refCount is peculiar. Is this an important optimization?
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:217
> +void SocketStreamHandle::stopThread()</span >
This function needs an ASSERT(isMainThread())</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>