Bug 158887

Summary: Use WTF::NoncopyableFunction in NetworkDataTaskClient
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andersca, beidson, commit-queue
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Chris Dumez 2016-06-17 14:17:43 PDT
Use WTF::NoncopyableFunction in NetworkDataTaskClient instead of std::function and consistently move it around. This avoids some unnecessary copying.
Comment 1 Chris Dumez 2016-06-17 14:19:37 PDT
Created attachment 281587 [details]
Patch
Comment 2 Alex Christensen 2016-06-17 15:52:23 PDT
Comment on attachment 281587 [details]
Patch

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

R=me

> Source/WebKit2/Shared/Authentication/mac/AuthenticationManager.mac.mm:69
> +bool AuthenticationManager::tryUseCertificateInfoForChallenge(const AuthenticationChallenge& challenge, const CertificateInfo& certificateInfo, const ChallengeCompletionHandler& completionHandler)

It would be cool if we could make a function pointer that can only be called once and then becomes null, because that is the intent of this callback.
Instead of returning bool, we could take a ChallengeCompletionHandler&& and return nullptr if it was used and return the completion handler if it wasn't used.
Comment 3 Chris Dumez 2016-06-17 15:55:23 PDT
(In reply to comment #2)
> Comment on attachment 281587 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281587&action=review
> 
> R=me
> 
> > Source/WebKit2/Shared/Authentication/mac/AuthenticationManager.mac.mm:69
> > +bool AuthenticationManager::tryUseCertificateInfoForChallenge(const AuthenticationChallenge& challenge, const CertificateInfo& certificateInfo, const ChallengeCompletionHandler& completionHandler)
> 
> It would be cool if we could make a function pointer that can only be called
> once and then becomes null, because that is the intent of this callback.
> Instead of returning bool, we could take a ChallengeCompletionHandler&& and
> return nullptr if it was used and return the completion handler if it wasn't
> used.

We may need to do this in cases where the implementation needs to take ownership of the NoncopyableFunction in some cases. However, in this case, it does not and therefore passing as a const reference works and is more readable IMHO.

I would prefer to leave as is.
Comment 4 WebKit Commit Bot 2016-06-17 16:27:30 PDT
Comment on attachment 281587 [details]
Patch

Clearing flags on attachment: 281587

Committed r202184: <http://trac.webkit.org/changeset/202184>
Comment 5 WebKit Commit Bot 2016-06-17 16:27:35 PDT
All reviewed patches have been landed.  Closing bug.