Bug 84232 – Add PassThis=* to support the callbacks which requires to pass "this" value.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Vineet Chaudhary (vineetc)]

Adding PassThis=XXX to attributes will be useful for the methods/callbacks which requires to pass "this".
This will help to identify the type(XXX) of "this" value in codegenerator.

This is a preparing patch for fixing 83233.
https://bugs.webkit.org/show_bug.cgi?id=83233#c44

[Vineet Chaudhary (vineetc)]

Created attachment 137660 [details]
Patch

[Kentaro Hara]

Comment on attachment 137660 [details]
Patch

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

r- due to missing CodeGenerator*.pm. Please include the change of CodeGenerator*.pm.

> Source/WebCore/bindings/scripts/test/TestObj.idl:161
> +        [PassThis=ThisClaas] void methodRequiresThisToPass(in ThisClass param);

Shouldn't we move this test case to TestCallback.idl, since [PassThis] works for callback interfaces?

Typo: This*Class*

[Vineet Chaudhary (vineetc)]

Created attachment 137669 [details]
Patch

Little worried about codegeneration for GObj, Cpp and ObjC bindings.

[Kentaro Hara]

Comment on attachment 137669 [details]
Patch

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

Almost looks good.

For GObject/CPP/ObjC, let's leave it as-is. If it caused a problem when we actually use [PassThis] in some IDL file, let's add [PassThis] to the Skip() methods in CodeGenerator{GObject,ObjC,CPP}.pm.

> Source/WebCore/ChangeLog:3
> +        Add PassThis=* to support the callbacks which requires to pass "this" value.

Maybe [PassThisToCallback] would be more descriptive. It's up to you though.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2693
>                  push(@args, GetNativeType($param->type) . " " . $param->name);

Nit: You can use $paramName.

> Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:154
> +    ASSERT(thisClassParam);
> +    if (!thisClassParam)

Writing both ASSERT() and if(!...) sounds strange. (I know you just copied the existing code though.)

- If thisClassParam should be always true, just put ASSERT() and remove if(!...).
- If thisClassParam can be false, just put if(!...) and put ASSERT().

> Source/WebCore/bindings/scripts/test/V8/V8TestCallback.cpp:199
> +    ASSERT(thisClassParam);
> +    if (!thisClassParam)

Ditto.

[Kentaro Hara]

> - If thisClassParam should be always true, just put ASSERT() and remove if(!...).
> - If thisClassParam can be false, just put if(!...) and put ASSERT().

Correction: If thisClassParam can be false, just put if(!...) and remove ASSERT().

[Vineet Chaudhary (vineetc)]

Created attachment 137671 [details]
Updated Patch

> For GObject/CPP/ObjC, let's leave it as-is. If it caused a problem when we actually use [PassThis] in some IDL file, let's add [PassThis] to the Skip() methods in CodeGenerator{GObject,ObjC,CPP}.pm.

Currently it doesn't creates ant problem in any of GObject,ObjC,CPP these bindings so we can leave it as-is.

> Maybe [PassThisToCallback] would be more descriptive. It's up to you though.

Right..

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2693
> >                  push(@args, GetNativeType($param->type) . " " . $param->name);
> 
> Nit: You can use $paramName.
Done 

> > Source/WebCore/bindings/scripts/test/JS/JSTestCallback.cpp:154
> > +    ASSERT(thisClassParam);

> Writing both ASSERT() and if(!...) sounds strange. (I know you just copied the existing code though.)

Hmm just copied form the v8 mutation custom code.
 
> - If thisClassParam should be always true, just put ASSERT() and remove if(!...).

thisClassParam should be always true. So lets ASSERT(thisClassParam);

Thanks for commnets..:)

[Kentaro Hara]

Comment on attachment 137671 [details]
Updated Patch

Thanks for the patch!

[WebKit Review Bot]

Comment on attachment 137671 [details]
Updated Patch

Clearing flags on attachment: 137671

Committed r114502: <http://trac.webkit.org/changeset/114502>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Erik Arvidsson]

