Bug 235730 - Begin using coroutines in testing code
Summary: Begin using coroutines in testing code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-27 13:27 PST by Alex Christensen
Modified: 2022-05-19 10:40 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.02 KB, patch)
2022-01-27 13:29 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (8.74 KB, patch)
2022-05-18 13:17 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (8.88 KB, patch)
2022-05-18 13:31 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2022-01-27 13:27:16 PST
Begin using coroutines in testing code
Comment 1 Alex Christensen 2022-01-27 13:29:09 PST
Created attachment 450171 [details]
Patch
Comment 2 Darin Adler 2022-01-30 23:14:03 PST
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 3 Darin Adler 2022-02-03 04:38:43 PST
Comment on attachment 450171 [details]
Patch

review- because of the slice problem; not sure how the test works!
Comment 4 Radar WebKit Bug Importer 2022-02-03 13:28:14 PST
<rdar://problem/88452783>
Comment 5 Alex Christensen 2022-05-18 13:15:22 PDT
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.
Comment 6 Alex Christensen 2022-05-18 13:17:43 PDT
Created attachment 459553 [details]
Patch
Comment 7 Alex Christensen 2022-05-18 13:31:00 PDT
Created attachment 459554 [details]
Patch
Comment 8 EWS 2022-05-19 10:40:26 PDT
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].