[webkit-reviews] review denied: [Bug 43130] Add callback arguments support to binding code generator scripts : [Attachment 62855] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jul 29 00:46:44 PDT 2010
Dumitru Daniliuc <dumi at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 43130: Add callback arguments support to binding code generator scripts
https://bugs.webkit.org/show_bug.cgi?id=43130
Attachment 62855: Patch
https://bugs.webkit.org/attachment.cgi?id=62855&action=review
------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
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"
More information about the webkit-reviews
mailing list