[webkit-reviews] review denied: [Bug 28038] WebSocket API implementation : [Attachment 34548] WebSocket API implementation. depends on bug 28037

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 28038: WebSocket API implementation
https://bugs.webkit.org/show_bug.cgi?id=28038

Attachment 34548: WebSocket API implementation.  depends on bug 28037
https://bugs.webkit.org/attachment.cgi?id=34548&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+    $(WebCore)/websockets \
     $(WebCore)/svg \
     $(WebCore)/websockets \

It's already there.

+	WebCore/websockets/WebSocketRequest.cpp \
+	WebCore/websockets/WebSocketRequest.h \
+	WebCore/websockets/WebSocketResponse.cpp \
+	WebCore/websockets/WebSocketResponse.h

This looks quite wrong - WebSocket does not use a request/response model, so
such class names are highly misguiding.

+#if ENABLE(WEB_SOCKETS)
+#include "JSWebSocket.h"
+#include "WebSocket.h"

In some places, generated files are included without guards, and in others,
with guards. The guards are only needed if the headers are not generated in
ports that do not have this feature enabled - otherwise, similar guards within
headers themselves would do the job.

There's never a need to protect e.g. WebSocket.h with external include guards.

+bool IsValidProtocolString(const WebCore::String& protocol)

This should be static, and inside the WebCore namespace. We don't normally use
anonymous namespaces.

+    // FIXME: need dispatchEvent?

Absolutely - it will dispatch to handlers installed via addEventListener(), as
opposed to setting socket.onopen or similar.

+bool CheckWebSocketHandshake(const WebCore::WebSocketRequest &req, const
WebCore::WebSocketResponse& resp)

Capitalization, argument naming, '&' positioning. This should also be static.

There are many style issues in protocol implementation, I'm not pointing out
most, because they are the same as in other patches.

+	     // FIXME: handle overflow.

Please do!

+KURL WebSocketRequest::URLoverHTTP() const

In WebKit style, this would be urlOverHTTP. I'm not sure what this is use for
though, hopefully an even better name can be found.

+    if (url().hasFragmentIdentifier()) {
+	 builder.append("#");
+	 builder.append(url().fragmentIdentifier());
+    }

What are fragment identifiers good for when using WebSockets? Seems that they
should be forbidden, or dropped immediately when passed in.

+const char* WebSocketResponse::parseHeader(const char* start, const char* end,
Vector<std::pair<String, String> >* headers)

Are WebSockets headers case sensitive? Otherwise, it may be better to use
HTTPHeaderMap.

I didn't closely review all the code; please fix style, possibly using
check-webkit-style script. The direction looks good to me, except for
request/response classes.