Bug 67537 – Implement a ProgressEvent constructor for JSC

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Kentaro Hara]

ProgressEvent should have a constructor (Spec: http://www.w3.org/TR/progress-events/#interface-progressevent). 

After this patch is landed, I will add the ProgressEvent constructor for V8 as a follow-up patch.

[Kentaro Hara]

Created attachment 106224 [details]
Patch

[Sam Weinig]

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.

[Sam Weinig]

One other value to test would be a value in the unsigned long long range that cannot be represented as a double.

[Kentaro Hara]

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)

[Sam Weinig]

(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.

[Cameron McCormack (:heycam)]

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.

[Kentaro Hara]

(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.

[Cameron McCormack (:heycam)]

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.

[Kentaro Hara]

(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...

[Cameron McCormack (:heycam)]

(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?

[Kentaro Hara]

(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:-)

[Kentaro Hara]

(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".

[Kentaro Hara]

Created attachment 106597 [details]
Patch

[Kentaro Hara]

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.

[Sam Weinig]

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.

[Kentaro Hara]

Created attachment 106606 [details]
Patch

[Sam Weinig]

Comment on attachment 106606 [details]
Patch

You didn't add the test with valueOf() :(.

[Kentaro Hara]

Created attachment 106609 [details]
Patch

[Kentaro Hara]

(In reply to comment #17)
> (From update of attachment 106606 [details])
> You didn't add the test with valueOf() :(.

Sorry, added.

[Gavin Barraclough]

> 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.

[Cameron McCormack (:heycam)]

(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.

[WebKit Review Bot]

Comment on attachment 106609 [details]
Patch

Clearing flags on attachment: 106609

Committed r94771: <http://trac.webkit.org/changeset/94771>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Expected result was wrong, update landed in http://trac.webkit.org/changeset/94791