Add CodeGenerator support for sequence<> in callbacks

[Rafael Weinstein]

Now that we have sequence<Type> it would be great to kill *MutationCallbackCustom objects.

[Vineet Chaudhary (vineetc)]

Created attachment 135884 [details]
proposed patch

Attaching proposed patch, to get initial feedback.
I will upload patch soon with adding test in TestObj.idl.

[Kentaro Hara]

Comment on attachment 135884 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135884&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests.

Would you add a test case to run-bindings-tests, so that we can see what change is going to be made in the generated code?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2610
> +                my $argType = $codeGenerator->GetArrayType($param->type);

$arrayType would be a better name than $argType. Same comments for other $argType-s.

[Vineet Chaudhary (vineetc)]

Created attachment 135890 [details]
patch_for_review

[Erik Arvidsson]

Comment on attachment 135884 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135884&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2697
> +                    if ($argType eq "MutationRecordArray") {
> +                        AddIncludesForTypeInImpl("MutationRecord", 1);

The idl file should just be changed to sequence<MutationRecord>

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:147
> +    if ($type eq "MutationRecordArray") {

same

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3077
> +                    push(@implContent, "    ASSERT(${paramName});\n");

Can you just use?

    push(@headerContent, <<END);
...
END

> Source/WebCore/dom/MutationCallback.idl:36
> +        boolean handleEvent(in sequence<MutationRecordArray> mutations, in WebKitMutationObserver observer);

This should be sequence<MutationRecord>

[WebKit Review Bot]

Attachment 135890 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/scripts/test/JS/JSTestCallback.h:50:  The parameter name "observer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h:55:  The parameter name "observer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vineet Chaudhary (vineetc)]

Comment on attachment 135890 [details]
patch_for_review

View in context: https://bugs.webkit.org/attachment.cgi?id=135890&action=review

> Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:232
> +    V8DOMWrapper::setJSWrapperForActiveDOMObject(impl.release(), v8::Persistent<v8::Object>::New(wrapper));

I am not sure if these are related to this patch. I think we rebaseline Test results.

[Adam Klein]

Comment on attachment 135890 [details]
patch_for_review

View in context: https://bugs.webkit.org/attachment.cgi?id=135890&action=review

>> Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:232
>> +    V8DOMWrapper::setJSWrapperForActiveDOMObject(impl.release(), v8::Persistent<v8::Object>::New(wrapper));
> 
> I am not sure if these are related to this patch. I think we rebaseline Test results.

Sorry, this is me. Rebaselining now.

[Erik Arvidsson]

Comment on attachment 135890 [details]
patch_for_review

View in context: https://bugs.webkit.org/attachment.cgi?id=135890&action=review

> Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:162
> +    MarkedArgumentBuffer list;

Can't you use a JSArray directly instead of going through a MarkedArgumentBuffer?

> Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:167
> +    args.append(constructArray(exec, m_data->globalObject(), list));

Yeah, this looks strange.

> Source/WebCore/bindings/scripts/test/V8/V8TestCallback.cpp:215
> +        mutationsArray->Set(v8::Integer::New(i), toV8(mutations->at(i).get()));

v8::Uint32::New(i)

seems a bit better

[Vineet Chaudhary (vineetc)]

Oke I will upload patch with all review commnts. Thanks :)

[WebKit Review Bot]

Comment on attachment 135884 [details]
proposed patch

Attachment 135884 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12336518

New failing tests:
fast/mutation/callback-arguments.html

[WebKit Review Bot]

Created attachment 135897 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 135890 [details]
patch_for_review

Attachment 135890 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12340498

New failing tests:
fast/mutation/callback-arguments.html

[WebKit Review Bot]

Created attachment 135906 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 135884 [details]
proposed patch

Attachment 135884 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12340509

New failing tests:
fast/mutation/callback-arguments.html

[WebKit Review Bot]

Created attachment 135912 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Vineet Chaudhary (vineetc)]

Created attachment 135974 [details]
updated_patch

> > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:162
> > +    MarkedArgumentBuffer list;
> 
> Can't you use a JSArray directly instead of going through a MarkedArgumentBuffer?
> 
> > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:167
> > +    args.append(constructArray(exec, m_data->globalObject(), list));
> 
> Yeah, this looks strange.

I checked the implementation of constructArray() If we use JSArray I think we have to repeate code of constructArray() so I thought to keep the current Implemenatation.
Please correct me If this isn't way.

