With 4-word strings there is no need in JS_AddExternalStringFinalizer API as the finalizer can be stored directly in the string. This should allow to simplify external string implementation and usage.
Created attachment 594964 [details] [diff] [review]
The patch removes JS_(AddRemove)ExternalStringFinalizer and changes JS_NewExternalString to take the finalizer directly. I also changed the finalizer to be a struct holding a pointer to the finalization callback. This allowed to remove JS_NewExternalStringWithClosure as the embedding can just subclass JSStringFinalizer.
I also changed the signature of the callback to take char * pointer, not the original JSString. This is to facilitate the forthcoming patch to finalize the external strings in the background and to avoid the needless JS_GetStringCharsZ or similar calls in the finalizers.
Created attachment 594971 [details] [diff] [review]
The patch removes the rt argument from the finalizer callback. It has no use in FF and, if necessary, an embedding can store any relevant pointers in the finalizer itself.
Righteous. This has always bugged me.
ejpbruel: I think this warrants and update to your blog post ;)
So how can finalizer now get access to the closure data or some such.
The old API was more useful.
One could use static class/method as finalizer and pass some relevant data as closure.
Now one would need to create a new instance of the finalizer each time external string is created.
Not good if we want to share for example nsIAtom data as external strings in a fast way.
It looks like noone was really using the closure word when the bug was filed so Igor just removed it. However, there is still space in the JSString header to store a closure word (String.d.s.u3) so if you think this is an important use case we can just put back JS_NewExternalStringWithClosure and add the closure parameter to the callback).
(In reply to Luke Wagner [:luke] from comment #8)
> It looks like noone was really using the closure word when the bug was filed
> so Igor just removed it. However, there is still space in the JSString
> header to store a closure word (String.d.s.u3) so if you think this is an
> important use case we can just put back JS_NewExternalStringWithClosure and
> add the closure parameter to the callback).
If you open a bug for this I'm willing to take it. I should be able to make time for this next week.
Sweet! Olli already did: bug 776000.