[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 #252729|review?                     |review-
              Flags|                            |

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

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

lets not use this unnecessary std::unique_ptr

> 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>

These aren’t sorted in the normal WebKit project way.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:88
> +        const char* data() const { return m_data.get(); }
> +        int size() const { return m_size; }

No need for these.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:90
> +        std::unique_ptr<const char[]> m_data;

Should just name this data.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:91
> +        int m_size;

Should just name this size. I also suggest initializing it to zero instead of leaving it uninitialized.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:99
> +    Deque<std::unique_ptr<SocketData>> m_sendData;
> +    Deque<std::unique_ptr<SocketData>> m_receiveData;

These should be Deque<SocketData> instead of Deque<std::unique_ptr<SocketData>). There’s no need for all the extra heap allocation.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:51
> +    startThread();

Should probably ASSERT(isMainThread()) here.

But also, why do we need to start the thread? I don’t think we do.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:57
> +    stopThread();

Should just be ASSERT(!m_workerThread). The handle is ref'd as long as the thread is going.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:58
> +    setClient(nullptr);

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.

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

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.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:124
> +    std::lock_guard<std::mutex> lock(m_mutexSend);

Do we really need to lock during the actual send? This seems like a really long time to hold a lock.

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

Would be nice to share this code with platformSend.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:158
> +    timeout.tv_sec = 0;
> +    timeout.tv_usec = std::chrono::duration_cast<std::chrono::microseconds>(selectTimeout).count();

This can do something crazy thing if selectTimeout is 1 second or more or if it’s negative.

http://stackoverflow.com/questions/17402657/how-to-convert-stdchronosystem-clockduration-into-struct-timeval

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:188
> +        CString host = m_url.host().utf8();
> +        curl_easy_setopt(curlHandle, CURLOPT_URL, host.data());

No need for the local variable here.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:200
> +            if (refCount() > 1)
> +                didOpenSocket();

This check of refCount is peculiar. Is this an important optimization?

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:217
> +void SocketStreamHandle::stopThread()

This function needs an ASSERT(isMainThread())

-- 
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/20150510/0b9c6a0a/attachment.html>