[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

--- Comment #12 from peavo at outlook.com ---
(In reply to comment #10)
> Comment on attachment 252729 [details]
> Patch
> 

Thank you for your patience :)

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

winsock2.h is needed before curl.h is included, so I kept this.

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

startThread() seems to be needed here, many tests timed out without it.

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

MSVC didn't want to convert this, I got the error:

error C2664: 'std::unique_ptr<const char [],std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : 
cannot convert argument 1 from 'std::unique_ptr<char [],std::default_delete<_Ty>>' to 'std::unique_ptr<const char [],std::default_delete<_Ty>> &&'


> 
> > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:200
> > +            if (refCount() > 1)
> > +                didOpenSocket();
> 
> This check of refCount is peculiar. Is this an important optimization?
> 

I'm also not too happy with this :) I added it to fix a crash where the call was invoked on the main thread after all other references were released, and the SocketStreamClient was deleted. Accessing the SocketStreamClient in didOpenSocket() then caused a crash.

I had to add a SocketData constructor since m_sendData.append(SocketData { WTF::move(copy), length }) didn't compile with MSVC. The error I got was:

error C2440 : '<function-style-cast>' : cannot convert from 'initializer-list' to 'WebCore::SocketStreamHandle::SocketData'
1>          No constructor could take the source type, or constructor overload resolution was ambiguous

I also had to add a move constructor, otherwise I got an error stating that 
Deque.h (line 422) was trying to reference the deleted unique_ptr copy constructor.

-- 
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/20150511/e425444d/attachment.html>