Bug 158862

Summary: No need to ref connection in lambda inside NetworkResourceLoader::tryStoreAsCacheEntry()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, benjamin, cgarcia, cmarcelo, commit-queue, darin, koivisto
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2016-06-16 18:58:04 PDT
No need to ref connection in lambda inside NetworkResourceLoader::tryStoreAsCacheEntry(). We already ref the NetworkResourceLoader which hold a ref to the connection.
Comment 1 Chris Dumez 2016-06-16 19:08:18 PDT
Created attachment 281521 [details]
Patch
Comment 2 WebKit Commit Bot 2016-06-16 19:10:54 PDT
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 3 Darin Adler 2016-06-16 19:12:17 PDT
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.
Comment 4 Chris Dumez 2016-06-16 19:13:24 PDT
(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.
Comment 5 Chris Dumez 2016-06-16 19:37:05 PDT
Created attachment 281523 [details]
Patch
Comment 6 WebKit Commit Bot 2016-06-16 19:38:19 PDT
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.
Comment 7 Chris Dumez 2016-06-16 19:46:32 PDT
Created attachment 281524 [details]
Patch
Comment 8 WebKit Commit Bot 2016-06-16 19:49:01 PDT
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 9 Chris Dumez 2016-06-16 19:50:32 PDT
Comment on attachment 281524 [details]
Patch

Clearing flags on attachment: 281524

Committed r202154: <http://trac.webkit.org/changeset/202154>
Comment 10 Chris Dumez 2016-06-16 19:50:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Carlos Garcia Campos 2016-06-16 22:53:34 PDT
(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.
Comment 12 Antti Koivisto 2016-06-17 06:10:47 PDT
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.
Comment 13 Brady Eidson 2016-06-17 11:50:11 PDT
(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.
Comment 14 Brady Eidson 2016-06-17 11:52:15 PDT
(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"