[webkit-reviews] review denied: [Bug 34784] WebSocketHandshake needs to provide request information : [Attachment 49363] Patch v6 (second try)

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Yuta Kitamura
<yutak at chromium.org>'s request for review:
Bug 34784: WebSocketHandshake needs to provide request information
https://bugs.webkit.org/show_bug.cgi?id=34784

Attachment 49363: Patch v6 (second try)
https://bugs.webkit.org/attachment.cgi?id=49363&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+    if (!url.protocolIs("wss") && m_secure)
+	 url.setProtocol("wss");
+    else if (!url.protocolIs("ws") && !m_secure)
+	 url.setProtocol("ws");

Can this ever happen? If not, this should be an assertion instead.

+    const CString& handshakeMessage =
m_handshake.clientHandshakeRequest().handshakeMessage();

Returning the handshake request object results in an unnecessary copy; I think
that WebSocketHandshake could still have a clientHandshakeMessage() that would
produce a CString without copying WebSocketHandshakeRequest around.

+    KURL cookieUrl = httpURLForAuthenticationAndCookies();

Should be cookieURL per WebKit style.

+    // FIXME: set authentication information or cookies for cookieUrl.
+    // Set "Authorization: <credentials>" if authentication information exists
for cookieUrl.

You didn't change this comment, but it looks strange, I'm not sure what it
actually means. We do set cookies right below the comment, and authorization
has been dropped from the spec.

+    fields.insert(fields.size() - 1, m_extraHeaderFields);

This should use append().

+#include "CString.h"
+#include "HTTPHeaderMap.h"

I don't think you need to include CString.h, and you definitely do not need
HTTPHeaderMap.h.

+    struct HeaderField {

I'd consider using std::pair instead of this struct - we use it quite
frequently in WebCore.

+    // According to current Web Socket protocol spec, four mandatory headers
(Upgrade, Connection, Host, and Origin) and

Please spell out "specification", not "spec".

+    // Build the character sequence for this handshake request.

Maybe "serialization"?

These comments are very minor. Still an r- to address them, but this is almost
done.