[webkit-reviews] review denied: [Bug 54083] Add WTF::cryptographicallyRandomNumber : [Attachment 81784] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 54083: Add WTF::cryptographicallyRandomNumber
https://bugs.webkit.org/show_bug.cgi?id=54083

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81784&action=review

Wow, looks so much better!

Of course now that you've cleaned it up more, I understand it more, and thus
have more questions about the rest of it.  I'd like to see one more round if
you have the stamina.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:44
> +struct ARC4Stream {

This could be internal to the class, but it's OK as is.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:78
> +    for (int n = 0; n < 256; n++)
> +	   m_stream.s[n] = n;
> +    m_stream.i = 0;
> +    m_stream.j = 0;

This feels like the constructor for the stream class.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:81
> +void ARC4RandomNumberGenerator::addRandomData(unsigned char *data, int
length)

* goes to the left.  Should this take a Vector/CString type as input?

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:85
> +	   m_stream.i = (m_stream.i + 1);

This is just ++.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:87
> +	   m_stream.j = (m_stream.j + si + data[n % length]);

This is just +=.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:89
> +	   m_stream.s[m_stream.i] = m_stream.s[m_stream.j];
> +	   m_stream.s[m_stream.j] = si;

I don't really understand what this is doing, but that's probably OK. :)

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:105
> +    m_count = 1600000;

What's this magical value do?  It looks like it makes us stir every 40,000
numbers.  Why?	Seems we should add that as a comment?

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:110
> +    if (m_count <= 0 || !m_initialized)

Does count ever wrap?  Or is this just a double-check of m_initialized? (since
they're both set to 0 in teh constructor).

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:116
> +    m_stream.i = (m_stream.i + 1);

++.  This feels familiar.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:118
> +    m_stream.j = (m_stream.j + si);

Again +=.  Difficult to tell what this is doing.  Should this be sharing code
with addRandomData?

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:132
> +    val = getByte() << 24;
> +    val |= getByte() << 16;
> +    val |= getByte() << 8;
> +    val |= getByte();
> +    return val;

I would probably have just written this as one line, but maybe that's lazy of
me.

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:143
> +    m_count -= 4;

Why is this decrement not in getWord() itself?

> Source/JavaScriptCore/wtf/CryptographicallyRandomNumber.cpp:160
> +	   if (--m_count <= 0)
> +	       stir();

Isn't this just stirIfNeeded()?