[Webkit-unassigned] [Bug 43130] Add callback arguments support to binding code generator scripts

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43130





--- Comment #5 from Kinuko Yasuda <kinuko at chromium.org>  2010-07-29 15:44:42 PST ---
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.