Bug 123661

Created attachment 215835 [details] Patch

Comment on attachment 215835 [details] Patch Attachment 215835 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/19668405

Comment on attachment 215835 [details] Patch Attachment 215835 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/19558427

Created attachment 215839 [details] Patch

Created attachment 215840 [details] Patch

This conflicts with my patch in bug 123669 a little, which has a partial implementation in JSDOMBinding. But it's fine to land this first.

Comment on attachment 215840 [details] Patch 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.

Comment on attachment 215840 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215840&action=review >> 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" Ok, I will fix this. >> 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? Ok, I will switch to int16_t / uint_16t for consistency. >> 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? Yes, we can simply cast. I will get rid of the modulo. >> Source/WebCore/bindings/js/JSDOMBinding.cpp:416 >> +short toInt16(ExecState* exec, JSValue value, IntegerConversionConfiguration configuration) > > Same comment about int16_t vs. short. Will fix. >> 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. Actually it does. The results are on 2 lines. Based on the line before, we know 0 was assigned. >> LayoutTests/js/dom/webidl-type-mapping.html:426 >> +convert(type, "0"); > > Would be nice to test negative zero everywhere. Hmm. The thing is that I am not 100% sure what the result is supposed to be when assigning -0. Is is supposed to become 0 in all cases? (it seems to be what the current code gives as a result). I was wondering if assigning -0 on to an unsigned short with [EnforceRange] should throw for example. Currently it does not.

Created attachment 215843 [details] Improved patch for landing I took feedback into consideration, thanks. The only thing is that I am not 100% sure about the results for -0 but at least it is covered by layout test now if we want to change the behavior.

Comment on attachment 215843 [details] Improved patch for landing Clearing flags on attachment: 215843 Committed r158521: <http://trac.webkit.org/changeset/158521>

All reviewed patches have been landed. Closing bug.

Thank you for dressing this so quickly! > The only thing is that I am not 100% sure about the results for -0 but at least it is covered by layout test now if we want to change the behavior. I think that it's a mistake in WebIDL - it should have a special case for zeroes, because using sign(x) on either +0 or -0 is undefined in ECMAScript.