[webkit-reviews] review denied: [Bug 215379] Implement HTTPServer for API tests in c++ : [Attachment 406386] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 11 11:24:04 PDT 2020


Alex Christensen <achristensen at apple.com> has denied Takashi Komori
<takashi.komori at sony.com>'s request for review:
Bug 215379: Implement HTTPServer for API tests in c++
https://bugs.webkit.org/show_bug.cgi?id=215379

Attachment 406386: Patch

https://bugs.webkit.org/attachment.cgi?id=406386&action=review




--- Comment #3 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 406386
  --> https://bugs.webkit.org/attachment.cgi?id=406386
Patch

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

I'm happy to see interest in HTTPServer.  However, I think this needs a
slightly better direction to work well with a multi-platform project like this.
TCPServer right now is too picky.  It uses std::threads for listening and
sending/receiving on a connection.  If the exact request/response pairs are not
perfect, it hangs.  That's why I wrote HTTPServer, which currently uses Apple's
network framework, which isn't available on other platforms.  I think a good
approach would be to abstract out nw_connection_t and other platform-specific
parts of HTTPServer, make an implementation that uses select and no std::thread
(just the main run loop), and leave TCPServer exactly how it is (except maybe
taking shared pieces from it like the test certificates).  I'm working to move
the TCPServer tests to HTTPServer, and having a non-Apple implementation would
help.

> Tools/TestWebKitAPI/TCPServer.cpp:238
> +		   auto rc = select(listeningSocket + 1, &readfd, nullptr,
nullptr, &timeout);

To be consistent, I think this should be ::select

> Tools/TestWebKitAPI/curl/HTTPServer.cpp:106
> +template<typename T>  void HTTPServer::respondToRequests(T connectionHandle)

This is mostly duplicate code.

> Tools/TestWebKitAPI/curl/HTTPServer.h:37
> +class HTTPServer {

There should be one class HTTPServer, and its platform parts should be
abstracted instead of this.

> Tools/TestWebKitAPI/curl/HTTPServer.h:56
> +struct HTTPServer::HTTPResponse {

This is also duplicate code.


More information about the webkit-reviews mailing list