[webkit-reviews] review denied: [Bug 22876] Should centralize random number handling for JavaScriptCore & WebCore : [Attachment 26048] Updated patch for WebCore

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 22876: Should centralize random number handling for JavaScriptCore &
WebCore
https://bugs.webkit.org/show_bug.cgi?id=22876

Attachment 26048: Updated patch for WebCore
https://bugs.webkit.org/attachment.cgi?id=26048&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -#if PLATFORM(QT)
> -#include <QtCore/QFileInfo>
> -#endif

This change isn't mentioned anywhere. You should mention it in the header if
there's a good reason for it.

> -#if PLATFORM(DARWIN)
> -    if (!randomSeeded) {
> -	   srandomdev();
> -	   randomSeeded = true;
> -    }
> -    return random();
> -#else

The srandomdev() technique for initializing the random number generator is
stronger than the srand(time(0)) technique, and random() is better than rand(),
so the version in JavaScriptCore should be changed to use that under DARWIN. We
may want to improve the random number generator further soon due to some
feedback I got from some other folks at Apple.

> -	   int randomness = randomNumber();
> +	   int randomness = static_cast<int>(WTF::randomNumber() * INT_MAX);

This won't work quite as well as the old code. It doesn't cover the entire
integer range. We want numbers in the range [INT_MIN, INT_MAX], but this gives
numbers in the range [0, INT_MAX). So we'll never get INT_MAX or any negative
number. On the other hand, we only really use 24 of the 32 bits of randomness,
so it's probably OK to take this basic approach. But it should be INT_MAX + 1,
not INT_MAX, that we multiply by. Of course INT_MAX + 1 won't fit into an int,
so it's a little tricky to write the code correctly. Something like this is
what I'd suggest:

    unsigned randomness = static_cast<unsigned>(randomNumber() *
(numeric_limits<unsigned>::max() + 1.0));

I'm going to say review- because I think it's important not to downgrade from
random() to rand() on DARWIN.