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.
Created attachment 62851 [details] Patch
Created attachment 62855 [details] Patch
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"
Created attachment 63004 [details] Patch
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.
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.
Created attachment 63032 [details] Patch for submit
Comment on attachment 63032 [details] Patch for submit r=me.
Committed r64366: <http://trac.webkit.org/changeset/64366>