Make length property return useful values for DOM bindings functions

Make length property return useful values for DOM bindings functions

The length property of JavaScript functions (and constructors) from DOM bindings
was always returning 0. This patch makes Blink return useful values instead and
matches the behavior of both Safari/JSC and Firefox.

BUG=231469
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148663

[abarth-chromium, 2013-04-15 23:49:21]

Cool!  This CL is for haraken to review.

[haraken, 2013-04-15 23:54:51]

I'm very glad to see you here! Will take a look in an hour.

[haraken, 2013-04-16 00:29:31]

Thanks for the patch. I'm very glad to see patches to make behavior of JSC/V8
consistent. This is the bug I've wanted to fix for a long time.

https://codereview.chromium.org/14244017/diff/1/Source/bindings/scripts/CodeG...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/14244017/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGeneratorV8.pm:2158: $numberOfConstructorParameters
= $currNumberOfParameters;
I'm not sure if this is correct. Per the spec, it looks like you have to report
# of mandatory arguments (i.e. not including optional parameters).
http://www.w3.org/TR/WebIDL/#es-interface-call

https://codereview.chromium.org/14244017/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGeneratorV8.pm:3216: push(@implContent, "   
desc->SetLength(${constructorLength});\n") if defined $constructorLength;
'if defined' is not needed.

https://codereview.chromium.org/14244017/diff/1/Source/bindings/scripts/IDLAt...
File Source/bindings/scripts/IDLAttributes.txt (right):

https://codereview.chromium.org/14244017/diff/1/Source/bindings/scripts/IDLAt...
Source/bindings/scripts/IDLAttributes.txt:31: ConstructorParameters=*
I was planning to fix the bug without introducing [ConstructorParameters].

How about letting [CustomConstructor] describe a signature of the constructor,
from which you can know # of arguments? For example,
[CustomConstructor(DOMString str, [Optional] DOMString str2)].

[do-not-use, 2013-04-16 07:26:23]

https://codereview.chromium.org/14244017/diff/1/Source/bindings/scripts/CodeG...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/14244017/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGeneratorV8.pm:2158: $numberOfConstructorParameters
= $currNumberOfParameters;
On 2013/04/16 00:29:31, haraken wrote:
> I'm not sure if this is correct. Per the spec, it looks like you have to
report
> # of mandatory arguments (i.e. not including optional parameters).
> http://www.w3.org/TR/WebIDL/#es-interface-call

Sorry, could you point me to the part where it says we should only count
mandatory arguments? I cannot find it. What I could find at the link you
provided is:

"""
 Interface objects for interfaces declared with a [Constructor] extended
attribute MUST have a property named “length” with attributes { [[Writable]]:
false, [[Enumerable]]: false, [[Configurable]]: false } whose value is a Number
determined as follows:

    Let id be the identifier of interface I.
    Initialize S to the effective overload set for constructors with identifier
id on interface I and with argument count 0.
    Return the maximum argument list length of the constructors in the entries
of S.
"""

If I understand correctly, we need to select the maximum argument list length
among all the constructors, counting optional arguments. My patch is actually
currently doing the opposite (selecting the minimum argument list length).

[haraken, 2013-04-16 07:29:02]

Sorry, I was misreading the spec. You're right.

[do-not-use, 2013-04-16 09:39:20]

