[webkit-reviews] review denied: [Bug 38728] WebSocket: Add WebSocketHandshakeResponse : [Attachment 58445] Patch (Fix pbxproj file)

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.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 58445: Patch (Fix pbxproj file)
https://bugs.webkit.org/attachment.cgi?id=58445&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+// Return true if we have received enough data to parse a line.

There is a readServerHandshake() function in the same file that returns the
same information differently (it returns handshake length, and -1 if it can't
be determined yet). That's highly confusing.

+static bool parseResponseLine(const char* header, int len, int& statusCode,
String& statusText, size_t& lineLength)

FWIW, "statusText" is called "reason phrase" in RFC 2616.

<...>
+    if (statusCode < 100 || statusCode >= 600) {
+	 statusCode = 0;
+	 return true;
+    }

This function doesn't validate other parts of the string but from its
signature, it looks as if it did. I'd probably call it
"extractStatusCodeAndText" to avoid confusion - or make it check that the
response line starts with HTTP-Version and other requirements.

In particular, it seems out of place to check numeric value of statusCode in
parsing function.

+// Format a console message so that control characters look pretty.
+static String escapeControlCharacters(const String& string)

Formatting for console is definitely not something that belongs to this file.
If we want control characters to look pretty, then we probably always want them
to, and this code should be in addConsoleMessage() implementation file.

+	 if (string[i] < 0x20 || string[i] == 0x7F)

This is not how a control character is defined. We have an isPrintableChar()
function in wtf/Unicode.h - one should be sure to iterate by 32-bit code points
to use it.

+#include <cstring>

We usually include string.h. I'm not actually sure if this include is needed,
or config.h takes care of that.

>      const char* headerFields = strnstr(header, "\r\n", len); // skip status
line

We already know the length of status line from parseResponseLine(), why search
for CRLF again?

This patch generally looks good, but I think that there were enough comments
for another review round.