[webkit-reviews] review denied: [Bug 83233] Add CodeGenerator support for sequence<> in callbacks : [Attachment 137291] Patch

[bugzilla-daemon at webkit.org]

Erik Arvidsson <arv at chromium.org> has denied Vineet Chaudhary (vineetc)
<rgf748 at motorola.com>'s request for review:
Bug 83233: Add CodeGenerator support for sequence<> in callbacks
https://bugs.webkit.org/show_bug.cgi?id=83233

Attachment 137291: Patch
https://bugs.webkit.org/attachment.cgi?id=137291&action=review

------- Additional Comments from Erik Arvidsson <arv at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=137291&action=review


Sorry, the MutationCallback is not the cleanest case to start with. I
understand why it has lead you to the current set of patches. The thing that
makes MutationCallback strange is that it uses the same object as 'this' and
the second argument.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2749
> +	       if ($function->signature->extendedAttributes->{"PassThis"}) {
> +		   foreach my $param (@params) {
> +		       next  if $codeGenerator->GetArrayType($param->type);
> +		       my $paramName = $param->name;
> +		       push(@implContent, <<END);
> +    JSValue js${paramName} = toJS(exec, m_data->globalObject(),
${paramName});
> +    m_data->invokeCallback(js${paramName}, args, &raisedException);

This is pretty strange. It seems like we treat the first non ArrayType
parameter as 'this'. Also, if there are more than 2 parameters we will invoke
the callback multiple times.

I think we need to always treat the first argument as this when PassThis is
set. That would probably mean we also have to include the type

[PassThis=WebKitMutationObserver] boolean handleEvent(in
sequence<MutationRecord> mutations, in WebKitMutationObserver observer);

That handleEvent above happens to pass the observer both as this and as a
parameter is an exception, not the norm.

> Source/WebCore/bindings/scripts/IDLAttributes.txt:94
> +PassThis

Please remember to document this on the wiki once landed

http://trac.webkit.org/wiki/WebKitIDL

> Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:153
> +bool JSTestCallback::callbackRequiresThisToPass(Class7Array* param1, Class8*
class8Param)

This should take one more parameter for 'this'

> Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:177
> +    m_data->invokeCallback(jsclass8Param, args, &raisedException);

... and use that here.