[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