[webkit-reviews] review granted: [Bug 123661] EnforceRange doesn't enforce range of a short : [Attachment 215840] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Christophe Dumez
<dchris at gmail.com>'s request for review:
Bug 123661: EnforceRange doesn't enforce range of a short
https://bugs.webkit.org/show_bug.cgi?id=123661

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=215840&action=review


Some of the comments below definitely don't need to be addressed in this patch.
Some probably need to be addressed one way or another.

> Source/WebCore/ChangeLog:17
> +	   No new tests, covered by js/dom/webidl-type-mapping.html.

I'd have said "added test cases to js/dom/webidl-type-mapping.html"

> Source/WebCore/bindings/js/JSDOMBinding.cpp:328
> +struct IntTypeLimits<short> {

It's a strange mixup that we use int8_t, but don't use int16_t. Is possible to
make this consistent?

> Source/WebCore/bindings/js/JSDOMBinding.cpp:386
> +	   return static_cast<T>(d % LimitsTrait::numberOfValues);

I don't understand why taking a remainder is necessary. Why can't we simply
cast?

> Source/WebCore/bindings/js/JSDOMBinding.cpp:416
> +short toInt16(ExecState* exec, JSValue value, IntegerConversionConfiguration
configuration)

Same comment about int16_t vs. short.

> LayoutTests/js/dom/webidl-type-mapping-expected.txt:746
> +PASS converter.testShort is 0

It's unfortunate that these results are unreadable, they don't tell what the
value being converted was.

> LayoutTests/js/dom/webidl-type-mapping.html:426
> +convert(type, "0");

Would be nice to test negative zero everywhere.