Bug 67537

Summary: Implement a ProgressEvent constructor for JSC
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: annevk, ap, barraclough, cam, dominicc, ossy, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 67824    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Kentaro Hara 2011-09-02 16:30:10 PDT
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.
Comment 1 Kentaro Hara 2011-09-02 16:48:33 PDT
Created attachment 106224 [details]
Patch
Comment 2 Sam Weinig 2011-09-02 21:19:36 PDT
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.
Comment 3 Sam Weinig 2011-09-02 21:20:43 PDT
One other value to test would be a value in the unsigned long long range that cannot be represented as a double.
Comment 4 Kentaro Hara 2011-09-06 13:33:34 PDT
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)
Comment 5 Sam Weinig 2011-09-06 14:01:51 PDT
(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.
Comment 6 Cameron McCormack 2011-09-06 17:27:39 PDT
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.
Comment 7 Kentaro Hara 2011-09-06 17:35:06 PDT
(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.
Comment 8 Cameron McCormack 2011-09-06 17:37:34 PDT
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.
Comment 9 Kentaro Hara 2011-09-06 20:47:17 PDT
(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...
Comment 10 Cameron McCormack 2011-09-06 21:18:50 PDT
(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?
Comment 11 Kentaro Hara 2011-09-06 21:27:38 PDT
(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:-)
Comment 12 Kentaro Hara 2011-09-06 22:13:52 PDT
(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".
Comment 13 Kentaro Hara 2011-09-07 10:45:19 PDT
Created attachment 106597 [details]
Patch
Comment 14 Kentaro Hara 2011-09-07 10:47:50 PDT
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 15 Sam Weinig 2011-09-07 11:09:08 PDT
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.
Comment 16 Kentaro Hara 2011-09-07 11:24:11 PDT
Created attachment 106606 [details]
Patch
Comment 17 Sam Weinig 2011-09-07 11:26:31 PDT
Comment on attachment 106606 [details]
Patch

You didn't add the test with valueOf() :(.
Comment 18 Kentaro Hara 2011-09-07 11:39:19 PDT
Created attachment 106609 [details]
Patch
Comment 19 Kentaro Hara 2011-09-07 11:39:41 PDT
(In reply to comment #17)
> (From update of attachment 106606 [details])
> You didn't add the test with valueOf() :(.

Sorry, added.
Comment 20 Gavin Barraclough 2011-09-07 12:14:30 PDT
> 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.
Comment 21 Cameron McCormack 2011-09-07 16:48:32 PDT
(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 22 WebKit Review Bot 2011-09-08 11:27:47 PDT
Comment on attachment 106609 [details]
Patch

Clearing flags on attachment: 106609

Committed r94771: <http://trac.webkit.org/changeset/94771>
Comment 23 WebKit Review Bot 2011-09-08 11:27:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Csaba Osztrogonác 2011-09-08 13:48:50 PDT
Expected result was wrong, update landed in http://trac.webkit.org/changeset/94791