Add SocketStream implementation for libcurl backend, using POSIX function calls.
This patch is working on the Nix port, but
This patch works fine on Nix, but I couldn't try it out on EFL. Could you test it, peavo? For example, on http://websocketstest.com/ or on http://www.websocket.org/echo.html it should work fine, except the SSL/TSL support, which might not behave correctly yet.
(In reply to comment #2) > This patch works fine on Nix, but I couldn't try it out on EFL. > Could you test it, peavo? > > For example, on http://websocketstest.com/ or on http://www.websocket.org/echo.html it should work fine, except the SSL/TSL support, which might not behave correctly yet. I will test it, but I cannot find the patch.
Created attachment 224191 [details] Proposed patch Sorry, somehow I forgot to upload it.
Attachment 224191 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:86: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:88: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:92: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:132: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:76: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:79: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:98: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:105: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:119: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:119: Use 0 instead of NULL. [readability/null] [5] Total errors found: 11 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
There seems to be some include errors compiling WinCairo: 1>..\platform\network\curl\SocketStreamHandleCurl.cpp(43): fatal error C1083: Cannot open include file: 'arpa/inet.h': No such file or directory The same goes for <netdb.h>, and <sys/socket.h> 1>..\platform\network\curl\SocketStreamHandleManager.cpp(34): fatal error C1083: Cannot open include file: 'sys/socket.h': No such file or directory
(In reply to comment #6) > There seems to be some include errors compiling WinCairo: > > 1>..\platform\network\curl\SocketStreamHandleCurl.cpp(43): fatal error C1083: Cannot open include file: 'arpa/inet.h': No such file or directory > > The same goes for <netdb.h>, and <sys/socket.h> > > 1>..\platform\network\curl\SocketStreamHandleManager.cpp(34): fatal error C1083: Cannot open include file: 'sys/socket.h': No such file or directory maybe we need the windows.h and winsock2.h includes on windows.
Created attachment 225069 [details] Proposed patch 2 I've added the Windows headers, this might fix the problem.
Attachment 225069 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.h:37: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:91: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:93: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:97: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:137: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:82: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:85: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:104: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:111: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:125: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:125: Use 0 instead of NULL. [readability/null] [5] Total errors found: 11 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 225069 [details] Proposed patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=225069&action=review > Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:3 > + * Copyright (C) 2014 University of Szeged > + * All rights reserved. Just a nit: Please use same copyright in all files - Copyright (C) 2014 University of Szeged. All rights reserved.
Created attachment 225767 [details] Proposed patch 3 I've updated the ChangeLog and fixed the license problem. Peavo, did the Windows includes fix the build errors?
Attachment 225767 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:136: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:82: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:85: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:125: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:125: Use 0 instead of NULL. [readability/null] [5] Total errors found: 6 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #11) > Created an attachment (id=225767) [details] > Proposed patch 3 > > I've updated the ChangeLog and fixed the license problem. > Peavo, did the Windows includes fix the build errors? Maybe include <Ws2tcpip.h> ? 1> SocketStreamHandleCurl.cpp 1>..\platform\network\curl\SocketStreamHandleCurl.cpp(75): error C2039: 'getaddrinfo' : is not a member of '`global namespace'' 1>..\platform\network\curl\SocketStreamHandleCurl.cpp(75): error C3861: 'getaddrinfo': identifier not found 1>..\platform\network\curl\SocketStreamHandleCurl.cpp(85): error C2039: 'getnameinfo' : is not a member of '`global namespace'' 1>..\platform\network\curl\SocketStreamHandleCurl.cpp(85): error C3861: 'getnameinfo': identifier not found 1>..\platform\network\curl\SocketStreamHandleCurl.cpp(104): error C2039: 'close' : is not a member of '`global namespace'' 1>..\platform\network\curl\SocketStreamHandleCurl.cpp(104): error C3861: 'close': identifier not found 1>..\platform\network\curl\SocketStreamHandleCurl.cpp(130): error C3861: 'freeaddrinfo': identifier not found 1> SocketStreamHandleManager.cpp 1>..\platform\network\curl\SocketStreamHandleManager.cpp(34): fatal error C1083: Cannot open include file: 'sys/socket.h': No such file or directory
That's odd, 'sys/socket.h' shouldn't be included on Windows. There are guards in order to use its Windows equivalents.
(In reply to comment #14) > That's odd, 'sys/socket.h' shouldn't be included on Windows. > There are guards in order to use its Windows equivalents. My mistake, somehow I didn't manage to completely apply the patch :) 1>..\platform\network\curl\SocketStreamHandleManager.cpp(90): error C2039: 'close' : is not a member of '`global namespace'' 1>..\platform\network\curl\SocketStreamHandleManager.cpp(90): error C3861: 'close': identifier not found 1>..\platform\network\curl\SocketStreamHandleManager.cpp(102): error C2065: 'ssize_t' : undeclared identifier 1>..\platform\network\curl\SocketStreamHandleManager.cpp(102): error C2146: syntax error : missing ';' before identifier 'readCount' 1>..\platform\network\curl\SocketStreamHandleManager.cpp(102): error C2065: 'readCount' : undeclared identifier 1>..\platform\network\curl\SocketStreamHandleManager.cpp(102): error C2039: 'read' : is not a member of '`global namespace'' 1>..\platform\network\curl\SocketStreamHandleManager.cpp(102): error C3861: 'read': identifier not found 1>..\platform\network\curl\SocketStreamHandleManager.cpp(104): error C2065: 'readCount' : undeclared identifier 1>..\platform\network\curl\SocketStreamHandleManager.cpp(110): error C2065: 'readCount' : undeclared identifier 1>..\platform\network\curl\SocketStreamHandleManager.cpp(111): error C2065: 'readCount' : undeclared identifier 1>..\platform\network\curl\SocketStreamHandleManager.cpp(114): error C2065: 'readCount' : undeclared identifier 1>..\platform\network\curl\SocketStreamHandleManager.cpp(126): error C2065: 'ssize_t' : undeclared identifier 1>..\platform\network\curl\SocketStreamHandleManager.cpp(126): error C2146: syntax error : missing ';' before identifier 'sent' 1>..\platform\network\curl\SocketStreamHandleManager.cpp(126): error C2065: 'sent' : undeclared identifier 1>..\platform\network\curl\SocketStreamHandleManager.cpp(126): error C2039: 'write' : is not a member of '`global namespace'' 1>..\platform\network\curl\SocketStreamHandleManager.cpp(126): error C3861: 'write': identifier not found 1>..\platform\network\curl\SocketStreamHandleManager.cpp(128): error C2065: 'sent' : undeclared identifier 1>..\platform\network\curl\SocketStreamHandleManager.cpp(134): error C2065: 'sent' : undeclared identifier 1>..\platform\network\curl\SocketStreamHandleManager.cpp(135): error C2065: 'sent' : undeclared identifier
Created attachment 227640 [details] Proposed patch 4 Sorry for the late reply, this patch should solve the problems.
Attachment 227640 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:145: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:84: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:87: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:127: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:127: Use 0 instead of NULL. [readability/null] [5] Total errors found: 6 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 227640 [details] Proposed patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=227640&action=review Thanks for working on this! I see a few memory leaks in the code, so I think it needs a bit of work before we proceed. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:65 > + memset(&hints, 0, sizeof(struct addrinfo)); Can't this just be sizeof(hints)? Less likely to break in the future if the type of hints is changed. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:75 > + const char* port = fastStrDup(portString.latin1().data()); I don't understand why we need to copy these strings. Are we concerned hostnameString and portString won't be around long enough to make the function call? This is also leaking memory. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:79 > + return nullptr; We just leaked hostname and port here. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:95 > +static int tryConnection(struct addrinfo* server) Is it ever appropriate for 'server' to be null? If it is, you should check for it and handle it. If not, make it a reference. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:113 > +void SocketStreamHandle::dispatchConnectionOnMainThread(void* context) Is it ever possible for context to be null? If not, make it a reference. If it is, handle that case. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:127 > + for (; server != NULL; server = server->ai_next) { We don't use NULL in our code. This should be "server != nullptr" > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:128 > + if ((socketFD = tryConnection(server))) server cannot be null, so why not just pass a const reference? > Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:62 > + DEFINE_STATIC_LOCAL(Mutex, mutex, ()); This is deprecated nwo (DEFINE_DEPRECATED_STATIC_LOCAL). You might want to use the "NeverDeleted<>" template. > Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:73 > + for (auto entry : m_streamHandleMap) { You are copying your map entries as you iterate. You probably want "for (auto& entry : m_streamHandleMap) > Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:82 > +void SocketStreamHandleManager::add(SocketStreamHandle* stream) These methods all take a pointer, which implies you need to check for nullptr. But I don't think you ever enter this code with a nullptr, so why not just use references? > Source/WebCore/platform/network/curl/SocketStreamHandleManager.cpp:84 > + Mutex* mutex = SocketStreamHandleManager::sharedMutex(); It seems weird to return a pointer to the mutex. Why not just have an API like "SocketStreamHandleManager::sharedMutex().lock() ?
This was implemented on WinCairo curl backend.