https://codereview.chromium.org/14244017/diff/1/Source/bindings/scripts/CodeG...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://codereview.chromium.org/14244017/diff/1/Source/bindings/scripts/CodeG...
Source/bindings/scripts/CodeGeneratorV8.pm:2158: $numberOfConstructorParameters
= $currNumberOfParameters;
On 2013/04/16 07:26:23, cdumez wrote:
> On 2013/04/16 00:29:31, haraken wrote:
> > I'm not sure if this is correct. Per the spec, it looks like you have to
> report
> > # of mandatory arguments (i.e. not including optional parameters).
> > http://www.w3.org/TR/WebIDL/#es-interface-call
> 
> Sorry, could you point me to the part where it says we should only count
> mandatory arguments? I cannot find it. What I could find at the link you
> provided is:
> 
> """
>  Interface objects for interfaces declared with a [Constructor] extended
> attribute MUST have a property named “length” with attributes { [[Writable]]:
> false, [[Enumerable]]: false, [[Configurable]]: false } whose value is a
Number
> determined as follows:
> 
>     Let id be the identifier of interface I.
>     Initialize S to the effective overload set for constructors with
identifier
> id on interface I and with argument count 0.
>     Return the maximum argument list length of the constructors in the entries
> of S.
> """
> 
> If I understand correctly, we need to select the maximum argument list length
> among all the constructors, counting optional arguments. My patch is actually
> currently doing the opposite (selecting the minimum argument list length).

Actually, the latest Editor Draft matches the behavior you were mentioning:
http://dev.w3.org/2006/webapi/WebIDL/#es-interface-call

This was changed about 6 month ago. So I guess we should follow the latest
editor draft and use the shortest argument list in the effective overload set
instead? This also means that optional arguments are not counted due to the
definition of an "effective overload set"
(http://dev.w3.org/2006/webapi/WebIDL/#dfn-effective-overload-set).

[haraken, 2013-04-16 09:42:41]

Good observation! What is behavior of other browsers?

[do-not-use, 2013-04-16 09:48:32]

On 2013/04/16 09:42:41, haraken wrote:
> Good observation! What is behavior of other browsers?

Safari/JSC follows the TR (mostly) but the change is being discussed at:
https://bugs.webkit.org/show_bug.cgi?id=110569

Firefox has recently changed its behavior to follow the ED:
https://bugzilla.mozilla.org/show_bug.cgi?id=793151

Relevant W3 bug where it was decided to NOT count optional arguments:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=19571

[haraken, 2013-04-16 10:13:08]

Thanks, then it sounds reasonable to me to follow the ED spec.

[do-not-use, 2013-04-16 11:38:04]

On 2013/04/16 10:13:08, haraken wrote:
> Thanks, then it sounds reasonable to me to follow the ED spec.

I updated the patch to take your feedback into consideration and follow the ED
spec. I also reset the results for the bindings tests.

We now comply with:
http://dev.w3.org/2006/webapi/WebIDL/#es-operations
http://dev.w3.org/2006/webapi/WebIDL/#es-interface-call

[haraken, 2013-04-16 11:58:27]

Looks almost good.

https://codereview.chromium.org/14244017/diff/15001/Source/bindings/scripts/I...
File Source/bindings/scripts/IDLParser.pm (right):

https://codereview.chromium.org/14244017/diff/15001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:1153: # CustomConstructor may also be used
on attributes.
Where can it happen?

https://codereview.chromium.org/14244017/diff/15001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:2114: my $extendedAttributeList =
$self->parseExtendedAttributeListAllowEmpty(1);
What is this change for?

[do-not-use, 2013-04-16 12:02:23]

https://codereview.chromium.org/14244017/diff/15001/Source/bindings/scripts/I...
File Source/bindings/scripts/IDLParser.pm (right):

https://codereview.chromium.org/14244017/diff/15001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:1153: # CustomConstructor may also be used
on attributes.
On 2013/04/16 11:58:27, haraken wrote:
> Where can it happen?

It is used in DOMWindow.idl for:
[CustomConstructor] attribute HTMLImageElementConstructorConstructor Image; //
Usable with new operator

It is a bit annoying to handle this special case but it appears to be used by
the V8 generators so I cannot remove it.

https://codereview.chromium.org/14244017/diff/15001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:2114: my $extendedAttributeList =
$self->parseExtendedAttributeListAllowEmpty(1);
On 2013/04/16 11:58:27, haraken wrote:
> What is this change for?

My bad. Forgot to remove this local change.

[haraken, 2013-04-16 12:05:44]

LGTM

https://codereview.chromium.org/14244017/diff/15001/Source/bindings/scripts/I...
File Source/bindings/scripts/IDLParser.pm (right):

https://codereview.chromium.org/14244017/diff/15001/Source/bindings/scripts/I...
Source/bindings/scripts/IDLParser.pm:1153: # CustomConstructor may also be used
on attributes.
ah....

Actually I do want to remove the [CustomConstructor], but I couldn't remove it
due to a very subtle issue in HTMLImageElement constructor. We should try to
remove the custom binding.

[arv (Not doing code reviews), 2013-04-16 18:38:02]

Awesome change. This one has been on my list for a while too.

Are you interested in fixing methods next?

[do-not-use, 2013-04-16 18:44:24]

On 2013/04/16 18:38:02, arv wrote:
> Awesome change. This one has been on my list for a while too.
> 
> Are you interested in fixing methods next?

This patch fixed the 'length' property for both JS Function objects and
constructors from DOM bindings.

Could you point me towards the problem with methods? I'm actually not sure what
you are referring to. I'm happy to help if you point me in the right direction
though :)

