Bug 160032 - Adding JSObjectGetClass method
Summary: Adding JSObjectGetClass method
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-21 10:44 PDT by Danilo de Paula
Modified: 2016-09-09 03:50 PDT (History)
8 users (show)

See Also:


Attachments
Patch (4.30 KB, patch)
2016-07-21 10:47 PDT, Danilo de Paula
no flags Details | Formatted Diff | Diff
Patch (4.31 KB, patch)
2016-07-21 10:58 PDT, Danilo de Paula
no flags Details | Formatted Diff | Diff
Patch (4.32 KB, patch)
2016-07-21 11:29 PDT, Danilo de Paula
no flags Details | Formatted Diff | Diff
Patch (4.88 KB, patch)
2016-07-21 18:11 PDT, Danilo de Paula
mcatanzaro: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Danilo de Paula 2016-07-21 10:44:16 PDT
Basically I ended up in the following situation:

I need to create a new Object (obj2) the same type of another one (obj1).
Ideally I would do:

JSClassRef clas = JSObjectGetClass(obj2);
JSObjectRef obj2 = JSObjectMake(context, clas, NULL);


However the current API set doesn't let me get the class used to create the original object.
This patch is about this.

Also, I can see this same API being used to create/update constructors for an object (specially if the constructor wasn't defined in the original object when it was first created).
Comment 1 Danilo de Paula 2016-07-21 10:47:53 PDT
Created attachment 284229 [details]
Patch
Comment 2 Danilo de Paula 2016-07-21 10:58:37 PDT
Created attachment 284230 [details]
Patch
Comment 3 Danilo de Paula 2016-07-21 11:01:51 PDT
I'm adding some developers that might be able to review this patch...
Comment 4 Danilo de Paula 2016-07-21 11:29:43 PDT
Created attachment 284232 [details]
Patch
Comment 5 Danilo de Paula 2016-07-21 11:30:36 PDT
(In reply to comment #4)
> Created attachment 284232 [details]
> Patch

Fixed a missing JS_EXPORT. Hope that fix the build issue on OSX.
Comment 6 Joseph Pecoraro 2016-07-21 14:40:25 PDT
Comment on attachment 284232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284232&action=review

I'll let someone else evaluate this from a new API perspective, but there is some low hanging fruit you can cleanup in the patch in the process.

> Source/JavaScriptCore/ChangeLog:10
> +        Usefull to create other objects with the same classe and constructor.

A few typos here.

> 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.

> Source/JavaScriptCore/API/JSObjectRef.cpp:248
> +    return 0;

Style: New code should use nullptr instead of 0 or NULL.

> 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).

> 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.

> Source/JavaScriptCore/API/JSObjectRef.h:500
> +@result A JSClassRef that was used to create the  object.

Nit: Fix the double space in here.

> 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.

> 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.

> 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.
Comment 7 Danilo de Paula 2016-07-21 16:28:43 PDT
Comment on attachment 284232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284232&action=review

>> Source/JavaScriptCore/ChangeLog:10
>> +        Usefull to create other objects with the same classe and constructor.
> 
> A few typos here.

Fixed.

>> 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.

I agree with that. I used unchecked because I took as example the JSObjectGetPrivate, that uses unchecked. Fixed anyway.

>> Source/JavaScriptCore/API/JSObjectRef.cpp:248
>> +    return 0;
> 
> Style: New code should use nullptr instead of 0 or NULL.

Fixed.

>> 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).

Oh, didn't noticed the Lock thing. I'm fixing it...

>> 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.

I took it out as the styler complained that "object" has no meaning... Makes sense to let it there and ignore the warning...

>> Source/JavaScriptCore/API/JSObjectRef.h:500
>> +@result A JSClassRef that was used to create the  object.
> 
> Nit: Fix the double space in here.

nice catch.

>> 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.

CF_AVAILABLE(10_12, 10_0)? If no, I guess I will need a small guidance on that.

>> 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.

I guess your're right.

>> 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.

Fixed.
Comment 8 Danilo de Paula 2016-07-21 18:11:01 PDT
Created attachment 284295 [details]
Patch
Comment 9 WebKit Commit Bot 2016-07-21 18:12:02 PDT
Attachment 284295 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSObjectRef.h:502:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Danilo de Paula 2016-07-21 18:17:23 PDT
(In reply to comment #8)
> Created attachment 284295 [details]
> Patch

So, the latest patch contains the suggestion made by Joseph. Thanks Joseph!

Answering Joseph's question regarding to tests: I do expect to get a nullptr in all cases you mentioned (functions, global, date, etc).

I wrote some smoke tests regarding to it and it seems to be working that way. I can send a new version of the test when I get the API reviewed.
Comment 11 Danilo de Paula 2016-07-21 18:18:44 PDT
(In reply to comment #9)
> Attachment 284295 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/API/JSObjectRef.h:502:  The parameter name
> "object" adds no information, so it should be removed. 
> [readability/parameter_name] [5]
> Total errors found: 1 in 4 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Yep, that's the problem I mentioned in Joseph's comment.

webkit-style complains that I use the parameter name "object", although all the rest of the API uses it in that way. Should I just ignore this warning or is there something else I should do?
Comment 12 Joseph Pecoraro 2016-07-21 20:01:18 PDT
> webkit-style complains that I use the parameter name "object", although all
> the rest of the API uses it in that way. Should I just ignore this warning
> or is there something else I should do?

You can ignore it here.
Comment 13 Geoffrey Garen 2016-07-24 09:19:09 PDT
This seems like a bad design.

The example code you've provided is broken because it unconditionally passes NULL to JSObjectMake, but many classes will require valid private data upon construction.

Can you give a more in-depth example of a program that would use this technique?
Comment 14 Michael Catanzaro 2016-09-08 20:48:49 PDT
Comment on attachment 284295 [details]
Patch

Can you respond to Geoff's comment?
Comment 15 Danilo de Paula 2016-09-09 03:50:36 PDT
(In reply to comment #13)
> This seems like a bad design.
> 
> The example code you've provided is broken because it unconditionally passes
> NULL to JSObjectMake, but many classes will require valid private data upon
> construction.
> 
> Can you give a more in-depth example of a program that would use this
> technique?

The example I mentioned it's close to my case. In a certain condition I need
to create an object of the same class of some other object I got.

JSClassRef clas = JSObjectGetClass(obj1);
JSObjectRef obj2 = JSObjectMake(context, clas, NULL);

The example I used here doesn't set a private data upon construction, but it shouldn't matter, I could set a pointer there and the example for the API I'm proposing would be the same.

As I mentioned in the mailing list, this same API is present in SpiderMonkey. I understand that the API dealing with JSObjectRef is stable for a while, however this API is simple, consistent and have already been proven useful by others javascript engines.