Bug 43130 – Add callback arguments support to binding code generator scripts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kinuko Yasuda]

Add Callback arguments support to binding code generator scripts.
Currently any methods that take callback objects/functions as arguments require custom binding implementation.
It'd be great if we could have better support for those methods that take callbacks.

[Kinuko Yasuda]

Created attachment 62851 [details]
Patch

[Kinuko Yasuda]

Created attachment 62855 [details]
Patch

[Dumitru Daniliuc]

Comment on attachment 62855 [details]
Patch

this is a great patch!!! a few minor comments:

WebCore/bindings/scripts/CodeGeneratorJS.pm:226
 +      return "JSCustom$className" if $className eq "VoidCallback";
nit: 'return "JSCustomVoidCallback"' instead of 'return "JSCustom$className"'. all other functions that have special cases seem to follow this pattern.

WebCore/bindings/scripts/CodeGeneratorJS.pm:1934
 +                          } elsif ($parameter->extendedAttributes->{"Callback"} || $parameter->extendedAttributes->{"CallbackFunction"}) {
is the CallbackFunction attribute used anywhere? i couldn't find a single IDL file that uses it.

WebCore/bindings/scripts/CodeGeneratorJS.pm:1936
 +                              $implIncludes{$callbackClassName . ".h"} = 1;
nit: i think you can do $implIncludes{"$callbackClassName.h"} or "${callbackClassName}.h"

WebCore/bindings/scripts/CodeGeneratorJS.pm:1939
 +                                  push(@implContent, "    if (!exec->argument($argsIndex).isObject() && !exec->argument($argsIndex).isNull()) {\n");
i think isNull() needs to be replaced with isUndefinedOrNull(). alternatively, i believe you need to check that exec->argumentCount() > $argsIndex.

WebCore/bindings/scripts/CodeGeneratorJS.pm:1946
 +                              push(@implContent, "        return JSValue::encode(jsUndefined());\n");
return jsUndefined();

WebCore/bindings/scripts/CodeGeneratorJS.pm:1948
 +                              push(@implContent, "    RefPtr<" . $parameter->type . "> $name = " . $callbackClassName . "::create(asObject(exec->argument($argsIndex)), static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()));\n");
replace static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()) with globalObject().

