'length' property of DOM bindings functions returns wrong value

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jungkee Song]

Steps to reproduce:

ProgressEvent CR (http://www.w3.org/TR/progress-events/) test
- Test wiki: http://www.w3.org/wiki/Webapps/Interop/ProgressEvents
- Test suite: http://w3c-test.org/webapps/ProgressEvents/tests/
- Test case: http://w3c-test.org/webapps/ProgressEvents/tests/submissions/Ms2ger/interface.html
(1) The ProgressEvent interface


Actual results:

Test case failed: ProgressEvent.length returns 0 where 1 is expected.
Note: Latest WebIDL and ES6 define the length attribute of function constructor does not count optional parameters. Hence, it should be 1.


Expected results:

ProgressEvent.length returns 1 and subsequent assertions are passed.

[Chris Dumez]

Assuming you are talking about V8, I believe this is a duplicated of:
https://code.google.com/p/v8/issues/detail?id=435

[Chris Dumez]

The ES6 spec (http://wiki.ecmascript.org/lib/exe/fetch.php?id=harmony%3Aspecification_drafts&cache=cache&media=harmony:working_draft_ecma-262_edition_6_03-08-13-nomarkup.pdf) actually says:
"""
15.3.5.1 length
The value of the length property is an integer that indicates the typical number of arguments expected by the function. However, the language permits the function to be invoked with some other number of arguments.
The behaviour of a function when invoked on a number of arguments other than the number specified by its length property depends on the function. This property has the attributes 
{ [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: false }.
"""

Therefore, it is not clear to me that we should *not* count optional parameters.
The spec only refers to the *typical* number of arguments.

Actually, if I'm not mistaken, it is actually more common to count the optional parameters:
- JSC (e.g. Safari) will return 2 for ProgressEvent.length, not 1.
- ES6 states the Date constructor has a length of 7 (15.9.4):
  Date (year, month [, date [, hours [, minutes [, seconds [, ms ] ] ] ] ])
- ES6 states the TypedArray constructor has a length of 3 (15.13.6.3):
  TypedArray ( buffer, byteOffset=0, length=undefined )
- window.confirm.length returns 1 for both Safari and Firefox even though the argument is optional.

Jungkee, could you please clarify? Why do you expect ProgressEvent.length to return 1? and why do you expect optional arguments not to be counted?

[Chris Dumez]

In any case, V8/Chrome should likely not return 0 all the time. I filed a bug for that at:
https://code.google.com/p/chromium/issues/detail?id=231469

[Ms2ger (he/him; ⌚ UTC+1/+2)]

ES6 is irrelevant here; the applicable specification is WebIDL, which states:

> Interface objects for interfaces MUST have a property named “length”
> with attributes { [[Writable]]: false, [[Enumerable]]: false,
> [[Configurable]]: false } whose value is a Number. If the [Constructor]
> extended attribute, does not appear on the interface definition, then
> the value is 0. Otherwise, the value is determined as follows:
>
> 1. Let id be the identifier of interface I.
> 2. Initialize S to the effective overload set for constructors with
>    identifier id on interface I and with argument count 0.
> 3. Return the length of the shortest argument list of the entries in S.

near the end of <http://dev.w3.org/2006/webapi/WebIDL/#es-interface-call>.

[Jungkee Song]

Thanks for the follow-up, Chris. Thanks for the pointer, Ms2ger.

I have no idea about the rationale behind the change of the WebIDL from the last TR to the current ED.

[Jungkee Song]

(In reply to comment #5)
> Thanks for the follow-up, Chris. Thanks for the pointer, Ms2ger.
> 
> I have no idea about the rationale behind the change of the WebIDL from the last TR to the current ED.

Current JSC WebKit follows the TR algorithm and the rest of the implementation seem to not support it yet.

What would be the Gecko's choice? Up until Firefox 20.0, I could not see any update.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Relevant bugs:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=19571
https://bugzilla.mozilla.org/show_bug.cgi?id=793151

[Cameron McCormack (:heycam)]

The change was to bring .length in line with the behaviour of native ES6 functions that have optional arguments.  This change was made about 6 months ago.  (Maybe a republication of the TR/ version is overdue, but in general please follow what is in the Editor's Draft, as the TR/ version can quickly become out of date.)

[Chris Dumez]

I will propose a patch to make the JSC bindings comply with the Web IDL Editor Draft with regards to the 'length' attribute, in particular:
http://dev.w3.org/2006/webapi/WebIDL/#es-interface-call
http://dev.w3.org/2006/webapi/WebIDL/#es-operations

It seems Firefox already complies with the ED:
https://bugzilla.mozilla.org/show_bug.cgi?id=793151

And I submitted a patch to make Blink/V8 bindings comply with the ED:
https://codereview.chromium.org/14244017/

[Jungkee Song]

Thanks, Chris!

Here's the ProgressEvents interop page:
http://www.w3.org/wiki/Webapps/Interop/ProgressEvents

and a few more bugs filed in WebKit:
https://bugs.webkit.org/show_bug.cgi?id=110571
https://bugs.webkit.org/show_bug.cgi?id=110573
https://bugs.webkit.org/show_bug.cgi?id=110574

If you ever had time to look at them also, it would be great! :-)

[Chris Dumez]

Created attachment 198329 [details]
Patch

[Chris Dumez]

Comment on attachment 198329 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=198329&action=review

> Source/WebCore/html/canvas/DataView.idl:28
> +    ConstructorParameters=1,

We may consider renaming ConstructorParameters to "ConstructorLength" or "ConstructorMandatoryParameters".

Alternatively, the approach that was chosen for Blink was to get rid of ConstructorParameters and have CustomConstructor specify the arguments. e.g.:
CustomConstructor(ArrayBuffer buffer, [Optional] unsigned long byteOffset, [Optional] unsigned long byteLength)

[Build Bot]

Comment on attachment 198329 [details]
Patch

Attachment 198329 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/28299

New failing tests:
fast/files/blob-constructor.html

[Build Bot]

Created attachment 198425 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 198329 [details]
Patch

Attachment 198329 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/174018

New failing tests:
fast/files/blob-constructor.html

[Build Bot]

Created attachment 198440 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Chris Dumez]

Created attachment 198467 [details]
Patch

Fix failure reported by mac ews.

[Chris Dumez]

Created attachment 198605 [details]
Patch

Update bug title in the patch.

[Chris Dumez]

ping review?

[Geoffrey Garen]

Comment on attachment 198605 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 198605 [details]
Patch

Rejecting attachment 198605 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 198605, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
idl
patching file Source/WebCore/html/canvas/Int8Array.idl
patching file Source/WebCore/html/canvas/Uint16Array.idl
patching file Source/WebCore/html/canvas/Uint32Array.idl
patching file Source/WebCore/html/canvas/Uint8Array.idl
patching file Source/WebCore/html/canvas/Uint8ClampedArray.idl
patching file Source/WebCore/page/WebKitPoint.idl

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Geoffrey Garen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/50264

[Chris Dumez]

Created attachment 199320 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 199320 [details]
Patch for landing

Clearing flags on attachment: 199320

Committed r148997: <http://trac.webkit.org/changeset/148997>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.