Bug 197641 – Add a release assertion that Functions can only be constructed from non-null CompletionHandlers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Add a release assertion that Functions can only be constructed from non-null CompletionHandlers

[Alex Christensen]

Created attachment 369213 [details]
Patch

[EWS Watchlist]

Comment on attachment 369213 [details]
Patch

Attachment 369213 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12122984

New failing tests:
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html

[EWS Watchlist]

Created attachment 369280 [details]
Archive of layout-test-results from ews210 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Chris Dumez]

Comment on attachment 369213 [details]
Patch

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

> Source/WTF/wtf/Function.h:33
> +namespace Detail {

Why is this better than a private class in the Function scope? I preferred the old way as it was hiding the implementation detail. Now it's public and anyone could use it directly in theory.

[Alex Christensen]

Private classes inside template classes seem to be unspecializable.  I only get these:
error: cannot specialize a member of an unspecialized template
This is what Detail namespaces are for.

[Chris Dumez]

Comment on attachment 369213 [details]
Patch

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

> Source/WTF/wtf/CompletionHandler.h:68
> +template<typename Out, typename... In>

Should we add a FIXME to indicate this is temporary? I don't this this would stay a RELEASE_ASSERT() forever and it seems like a lot of extra code for a debug ASSERT().

[Alex Christensen]

I'm not convinced this is temporary.  We should always be unable to make a non-null Function that when called will always dereference null.
Also, we could use this same technique to make a CallableWrapperStorage class that is usually a unique_ptr but is just a CompletionHandler for the Function-wrapping-CompletionHandler case.  We should also make Function more const correct by making operator() const if the lambda is not mutable and non-const for mutable lambdas.  There's work to be done here.

[Alex Christensen]

http://trac.webkit.org/r245024

[Radar WebKit Bug Importer]

<rdar://problem/50551693>