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

[bugzilla-daemon at webkit.org]

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 450757: Patch

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




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

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

This is starting to head in the right direction, but needs to go further in
that direction before landing.

> Tools/TestWebKitAPI/ConnectionListenerPOSIX.cpp:302
> +TestWebKitAPI::SecurityIdentityPEM certificatePrivateKeyDefault()

HTTPServer::testCertificate() should be reused rather than adding more test
keys.

> Tools/TestWebKitAPI/Tests/WebKit/SimpleHTTPServer.cpp:231
> +TEST(HTTPServer, SimpleHTTP)

We shouldn't need to test the server itself.  The server should be used in unit
tests to test other things.

> Tools/TestWebKitAPI/cocoa/HTTPServer.h:56
> +    HTTPServer(std::initializer_list<std::pair<String, HTTPResponse>>,
Protocol = Protocol::Http, std::unique_ptr<SecurityIdentity>&& = nullptr,
std::optional<uint16_t> port = std::nullopt);

This is soooooo much closer to a good platform abstraction than previous
patches uploaded here.	This is the first one that looks remotely like anything
I would be ok with.  But I think it would be even better if you used platform
specific types for CertificateVerifier and RetainPtr<SecIdentityRef> rather
than a different constructor on different platforms.

> Tools/TestWebKitAPI/cocoa/HTTPServer.h:65
> +    URL url(const String& path = { });

I think the default path should be consistent among the different platforms.
There's also nothing Windows specific about this code, so it should be in a
shared location.

> Tools/TestWebKitAPI/cocoa/HTTPServer.h:91
> +#if PLATFORM(COCOA)
>      RetainPtr<nw_listener_t> m_listener;
> +#elif PLATFORM(WIN)
> +    ConnectionListener m_listener;
> +#endif

Excellent

> Tools/TestWebKitAPI/cocoa/HTTPServer.h:109
> +    Socket::Socket socket() const;
> +    SSL* ssl() const;

I don't think you should need to access the socket or SSL directly.  In the
Cocoa implementation, a Connection is only given to the
Function<void(Connection)> if it is valid.  Can your implementation do the
same?

> Tools/TestWebKitAPI/cocoa/HTTPServer.h:125
> +    static void receiveBytesProcess(const Connection&,
CompletionHandler<void(Vector<uint8_t>&&)>&&);

Can these just be static functions in ConnectionPOSIX.cpp rather than static
member functions?