[webkit-reviews] review granted: [Bug 51364] Web Inspector: Remote Web Inspector - Cross Platform InspectorServer : [Attachment 80249] [PATCH] New Part 1 - Generic Server Socket to be used for a WebSocket Server

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 51364: Web Inspector: Remote Web Inspector - Cross Platform InspectorServer
https://bugs.webkit.org/show_bug.cgi?id=51364

Attachment 80249: [PATCH] New Part 1 - Generic Server Socket to be used for a
WebSocket Server
https://bugs.webkit.org/attachment.cgi?id=80249&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=80249&action=review

> Source/WebCore/ChangeLog:13
> +	   didReceiveConnection callback with a generic SocketStreamHandle.

I'm not sure if "didReceiveConnection" parses. Maybe "establish"?

> Source/WebCore/platform/network/ServerSocketHandleBase.cpp:49
> +    ASSERT(m_port);
> +    if (!m_port)
> +	   return;

We only do this when fixing super elusive bugs by adding semi-arbitrary null
checks. If m_port can't be null, just assert, and don't make runtime checks.

> Source/WebCore/platform/network/ServerSocketHandleClient.h:45
> +} // namespace WebCore

I'm not a fan of such comments - if you really need to check what this brace at
the end of the file is for, you'll double-click it for IDE to balance, and if
you're not, why is this boilerplate asking for attention?

> Source/WebCore/platform/network/cf/ServerSocketHandleCFNet.cpp:58
> +    m_serverFileDescriptor = socket(AF_INET6, SOCK_STREAM, IPPROTO_TCP);

We don't have CF level APIs to create listening sockets? This whole function
feels alien, with perror and such.

> Source/WebCore/platform/network/cf/ServerSocketHandleCFNet.cpp:70
> +    sockaddr6.sin6_addr = in6addr_any;

Should there be any limit on which interfaces to bind to? Even when running
regression tests, we only listen on loopback, because opening up on all
interfaces isn't prudent security wise.

> Source/WebCore/platform/network/cf/ServerSocketHandleCFNet.cpp:157
> +    int fd = *(const int*)data;

I don't think that const has any real meaning here.

Please use a C++ style cast.