Bug 215379 – Implement HTTPServer for API tests in c++

• Component: Platform
• Status: NEW

[Takashi Komori]

For testing Curl port we should have a http server which works for API tests, but we don't have.
Mac port has http server(cocoa/HTTPServer.mm), but we can not use it on wincairo.
So we have to make new HTTPServer in c++

[Takashi Komori]

Created attachment 406386 [details]
Patch

[Takashi Komori]

HTTPServer.cpp is implemented by TCPServer.cpp, so we can use this new server on other ports by tweaking TCPServer.
And the interface of HTTPServer.cpp is not completely same as mac port's (HTTPServer.mm)  but is designed to resemble.
We might be able to utilize the new server as common server for API testing.

[Alex Christensen]

Comment on attachment 406386 [details]
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.

[Radar WebKit Bug Importer]

<rdar://problem/67331266>

[Takashi Komori]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 406386 [details]
> 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.

I agree TCPServer is not good base for the test server.
We started to re-design new server for API test.

Thank you for your review and comment!

[Takashi Komori]

Created attachment 409506 [details]
Patch

[Takashi Komori]

In this patch we tried to abstract the concept of connection (Connection.h), and made HTTPServerPOSIX.
The Connection just has interfaces send(), receiveBytes(), terminate() and something and it is kind of wrapper of unix socket but it can handle tls connection.
We implemented this in ConnectionPOSIX.cpp for win port by using winsock2.h, if we use socket we can easily implement for other ports.

Actually, we just re-wrote the combination of TCPServer and PlatformSOcket in the patch before, so our new HTTPServerPOSIX doesn't have enough functions which HTTPSver.mm has.
But we can add those functionality which are implemented by nw_connection_t into HTTPServerPOSIX.cpp, so this patch is good starting point I think.

What do you think?

[Alex Christensen]

Comment on attachment 409506 [details]
Patch

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

This is a little better in that it doesn't directly use TCPServer, but it's still not a well-abstracted multi-platform server.  I was thinking something like this:

class Connection {
    ...
private:
#if PLATFORM(COCOA)
    RetainPtr<nw_connection_t> m_connection;
#else
    int socket { 0 };
    uint16_t m_port { 0 };
    std::unique_ptr<SSL, deleter<SSL>> m_ssl;
#endif
};

etc.

