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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue May 5 09:04:39 PDT 2015


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

Darin Adler <darin at apple.com> changed:

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

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

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

Code looks OK, but lots of room for improvement.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:57
> +class SocketStreamHandle : public RefCounted<SocketStreamHandle>, public SocketStreamHandleBase {

Are we guaranteed that all the ref/deref are on the same thread? I am concerned that is not so.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:59
> +    static PassRefPtr<SocketStreamHandle> create(const URL& url, SocketStreamHandleClient* client) { return adoptRef(new SocketStreamHandle(url, client)); }

Should return Ref, not PassRefPtr.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:63
> +protected:

A class that is never used as a base class should not have any protected members at all. These should all be private, not protected.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:69
> +    bool readData(CURL* curlHandle);
> +    bool sendData(CURL* curlHandle);
> +    bool waitForAvailableData(CURL* curlHandle, int selectTimeoutMS);

These CURL* don’t need argument names in the header, and also, should be CURL&.

The selectTimeoutMS is an old fashioned choice and not what we should do in new code. A better type for the argument would be std::chrono::microseconds or perhaps std::chrono::milliseconds. Either way we would avoid all the comments about units and actually get compile time checking for it.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:72
> +    static void worker(void*);

This should be a lambda inside the member function that needs it, not a static member function.

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

This seems like a peculiar class. Maybe it should just be a struct instead of a class.

> Source/WebCore/platform/network/curl/SocketStreamHandle.h:106
> +        const char* m_data;

This needs to be a std::unique_ptr<const char[]>; doing all the new and delete by hand is not a good idea.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:57
>      setClient(0);

nullptr

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:100
> +    char* data = new char[bufferSize];

Should use unique_ptr, not manual new/delete.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:108
> +        callOnMainThread(didReceiveData, this);

It would be better to use a lambda here rather than the mess with a static data member and a case of the data pointer inside it. If we had a thread safe reference count then we could use have the lambda capture a reference count to the handle, and then we would not need to use cancelCallOnMainThread in the destructor.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:222
> +    SocketStreamHandle* handle = reinterpret_cast<SocketStreamHandle*>(context);

This should be static_cast, not reinterpret_cast.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:233
> +    auto receiveData = m_receiveData;
> +    m_receiveData.clear();

This should use WTF::move rather than copy and clear. Much more efficient and required if we change SocketData to read-only, which we should do.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:237
> +    for (auto socketData : receiveData) {

This needs to be auto& to avoid copying the socket data.

> Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:252
> +    SocketStreamHandle* handle = reinterpret_cast<SocketStreamHandle*>(context);

This should be static_cast instead.

-- 
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/20150505/2ad1021b/attachment-0001.html>


More information about the webkit-unassigned mailing list