Summary: | Add callback arguments support to binding code generator scripts | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kinuko Yasuda <kinuko> | ||||||||||
Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, dumi, eric | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 43134 | ||||||||||||
Attachments: |
|
Description
Kinuko Yasuda
2010-07-28 11:30:35 PDT
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> |