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

[bugzilla-daemon at webkit.org]

Vineet Chaudhary (vineetc) <rgf748 at motorola.com> has asked  for review:
Bug 83233: Add CodeGenerator support for sequence<> in callbacks
https://bugs.webkit.org/show_bug.cgi?id=83233

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

------- Additional Comments from Vineet Chaudhary (vineetc)
<rgf748 at motorola.com>
(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