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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu May 6 14:14:15 PDT 2010


Alexey Proskuryakov <ap at webkit.org> has granted 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 55209: Patch
https://bugs.webkit.org/attachment.cgi?id=55209&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+    // FIXME: using WTF in RandomNumber.h

This comment will be hard to understand for someone who didn't follow our
discussion here. If you are going to address this soon, it would be best to
just omit it.

It's not important enough to disrupt someone reading this code in the way a
FIXME does.

+    // FIXME: md5.checksum() should take a reference to output instead of
returning the result by value.

Also, this is probably not a good place to complain about MD5 methods - MD5.h
would be such a place if we wanted to keep this unfixed for a long time.

+    DEFINE_STATIC_LOCAL(String, spaceChar, (" "));

There is no need to make a string for this - there is a version of insert()
that takes a UChar.

+static void generateChallengeResponseExpected(uint32_t number1, uint32_t
number2, unsigned char key3[8], unsigned char challengeResponseExpected[16])
+{
+    unsigned char challenge[16];
+    setChallengeNumber(&challenge[0], number1);
+    setChallengeNumber(&challenge[4], number2);
+    memcpy(&challenge[8], key3, 8);
+    MD5 md5;
+    md5.addBytes(challenge, sizeof(challenge));
+    // FIXME: md5.checksum() should take a reference to output instead of
returning the result by value.
+    Vector<uint8_t, 16> digest = md5.checksum();
+    memcpy(challengeResponseExpected, digest.data(), 16);
+}

This also needs a grammar fix (responseExpected->expectedResponse).

r=me


More information about the webkit-reviews mailing list