[Webkit-unassigned] [Bug 144628] [Curl] WebSocket platform part is not implemented.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=144628

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #252612|review?                     |review+
              Flags|                            |

--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 252612
  --> https://bugs.webkit.org/attachment.cgi?id=252612
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252612&action=review

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:49
> +#include <wtf/threading.h>

This should have a capital "t".

The headers here aren’t sorted alphabetically the way the WebKit style guide says they should be.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:87
> +    class SocketData {

This should just be a struct, not a class. I don’t see us getting any benefit from making it a class.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:89
> +        SocketData(const char* data, int size)

I think it’s too subtle that this takes ownership of the data. Making this a struct might help.

> 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;
> +        }

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.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:104
> +        ~SocketData()
> +        {
> +        }

No need to define this.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:69
> +    char* copy = new char[length];

This should go right into a std::unique_ptr when it’s allocated.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:101
> +        m_receiveData.append(SocketData(data.release(), bytesRead));

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 });

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:127
> +        auto sendData = WTF::move(m_sendData.at(0));
> +        m_sendData.remove(0);

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.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:143
> +            char* copy = new char[restLength];

Should go right into a std::unique_ptr.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:173
> +    if (!m_workerThread) {

Early return would be better than nesting the entire function inside an if.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:174
> +        ref();

Need a comment explaining where the deref that matches this is.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:184
> +            CString host = m_url.host().ascii();

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()?

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:217
> +    if (m_workerThread) {

Early return would be better than nesting the entire function inside an if.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:218
> +        m_stopThread = true;

Probably needs to be an atomic bool, not just a normal bool.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:220
> +        m_workerThread = 0;

nullptr

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150507/6d3e453f/attachment.html>