No need to ref connection in lambda inside NetworkResourceLoader::tryStoreAsCacheEntry(). We already ref the NetworkResourceLoader which hold a ref to the connection.
Created attachment 281521 [details] Patch
Attachment 281521 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:385: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:109: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 281521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281521&action=review > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:518 > - // Keep the connection alive. > - RefPtr<NetworkConnectionToWebProcess> connection(&connectionToWebProcess()); > - RefPtr<NetworkResourceLoader> loader(this); > - NetworkCache::singleton().store(m_networkLoad->currentRequest(), m_response, WTFMove(m_bufferedDataForCache), [loader, connection](auto& mappedBody) { > + NetworkCache::singleton().store(m_networkLoad->currentRequest(), m_response, WTFMove(m_bufferedDataForCache), [this, loader = Ref<NetworkResourceLoader>(*this)](auto& mappedBody) { I have mixed feelings about this pattern with a lambda that captures both "this" and a named strong reference to this that refs/derefs. I would be tempted to just not capture this, the way the old code did. I think it makes the lifetime clearer.
(In reply to comment #3) > Comment on attachment 281521 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281521&action=review > > > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:518 > > - // Keep the connection alive. > > - RefPtr<NetworkConnectionToWebProcess> connection(&connectionToWebProcess()); > > - RefPtr<NetworkResourceLoader> loader(this); > > - NetworkCache::singleton().store(m_networkLoad->currentRequest(), m_response, WTFMove(m_bufferedDataForCache), [loader, connection](auto& mappedBody) { > > + NetworkCache::singleton().store(m_networkLoad->currentRequest(), m_response, WTFMove(m_bufferedDataForCache), [this, loader = Ref<NetworkResourceLoader>(*this)](auto& mappedBody) { > > I have mixed feelings about this pattern with a lambda that captures both > "this" and a named strong reference to this that refs/derefs. I would be > tempted to just not capture this, the way the old code did. I think it makes > the lifetime clearer. Ok, I can keep this part of the change out. I think I have seen Brady do it so I was trying to match.
Created attachment 281523 [details] Patch
Attachment 281523 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:385: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:109: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 281524 [details] Patch
Attachment 281524 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:385: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:109: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 281524 [details] Patch Clearing flags on attachment: 281524 Committed r202154: <http://trac.webkit.org/changeset/202154>
All reviewed patches have been landed. Closing bug.
(In reply to comment #3) > Comment on attachment 281521 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281521&action=review > > > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:518 > > - // Keep the connection alive. > > - RefPtr<NetworkConnectionToWebProcess> connection(&connectionToWebProcess()); > > - RefPtr<NetworkResourceLoader> loader(this); > > - NetworkCache::singleton().store(m_networkLoad->currentRequest(), m_response, WTFMove(m_bufferedDataForCache), [loader, connection](auto& mappedBody) { > > + NetworkCache::singleton().store(m_networkLoad->currentRequest(), m_response, WTFMove(m_bufferedDataForCache), [this, loader = Ref<NetworkResourceLoader>(*this)](auto& mappedBody) { > > I have mixed feelings about this pattern with a lambda that captures both > "this" and a named strong reference to this that refs/derefs. I would be > tempted to just not capture this, the way the old code did. I think it makes > the lifetime clearer. I think capturing both is a good idea when the body of the lambda is not just a few lines, the code looks cleaner to me than using protectedThis. all the time. Maybe if we document it, and enforce that the pattern should always be [this, protectedThis = Ref<Foo>(*this)...] to make it more obvious that we are capturing both just to use this implicitly and ensure its lifetime.
If the lambda outlives the method scope I think it significantly clearer to access the object via informatively named strong reference (not called protectedThis!) rather than use the double capture trick.
(In reply to comment #12) > If the lambda outlives the method scope I think it significantly clearer to > access the object via informatively named strong reference (not called > protectedThis!) rather than use the double capture trick. Every lambda I've written I've double captured. Whether or not the lambda outlives method scope, the style consideration is about how the lambda reads to a human, and it is always obvious when you're reading what "this" is. Indeed, one main reason in my push for "protectedThis" was based on the notion that you never actually use that variable in a lambda because you should be capturing this. So, while this is obviously subjective, I disagree completely.
(In reply to comment #13) > Whether or not the lambda outlives method scope, the style consideration is > about how the lambda reads to a human, and it is always obvious when you're > reading what "this" is. To be explicit, as I left it unsaid - When you're reading a lambda inside a method scope, you're automatically reading that code as if it were in that method scope. In fact, when you have an explicit non-this variable inside the lambda, I think it reads *worse*, because you're making a jump from "I'm reading a standard a member function" to "I'm reading something else"