Bug 158887 - Use WTF::NoncopyableFunction in NetworkDataTaskClient
Summary: Use WTF::NoncopyableFunction in NetworkDataTaskClient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-17 14:17 PDT by Chris Dumez
Modified: 2016-06-17 16:27 PDT (History)
4 users (show)

See Also:


Attachments
Patch (30.39 KB, patch)
2016-06-17 14:19 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.