I have updated patch with all other review comments and fixing the failing test case fast/mutation/callback-arguments.html.
The reason for faillure of fast/mutation/callback-arguments.html was As per the WebIDL specification MutationObserver should pass "this" as second argument.
Ref: https://bugs.webkit.org/show_bug.cgi?id=81712 and http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-mo-invoke

[WebKit Review Bot]

Attachment 135974 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/scripts/test/JS/JSTestCallback.h:50:  The parameter name "observer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h:55:  The parameter name "observer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 135974 [details]
updated_patch

Attachment 135974 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12264673

[Kentaro Hara]

Comment on attachment 135974 [details]
updated_patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135974&action=review

> Source/WebCore/bindings/scripts/CodeGenerator.pm:463
> +    foreach my $param (@params) {
> +        return 1 if $param && $generator->GetArrayType($param->type) eq "MutationRecord";
> +    }

Do you need to hard-code "MutationRecord" here? Maybe the check could be...

    return 0 if !$dataNode->extendedAttributes{"Callback"};
    foreach my $param (@params) {
        return 1 if $generator->GetArrayType($param->type);  # If the interface has [Callback] and one of the method arguments is of type Array.
    }
    return 0;

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2728
> +

Nit: This new line would not be necessary.

> Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:153
> +{

We want to insert 'if (!mutations)' check here, just like V8TestCallback.cpp.

> Source/WebCore/bindings/scripts/test/V8/V8TestCallback.cpp:212
> +    ASSERT(mutations);
> +    if (!mutations)
> +        return true;

Nit: Shall we move this check to the top of this method?

[Vineet Chaudhary (vineetc)]

Created attachment 136012 [details]
patch

[WebKit Review Bot]

Attachment 136012 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/scripts/test/JS/JSTestCallback.h:50:  The parameter name "observer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h:55:  The parameter name "observer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

Comment on attachment 136012 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136012&action=review

> Source/WebCore/bindings/js/JSMutationCallbackCustom.cpp:45
> +/*

Remove the function.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:457
> +sub SendCallbackThisValue

Nit: Maybe we want more descriptive method name. IsCallbackWithArrayType()?

> Source/WebCore/bindings/scripts/CodeGenerator.pm:465
> +

Nit: remove this line.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2715
> +            push(@implContent, "@argsCheck\n") if scalar(@argsCheck) > 0;

Nit: Just 'if @argsCheck;' would work.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3069
> +            push(@implContent, "@argsCheck\n") if scalar(@argsCheck) > 0;

Ditto.

> Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:174
> +    bool raisedException = false;

Nit: we normally use 'ec' for the variable name.

> Source/WebCore/bindings/v8/custom/V8MutationCallbackCustom.cpp:49
> +/*

Remove the function.

[Kentaro Hara]

(In reply to comment #22)
> > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:174
> > +    bool raisedException = false;
> 
> Nit: we normally use 'ec' for the variable name.

Please ignore this comment. This is not an exception code. 'raisedException' is fine.

[Vineet Chaudhary (vineetc)]

Created attachment 136025 [details]
patch_for_landing


> > Source/WebCore/bindings/js/JSMutationCallbackCustom.cpp:45
> > Source/WebCore/bindings/v8/custom/V8MutationCallbackCustom.cpp:49
> Remove the function.

Done.
 
> > Source/WebCore/bindings/scripts/CodeGenerator.pm:457
> > +sub SendCallbackThisValue

> Nit: Maybe we want more descriptive method name. IsCallbackWithArrayType()?
Done 

> > Source/WebCore/bindings/scripts/CodeGenerator.pm:465
> > +
> 
> Nit: remove this line.
Done 
 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2715
> > +            push(@implContent, "@argsCheck\n") if scalar(@argsCheck) > 0;
> 
> Nit: Just 'if @argsCheck;' would work.
> > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:174
Done

[Kentaro Hara]

Comment on attachment 136025 [details]
patch_for_landing

Perfect!

[WebKit Review Bot]

Attachment 136025 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/scripts/test/JS/JSTestCallback.h:50:  The parameter name "observer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h:55:  The parameter name "observer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kentaro Hara]

Comment on attachment 136025 [details]
patch_for_landing

Clearing flags on attachment: 136025

Committed r113442: <http://trac.webkit.org/changeset/113442>

[Kentaro Hara]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Comment on attachment 136012 [details]
patch

Landed manually to avoid style check errors.

[Erik Arvidsson]

Comment on attachment 136025 [details]
patch_for_landing

View in context: https://bugs.webkit.org/attachment.cgi?id=136025&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2746
> +    JSValue jsObserver = toJS(exec, m_data->globalObject(), observer);
> +    m_data->invokeCallback(jsObserver, args, &raisedException);

observer is a bit too specific. The argument represents the 'this' object. 

Now that I think of this more, this looks like a MutationObserver specific hack. Can this be more generic? Right now this will fail if there is another callback that does not have an observer parameter.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3081
> +                my @GenerateEventListenerImpl = ();

This is not used anywhere

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3110
> +                push(@implContent, "    return !invokeCallback(m_callback, v8::Handle<v8::Object>::Cast(observerHandle), " . scalar(@params) . ", argv, callbackReturnValue, scriptExecutionContext());\n");

Same here: observerHandle cannot be correct in the general case.

> Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:176
> +    JSValue jsObserver = toJS(exec, m_data->globalObject(), observer);
> +    m_data->invokeCallback(jsObserver, args, &raisedException);

This part need to come from the idl somehow.

[Erik Arvidsson]

Adam (K), how is the this-binding explained in the idl?

[Erik Arvidsson]

This was reverted in http://trac.webkit.org/changeset/113448

[Build Bot]

Comment on attachment 136025 [details]
patch_for_landing

Attachment 136025 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12358255

[Vineet Chaudhary (vineetc)]

Created attachment 136206 [details]
Patch

(In reply to comment #30)
> (From update of attachment 136025 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136025&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2746
> > +    JSValue jsObserver = toJS(exec, m_data->globalObject(), observer);
> > +    m_data->invokeCallback(jsObserver, args, &raisedException);
> 
> observer is a bit too specific. The argument represents the 'this' object. 
> 
> Now that I think of this more, this looks like a MutationObserver specific hack. Can this be more generic? Right now this will fail if there is another callback that does not have an observer parameter.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3081
> > +                my @GenerateEventListenerImpl = ();
> 
> This is not used anywhere
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3110
> > +                push(@implContent, "    return !invokeCallback(m_callback, v8::Handle<v8::Object>::Cast(observerHandle), " . scalar(@params) . ", argv, callbackReturnValue, scriptExecutionContext());\n");
> 
> Same here: observerHandle cannot be correct in the general case.

I tried to using $paramName instead.
 
> > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:176
> > +    JSValue jsObserver = toJS(exec, m_data->globalObject(), observer);
> > +    m_data->invokeCallback(jsObserver, args, &raisedException);
> 
> This part need to come from the idl somehow.

Please let me know if there is some way to get this from idl.
// I am not sure how to do this but Is something below we want?
[PassThis] boolean handleEvent(in sequence<MutationRecord> mutations, in WebKitMutationObserver observer); 

Or we can investigate this with other callbacks and fix then.

Also fixing windows build. //Needs removal from JSBindingsAllInOne.cpp

[WebKit Review Bot]

Attachment 136206 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/scripts/test/JS/JSTestCallback.h:50:  The parameter name "observer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h:55:  The parameter name "observer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vineet Chaudhary (vineetc)]

> > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:176
> > +    JSValue jsObserver = toJS(exec, m_data->globalObject(), observer);
> > +    m_data->invokeCallback(jsObserver, args, &raisedException);
>  
> This part need to come from the idl somehow.
> 
Please let me know if there is some way to get this from idl.
// I am not sure how to do this but Is something like below we want?
[PassThis] boolean handleEvent(in sequence<MutationRecord> mutations, in WebKitMutationObserver observer); 
 
Or we can investigate this with other callbacks and fix then.

Can you please provide your feedback on new patch?

[Erik Arvidsson]

(In reply to comment #36)
> > > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:176
> > > +    JSValue jsObserver = toJS(exec, m_data->globalObject(), observer);
> > > +    m_data->invokeCallback(jsObserver, args, &raisedException);
> >  
> > This part need to come from the idl somehow.
> > 
> Please let me know if there is some way to get this from idl.
> // I am not sure how to do this but Is something like below we want?
> [PassThis] boolean handleEvent(in sequence<MutationRecord> mutations, in WebKitMutationObserver observer); 
> 
> Or we can investigate this with other callbacks and fix then.
> 
> Can you please provide your feedback on new patch?

The WebIDL spec does not provide a way to annotate the 'this' value. By default it is null:

http://dev.w3.org/2006/webapi/WebIDL/#es-invoking-callback-functions

Looking at the spec of MutationCallback, http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers, I really don't see a way to do this in IDL. It seems like this callback is stuck in custom bindings land.

Raf, Adam, do you guys have any ideas?

[Adam Barth]

> Raf, Adam, do you guys have any ideas?

We shouldn't feel constrained by WebIDL.  It's fine for us to invent new attributes in WebKitIDL.  I haven't looked at the details here, but you could imagine inventing one here.

[Erik Arvidsson]

(In reply to comment #38)
> > Raf, Adam, do you guys have any ideas?
> 
> We shouldn't feel constrained by WebIDL.  It's fine for us to invent new attributes in WebKitIDL.  I haven't looked at the details here, but you could imagine inventing one here.

Sure, that was my intention. I still don't see how this can be expressed any other way but prose.

[Vineet Chaudhary (vineetc)]

(In reply to comment #39)
> (In reply to comment #38)
> > > Raf, Adam, do you guys have any ideas?
> > 
> > We shouldn't feel constrained by WebIDL.  It's fine for us to invent new attributes in WebKitIDL.  I haven't looked at the details here, but you could imagine inventing one here.
> 
> Sure, that was my intention. I still don't see how this can be expressed any other way but prose.

Thanks Adam. Erik,

So if I am not getting this wrong, We should introduce new idl attribute to direct the codegenerator. Something like 
[PassThis] boolean handleEvent();

[Vineet Chaudhary (vineetc)]

Created attachment 137291 [details]
Patch

[Erik Arvidsson]

Comment on attachment 137291 [details]
Patch

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.

[Vineet Chaudhary (vineetc)]

Created attachment 137503 [details]
Patch

Thanks arv, I have updated patch with above comments.

>  Also, if there are more than 2 parameters we will invoke the callback multiple times.
Done. Avoiding multiple invocation of callback.

> [PassThis=WebKitMutationObserver] boolean handleEvent(in sequence<MutationRecord> mutations, in WebKitMutationObserver observer);
Done. Now "this" value which needs to pass depends on PassThis="*" type irrespective of the position of arguments.
Added test to TestCallback.idl verifies this behaviour.

[Erik Arvidsson]

Comment on attachment 137503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137503&action=review

This is pretty good at the moment. My main concern is now that this patch does 2 distinct things. PassThis should work with or without sequence.

I think it would be good to split this into two bugs/patches. Sorry.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2746
> +                    next if $codeGenerator->GetArrayType($param->type) or $param->type ne $thisType;

Why can't $thisType be an array type?

next if $param->type ne $thisType;

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2964
> +                    push(@args, GetNativeTypeForCallbacks("${arrayType}Array") . " " . $param->name);

Would it be cleaner if GetNativeTypeForCallbacks took care of whether this was an array type or not?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3073
> +                my @GenerateEventListenerImpl = ();

This is not used

> Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:158
> +    ASSERT(param1);
> +    if (!param1)
> +        return true;

Do we want the same for thisClassParam? My gut feeling is that it is not necessary but an ASSERT might be good in any case.

[Vineet Chaudhary (vineetc)]

(In reply to comment #44)
> (From update of attachment 137503 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137503&action=review
> 
> This is pretty good at the moment. My main concern is now that this patch does 2 distinct things. PassThis should work with or without sequence.
> 
> I think it would be good to split this into two bugs/patches. Sorry.

Oke filed https://bugs.webkit.org/show_bug.cgi?id=84232

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2746
> > +                    next if $codeGenerator->GetArrayType($param->type) or $param->type ne $thisType;
> 
> Why can't $thisType be an array type?

Right should 
next if $param->type ne $thisType;
 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2964
> > +                    push(@args, GetNativeTypeForCallbacks("${arrayType}Array") . " " . $param->name);
> 
> Would it be cleaner if GetNativeTypeForCallbacks took care of whether this was an array type or not?

Will do.

> > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:158
> > +    ASSERT(param1);
> > +    if (!param1)
> > +        return true;
> 
> Do we want the same for thisClassParam? My gut feeling is that it is not necessary but an ASSERT might be good in any case.

Will do.

[Vineet Chaudhary (vineetc)]

Created attachment 137690 [details]
Updated Patch 

Updated patch after bug fix 84232.
I have added test to TestCallback.idl because with sequence<T> codegenerator with callback with [PassThisToCallback] behaves differently.

Please let me know your review comments on this.

[WebKit Review Bot]

Attachment 137690 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Vineet Chaudhary (vineetc)]

Created attachment 137696 [details]
Patch

Sorry for the previous patch :(.

[WebKit Review Bot]

Comment on attachment 137696 [details]
Patch

Attachment 137696 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12424562

[Michael Saboff]

Comment on attachment 137696 [details]
Patch

Is this still desired?  If so, rebase eliminating the V8 references and resubmit.