[webkit-reviews] review granted: [Bug 38728] WebSocket: Add WebSocketHandshakeResponse : [Attachment 59102] Patch (Address ap's comments)

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted 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 59102: Patch (Address ap's comments)
https://bugs.webkit.org/attachment.cgi?id=59102&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
>	   s += "...";

You didn't change this code, but ellipsis should use its own Unicode character,
not three dots. And, "trimConsoleMessage()" might also be out of place in this
file, I'm not sure.

+int WebSocketHandshake::readStatusLine(const char* header, size_t len, int&
statusCode, String& statusText)

We prefer to not abbreviate "length". Also, a more descriptive name could be
used here (maybe headerLength).

+    m_response.setChallengeResponse(reinterpret_cast<const unsigned
char*>(p));

Doesn't static_cast work here?

+    const HTTPHeaderMap& headers = m_response.headerFields();
     for (HTTPHeaderMap::const_iterator it = headers.begin(); it !=
headers.end(); ++it) {

I missed this yesterday: it seems that we don't need to iterate any more, do
we? There is no order in HTTPHeaderMap. But it's not new with your patch.

r=me