While debugging bug #158613 today, I realized that we could clean up and simplify the SocketStreamHandle soup implementation.
Created attachment 281842 [details] Patch
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.
(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.
(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
Committed r202370: <http://trac.webkit.org/changeset/202370>