Bug 69801

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

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=...].

See also Bug 69799 > I want to keep the position of arguments of existing constructors as it is Why?

(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.

Created attachment 110832 [details] Patch

Created attachment 110833 [details] Patch

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.

(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!

Comment on attachment 110833 [details] Patch Clearing flags on attachment: 110833 Committed r97424: <http://trac.webkit.org/changeset/97424>

All reviewed patches have been landed. Closing bug.