<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Adding JSObjectGetClass method"
href="https://bugs.webkit.org/show_bug.cgi?id=160032#c7">Comment # 7</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Adding JSObjectGetClass method"
href="https://bugs.webkit.org/show_bug.cgi?id=160032">bug 160032</a>
from <span class="vcard"><a class="email" href="mailto:danilo.cesar@collabora.co.uk" title="Danilo Cesar Lemes de Paula <danilo.cesar@collabora.co.uk>"> <span class="fn">Danilo Cesar Lemes de Paula</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=284232&action=diff" name="attach_284232" title="Patch">attachment 284232</a> <a href="attachment.cgi?id=284232&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=284232&action=review">https://bugs.webkit.org/attachment.cgi?id=284232&action=review</a>
<span class="quote">>> Source/JavaScriptCore/ChangeLog:10
>> + Usefull to create other objects with the same classe and constructor.
>
> A few typos here.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/API/JSObjectRef.cpp:244
>> + JSObject* jsObject = uncheckedToJS(object);
>
> As public API we shouldn't do an unsafe cast like this, it would be better to use the regular toJS() like other API functions, which looks like it would fail if clients call this incorrectly.</span >
I agree with that. I used unchecked because I took as example the JSObjectGetPrivate, that uses unchecked. Fixed anyway.
<span class="quote">>> Source/JavaScriptCore/API/JSObjectRef.cpp:248
>> + return 0;
>
> Style: New code should use nullptr instead of 0 or NULL.</span >
Fixed.
<span class="quote">>> Source/JavaScriptCore/API/JSObjectRef.h:498
>> +@param ctx The execution context to use.
>
> Nit: There is no ctx parameter here. There probably should be one, used just like the other API methods to take the API lock (JSLockHolder).</span >
Oh, didn't noticed the Lock thing. I'm fixing it...
<span class="quote">>> Source/JavaScriptCore/API/JSObjectRef.h:499
>> +@param object A JSObject whose prototype you want to get.
>
> Nit: You are referencing a param named "object" so you should give the parameter this name in the signature below.</span >
I took it out as the styler complained that "object" has no meaning... Makes sense to let it there and ignore the warning...
<span class="quote">>> Source/JavaScriptCore/API/JSObjectRef.h:500
>> +@result A JSClassRef that was used to create the object.
>
> Nit: Fix the double space in here.</span >
nice catch.
<span class="quote">>> Source/JavaScriptCore/API/JSObjectRef.h:502
>> +JS_EXPORT JSClassRef JSObjectGetClass(JSObjectRef);
>
> As a new API, this would need an availability macro CF_AVAILABLE(..., ...). I'm not sure what the right values to fill in would be. WebKit2 has WK_MAC_TBA values that are post-processed. It would be similar here but the machinery would need to be added.</span >
CF_AVAILABLE(10_12, 10_0)? If no, I guess I will need a small guidance on that.
<span class="quote">>> Source/JavaScriptCore/API/tests/testapi.c:1312
>> + if (myObjectClassRef != JSObjectGetClass(myObject)) {
>
> What happens if you try to get the class of a built-in JavaScript object that you did not create with a ClassRef? For example, the class of the Global object, a Function instance? It would be good to have tests for the non-basic/expected cases.</span >
I guess your're right.
<span class="quote">>> Source/JavaScriptCore/API/tests/testapi.c:1313
>> + printf("FAIL: JSObjectGetClass didn't return the correct JSClassRef \n");
>
> Nit: Follow suit with the others and end in a period instead of a space.</span >
Fixed.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>