[arv (Not doing code reviews), 2013-04-16 19:35:35]

On 2013/04/16 18:44:24, cdumez wrote:
> On 2013/04/16 18:38:02, arv wrote:
> > Awesome change. This one has been on my list for a while too.
> > 
> > Are you interested in fixing methods next?
> 
> This patch fixed the 'length' property for both JS Function objects and
> constructors from DOM bindings.
> 
> Could you point me towards the problem with methods? I'm actually not sure
what
> you are referring to. I'm happy to help if you point me in the right direction
> though :)

I'm confused. Looking at the code in this patch I only see updates to
constructors. For example, does the following pass after your patch?

shouldBe('Node.prototype.appendChild.length', '1')

[do-not-use, 2013-04-16 19:48:54]

On 2013/04/16 19:35:35, arv wrote:
> On 2013/04/16 18:44:24, cdumez wrote:
> > On 2013/04/16 18:38:02, arv wrote:
> > > Awesome change. This one has been on my list for a while too.
> > > 
> > > Are you interested in fixing methods next?
> > 
> > This patch fixed the 'length' property for both JS Function objects and
> > constructors from DOM bindings.
> > 
> > Could you point me towards the problem with methods? I'm actually not sure
> what
> > you are referring to. I'm happy to help if you point me in the right
direction
> > though :)
> 
> I'm confused. Looking at the code in this patch I only see updates to
> constructors. For example, does the following pass after your patch?
> 
> shouldBe('Node.prototype.appendChild.length', '1')

Yes, this passes with my patch:
PASS Node.prototype.appendChild.length is 1

The patch actually includes a layout tests for checking the length attribute of
Function objects.

Related changes are in V8DOMConfiguration and this change in
generateConstructorCallback:
- {"$name", ${interfaceName}V8Internal::${name}MethodCallback,
${methodForMainWorld}},
+ {"$name", ${interfaceName}V8Internal::${name}MethodCallback,
${methodForMainWorld}, ${numMandatoryParams}},

One remaining issue that I'm planning to address separately is that the length
attribute still does not behave correctly for a few methods, such as
window.postMessage().

This is because those are handled differently (I believe because of
DoNotCheckSecurity extended attribute), the generated code looks like:
proto->SetAccessor(v8::String::NewSymbol("postMessage"),
DOMWindowV8Internal::postMessageAttrGetterCallback,
DOMWindowV8Internal::DOMWindowDomainSafeFunctionSetter, v8Undefined(),
v8::ALL_CAN_READ, static_cast<v8::PropertyAttribute>(v8::DontDelete));

It shouldn't be too hard to fix though. I'll look into it soon for a follow up
patch.

[haraken, 2013-04-16 23:34:38]

