Bug 110569 - 'length' property of DOM bindings functions returns wrong value
: 'length' property of DOM bindings functions returns wrong value
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All Unspecified
: P2 Normal
Assigned To:
: http://dev.w3.org/cvsweb/~checkout~/2...
: WebExposed
:
:
  Show dependency treegraph
 
Reported: 2013-02-22 01:25 PST by
Modified: 2013-04-23 15:00 PST (History)


Attachments
Patch (59.42 KB, patch)
2013-04-16 07:45 PST, Christophe Dumez
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (1.34 MB, application/zip)
2013-04-16 13:47 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (666.44 KB, application/zip)
2013-04-16 15:13 PST, Build Bot
no flags Details
Patch (60.92 KB, patch)
2013-04-16 22:54 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff
Patch (60.92 KB, patch)
2013-04-17 13:46 PST, Christophe Dumez
ggaren: review+
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch for landing (59.54 KB, patch)
2013-04-23 13:15 PST, Christophe Dumez
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2013-02-22 01:25:47 PST
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.
------- Comment #1 From 2013-04-15 04:36:06 PST -------
Assuming you are talking about V8, I believe this is a duplicated of:
https://code.google.com/p/v8/issues/detail?id=435
------- Comment #2 From 2013-04-15 08:57:37 PST -------
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?
------- Comment #3 From 2013-04-15 12:02:01 PST -------
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
------- Comment #4 From 2013-04-16 00:34:54 PST -------
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>.
------- Comment #5 From 2013-04-16 01:28:01 PST -------
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.
------- Comment #6 From 2013-04-16 01:37:27 PST -------
(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.
------- Comment #8 From 2013-04-16 01:42:00 PST -------
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.)
------- Comment #9 From 2013-04-16 04:46:41 PST -------
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/
------- Comment #10 From 2013-04-16 05:48:40 PST -------
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! :-)
------- Comment #11 From 2013-04-16 07:45:49 PST -------
Created an attachment (id=198329) [details]
Patch
------- Comment #12 From 2013-04-16 07:57:27 PST -------
(From update of attachment 198329 [details])
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)
------- Comment #13 From 2013-04-16 13:47:11 PST -------
(From update of attachment 198329 [details])
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
------- Comment #14 From 2013-04-16 13:47:14 PST -------
Created an attachment (id=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
------- Comment #15 From 2013-04-16 15:13:06 PST -------
(From update of attachment 198329 [details])
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
------- Comment #16 From 2013-04-16 15:13:12 PST -------
Created an attachment (id=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
------- Comment #17 From 2013-04-16 22:54:39 PST -------
Created an attachment (id=198467) [details]
Patch

Fix failure reported by mac ews.
------- Comment #18 From 2013-04-17 13:46:57 PST -------
Created an attachment (id=198605) [details]
Patch

Update bug title in the patch.
------- Comment #19 From 2013-04-23 12:23:31 PST -------
ping review?
------- Comment #20 From 2013-04-23 12:33:37 PST -------
(From update of attachment 198605 [details])
r=me
------- Comment #21 From 2013-04-23 12:34:56 PST -------
(From update of attachment 198605 [details])
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
------- Comment #22 From 2013-04-23 13:15:17 PST -------
Created an attachment (id=199320) [details]
Patch for landing
------- Comment #23 From 2013-04-23 14:58:18 PST -------
(From update of attachment 199320 [details])
Clearing flags on attachment: 199320

Committed r148997: <http://trac.webkit.org/changeset/148997>
------- Comment #24 From 2013-04-23 14:58:25 PST -------
All reviewed patches have been landed.  Closing bug.