Bug 159024 – [Soup] Clean up SocketStreamHandle soup implementation

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

While debugging bug #158613 today, I realized that we could clean up and simplify the SocketStreamHandle soup implementation.

[Carlos Garcia Campos]

Created attachment 281842 [details]
Patch

[Zan Dobersek]

Comment on attachment 281842 [details]
Patch

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

> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:223
> +    g_source_set_callback(m_writeReadySource.get(), reinterpret_cast<GSourceFunc>(writeReadyCallback), this, [](gpointer handle) {
> +        static_cast<SocketStreamHandle*>(handle)->deref();
> +    });

IMO the lambda would be more readable if its header was also placed on its own line, and all three lines being indented by 4 spaces compared to the start of this expression.

The rest of this patch adopts the leaked pointer into a RefPtr<> wrapper, this should do the same.

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 281842 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281842&action=review

Thanks for the review!

> > Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:223
> > +    g_source_set_callback(m_writeReadySource.get(), reinterpret_cast<GSourceFunc>(writeReadyCallback), this, [](gpointer handle) {
> > +        static_cast<SocketStreamHandle*>(handle)->deref();
> > +    });
> 
> IMO the lambda would be more readable if its header was also placed on its
> own line, and all three lines being indented by 4 spaces compared to the
> start of this expression.

I agree, I'll do that.

> The rest of this patch adopts the leaked pointer into a RefPtr<> wrapper,
> this should do the same.

Because in the other cases the callback is always called, even if the operation is cancelled, but in this case, if the source is destroyed before being dispatched the callback is not called and the handler leaked, that's why I'm using the destroy notify that is always called. Also note that the callback is called multiple times, because by default the source continues until it's cancelled or destroyed.

[Carlos Garcia Campos]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 281842 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=281842&action=review
> 
> Thanks for the review!
> 
> > > Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:223
> > > +    g_source_set_callback(m_writeReadySource.get(), reinterpret_cast<GSourceFunc>(writeReadyCallback), this, [](gpointer handle) {
> > > +        static_cast<SocketStreamHandle*>(handle)->deref();
> > > +    });
> > 
> > IMO the lambda would be more readable if its header was also placed on its
> > own line, and all three lines being indented by 4 spaces compared to the
> > start of this expression.
> 
> I agree, I'll do that.
> 
> > The rest of this patch adopts the leaked pointer into a RefPtr<> wrapper,
> > this should do the same.
> 
> Because in the other cases the callback is always called, even if the
> operation is cancelled, but in this case, if the source is destroyed before
> being dispatched the callback is not called and the handler leaked, that's
> why I'm using the destroy notify that is always called. Also note that the
> callback is called multiple times, because by default the source continues
> until it's cancelled or destroyed.

Sorry, I misunderstood, I thought you meant to adopt the ref in the source callback, not in the destroy notify callback. In this case I used explicit ref/deref because it's clearer than having a function that only adopts a ref and returns

[Carlos Garcia Campos]

Committed r202370: <http://trac.webkit.org/changeset/202370>