[Webkit-unassigned] [Bug 187984] [Curl] Use shared single thread for WebSocket connections

[bugzilla-daemon at webkit.org]

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

--- Comment #18 from Takashi Komori <Takashi.Komori at sony.com> ---
(In reply to Basuke Suzuki from comment #11)
> 
> > Source/WebCore/platform/network/curl/CurlStream.h:46
> > +        virtual void didOpen() = 0;
> 
> These interface method should have caller's instance as a first argument.
> 
>     virtual void didOpen(CurlStream&) = 0;
> 
> That is similar to Cocoa's delegation pattern. Adding that to each method
> gives more context to implementation side, i.e. who will require them. Also
> it may solve future method name conflict.

CurlStreamHandleImpl's lifetime is different from CurlStream's, so it is possible that when CurlStreamHandleImple::didFail is invoked, the CurlStream instance already had been destructed.
To implement that we should consider not to access destructed instances too.


> > Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:52
> > +    auto task = [this, streamID, url = url.isolatedCopy()]() mutable {
> 
> This might be just a preference, but for me, assigning to temporal variable
> and use it in next line is not good rhythm to read. Calling
> `callOnWorkerThread()` with direct lambda is easy to read.

Fixed.

>
> > Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:70
> > +        ASSERT(!isMainThread());
> 
> These assertion should be responsibility of callOnWorkerThead() or
> executeTask() method and no need to check in every places.

Fixed.

> 
> > Source/WebCore/platform/network/curl/CurlStreamScheduler.cpp:97
> > +void CurlStreamScheduler::callOnMainThread(CurlStreamID streamID, WTF::Function<void(CurlStream::Client&)>&& task)
> 
> This is not general purpose invocation method. This function do more. How
> about callClientOnMainThread?

Fixed.

> 
> > Source/WebCore/platform/network/curl/CurlStreamScheduler.h:65
> > +    HashMap<CurlStreamID, CurlStream::Client*> m_clientLists;
> 
> The value of item cannot be nullptr, so is should be CurlStream::Client&

We can't use reference here because CurlStream::Client is abstract class as we can't get the instance by calling m_clientLists.get

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200203/5a139e3f/attachment.htm>