[webkit-reviews] review canceled: [Bug 93476] [Qt] Port meta method/signal/slot handling in run-time bridge to use JSC C API : [Attachment 157731] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has canceled Simon Hausmann
<hausmann at webkit.org>'s request for review:
Bug 93476: [Qt] Port meta method/signal/slot handling in run-time bridge to use
JSC C API
https://bugs.webkit.org/show_bug.cgi?id=93476

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=157731&action=review


Great, thanks a lot for the review, Caio! I'm taking this patch now out of the
review queue to revise it.

>> Source/WebCore/bridge/qt/qt_class.cpp:110
>> +		return toJS(method->jsObjectRef(context, exception));
> 
> We could try to avoid this duplication. One possible approach could be first
figure out if there's a valid index and then if so run this block of code.
> 
> // Sketchy pseudo code.
> int index = -1;
> if (index = indexOfMethod(normal)) {
>     if (isPrivate)
>	  index = -1;
> }
> 
> if (index == -1) {
>     // Search by basename.
> }
> 
> if (index == -1) {
>     return jsUndefined;
> }
> 
> // Create runtime method with index.
> 
> 
> Optional: I'm also wondering while reading this if we could just "return
early" if the exact match is private, simplifying the code further. This would
change behavior for cases where our object have a private foo(int) and another
public foo(), and we are asking explicitly for obj["foo(int)"]. Currently we
return the method for "foo", I think should be OK to return jsUndefined in this
case.

Good idea, I'll play with cleaning this up a little bit.

>> Source/WebCore/bridge/qt/qt_instance.cpp:97
>> +	// Clean up (unprotect from gc) the JSValues we've created.
> 
> This comment doesn't seem to be truth. If I got this right, we are not GC
protecting the methods.

That's true, since the methods aren't JS objects anymore.

>> Source/WebCore/bridge/qt/qt_runtime.cpp:1300
>> +	m_classRef = JSClassCreate(&classDef);
> 
> Do we need a different class for each meta method?

If we want to store the identifier in it then yes.

>> Source/WebCore/bridge/qt/qt_runtime.cpp:1311
>> +	QObject* obj = d->m_object;
> 
> We might have a problem here.
> 
> Without this patch runtime methods are JS values, and our method table stored
weak references for it. Even if we cleared our method table, a reference for a
method in JS would keep its QtRuntimeMetaMethod alive.
> 
> With the patch, QtRuntimeMethod objects themselves are deleted when we clear
the method table. So if we still have a reference for that method in JS, and
call it, we'll end up here with an invalid QtRuntimeMethod pointer.
> 
> One solution I could think of is adding a Null check here for the
JSObjectGetPrivate result, and in QtRuntimeMethod destructor, if we have
m_jsObject, use JSObjectSetPrivate to clear its private data.

That's a good idea. I have to check how this combines with my follow-up patches
that change the storage model slightly. But your point seems valid.

>> Source/WebCore/bridge/qt/qt_runtime.cpp:1374
>> +	JSStringRef actualNameStr =
JSStringCreateWithUTF8CString(m_identifier.constData());
> 
> Use JSRetainPtr then you can avoid the JSStringRelease below.

Yeah, I guess :)

>> Source/WebCore/bridge/qt/qt_runtime.cpp:1442
>> +		targetFunction = JSValueToObject(context, arguments[1],
exception);
> 
> In this case we need to check if arguments[1] is really a function.
> 
> Also: in the previous version we treated the case in which arguments[1] was a
string, for example: 'object.signal.connect(targetObject, "slotName")'. I think
we could still do it here. Even better if we had a test for it. :)

Well spotted! I'll try to fix this.