[webkit-reviews] review denied: [Bug 35572] WebSocket handshake incompatible change in draft-hixie-thewebsocketprotocol-76 : [Attachment 54264] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon May 3 10:42:35 PDT 2010


Alexey Proskuryakov <ap at webkit.org> has denied Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 35572: WebSocket handshake incompatible change in
draft-hixie-thewebsocketprotocol-76
https://bugs.webkit.org/show_bug.cgi?id=35572

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+
+#include <algorithm>
+

No need for the empty lines. But you probably don't need this header file, see
below.

+    for (p = header, lineLength = 0; p - header < len; p++, lineLength++) {

I think that it would be slightly better to zero out lineLength before the
loop. Now it misleadingly looks like a loop variable.

+static String hostName(const KURL& url, bool secure)

Since this is a separate function now, perhaps we should ASSERT that secure
flag matches url scheme here.

+    uint32_t space = static_cast<uint32_t>(WTF::randomNumber() * 12) + 1;

RandomNumber.h needs using directives - we do that for all WTF symbols that are
exposed for use outside of WTF. This can be done in a later patch.

I didn't study the key generation algorithms in detail. How did you test them?
Do examples from the spec pass?

+    String randomChars(randomCharacterInSecWebSocketKey);

It seems wasteful to construct this string from a buffer each time.

+    Vector<uint8_t, 16> digest = md5.checksum();

I don't understand why checksum() returns the result by value. It should
probably take a reference to output (not something to do in this patch).

+    std::random_shuffle(fields.begin(), fields.end());

We prefer to have "using namespace std" at the top of the .cpp file. But also,
I don't think that we should be randomizing the order here. This makes no sense
to me, and the spec doesn't seem to require that (it says "Fields in the
handshake are sent by the client in a random order; the order is not
meaningful." - which I interpret as allowing arbitrary order).

+    // TODO(ukai): key1,2,3

We use FIXME, not TODO. I don't understand this comment, did you mean to
address it before submitting the patch?

+	 unsigned char m_challengeResponseExpected[16];

Should it be "m_expectedChallengeResponse" for better grammar?

+    for (size_t i = 0; i < fields.size(); i++)
+	 builder.append(fields[i] + "\r\n");

Seems better to append() these separately - that's what StringBuilder is for.

+bool WebSocketHandshake::checkResponseHeaders()
 {
     ASSERT(m_mode == Normal);
     m_mode = Failed;
...
-    m_mode = Connected;
-    return;
+    return true;
 }

I don't understand how this function works - it now always sets m_mode to
Failed, but it also returns a boolean. What is the relationship between these?


More information about the webkit-reviews mailing list