RFC --- new scripting extension sketches.

[koos vriezen]

2010/5/16 Maksim Orlovich <mo85 at cornell.edu>:
>> 2010/5/12 Maksim Orlovich <mo85 at cornell.edu>:
>>> <snip>
>>> OK, I went ahead and committed a new version of this extension. I've
>>> also
>>> ported KHTML to it, and provided a backwards compatibility adapter that
>>> bridges things that implement LiveConnectExtension to the new interface.
>>
>> Awesome work!
>> Small nitpick is maybe a constructor for Object/FunctionRef that takes
>> all members to make QVariant::setValue an one liner (be it a long
>> line).
>
> How would that be different from those that are currently there?

Completely overlooked the second constructor. Forget this remark :)

>>> Thus far, nspluginviewer seems to work fine, as do the basic methods in
>>> kmplayer -- unfortunately it doesn't want to launch knnplayer for me for
>>> some reason.
>>
>> Odd, seem to work fine for me.
>
> Well, I am probably doing something stupid setup-wise --- it seems to want
> to use mplayer to play flash (on a swf, not a flv)

Hmm, it shouldn't. Flash mimetype is hardcoded to npplayer, maybe the
mime by content checking is failing for you.
flv isn't supported w/o the surrounding .swf afaik. Besides, mplayer
plays them much less cpu hungry.


>> I went ahead with a simple Play/Stop function support for any media.
>> It works, but only because I don't need object live time tracking for
>> this. The issue I see is when calling 'embedId.Play(), with this as
>> debug output:
>>
>> rootObject
>> acquire 0
>> KMPlayerScriptableExtension::get 0 Play
>> acquire 1
>> release 0
>> callAsFunction
>> callMediaFunction
>> release 1
>>
>> Now the call to 'rootObject' returns object with Id 0 and the 'get'
>> for function 'Play', returns objId 1. Objects returned should have
>> refCount set to 1
>
> That's not how it's documented right now. Objects start with refcount of 0
> --- I can certainly change that, if there is a huge reason to.
>
>> to make it not initial unowned, since the callee
>> can't deref the object after returning it.
>
> Ahh. I see. So you want the callee to be able to consistently deref
> arguments, so you don't end up with them leaking when it doesn't retain
> them permanently? I guess I'll change stuff then when I get a chance.

The issue I think is that if the caller isn't interested in the return
value, a refcount of 0 means the allocated memory is never freed.

I would think that the caller always is responsible for the live time
tracking. So get/rootObject/enclosingObject and the return value of
'call', which all can return a Object/FunctionRef, should be ref'ed by
the callee before returning, so the caller can store it without
acquiring it, and should release it after use. Saves a dbus round trip
too.

My understanding for 'call' and 'put' argument(s) are that the callee
should only ref it when it wants to store it (obviously 'put' always
does that), since after the call, the passed object(s) might be freed.
I.e. the arguments are const. Which means that the caller can also not
pass a zero refcounted object, unless it checks that the refcount
hasn't become >0 (which it can't in the current interface).
Also the callee might want to acquire it, later on in the same call,
it decides to release it again, the object becomes dangling.
(Just to write down how I expect it to work, correct me if I'm wrong)


> Perhaps simply stating that you ref values you pass as arguments or
> return, deref those that you get as parameters or return value, too?

See above, so no need to deref parameters I would think.


> Hmm, may be worth adding some sort of a helper, too.
>
> e.g.
> static void acquireObject(QVariant)/releaseObject(QVariant), that
> call the appropriate virtual acquire/release on function/value types.
>
>
>> At least that is my understanding of it. Here how I implemented the
>> function that evaluates a script in the host browser:
>>
>>     KParts::ScriptableExtension* host = m_scriptableextension->host ();
>>     if (host) {
>>         QVariant embed = m_scriptableextension->enclosingObject ();
>>         if (embed.canConvert <KParts::ScriptableExtension::Object> ()) {
>>             quint64 id = embed.value
>> <KParts::ScriptableExtension::Object> ().objId;
>>             QVariant result = host->evaluateScript
>> (m_scriptableextension, id, script);
>>             host->release (id);
>>             return result.toString ();
>>         }
>>     }
>>     return "undefined";
>>
>> As you can see, I only released the enclosing object.
>
> Regardless of that you may have to release the value you got, since
> it may be an object, too.

Good point, thanks.


> Also, from the other mail:
>
>> Shouldn't evaluateScript return a QString, ie the result of the
>> evalutation? If I'm not mistaken, ns plugins really depend on the
>> string outcome of js script evaluation.
>
> Well, NPN_Evaluate is supposed to return typesafe values. Perhaps we
> should add a toString function to the interface to permit both uses?

Hmm, QVariant should be find. I was merely looking at
../khtml/ecma/kjs_scriptable.cpp:849

return scriptableNull();

which looks like it always returns a null object, no? (Yep there is a
TODO above it :)


Koos

> Thanks,
> Maks
>
>
>