Thanks for working on this.

Just to clarify: You can land your patch before removing [CustomConstructor] for
HTMLImageElement.

[do-not-use, 2013-04-17 06:02:46]

On 2013/04/16 23:34:38, haraken wrote:
> Thanks for working on this.
> 
> Just to clarify: You can land your patch before removing [CustomConstructor]
for
> HTMLImageElement.

This is my first Blink patch so I'm not familiar with the process yet. However,
AFAIK I'm not a Blink committer yet. Should someone land this on my behalf?

Actually I'm still not sure this patch was r+. All I see is a LGTM from haraken
so I was still waiting for a r+ / approved notification.

[abarth-chromium, 2013-04-17 06:04:53]

Typing L-G-T-M is Chromium's version of an r+.  It makes the comment box light
up green and makes the presubmit check pass.  :)

[haraken, 2013-04-17 07:12:18]

I'll land your patch for you. Are you in rush? (If not, I'd like to land it
after the commit queue is recovered.)

[do-not-use, 2013-04-17 08:54:39]

On 2013/04/17 07:12:18, haraken wrote:
> I'll land your patch for you. Are you in rush? (If not, I'd like to land it
> after the commit queue is recovered.)

Thanks Kentaro, no rush :)

[commit-bot: I haz the power, 2013-04-17 23:56:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14244017/13003

[commit-bot: I haz the power, 2013-04-17 23:56:34]

Failed to apply patch for Source/WebCore/dom/MutationObserver.idl:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         Source/WebCore
  Created missing directory Source/WebCore.
  A         Source/WebCore/dom
  Created missing directory Source/WebCore/dom.
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/WebCore/dom/MutationObserver.idl
  |diff --git a/Source/WebCore/dom/MutationObserver.idl
b/Source/WebCore/dom/MutationObserver.idl
  |index
29aa06e7f1f71f6abd27feca3f0a8af36e3d6cdb..6df62b70e5f162f5dbff59a5a1fd70e5823f2df4
100644
  |--- a/Source/WebCore/dom/MutationObserver.idl
  |+++ b/Source/WebCore/dom/MutationObserver.idl
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       Source/WebCore/dom/MutationObserver.idl
Index: Source/WebCore/dom/MutationObserver.idl
diff --git a/Source/WebCore/dom/MutationObserver.idl
b/Source/WebCore/dom/MutationObserver.idl
index
29aa06e7f1f71f6abd27feca3f0a8af36e3d6cdb..6df62b70e5f162f5dbff59a5a1fd70e5823f2df4
100644
--- a/Source/WebCore/dom/MutationObserver.idl
+++ b/Source/WebCore/dom/MutationObserver.idl
@@ -29,7 +29,7 @@
  */
 
 [
-    CustomConstructor,
+    CustomConstructor(MutationCallback callback),
     ImplementationLacksVTable
 ] interface MutationObserver {
     [RaisesException] void observe(Node target, Dictionary options);

[do-not-use, 2013-04-18 14:36:50]

On 2013/04/17 23:56:34, I haz the power (commit-bot) wrote:
> Failed to apply patch for Source/WebCore/dom/MutationObserver.idl:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   A         Source/WebCore
>   Created missing directory Source/WebCore.
>   A         Source/WebCore/dom
>   Created missing directory Source/WebCore/dom.
>   can't find file to patch at input line 6
>   Perhaps you used the wrong -p or --strip option?
>   The text leading up to this was:
>   --------------------------
>   |Index: Source/WebCore/dom/MutationObserver.idl
>   |diff --git a/Source/WebCore/dom/MutationObserver.idl
> b/Source/WebCore/dom/MutationObserver.idl
>   |index
>
29aa06e7f1f71f6abd27feca3f0a8af36e3d6cdb..6df62b70e5f162f5dbff59a5a1fd70e5823f2df4
> 100644
>   |--- a/Source/WebCore/dom/MutationObserver.idl
>   |+++ b/Source/WebCore/dom/MutationObserver.idl
>   --------------------------
>   No file to patch.  Skipping patch.
>   1 out of 1 hunk ignored
> 
> Patch:       Source/WebCore/dom/MutationObserver.idl
> Index: Source/WebCore/dom/MutationObserver.idl
> diff --git a/Source/WebCore/dom/MutationObserver.idl
> b/Source/WebCore/dom/MutationObserver.idl
> index
>
29aa06e7f1f71f6abd27feca3f0a8af36e3d6cdb..6df62b70e5f162f5dbff59a5a1fd70e5823f2df4
> 100644
> --- a/Source/WebCore/dom/MutationObserver.idl
> +++ b/Source/WebCore/dom/MutationObserver.idl
> @@ -29,7 +29,7 @@
>   */
>  
>  [
> -    CustomConstructor,
> +    CustomConstructor(MutationCallback callback),
>      ImplementationLacksVTable
>  ] interface MutationObserver {
>      [RaisesException] void observe(Node target, Dictionary options);

Patch no longer applies cleanly so I rebased it on master.

[commit-bot: I haz the power, 2013-04-18 14:40:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14244017/33001

[commit-bot: I haz the power, 2013-04-18 14:40:38]

Presubmit check for 14244017-33001 failed and returned exit status 1.

INFO:root:Found 28 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/blink/PRESUBMIT.py

** Presubmit ERRORS **

LayoutTests/TestExpectations:311 Path does not exist. fast/frames/flattening

Lint failed.


Presubmit checks took 3.4s to calculate.

[do-not-use, 2013-04-18 14:59:47]

On 2013/04/18 14:40:38, I haz the power (commit-bot) wrote:
> Presubmit check for 14244017-33001 failed and returned exit status 1.
> 
> INFO:root:Found 28 file(s).
> Running presubmit commit checks ...
> Running /b/commit-queue/workdir/blink/PRESUBMIT.py
> 
> ** Presubmit ERRORS **
> 
> LayoutTests/TestExpectations:311 Path does not exist. fast/frames/flattening
> 
> Lint failed.
> 
> 
> Presubmit checks took 3.4s to calculate.

It appears something has changed upstream in between. The
fast/js/constructor-length.html is now failing :(

The DataView constructor length is now 3 instead of 1, despite having the
following Custom constructor in the IDL:

CustomConstructor(ArrayBuffer buffer, [Optional] unsigned long byteOffset,
[Optional] unsigned long byteLength)

[do-not-use, 2013-04-18 15:17:13]

On 2013/04/18 14:59:47, cdumez wrote:
> On 2013/04/18 14:40:38, I haz the power (commit-bot) wrote:
> > Presubmit check for 14244017-33001 failed and returned exit status 1.
> > 
> > INFO:root:Found 28 file(s).
> > Running presubmit commit checks ...
> > Running /b/commit-queue/workdir/blink/PRESUBMIT.py
> > 
> > ** Presubmit ERRORS **
> > 
> > LayoutTests/TestExpectations:311 Path does not exist. fast/frames/flattening
> > 
> > Lint failed.
> > 
> > 
> > Presubmit checks took 3.4s to calculate.
> 
> It appears something has changed upstream in between. The
> fast/js/constructor-length.html is now failing :(
> 
> The DataView constructor length is now 3 instead of 1, despite having the
> following Custom constructor in the IDL:
> 
> CustomConstructor(ArrayBuffer buffer, [Optional] unsigned long byteOffset,
> [Optional] unsigned long byteLength)

Ok, "[Optional]" should now be "optional". I have fixed the patch and
fast/js/constructor-length.html is passing again.

[commit-bot: I haz the power, 2013-04-18 16:07:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/14244017/40001

[commit-bot: I haz the power, 2013-04-18 16:31:17]

Change committed as 148663