| Summary: | Begin using coroutines in testing code | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | darin, hi, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Alex Christensen
2022-01-27 13:27:16 PST
Created attachment 450171 [details]
Patch
Comment on attachment 450171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450171&action=review > Tools/TestWebKitAPI/cocoa/HTTPServer.h:65 > + enum class UseCoroutines : bool { Yes }; > + HTTPServer(UseCoroutines, Function<CoroutineHandle(Connection)>&&, Protocol = Protocol::Http); I don’t think we need this UseCoroutines::Yes argument. The Function argument already takes CoroutineHandle, so this should be found by normal overloading. > Tools/TestWebKitAPI/cocoa/HTTPServer.mm:53 > + for (auto& handle : coroutineHandles) > + handle.destroy(); Maybe we should wrap the coroutine handle in a type of our own that does a destroy in the handle’s destructor. Then we would not need this. Like CoroutineHandle could do this. > Tools/TestWebKitAPI/cocoa/HTTPServer.mm:452 > + return Awaitable(*this); This return statement slices the Awaitable, copying out only the suspend_never base class part of the object. I don’t think that’s what you wanted to do here. Comment on attachment 450171 [details]
Patch
review- because of the slice problem; not sure how the test works!
Comment on attachment 450171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450171&action=review >> Tools/TestWebKitAPI/cocoa/HTTPServer.h:65 >> + HTTPServer(UseCoroutines, Function<CoroutineHandle(Connection)>&&, Protocol = Protocol::Http); > > I don’t think we need this UseCoroutines::Yes argument. The Function argument already takes CoroutineHandle, so this should be found by normal overloading. The Function returns a CoroutineHandle but takes the same parameter, a Connection. We currently do need UseCoroutines because otherwise it thinks the constructor is ambiguous. Created attachment 459553 [details]
Patch
Created attachment 459554 [details]
Patch
Committed r294492 (250751@main): <https://commits.webkit.org/250751@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 459554 [details]. |