Comment on attachment 137671 [details]
Updated Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2694
> +                if ($thisType and $thisType eq $param->type) {

I feel like we tend to prefer && over and

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2727
> +                    next if $param->type ne $thisType;

This does not feel correct. We should either say that we pass this as first or last argument. With this code I cannot create a callback like this

[PassThisToCallBack=Node] void myCallback(Node one, Node two, Node three);

[Vineet Chaudhary (vineetc)]

(In reply to comment #10)
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2727
> > +                    next if $param->type ne $thisType;
> 
> This does not feel correct. We should either say that we pass this as first or last argument. With this code I cannot create a callback like this
> 
> [PassThisToCallBack=Node] void myCallback(Node one, Node two, Node three);

As of now there is only callback(I think) which require to pass this as argument ie MutationObserver(mo).

Currently "this" param could not be the first parameter as per the spec http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers 
 "Invoke mo's callback with queue as first argument, and mo (itself) as second argument and callback this value."

IMO passing this as last argument makes sence but then this argunets order should be captured in the spec too may be like
 "Invoke XXX's callback with 'callback this value' as last argument."

Ohterwise someone might try various combinations with arguments. What do you think?

[Erik Arvidsson]

(In reply to comment #11)
> Currently "this" param could not be the first parameter as per the spec http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers 
>  "Invoke mo's callback with queue as first argument, and mo (itself) as second argument and callback this value."

Maybe I'm missing something but the PassThisToCallBack is not part of WebIDL. It is just a way for us to describe how to pass this into the callback function. For example,

[PassThisToCallback=T] void handleEvent();

needs to generate a function that looks something like:

bool V8TestCallback::handleEvent(T* thisParam)

which is then called with the this object. In the case of MutationObserverCallback we need to add one more param to handleEvent to forward the this object.

http://code.google.com/p/chromium/source/search?q=handleEvent+MutationCallback&origq=handleEvent+MutationCallback&btnG=Search+Trunk

[Vineet Chaudhary (vineetc)]

Created attachment 138066 [details]
raw_patch_not_to_ews

(In reply to comment #12)
> (In reply to comment #11)
> > Currently "this" param could not be the first parameter as per the spec http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#mutation-observers 
> >  "Invoke mo's callback with queue as first argument, and mo (itself) as second argument and callback this value."
> 
> Maybe I'm missing something but the PassThisToCallBack is not part of WebIDL. It is just a way for us to describe how to pass this into the callback function. For example,
> 
> [PassThisToCallback=T] void handleEvent();
> 
> needs to generate a function that looks something like:
> 
> bool V8TestCallback::handleEvent(T* thisParam)
> 
> which is then called with the this object. In the case of MutationObserverCallback we need to add one more param to handleEvent to forward the this object.
> 
> http://code.google.com/p/chromium/source/search?q=handleEvent+MutationCallback&origq=handleEvent+MutationCallback&btnG=Search+Trunk

I have prepared raw patch just to clear my understanding. Patch creates  bool V8TestCallback::handleEvent(T* thisParam) for [PassThisToCallback=T] void handleEvent(); 
Is this did you meant for? Please correct me if my understanding wrong.

I am afraid if this is correct way because when I proceed further and added one more param to handleEvent() to forward the this object for MutationObserverCallback, but I messed up with codegenerator*.pm. Specifically for V8Bindigs I need to repeat of almost every line of code. I can share patch if you want to have a look.

[Vineet Chaudhary (vineetc)]

IMO we should always consider last param as thisParam if extendedAttribute is PassThisToCallBack like:

[PassThisToCallBack] void myCallback(Node one, Node two, Node this) which generates

void myCallback(Node* one, Node* two, Node* this)

This way we don't require to paas thisType with PassThisToCallBack. This will simplify codegenerator code and solve all other concerns.

FOR MutationObserver code will look like:

[PassThisToCallback] boolean handleEvent(in sequence<MutationRecord> mutations, in WebKitMutationObserver observer);  //which generates

boolean handleEvent(MutationRecordArray* mutations, WebKitMutationObserver* observer); //which ever is the last parm consider as this if PassThisToCallback.

Please let me know if you have any suggestion/objections on this approach.

[Erik Arvidsson]

Lets back up a bit. Callbacks are used for WebCore to call a user defined JavaScript function. The IDL defines what parameters are passed as real arguments. The IDL does not define what [[This]] should be, it is done in prose.

In cases where the js callback object is a function we want to call the js function using the [PassThisToCallback=T] as the [[This]] implicit argument. If the callback is a non function object we use the object itself as the [[This]]. A few examples:

var element = ...

var object = {
  handleEvent: function(e) {
    assert(this === object);
  }
};

function f(e) {
  assert(element === this)
}

element.addEventListener('click', f);
element.addEventListener('click', object);

And the IDL for this is here: http://www.w3.org/TR/2012/WD-dom-20120405/#interface-eventtarget

interface EventTarget {
  void addEventListener(DOMString type, EventListener? callback, optional boolean capture = false);
  ...
};

callback interface EventListener {
  void handleEvent(Event event);
};


The event target is a bit special since we have custom code to get the currentTarget out of the event and use that as [[This]]

 I would like us to manually pass in [[This]] from C++ in the cases where we have [PassThisToCallback].

[Erik Arvidsson]

To sum up: 

If [PassThisToCallback] is present the first argument passed needs to be the this pointer