Summary: | EnforceRange doesn't enforce range of a short | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||||
Component: | Bindings | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, heycam, michael, rniwa, sam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 122679 | ||||||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2013-11-02 01:00:08 PDT
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.
|