WebCore/bindings/scripts/CodeGeneratorV8.pm:1186
 +          if ($parameter->extendedAttributes->{"Callback"} || $parameter->extendedAttributes->{"CallbackFunction"}) {
same comment about CallbackFunction.

WebCore/bindings/scripts/CodeGeneratorV8.pm:1188
 +              $implIncludes{$className . ".h"} = 1;
$implIncludes{"$className.h"} = 1;

WebCore/bindings/scripts/CodeGeneratorV8.pm:1191
 +                  push(@implContentDecls, "    if (!args[$paramIndex]->IsObject())\n");
need to also check that args.Length() > $paramIndex.

WebCore/bindings/scripts/CodeGeneratorV8.pm:1197
 +              push(@implContentDecls, "    " . GetNativeType($parameter->type) . " $parameterName = " . $className . "::create(args[$paramIndex]);\n");
might be clearer to explicitly spell this out as "RefPtr<" . $parameter->type . ">".

WebCore/bindings/scripts/CodeGeneratorV8.pm:3258
 +      return "V8Custom$interfaceName" if $interfaceName eq "VoidCallback";
return "V8CustomVoidCallback"

[Kinuko Yasuda]

Created attachment 63004 [details]
Patch

[Kinuko Yasuda]

Thanks!  Updated the patch.

(In reply to comment #3)
> (From update of attachment 62855 [details])
> this is a great patch!!! a few minor comments:
> 
> WebCore/bindings/scripts/CodeGeneratorJS.pm:226
>  +      return "JSCustom$className" if $className eq "VoidCallback";
> nit: 'return "JSCustomVoidCallback"' instead of 'return "JSCustom$className"'. all other functions that have special cases seem to follow this pattern.

Fixed.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:1934
>  +                          } elsif ($parameter->extendedAttributes->{"Callback"} || $parameter->extendedAttributes->{"CallbackFunction"}) {
> is the CallbackFunction attribute used anywhere? i couldn't find a single IDL file that uses it.

No, it's not used anywhere.  I removed the code related to CallbackFunction for the sake of simplicity.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:1936
>  +                              $implIncludes{$callbackClassName . ".h"} = 1;
> nit: i think you can do $implIncludes{"$callbackClassName.h"} or "${callbackClassName}.h"

Fixed.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:1939
>  +                                  push(@implContent, "    if (!exec->argument($argsIndex).isObject() && !exec->argument($argsIndex).isNull()) {\n");
> i think isNull() needs to be replaced with isUndefinedOrNull(). alternatively, i believe you need to check that exec->argumentCount() > $argsIndex.

I changed the code to do the exec->argumentCount() > $argsIndex check.  Did the same in V8 code.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:1946
>  +                              push(@implContent, "        return JSValue::encode(jsUndefined());\n");
> return jsUndefined();

Fixed.

> WebCore/bindings/scripts/CodeGeneratorJS.pm:1948
>  +                              push(@implContent, "    RefPtr<" . $parameter->type . "> $name = " . $callbackClassName . "::create(asObject(exec->argument($argsIndex)), static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()));\n");
> replace static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()) with globalObject().

Fixed.  (Changed to castedThis->globalObject())

> WebCore/bindings/scripts/CodeGeneratorV8.pm:1186
>  +          if ($parameter->extendedAttributes->{"Callback"} || $parameter->extendedAttributes->{"CallbackFunction"}) {
> same comment about CallbackFunction.
> WebCore/bindings/scripts/CodeGeneratorV8.pm:1188
>  +              $implIncludes{$className . ".h"} = 1;
> $implIncludes{"$className.h"} = 1;
> WebCore/bindings/scripts/CodeGeneratorV8.pm:1191
>  +                  push(@implContentDecls, "    if (!args[$paramIndex]->IsObject())\n");
> need to also check that args.Length() > $paramIndex.
> WebCore/bindings/scripts/CodeGeneratorV8.pm:1197
>  +              push(@implContentDecls, "    " . GetNativeType($parameter->type) . " $parameterName = " . $className . "::create(args[$paramIndex]);\n");
> might be clearer to explicitly spell this out as "RefPtr<" . $parameter->type . ">".
> WebCore/bindings/scripts/CodeGeneratorV8.pm:3258
>  +      return "V8Custom$interfaceName" if $interfaceName eq "VoidCallback";
> return "V8CustomVoidCallback"

Fixed.

[Dumitru Daniliuc]

Comment on attachment 63004 [details]
Patch

r=me, but please address the comments below before landing (sorry i forgot to mention them in the first review).

WebCore/bindings/scripts/CodeGeneratorJS.pm:1939
 +                              push(@implContent, "    if (exec->argumentCount() > $argsIndex && !exec->argument($argsIndex).isNull()) {\n");
this code works only if the parameter is Optional. i think it should look more like this:

push(@implContent, "    RefPtr<" . $parameter->type . "> $name;\n");
if ($parameter->extendedAttributes->{"Optional"}) {
    // push the code that you have now
} else {
    push(@implContent, "    if ((exec->argumentCount() <= $argsIndex) || !exec->argument($argsIndex).isObject()) {\n");
    push(@implContent, "        setDOMException(exec, TYPE_MISMATCH_ERR);\n");
    push(@implContent, "        return jsUndefined();\n");
    push(@implContent, "    }\n");
    push(@implContent, "    $name = " . $callbackClassName . "::create(asObject(exec->argument($argsIndex)), castedThis->globalObject());\n");
}

WebCore/bindings/scripts/CodeGeneratorV8.pm:1191
 +              push(@implContentDecls, "    if (args.Length() > $paramIndex && !isUndefinedOrNull(args[$paramIndex])) {\n");
same thing.

[Kinuko Yasuda]

Created attachment 63032 [details]
Patch for submit

[Dumitru Daniliuc]

Comment on attachment 63032 [details]
Patch for submit

r=me.

[Kinuko Yasuda]

Committed r64366: <http://trac.webkit.org/changeset/64366>