[webkit-reviews] review requested: [Bug 38112] [Qt] QScriptValue::toString has a memory leak. : [Attachment 54912] Fix v2

[bugzilla-daemon at webkit.org]

Jędrzej Nowacki <jedrzej.nowacki at nokia.com> has asked  for review:
Bug 38112: [Qt] QScriptValue::toString has a memory leak.
https://bugs.webkit.org/show_bug.cgi?id=38112

Attachment 54912: Fix v2
https://bugs.webkit.org/attachment.cgi?id=54912&action=review

------- Additional Comments from Jędrzej Nowacki <jedrzej.nowacki at nokia.com>
(In reply to comment #2)
Thanks for review!
> (From update of attachment 54273 [details])
> This looks OK, but I think it could be better.
> 
> 1.  We should probably have QScriptConverter handle this correctly.  i.e.
have
> a toString() method which takes a value() and context() directly, no?
I don't think that it is a good idea or at least not for now. 
 - For simplicity, QtScript ignores exceptions, but it is a temporary solution,
they will be exposed trough the QScriptEngine API. Methods of QScriptConverter
are not good enough, for exception handling. Moreover for exception handling we
will need QScriptEnginePrivate pointer, JSContextRef won't be sufficient, so it
means that QScriptConverter couldn't be inlined inside QScriptEnginePrivate.
 - I think that context-less part of the QScriptConverter API should be moved
to JSC C API as a generic JavaScript API (toArrayIndex(), toString(double)...)
 - For conversion, I like a function that takes argument and converts it
without taking an additional factor (like context). But of course it is a
matter taste.
I'm going to reconsider/rethink QScriptConverter idea on exception handling
implementation stage.

> 2.  Isn't there a RetainPtr to use here so that you never have to release the

> string explicitly?  Then it would look something like this:
> 
> RetainPtr<JSStringRef> string = JSValueToStringCopy();
> return QScriptConverter::toString(string.get());
Good hint :-). I changed the path to use RetainPtr.