[webkit-reviews] review granted: [Bug 52768] Construction of Uint8Array from JS Array (and possibly others) incorrectly clamps values : [Attachment 82729] Patch

[bugzilla-daemon at webkit.org]

Chris Marrin <cmarrin at apple.com> has granted Kenneth Russell <kbr at google.com>'s
request for review:
Bug 52768: Construction of Uint8Array from JS Array (and possibly others)
incorrectly clamps values
https://bugs.webkit.org/show_bug.cgi?id=52768

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

------- Additional Comments from Chris Marrin <cmarrin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=82729&action=review

Generally fine, one nit, and one question about the necessity of LargestIntType
in the template. r=me, after settling those.

> Source/WebCore/ChangeLog:13
> +	   required template parameter.

I don't understand the second parameter to IntegralTypedArrayBase. It's always
a 64 bit int, which it would have to be to hold a any possible double. I don't
think having it be a uint64 in some places is useful because a signed 64 bit
number is plenty to hold all possible double values. I think JS requires
numbers to be doubles so why not just eliminate it? Seems like unnecessary
future-proofing

> Source/WebCore/bindings/js/JSArrayBufferViewHelper.h:137
> +	   unsigned length = srcArray->get(exec, JSC::Identifier(exec,
"length")).toUInt32(exec);

It would be more consistent to use uint32_t instead of unsigned here?