[webkit-reviews] review denied: [Bug 38728] WebSocket: Add WebSocketHandshakeResponse : [Attachment 57514] Patch

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied Yuta Kitamura
<yutak at chromium.org>'s request for review:
Bug 38728: WebSocket: Add WebSocketHandshakeResponse
https://bugs.webkit.org/show_bug.cgi?id=38728

Attachment 57514: Patch
https://bugs.webkit.org/attachment.cgi?id=57514&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
Putting r- to ask if we really don't need a new test.

WebCore/websockets/WebSocketHandshake.cpp:84
 +	if (statusCodeString.length() != 3) // Status code must consist of
three digits.
I'm not sure, but the code around here looks redundant to me. Cannot we just
rely on the result of WTFString::toInt ? Specifically

bool ok = false;
statusCode = statusCodeString.toInt(&ok);
if (!ok || statusCode < 100 || statusCode >= 600) {
    statusCode = 0;
    return true;
}

wont' work?


WebCore/websockets/WebSocketHandshakeResponse.h:63
 +	void setChallengeResponse(const unsigned char challengeResponse[16]);
I'd take signed char here so we don't need reinterpret_cast, but it's OK as is.


WebCore/websockets/WebSocketHandshake.cpp:80
 +	if (!space1 || !space2 || *(p - 1) != '\r') // The line must end with
"\r\n".
So, now we are rejecting responses like "HTTP/1.0 101 WebSocket Protocol
Handshake\n" and before this change we don't reject this? If so, I think it
would be better to test this change.