[webkit-reviews] review denied: [Bug 215379] Implement HTTPServer for API tests in c++ : [Attachment 452165] 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 452165: Patch

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




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

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

> Tools/TestWebKitAPI/ConnectionListener.h:102
> +    ssl::unique_ptr<EVP_PKEY> m_privateKey { nullptr };
> +    ssl::unique_ptr<X509> m_x509 { nullptr };

{ nullptr } unnecessary

> Tools/TestWebKitAPI/ConnectionListener.h:131
> +    ConnectionEntity(Socket::Socket socket, SSL* ssl)

ssl::unique_ptr<SSL>

> Tools/TestWebKitAPI/ConnectionListener.h:142
> +    SSL* m_ssl { nullptr };

ssl::unique_ptr<SSL> that calls SSL_free for you.  That'll be safer.

> Tools/TestWebKitAPI/ConnectionListener.h:166
> +    uint16_t m_port;

{ 0 };
uint16_t does not have a default constructor, so this is currently
uninitialized memory.

> Tools/TestWebKitAPI/ConnectionListenerPOSIX.cpp:184
> +static std::optional< std::pair<Socket::Socket, uint16_t> >
createListeningSocket(uint16_t targetPort)

unnecessary spaces

> Tools/TestWebKitAPI/HTTPServer.cpp:28
> +#include "cocoa/HTTPServer.h"

We should probably move HTTPServer from cocoa to a non-platform-specific
directory.

> Tools/TestWebKitAPI/HTTPServer.cpp:37
> +struct HTTPServer::RequestData : public
ThreadSafeRefCounted<HTTPServer::RequestData, 
WTF::DestructionThread::MainRunLoop> {

basically duplicate code

> Tools/TestWebKitAPI/HTTPServer.cpp:188
> +void HTTPServer::respondWithOK(Connection connection)

duplicate code.

> Tools/TestWebKitAPI/HTTPServer.cpp:221
> +static String statusText(unsigned statusCode)

duplicate code.  This already exists as "static ASCIILiteral statusText". 
Please move that code to a shared location instead of adding another copy of
the same thing.

> Tools/TestWebKitAPI/HTTPServer.cpp:252
> +Vector<uint8_t> HTTPResponse::serialize(IncludeContentLength
includeContentLength) const

This is still 100% duplicate code.  We don't want to have multiple copies of
the same thing.  Instead of doing this, move the existing code to a shared
location.
Please go through this whole patch, compare with the existing HTTPServer
implementation, and move things to a shared location instead of adding another
copy.

> Tools/TestWebKitAPI/HTTPServer.cpp:321
> +    "MIIFgDCCA2gCCQCKHiPRU5MQuDANBgkqhkiG9w0BAQsFADCBgTELMAkGA1UEBhMC"

The point wasn't to copy the existing keys.  The point was to move the existing
keys so that we don't have multiple copies of the same thing.

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

Instead of adding a different constructor, please reuse the same constructor
and make platform abstractions for the types that are currently platform
specific.

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

Why is the default path empty here but "/" on the request functions below?  I
think they should all be the same.

> Tools/TestWebKitAPI/cocoa/HTTPServer.h:118
> +    RefPtr<ConnectionEntity> m_connection { nullptr };

{ nullptr } is unnecessary here because RefPtr already has a default
constructor.

> Tools/TestWebKitAPI/win/PlatformSocketWin.cpp:32
> +namespace TestWebKitAPI {
> +namespace Socket {

nit: namespace TestWebKitAPI::Socket