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

See Also:


Attachments
Patch (59.42 KB, patch)
2013-04-16 07:45 PDT, Chris Dumez
buildbot: commit‑queue-
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 PDT, 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 PDT, Build Bot
no flags Details
Patch (60.92 KB, patch)
2013-04-16 22:54 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (60.92 KB, patch)
2013-04-17 13:46 PDT, Chris Dumez
ggaren: review+
commit-queue: commit‑queue-
Details | Formatted Diff | Diff
Patch for landing (59.54 KB, patch)
2013-04-23 13:15 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jungkee Song 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 Chris Dumez 2013-04-15 04:36:06 PDT
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 Chris Dumez 2013-04-15 08:57:37 PDT
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 Chris Dumez 2013-04-15 12:02:01 PDT
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 Ms2ger 2013-04-16 00:34:54 PDT
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 Jungkee Song 2013-04-16 01:28:01 PDT
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 Jungkee Song 2013-04-16 01:37:27 PDT
(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 Cameron McCormack 2013-04-16 01:42:00 PDT
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 Chris Dumez 2013-04-16 04:46:41 PDT
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 Jungkee Song 2013-04-16 05:48:40 PDT
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 Chris Dumez 2013-04-16 07:45:49 PDT
Created attachment 198329 [details]
Patch
Comment 12 Chris Dumez 2013-04-16 07:57:27 PDT
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)
Comment 13 Build Bot 2013-04-16 13:47:11 PDT
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
Comment 14 Build Bot 2013-04-16 13:47:14 PDT
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
Comment 15 Build Bot 2013-04-16 15:13:06 PDT
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
Comment 16 Build Bot 2013-04-16 15:13:12 PDT
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
Comment 17 Chris Dumez 2013-04-16 22:54:39 PDT
Created attachment 198467 [details]
Patch

Fix failure reported by mac ews.
Comment 18 Chris Dumez 2013-04-17 13:46:57 PDT
Created attachment 198605 [details]
Patch

Update bug title in the patch.
Comment 19 Chris Dumez 2013-04-23 12:23:31 PDT
ping review?
Comment 20 Geoffrey Garen 2013-04-23 12:33:37 PDT
Comment on attachment 198605 [details]
Patch

r=me
Comment 21 WebKit Commit Bot 2013-04-23 12:34:56 PDT
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
Comment 22 Chris Dumez 2013-04-23 13:15:17 PDT
Created attachment 199320 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2013-04-23 14:58:18 PDT
Comment on attachment 199320 [details]
Patch for landing

Clearing flags on attachment: 199320

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