> Tools/TestWebKitAPI/HTTPServerPOSIX.h:68
> +struct HTTPServerPOSIX::HTTPResponse {

This is an entirely duplicate class.  We should move things around to reuse the same class.

[Alex Christensen]

Also, using multiple threads was one of the mistakes of TCPServer.  We should use the main run loop if we can.

[Takashi Komori]

(In reply to Alex Christensen from comment #9)
> Also, using multiple threads was one of the mistakes of TCPServer.  We
> should use the main run loop if we can.

I think we can remove multiple threads from Connection for WinCairo.
We will fix that, and rethink about abstraction.

[Takashi Komori]

Created attachment 436269 [details]
Patch

[Takashi Komori]

(In reply to Alex Christensen from comment #9)
> Also, using multiple threads was one of the mistakes of TCPServer.  We
> should use the main run loop if we can.

We made a http server single thread version using run loop.

[Takashi Komori]

Created attachment 436271 [details]
Patch

[Alex Christensen]

Comment on attachment 436271 [details]
Patch

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

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:37
> +static void initializeSSLLibrary()
> +{
> +#if HAVE(SSL)

I think HAVE(SSL) can go outside the declaration of initializeSSLLibrary, or we would have an unused function if we don't have SSL.

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:71
> +        X509_free(x509);

It would be nicer to use a smart pointer like I did with ssl::unique_ptr in TCPServer.cpp

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:128
> +        while (sentTotal < message.utf8().length()) {

I feel like each write call should be on a separate run loop iteration so that long messages can be read on the same run loop.  That also makes the CompletionHandler useful.

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:169
> +        completionHandler(WTFMove(result));

If the bytes aren't available immediately, this will be called with an empty Vector.  I think the intent was to call the completion handler whenever bytes are received and wait until they are.

> Tools/TestWebKitAPI/GenericHTTPServer.cpp:73
> +    result.append("Content-Length:");

This is basically duplicate code with HTTPResponse::serialize.  They should ideally be refactored to share common code.  A lot of this patch is duplicate code.

[Takashi Komori]

Created attachment 436989 [details]
Patch

[Takashi Komori]

(In reply to Alex Christensen from comment #14)
> Comment on attachment 436271 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436271&action=review
> 
> > Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:37
> > +static void initializeSSLLibrary()
> > +{
> > +#if HAVE(SSL)
> 
> I think HAVE(SSL) can go outside the declaration of initializeSSLLibrary, or
> we would have an unused function if we don't have SSL.
> 

Moved.

> > Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:71
> > +        X509_free(x509);
> 
> It would be nicer to use a smart pointer like I did with ssl::unique_ptr in
> TCPServer.cpp
> 

Changed using smart pointer.

> > Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:128
> > +        while (sentTotal < message.utf8().length()) {
> 
> I feel like each write call should be on a separate run loop iteration so
> that long messages can be read on the same run loop.  That also makes the
> CompletionHandler useful.
> 
> > Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:169
> > +        completionHandler(WTFMove(result));
> 
> If the bytes aren't available immediately, this will be called with an empty
> Vector.  I think the intent was to call the completion handler whenever
> bytes are received and wait until they are.
> 

Fixed send/receive routines. Now the routines don't make other tasks wait when a big chunk coming.
And the receive routine call the completion handler immediately if bytes coming.

> > Tools/TestWebKitAPI/GenericHTTPServer.cpp:73
> > +    result.append("Content-Length:");
> 
> This is basically duplicate code with HTTPResponse::serialize.  They should
> ideally be refactored to share common code.  A lot of this patch is
> duplicate code.

Changed HTTPResponse to GenericHTTPResponse and removed duplicated code.

[Fujii Hironori]

Comment on attachment 436989 [details]
Patch

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

> Tools/ChangeLog:9
> +            GenericHTTPServer: The server. It uses Connection which is abstracted socket.

Connection → GenericConnection ?

> Tools/Scripts/webkitpy/port/win.py:201
> +    API_TEST_BINARY_NAMES = ['TestWTF.exe', 'TestWebCore.exe', 'TestWebKitLegacy.exe', 'TestWebKit.exe']

AppleWin doesn't enable WebKit2. Is this OK for AppleWin? What will happen for the build check?

> Tools/TestWebKitAPI/GenericConnection.h:121
> +

Remove this blank line.

> Tools/TestWebKitAPI/GenericConnection.h:125
> +

Ditto.

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:53
> +    auto certificate = WTF::makeUnique<GenericCertificate>();

Remove "WTF::".

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:67
> +std::unique_ptr<GenericPrivateKey> GenericPrivateKey::createFromPEM(const String pem, const KeyType, const int)

What is this "const int" argument?

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:71
> +    auto privateKey =  WTF::makeUnique<GenericPrivateKey>();

Remove "WTF::".

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:87
> +    const int timeoutInUsec = 1*1000; // 1 millisecond

You need 1 millisecond? Did you try 0 second?
I think you should put whitespaces around '*' like "1 * 1000". I don't think you need the variable timeoutInUsec.

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:104
> +    auto rc = ::select(fdMax + 1,

How about using poll instead of select? poll is modern than select.

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:106
> +        Socket::isValidSocket(writeSocket) ? &fdsetWrite : nullptr,

I think you can simply this code by always initializing fdsetRead and fdsetWrite with FD_ZERO.

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:123
> +GenericConnection::GenericConnection()

You should remove if you don't need this.

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:127
> +GenericConnection::~GenericConnection()

Ditto.

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:161
> +    WTF::RunLoop::main().dispatchAfter(

Remove "WTF::".

> Tools/TestWebKitAPI/GenericConnectionPOSIX.cpp:203
> +    Vector<uint8_t> result;

Allocate a Vector and shrink. Remove 'buffer'.

Vector<uint8_t> result(bufferSize);
...
result.shrink(bytesRead);

> Tools/TestWebKitAPI/cocoa/HTTPServer.mm:3
> + bGen *

Remove " bGen".

[Alex Christensen]

Comment on attachment 436989 [details]
Patch

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

> Tools/TestWebKitAPI/GenericConnection.h:97
> +class GenericConnection {

This is still not the unified code that should go in the repo.  In order to be properly abstracted, this should completely replace class Connection in HTTPServer.h and have the same interface.

> Tools/TestWebKitAPI/GenericHTTPServer.h:43
> +class GenericHTTPServer {

When this is properly abstracted, this should completely replace class HTTPServer in HTTPServer.h and have the same interface, except the function that returns a NSURLRequest is platform-specific.  I designed those classes to be able to be implemented on other platforms as they are.
We also don't need "Generic" prefix on all the class names.

> Tools/TestWebKitAPI/cocoa/GenericConnectionCocoa.mm:113
> +#if 0

:(

[Takashi Komori]

Created attachment 450596 [details]
Patch

[Takashi Komori]

We implemented HTTPServer and Connection classes as cocoa/HTTPServer.h declares.

[Alex Christensen]

Comment on attachment 450596 [details]
Patch

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

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

This still has duplicate code instead of nice abstractions that have platform-specific implementations.

[Takashi Komori]

Created attachment 450757 [details]
Patch

[Takashi Komori]

(In reply to Alex Christensen from comment #21)
> Comment on attachment 450596 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450596&action=review
> 
> > Tools/TestWebKitAPI/HTTPServer.cpp:253
> > +Vector<uint8_t> HTTPResponse::serialize(IncludeContentLength includeContentLength) const
> 
> This still has duplicate code instead of nice abstractions that have
> platform-specific implementations.

Because HTTPResponse class is platform independent, I think the best way is sharing the HTTPResponse::serialize in HTTPServer.mm
However, wincairo port can't use HTTPServer.mm, so in the latest patch we copied the function from HTTPServer.mm to HTTPServer.cpp
I'm not sure the copy makes sense or not for fixing duplication in the comment, because it produced complete duplicated code.

Or, does this comment mean that we should fix whole implementation of HTTPResponse class and share code between mac and wincairo ports somehow?

[Alex Christensen]

Comment on attachment 450757 [details]
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?

[Takashi Komori]

Created attachment 452165 [details]
Patch

[Takashi Komori]

(In reply to Alex Christensen from comment #24)
> Comment on attachment 450757 [details]
> 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.

Copied certificates and private keys defined in HTTPServer.mm and Challenge.mm
 
> > 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.

Removed simple server tests.

> > 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.

Renamed to SecIdentityRef.

> > 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.

Moved to shared location.

> > 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?

Fixed.

[Alex Christensen]

Comment on attachment 452165 [details]
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

[Takashi Komori]

Created attachment 453917 [details]
Patch

[Takashi Komori]

(In reply to Alex Christensen from comment #27)
> Comment on attachment 452165 [details]
> 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.

Fixed.

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

Removed.
 
> > Tools/TestWebKitAPI/HTTPServer.cpp:28
> > +#include "cocoa/HTTPServer.h"
> 
> We should probably move HTTPServer from cocoa to a non-platform-specific
> directory.

Moved.

> 
> > 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"
>

Moved duplicated code to HTTPServerCommon.cpp

> 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.
> 

I agree reusing constructor is the best, but for me, it does not seem to be good using RetainPtr on windows port because our SecIdentityRef is not CF object.
Do we need changing constructor arguments for both platform for abstraction?
In other words, should we change the constructor argument RetainPtr<SecIdentityRef>&& to something better?

> > 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.
> 

Fixed.

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

Fixed.

[Alex Christensen]

(In reply to Takashi Komori from comment #29)
> I agree reusing constructor is the best, but for me, it does not seem to be
> good using RetainPtr on windows port because our SecIdentityRef is not CF
> object.
> Do we need changing constructor arguments for both platform for abstraction?
> In other words, should we change the constructor argument
> RetainPtr<SecIdentityRef>&& to something better?
Yes, change the constructor argument from RetainPtr to something that contains a RetainPtr on Cocoa platforms and something conceptually equivalent on non-Cocoa platforms.

[Alex Christensen]

Comment on attachment 453917 [details]
Patch

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

> Tools/TestWebKitAPI/ConnectionListener.h:99
> +class SecIdentityRef {

This isn't a great name.  It is a class that represents similar information to what a SecIdentityRef points to on Cocoa platforms.  How about just "Identity"?

> Tools/TestWebKitAPI/ConnectionListener.h:114
> +class SecurityIdentityPEM {

Do you need this AND what is currently named SecIdentityRef?  They are kind of two classes with the same function.

> Tools/TestWebKitAPI/ConnectionListener.h:132
> +    int m_bitLength { 0 };

nit: unsigned or size_t

> Tools/TestWebKitAPI/ConnectionListener.h:189
> +    bool complete { false };

I think it would be better to have parseHTTPRequest return a std::optional<HTTPRequest> than to have this in the struct and require callers to remember to check it.

> Tools/TestWebKitAPI/ConnectionPOSIX.cpp:130
> +    }

else?
We still need to call completionHandler.

> Tools/TestWebKitAPI/HTTPServer.cpp:148
> +    String pemEncodedPrivateKey(""

This should be moved instead of copied.  We don't want to introduce duplicate copies of things like this.