Bug 69801 – Regarding constructor, replace [ConstructorWith=...] IDL with [CallWith=...] IDL

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Kentaro Hara]

As for constructors, [ConstructorWith=...] has the same meaning as [CallWith=...]. We should deprecate [ConstructorWith=...]. This is a clean-up bug for the bug 65839.

[Adam Barth]

@Kentaro: CallWith should also be the first arguments to the constructors, not the last as ConstructorWith appears to be.

[Kentaro Hara]

abarth: There are two options. Deprecate [ConstructorWith=...] or deprecate [CallWith=...]. I first thought that deprecating [ConstructorWith=...] will make more sense, since [CallWith=...] is used here and there and I did not care the position of arguments. 

I want to keep the position of arguments of existing constructors as it is (i.e. the last). So, as for constructors, I would like to deprecate [CallWith=...].

[Adam Barth]

See also Bug 69799

> I want to keep the position of arguments of existing constructors as it is

Why?

[Kentaro Hara]

(In reply to comment #3)
> > I want to keep the position of arguments of existing constructors as it is
> 
> Why?

Just because all the existing custom constructors that are using ScriptExecutionContext are putting it as the last argument.

> See also Bug 69799

Ah, sorry, I noticed it now. I agree to it. Before uploading this patch, I will wait for the above patch landed.

[Kentaro Hara]

Created attachment 110832 [details]
Patch

[Kentaro Hara]

Created attachment 110833 [details]
Patch

[Adam Barth]

Comment on attachment 110833 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        As for constructors, [ConstructorWith=...] has the same meaning as [CallWith=...].
> +        We should deprecate [ConstructorWith=...]. This is a clean-up bug for the bug 65839.

It's somewhat less obvious what CallWith means as an attribute of the whole interface.  I guess it makes sense for JavaScript where the interface *is* the constructor, but it probably makes somewhat less sense for other languages.  I still think this patch is a win though.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1549
> -    if ($dataNode->extendedAttributes->{"ConstructorWith"} && $dataNode->extendedAttributes->{"ConstructorWith"} eq "ScriptExecutionContext") {
> +    if ($dataNode->extendedAttributes->{"CallWith"} && $dataNode->extendedAttributes->{"CallWith"} eq "ScriptExecutionContext") {

Do we not need a similar change to CodeGeneratorJSC.pm ?  I guess all these interfaces have JSCustomConstructor.

> Source/WebCore/fileapi/FileReader.idl:-41
> -        ConstructorWith=ScriptExecutionContext,

This doesn't need CallWith ?  I guess not if we can compile without it.

[Kentaro Hara]

(In reply to comment #7)
> (From update of attachment 110833 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110833&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +        As for constructors, [ConstructorWith=...] has the same meaning as [CallWith=...].
> > +        We should deprecate [ConstructorWith=...]. This is a clean-up bug for the bug 65839.
> 
> It's somewhat less obvious what CallWith means as an attribute of the whole interface.  I guess it makes sense for JavaScript where the interface *is* the constructor, but it probably makes somewhat less sense for other languages.  I still think this patch is a win though.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1549
> > -    if ($dataNode->extendedAttributes->{"ConstructorWith"} && $dataNode->extendedAttributes->{"ConstructorWith"} eq "ScriptExecutionContext") {
> > +    if ($dataNode->extendedAttributes->{"CallWith"} && $dataNode->extendedAttributes->{"CallWith"} eq "ScriptExecutionContext") {
> 
> Do we not need a similar change to CodeGeneratorJSC.pm ?  I guess all these interfaces have JSCustomConstructor.

No, because [ConstructorWith] is the IDL that I introduced when I implemented [Constructor] IDL for V8, in other words, [ConstructorWith] is not yet implemented in JSC.

> 
> > Source/WebCore/fileapi/FileReader.idl:-41
> > -        ConstructorWith=ScriptExecutionContext,
> 
> This doesn't need CallWith ?  I guess not if we can compile without it.

It has CallWith. You can find it by clicking "20 below" link of FileReader.idl diff. I just removed ConstructorWith because FileReader.idl has both ConstructorWith and CallWith.

Thanks for reviewing these patches, Adam!

[WebKit Review Bot]

Comment on attachment 110833 [details]
Patch

Clearing flags on attachment: 110833

Committed r97424: <http://trac.webkit.org/changeset/97424>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.