Summary: | Implement a ProgressEvent constructor for JSC | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | annevk, ap, barraclough, dominicc, heycam, ossy, sam, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 67824 | ||||||||||||
Attachments: |
|
Description
Kentaro Hara
2011-09-02 16:30:10 PDT
Created attachment 106224 [details]
Patch
Comment on attachment 106224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106224&action=review > LayoutTests/fast/events/constructors/progress-event-constructor.html:46 > +shouldBe("new ProgressEvent('eventType', { bubbles: true, cancelable: true, lengthComputable: true, loaded: 12345, total: 12345 }).bubbles", "true"); > +shouldBe("new ProgressEvent('eventType', { bubbles: true, cancelable: true, lengthComputable: true, loaded: 12345, total: 12345 }).cancelable", "true"); > +shouldBe("new ProgressEvent('eventType', { bubbles: true, cancelable: true, lengthComputable: true, loaded: 12345, total: 12345 }).lengthComputable", "true"); > +shouldBe("new ProgressEvent('eventType', { bubbles: true, cancelable: true, lengthComputable: true, loaded: 12345, total: 12345 }).loaded", "12345"); > +shouldBe("new ProgressEvent('eventType', { bubbles: true, cancelable: true, lengthComputable: true, loaded: 12345, total: 12345 }).total", "12345"); It might makes sense to test the edge cases for loaded or total, that is, values values in the range [0, 18446744073709551615] (unsigned long long range), NaN, negative values, non-numeric values, object literals with valueOf functions, etc. > Source/WebCore/dom/ProgressEvent.h:66 > + ProgressEvent(const AtomicString& type, const ProgressEventInit& initializer); The parameter initializer is not really needed here. One other value to test would be a value in the unsigned long long range that cannot be represented as a double. Comment on attachment 106224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106224&action=review >> LayoutTests/fast/events/constructors/progress-event-constructor.html:46 >> +shouldBe("new ProgressEvent('eventType', { bubbles: true, cancelable: true, lengthComputable: true, loaded: 12345, total: 12345 }).total", "12345"); > > It might makes sense to test the edge cases for loaded or total, that is, values values in the range [0, 18446744073709551615] (unsigned long long range), NaN, negative values, non-numeric values, object literals with valueOf functions, etc. Sam: Thank you, but one question. When we pass 18446744073709551615, what should the result be? In the current implementation, shouldBe("new ProgressEvent('eventType', { loaded: 18446744073709551615 }).loaded", "18446744073709551615"); results in FAIL new ProgressEvent('eventType', { loaded: 18446744073709551615 }).loaded should be 18446744073709552000. Was 0. The reason why 'loaded' becomes 0 is as follows: (1) "18446744073709551615" is passed to JSDictionary::convertValue(), and then to JSValue::toInteger(). (2) toNumber("18446744073709551615"), which is implemented in JSC, returns a double value 18446744073709551616.000000. (3) JSDictionary::convertValue() does static_cast of 18446744073709551616.000000 into an unsigned long long value, resulting in 0. I do not investigate the reason why "18446744073709551615" is evaluated as 18446744073709552000, but anyway this conversion is done in JSC code. Do we need to fix the JSC code so that it returns the exact value for 64bit integers? cf. Spec: ECMA-262, section 9.3 and 9.4 (http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf) (In reply to comment #4) > (From update of attachment 106224 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106224&action=review > > >> LayoutTests/fast/events/constructors/progress-event-constructor.html:46 > >> +shouldBe("new ProgressEvent('eventType', { bubbles: true, cancelable: true, lengthComputable: true, loaded: 12345, total: 12345 }).total", "12345"); > > > > It might makes sense to test the edge cases for loaded or total, that is, values values in the range [0, 18446744073709551615] (unsigned long long range), NaN, negative values, non-numeric values, object literals with valueOf functions, etc. > > Sam: Thank you, but one question. When we pass 18446744073709551615, what should the result be? I am not sure. You should check the WebIDL spec. > In the current implementation, > > shouldBe("new ProgressEvent('eventType', { loaded: 18446744073709551615 }).loaded", "18446744073709551615"); > > results in > > FAIL new ProgressEvent('eventType', { loaded: 18446744073709551615 }).loaded should be 18446744073709552000. Was 0. > > The reason why 'loaded' becomes 0 is as follows: > (1) "18446744073709551615" is passed to JSDictionary::convertValue(), and then to JSValue::toInteger(). > (2) toNumber("18446744073709551615"), which is implemented in JSC, returns a double value 18446744073709551616.000000. > (3) JSDictionary::convertValue() does static_cast of 18446744073709551616.000000 into an unsigned long long value, resulting in 0. > > I do not investigate the reason why "18446744073709551615" is evaluated as 18446744073709552000, but anyway this conversion is done in JSC code. > > Do we need to fix the JSC code so that it returns the exact value for 64bit integers? I don't think there is anything to change in JSC, as in JavaScript, all numbers are doubles, so not all 64bit integers can be represented. One think you can check with (though it also may be wrong) is what initProgressEvent() does. Yes, you can only represent unsigned long long values in the range [0, 2**53-1] exactly in JS. Your test could validly assert that 18446744073709551615 comes out as 18446744073709552000. (Well, I didn't check that second number but I assume it's the closest integral double to 2**64-1.) We discussed introducing a 53-bit integer type in Web IDL at one point, but it never got added in the end. (In reply to comment #6) > Yes, you can only represent unsigned long long values in the range [0, 2**53-1] exactly in JS. Your test could validly assert that 18446744073709551615 comes out as 18446744073709552000. (Well, I didn't check that second number but I assume it's the closest integral double to 2**64-1.) > > We discussed introducing a 53-bit integer type in Web IDL at one point, but it never got added in the end. Thank you, I found the spec (http://www.w3.org/TR/WebIDL/#es-unsigned-long-long). According to the spec, "new ProgressEvent("foo", {loaded: 18446744073709551615}).loaded" should be 0, because 18446744073709551615 is represented as 18446744073709551616.0 in double, and modulo(18446744073709551616, 2^64) becomes 0. I think I was too hasty, there. This is what should happen: * The literal 18446744073709551615 gets parsed as the JS Number 18446744073709552000. * 18446744073709552000 gets run through the "convert JS value to IDL unsigned long long" http://dev.w3.org/2006/webapi/WebIDL/#es-unsigned-long-long, which in step 5 would do 18446744073709552000 mod 18446744073709551616 == 384. So 384 is the actual IDL unsigned long long value that gets used, and which should be returned as the JS Number 384 when getting .loaded. (In reply to comment #8) > I think I was too hasty, there. This is what should happen: > > * The literal 18446744073709551615 gets parsed as the JS Number 18446744073709552000. Cameron: Why do you think that 18446744073709551615 should be parsed as 18446744073709552000? The ECMA-262 says that the ECMA Number should be the closest double value (Section 8.5: http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf). Therefore, toNumber(18446744073709551615) should be 18446744073709551616, since 18446744073709551616=2^64 can be exactly represented as a 64-bit double. In fact, JSValue::toNumber() converts 18446744073709551615 to 18446744073709551616. On the other hand, as you know, 18446744073709551615 is evaluated in JavaScript as 18446744073709552000 (e.g. alert(18446744073709551615)), but I do not know why this behavior is correct... (In reply to comment #9) > Cameron: Why do you think that 18446744073709551615 should be parsed as 18446744073709552000? > > The ECMA-262 says that the ECMA Number should be the closest double value (Section 8.5: http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf). Therefore, toNumber(18446744073709551615) should be 18446744073709551616, since 18446744073709551616=2^64 can be exactly represented as a 64-bit double. In fact, JSValue::toNumber() converts 18446744073709551615 to 18446744073709551616. Hmm, yes, you are correct. 2^64 is the closest number that can be represented exactly by a double. So the resulting unsigned long long should indeed be 0. > On the other hand, as you know, 18446744073709551615 is evaluated in JavaScript as 18446744073709552000 (e.g. alert(18446744073709551615)), but I do not know why this behavior is correct... Is it the correct output from http://people.mozilla.org/~jorendorff/es5.html#sec-9.8.1? (In reply to comment #10) > (In reply to comment #9) > > On the other hand, as you know, 18446744073709551615 is evaluated in JavaScript as 18446744073709552000 (e.g. alert(18446744073709551615)), but I do not know why this behavior is correct... > > Is it the correct output from http://people.mozilla.org/~jorendorff/es5.html#sec-9.8.1? Ah, makes sense! Thanks! By the way, then the output of shouldBe("new ProgressEvent('eventType', { loaded: -1 }).loaded", "18446744073709551615"); will become... PASS new ProgressEvent('eventType', { total: -1 }).total is 18446744073709552000 Confusing:-) (In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > On the other hand, as you know, 18446744073709551615 is evaluated in JavaScript as 18446744073709552000 (e.g. alert(18446744073709551615)), but I do not know why this behavior is correct... > > > > Is it the correct output from http://people.mozilla.org/~jorendorff/es5.html#sec-9.8.1? > > Ah, makes sense! Thanks! I am sorry for this rubber-stamp "make sense". It does not make sense. > 5. Otherwise, let n, k, and s be integers such that k ≥ 1, 10^(k−1) ≤ s < 10^k, the Number value for s × 10^(n−k) is m, and k is as small as possible. Note that k is the number of digits in the decimal representation of s, that s is not divisible by 10, and that the least significant digit of s is not necessarily uniquely determined by these criteria. If m = 18446744073709551615, then k = 20, n = 20, s = 18446744073709551615. > 6. If k ≤ n ≤ 21, return the String consisting of the k digits of the decimal representation of s (in order, with no leading zeroes), followed by n−k occurrences of the character ‘0’. Returns a String "18446744073709551615". Created attachment 106597 [details]
Patch
Sam: I uploaded the patch. Would you please take a look? Sam and Cameron: Sorry for much confusion so far. I still do not figure out why ToString(18446744073709551615) is evaluated as 18446744073709552000 in JavaScript and whether the behavior is correct or not, but anyway, I think that this is an independent issue of this patch (and thus I would like to investigate this issue in another bug). In other words, I think that committing this patch would be acceptable, since no matter how the Number is evaluated when it is converted to String, shouldBe("new ProgressEvent('eventType', { loaded: -1 }).loaded", "18446744073709551615"); should pass. Comment on attachment 106597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106597&action=review I think there should also be an object with a valueOf() function, but this looks much better. > Source/WebCore/bindings/js/JSDictionary.cpp:40 > +static const double D64 = 18446744073709551616.0; This should have a more descriptive name. > Source/WebCore/dom/ProgressEvent.h:66 > + ProgressEvent(const AtomicString& type, const ProgressEventInit& initializer); initializer parameter names is not needed. Created attachment 106606 [details]
Patch
Comment on attachment 106606 [details]
Patch
You didn't add the test with valueOf() :(.
Created attachment 106609 [details]
Patch
(In reply to comment #17) > (From update of attachment 106606 [details]) > You didn't add the test with valueOf() :(. Sorry, added. > I still do not figure out why ToString(18446744073709551615) is evaluated as 18446744073709552000
See section 9.8.1 of the spec, NOTE 2. The upshot of this clause is that JS implementations are required to generate the least number of non-zero digits that uniquely identify a value.
The string "18446744073709551615" converted to a double will internally have the value 18446744073709551616, however the number 18446744073709551616 converted to a string will convert to the string "18446744073709552000", if this is converted to a number it will round trip back to 18446744073709551616.
(In reply to comment #20) > The string "18446744073709551615" converted to a double will internally have the value 18446744073709551616, however the number 18446744073709551616 converted to a string will convert to the string "18446744073709552000", if this is converted to a number it will round trip back to 18446744073709551616. Ah yes, I understand now, thanks. Comment on attachment 106609 [details] Patch Clearing flags on attachment: 106609 Committed r94771: <http://trac.webkit.org/changeset/94771> All reviewed patches have been landed. Closing bug. Expected result was wrong, update landed in http://trac.webkit.org/changeset/94791 |