<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 #252612 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#c7">Comment # 7</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=252612&action=diff" name="attach_252612" title="Patch">attachment 252612</a> <a href="attachment.cgi?id=252612&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=252612&action=review">https://bugs.webkit.org/attachment.cgi?id=252612&action=review</a>
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandle.h:49
> +#include <wtf/threading.h></span >
This should have a capital "t".
The headers here aren’t sorted alphabetically the way the WebKit style guide says they should be.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandle.h:87
> + class SocketData {</span >
This should just be a struct, not a class. I don’t see us getting any benefit from making it a class.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandle.h:89
> + SocketData(const char* data, int size)</span >
I think it’s too subtle that this takes ownership of the data. Making this a struct might help.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandle.h:100
> + SocketData(SocketData&& other)
> + {
> + m_data = WTF::move(other.m_data);
> + m_size = other.m_size;
> + other.m_size = 0;
> + }</span >
Is it important that the other’s size gets set to 0 when moving? If not, then I suggest we just let the compiler generate this.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandle.h:104
> + ~SocketData()
> + {
> + }</span >
No need to define this.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:69
> + char* copy = new char[length];</span >
This should go right into a std::unique_ptr when it’s allocated.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:101
> + m_receiveData.append(SocketData(data.release(), bytesRead));</span >
This shows us we have a poor design for SocketData. It’s not good to call unique_ptr.release just to put something into another unique_ptr. If SocketData was a struct we could just write:
m_receiveData.append(SocketData { WTF::move(data), bytesRead });
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:127
> + auto sendData = WTF::move(m_sendData.at(0));
> + m_sendData.remove(0);</span >
Should use takeFirst, not move/at/remove. And the fact that a Vector doesn’t have a takeFirst should make you realize that this should be a Duque, not a Vector.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:143
> + char* copy = new char[restLength];</span >
Should go right into a std::unique_ptr.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:173
> + if (!m_workerThread) {</span >
Early return would be better than nesting the entire function inside an if.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:174
> + ref();</span >
Need a comment explaining where the deref that matches this is.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:184
> + CString host = m_url.host().ascii();</span >
The String::ascii() function should not be used unless there is a guarantee that all characters are ASCII. I don’t think there is a guarantee that all characters from URL::host will be ASCII. Maybe you want latin1() or utf8()?
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:217
> + if (m_workerThread) {</span >
Early return would be better than nesting the entire function inside an if.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:218
> + m_stopThread = true;</span >
Probably needs to be an atomic bool, not just a normal bool.
<span class="quote">> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:220
> + m_workerThread = 0;</span >
nullptr</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>