Bug 105889 – Objective-C API for JavaScriptCore

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

JavaScriptCore should support an Objective-C API.  The C API is great for portability, but when interfacing with Objective-C code we could make type conversion, object life cycle management, etc much more automatic.

[Gavin Barraclough]

Created attachment 180985 [details]
First cut

[WebKit Review Bot]

Attachment 180985 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/APIJSValue.h', u..." exit_code: 1
Source/JavaScriptCore/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/JavaScriptCore/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/JavaScriptCore/API/APIJSValue.h:147:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/API/APIJSValue.h:287:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:116:  Extra space before [  [whitespace/braces] [5]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:126:  Missing space inside { }.  [whitespace/braces] [5]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:171:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:154:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:175:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:190:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/API/JSValueInternal.h:39:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:39:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:40:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:40:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:41:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:41:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:42:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:42:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:43:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:43:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:44:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:44:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:45:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSValueInternal.h:45:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSContextInternal.h:45:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSContextInternal.h:57:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/API/JSVirtualMachineInternal.h:33:  The parameter name "virtualMachine" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 27 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Created attachment 180986 [details]
Style fixes

[WebKit Review Bot]

Attachment 180986 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/APIJSValue.h', u..." exit_code: 1
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:116:  Extra space before [  [whitespace/braces] [5]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:174:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:189:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Patch up for review.

The file APIJSValue.h should really be named JSValue.h, but I'd propose landing as is, then landing a patch to rename JSValue.h -> JSCJSValue.h, then a patch to rename APIJSValue.h -> JSValue.h.  (Without the new API, there is no justification for the rename).

There are a number of FIXMEs tracking further work needed, these all reference bugs:
    https://bugs.webkit.org/show_bug.cgi?id=105891
    https://bugs.webkit.org/show_bug.cgi?id=105892
    https://bugs.webkit.org/show_bug.cgi?id=105894
    https://bugs.webkit.org/show_bug.cgi?id=105895

[Gavin Barraclough]

The remaining style issues seem bogus; looks like stylebot is being thrown by Objective-C.

[WebKit Review Bot]

Comment on attachment 180986 [details]
Style fixes

Attachment 180986 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15625156

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Build Bot]

Comment on attachment 180986 [details]
Style fixes

Attachment 180986 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15626177

[Gavin Barraclough]

Created attachment 180992 [details]
Fixes for the build bot

[WebKit Review Bot]

Attachment 180992 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/APIJSValue.h', u..." exit_code: 1
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:116:  Extra space before [  [whitespace/braces] [5]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:174:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:189:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 180992 [details]
Fixes for the build bot

Attachment 180992 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15627270

[Gavin Barraclough]

Created attachment 180993 [details]
Build bot fix

[WebKit Review Bot]

Attachment 180993 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/APIJSValue.h', u..." exit_code: 1
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:116:  Extra space before [  [whitespace/braces] [5]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:174:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:189:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 180993 [details]
Build bot fix

Attachment 180993 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15603853

[Gavin Barraclough]

Created attachment 180994 [details]
Build bot fix

[WebKit Review Bot]

Attachment 180994 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/APIJSValue.h', u..." exit_code: 1
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:116:  Extra space before [  [whitespace/braces] [5]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:174:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:189:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 180994 [details]
Build bot fix

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

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:264
> +// Helper function to add offset information back into a bock signature.

Typo: block

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:296
> +    // The first argument to a block is the bloxk itself.

Typo in comment.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:317
> +    // Prefic the signature with the return type & total stackframe size.

Typo: prefic

[Filip Pizlo]

Comment on attachment 180994 [details]
Build bot fix

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

The comments about anonymous namespaces are strictly optional - it's just a style that I sort of like. 

But you should definitely think about the multiple inheritance case. I think that's a bug. 

Holding off r+ until we figure out the multi inheritance part.

> Source/JavaScriptCore/API/JSValue.mm:630
> +class JSContainerConvertor {

I would suggest putting classes like these (declared + defined in implementation files and invisible from other modules) in anonymous namespaces ("namespace { ... }"). You could then shorten the names and omit the 'JS' prefix since you'll not have to worry about namespace pollution.

> Source/JavaScriptCore/API/JSValue.mm:721
> +static id containerValueToObject(JSGlobalContextRef context, JSContainerConvertor::Task task)

If this code was in an anonymous namespace then 'static' could be omitted.

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:53
> +            forEachProtocolImplementingProtocol(protocols[i], protocol, callback);

Thus seems like it will repeatedly visit the same protocol in case of multiple inheritance. I believe that this should use a DFS type formulation - ie a work list of protocols to visit and a hashset of already-seen protocols.

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:66
> +    if (protocolsCount) {

Seems like the part of this code once you have a Protocol** should be in a shared helper for the Class and Protocol cases.

[Filip Pizlo]

(In reply to comment #18)
> (From update of attachment 180994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180994&action=review
> 
> The comments about anonymous namespaces are strictly optional - it's just a style that I sort of like. 
> 
> But you should definitely think about the multiple inheritance case. I think that's a bug. 
> 
> Holding off r+ until we figure out the multi inheritance part.
> 
> > Source/JavaScriptCore/API/JSValue.mm:630
> > +class JSContainerConvertor {
> 
> I would suggest putting classes like these (declared + defined in implementation files and invisible from other modules) in anonymous namespaces ("namespace { ... }"). You could then shorten the names and omit the 'JS' prefix since you'll not have to worry about namespace pollution.

Just realized that the reason for the 'JS' prefix has nothing to do with namespace pollution but rather is meant to distinguish between this and the 'ObjC' version. 

Still, I would have used an anonymous namespace to make clear that this is a utility class that is internal to this file. 

> 
> > Source/JavaScriptCore/API/JSValue.mm:721
> > +static id containerValueToObject(JSGlobalContextRef context, JSContainerConvertor::Task task)
> 
> If this code was in an anonymous namespace then 'static' could be omitted.
> 
> > Source/JavaScriptCore/API/ObjcRuntimeExtras.h:53
> > +            forEachProtocolImplementingProtocol(protocols[i], protocol, callback);
> 
> Thus seems like it will repeatedly visit the same protocol in case of multiple inheritance. I believe that this should use a DFS type formulation - ie a work list of protocols to visit and a hashset of already-seen protocols.
> 
> > Source/JavaScriptCore/API/ObjcRuntimeExtras.h:66
> > +    if (protocolsCount) {
> 
> Seems like the part of this code once you have a Protocol** should be in a shared helper for the Class and Protocol cases.

[Gavin Barraclough]

Created attachment 181003 [details]
DFS, typo fixes

[WebKit Review Bot]

Attachment 181003 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/APIJSValue.h', u..." exit_code: 1
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:138:  Extra space before [  [whitespace/braces] [5]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:196:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:211:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 181003 [details]
DFS, typo fixes

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

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:44
> +inline void forEachProtocolImplementingProtocol(Protocol* base, Protocol* protocol, void (^callback)(Protocol*))

This method seems like it's not used anymore, and is also still wrong, since it doesn't do DFS.  I think this method was just a helper for the one below, before you implemented DFS?

[Gavin Barraclough]

Created attachment 181004 [details]
Removed redundant forEachProtocolImplementingProtocol

[Filip Pizlo]

Comment on attachment 181004 [details]
Removed redundant forEachProtocolImplementingProtocol

r=me!

[WebKit Review Bot]

Attachment 181004 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/APIJSValue.h', u..." exit_code: 1
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:121:  Extra space before [  [whitespace/braces] [5]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:179:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/API/ObjcRuntimeExtras.h:194:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Fixed in r138604

[Darin Adler]

Comment on attachment 181004 [details]
Removed redundant forEachProtocolImplementingProtocol

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

Formatting of Objective-C type pointers here is all over the map. Our coding style says “NSInvocation *” but I don’t feel strongly about that except in public headers. Maybe we can change the WebKit coding style. I wish this patch was self-consistent about that, though. But that’s less important than other comments I made here.

The use of the WTF:: prefix here is extensive and unnecessary. WTF is designed to be used without having to use an explicit prefix except in a few unusual cases.

> Source/JavaScriptCore/API/APIJSValue.h:37
> +// The JSContext is used to manage the life-cycle of the refereced JavaScript

Typo: refereced

> Source/JavaScriptCore/API/APIJSValue.h:39
> +// the JSValue will continue to keep the referencd value within the JavaScript

Typo: referencd

> Source/JavaScriptCore/API/APIJSValue.h:50
> +// (the associated JSVirtualMachine is avaible indirectly via the context

Typo: avaible

> Source/JavaScriptCore/API/APIJSValue.h:93
> +// class methods will be returned. See JSExport.h sor more information on

Typo: sor

> Source/JavaScriptCore/API/APIJSValue.h:95
> +NS_CLASS_AVAILABLE(10_9, NA)

I suggest adding a blank line before this line.

> Source/JavaScriptCore/API/APIJSValue.h:118
> +- (id)toObjectOfClass:(Class)cls;

I’m not a big fan of “cls” to avoid the C++ class keyword. I suggest something like expectedClass. I won’t comment elsewhere, but I think use of the abbreviation cls is an anti-pattern.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:43
> +id __NSMakeSpecialForwardingCaptureBlock(const char *signature, void (^handler)(NSInvocation *inv));

No need to abbreviate invocation here as “inv”.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:44
> +};

Extra semicolon here.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:60
> +    JSValueRef get(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef*)

Should specify both virtual and override here.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:71
> +    JSValueRef get(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef*)

Should specify both virtual and override here.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:81
> +    JSValueRef get(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef*)

Should specify both virtual and override here.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:98
> +        m_allocation = (char*)malloc(size + alignment);

Should use static_cast here instead of C-style cast.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:108
> +    virtual JSValueRef get(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef*)

Should specify override here.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:151
> +        return nil;

Is nil right here? Normally we use nil only for Objective-C object pointer types.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:166
> +        return nil;

Is nil right here? Normally we use nil only for Objective-C object pointer types.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:174
> +        return nil;

Is nil right here? Normally we use nil only for Objective-C object pointer types.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:189
> +    void set(NSInvocation*, JSContext*, JSValueRef, JSValueRef*)

Should specify both virtual and override here.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:197
> +    void set(NSInvocation* invocation, JSContext* context, JSValueRef result, JSValueRef* exception)

Should specify both virtual and override here.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:207
> +    void set(NSInvocation* invocation, JSContext* context, JSValueRef result, JSValueRef* exception)

Should specify both virtual and override here.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:223
> +class BlockResultStruct : public BlockResult {

Is there a way to share some of the code with BlockArgumentStruct? The two classes seem quite close in implementation.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:232
> +        m_allocation = (char*)malloc(size + alignment);

Should use static_cast here instead of C-style cast.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:242
> +    void set(NSInvocation* invocation, JSContext* context, JSValueRef result, JSValueRef*)

Should specify override here.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:256
> +@implementation JSBlockAdaptor {

The use of m_ prefixes for Objective-C object fields is a bit non-standard.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:258
> +    WTF::RetainPtr<NSString> m_signatureWithOffsets;
> +    WTF::RetainPtr<NSString> m_signatureWithoutOffsets;

Should not need these WTF:: prefixes here. WTF works without a prefix.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:268
> +    NSString* result = [NSString stringWithFormat:@"%@%s%lu%@", prefix, copy.get(), offset, postfix];

The %lu format string is not right for an NSUInteger. If you want to use %lu you will need to cast it to an unsigned long or find some other way to get the format string right.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:293
> +    if (encodedType[0] != '@' || encodedType[1] != '?') {
> +        return self;
> +    }

WebKit coding style omits the braces in a case like this.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:300
> +    OwnPtr<BlockArgument> arguments = 0;

No need for this "= 0" for an OwnPtr.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:319
> +    // which is a nonsesne, but harmless since this is the last use).

Typo: a nonsesne

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:337
> +    const char* withoutOffsets= [m_signatureWithoutOffsets UTF8String];

Missing space before the "=" here.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:357
> +    JSGlobalContextRef ctx = contextInternalContext(context);

I suggest contextRef or globalContext rather than ctx for this local.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:399
> +        JSGlobalContextRef ctx = contextInternalContext(context);

I suggest contextRef or globalContext rather than ctx for this local.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:401
> +        JSValueRef args[adaptor->m_argumentCount];

Confusing to have locals named both args and arguments in the same function. I suggest calling this argumentArray.

> Source/JavaScriptCore/API/JSContext.h:33
> +// An instance of JSContext represents a JavaScript execution environment, all
> +// JavaScript execution takes place within a context.

This is a comma splice. Should be two sentences.

> Source/JavaScriptCore/API/JSContext.h:41
> +NS_CLASS_AVAILABLE(10_9, NA)

I suggest adding a space above this line.

> Source/JavaScriptCore/API/JSContext.h:54
> +// Instances of JSContext originating from WebKit will return a reference to the
> +// WindowProxy object.

Interesting, but confusing. I don’t know what a WindowProxy object is.

> Source/JavaScriptCore/API/JSContext.mm:40
> +typedef WTF::HashMap<JSValueRef, size_t> ProtectMap;

No need for the WTF:: prefix here.

WTF has a HashCountedSet class, which might be a better fit here than a HashMap with values of size_t.

> Source/JavaScriptCore/API/JSContext.mm:46
> +    ProtectMap m_protected;

I think this should be called m_protectCounts rather than m_protected.

> Source/JavaScriptCore/API/JSValue.mm:63
> ++ (JSValue *)valueWithBool:(BOOL)value inContext:(JSContext *)context

I suggest blank lines between methods, even when they are short like these.

> Source/JavaScriptCore/API/JSValue.mm:130
> +    return (context && JSValueToBoolean(contextInternalContext(context), m_value)) ? YES : NO;

I don’t believe the "? YES : NO" here does any good. It would only be helpful if the value could be something other than 0 or 1, and that is not the case.

> Source/JavaScriptCore/API/JSValue.mm:473
> +    JSValueRef func = JSObjectGetProperty(contextInternalContext(context), thisObject, name, &exception);

No need to abbreviate the name function here.

> Source/JavaScriptCore/API/JSValue.mm:533
> +        @"x":[NSNumber numberWithFloat:point.x],

Could use @(point.x) instead of [NSNumber numberWithFloat:point.x]

The explicit use of float is potentially wrong since CGFloat can be either float or double.

> Source/JavaScriptCore/API/JSValue.mm:542
> +        @"location":[NSNumber numberWithFloat:range.location],
> +        @"length":[NSNumber numberWithFloat:range.length]

Why float? These are integers. I suggest just @(range.location) and @(range.length).

> Source/JavaScriptCore/API/JSValue.mm:574
> +    if (![(NSObject*)key isKindOfClass:[NSString class]]) {

Is the typecast here really needed?

> Source/JavaScriptCore/API/JSValue.mm:594
> +    if (![(NSObject*)key isKindOfClass:[NSString class]]) {

Same question about the typecast.

> Source/JavaScriptCore/API/JSValue.mm:651
> +    WTF::HashMap<JSValueRef, id> m_objectMap;
> +    WTF::Vector<Task> m_worklist;

No need for the WTF:: prefixes here or elsewhere.

> Source/JavaScriptCore/API/JSValue.mm:861
> +    JSValueRef convert(id object);

Maybe const?

> Source/JavaScriptCore/API/JSValue.mm:862
> +    void add(Task task);

Better to leave out the argument name here.

> Source/JavaScriptCore/API/JSValue.mm:864
> +    bool isWorkListEmpty() { return !m_worklist.size(); }

This member function should be const.

> Source/JavaScriptCore/API/JSValue.mm:932
> +            id literalTrue = @YES;

I don’t think the local variable makes this clearer.

> Source/JavaScriptCore/API/JSValue.mm:935
> +            id literalFalse = @NO;

Ditto.

> Source/JavaScriptCore/API/JSValue.mm:968
> +            NSArray * array = (NSArray *)current.objc;

Unusual extra space here.

> Source/JavaScriptCore/API/JSValue.mm:977
> +            NSEnumerator* enumerator = [dictionary keyEnumerator];
> +            while (id key = [enumerator nextObject]) {

Better to use the Objective-C for loop for this. I think it’s even more efficient.

> Source/JavaScriptCore/API/JSValue.mm:1007
> +    [super init];

Normally init methods look at the return value of init and return nil if it returned nil.

> Source/JavaScriptCore/API/JSValue.mm:1018
> +static StructTagHandler* getStructTagHandler(const char* encodedType)

Normally we don’t use get in the names of these types of functions inside WebKit. I think maybe find is a better verb to use.

> Source/JavaScriptCore/API/JSValue.mm:1024
> +    static StructHandlers *m_structHandlers = 0;

Incorrect use of m_ here since this is not a member. Also wrong position of the *.

> Source/JavaScriptCore/API/JSValue.mm:1027
> +    if (!m_structHandlers) {
> +        m_structHandlers = new StructHandlers();

I suggest putting this code to create and initialize the handlers into its own separate function.

> Source/JavaScriptCore/API/JSValue.mm:1032
> +            SEL sel = method_getName(method);

Why not spell out selector?

> Source/JavaScriptCore/API/JSValue.mm:1034
> +            size_t len = strlen(name);

I suggest naming this length or nameLength rather than len.

> Source/JavaScriptCore/API/JSValue.mm:1036
> +            if (len < valueWithMinLen || strncmp(name, "valueWith", 9) || strncmp(name + len - 11, ":inContext:", 11))

Why not use memcmp instead of strncmp? Is strncmp adding value?

> Source/JavaScriptCore/API/JSValue.mm:1044
> +            if (strcmp(secondType, "@")) {

We normally use != 0 here to emphasize the != for the result of functions like strcmp and strncmp.

> Source/JavaScriptCore/API/JSValue.mm:1054
> +            m_structHandlers->add(WTF::String(type).impl(), (StructTagHandler){ sel, 0 });

There’s some unneeded reference count churn here where we create a String and then put its StringImpl into the map and then release the impl. To write it without reference count churn we can do StringImpl::create(type) rather than String(type).impl().

> Source/JavaScriptCore/API/JSValue.mm:1065
> +            if (len < minLenValue || strncmp(name, "to", 2))

Again, I think memcmp is better than strncmp here.

> Source/JavaScriptCore/API/JSValue.mm:1073
> +            StructHandlers::iterator iter = m_structHandlers->find(WTF::String(type).impl());

Our hash tables are defined so we can find strings without creating a StringImpl just to do the search. Maybe we should do that here. Or we can just use char* for the keys instead of RefPtr<StringImpl>.

> Source/JavaScriptCore/API/JSValue.mm:1103
> +    StructHandlers::iterator iter = m_structHandlers->find(WTF::String(encodedType).impl());

It’s a real shame that we have to allocate a StringImpl each time here. We can and should rewrite this so the map contains just char* rather than RefPtr<StringImpl>.

> Source/JavaScriptCore/API/JSVirtualMachine.mm:41
> +    [super init];

Idiomatically we put the return value of super init into self and check it for nil.

> Source/JavaScriptCore/API/JSWrapperMap.mm:75
> +    char* buffer = (char*)malloc(header + strlen(firstColon + 1) + 1);

static_cast here?

> Source/JavaScriptCore/API/JSWrapperMap.mm:324
> +        m_constructor = createObjectWithCustomBrand(context, [NSString stringWithFormat:@"%sConstructor", className], wrapperClass(), [cls retain]);

Where is the class that’s stored in the private data released?

> Source/JavaScriptCore/API/JSWrapperMap.mm:424
> +    // (1) For immortal objects JSValues will effectively leaj and this results in error output being logged - we should avoid adding associated objects to immortal objects.

Typo: leaj

> Source/JavaScriptCore/API/JSWrapperMap.mm:439
> +    //ASSERT(!exception);

Why is this still here and commented out?

> Source/JavaScriptCore/API/JSWrapperMap.mm:451
> +    static Protocol* protocol = 0;
> +    if (!protocol)
> +        protocol = objc_getProtocol("JSExport");

Given that this is C++, it can be written:

    static Protocol* protocol = objc_getProtocol("JSExport");

No if statement needed.

> Source/JavaScriptCore/API/JSWrapperMap.mm:459
> +    static Class cls = 0;
> +    if (!cls)
> +        cls = objc_getClass("NSBlock");

Same comment as above.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:41
> +#import "objc/runtime.h"
> +#import "wtf/RetainPtr.h"

These should be angle bracket includes.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:57
> +    void set(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef argument, JSValueRef*)

Should say virtual and override here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:67
> +    void set(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef argument, JSValueRef* exception)

Should say virtual and override here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:77
> +    void set(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef argument, JSValueRef* exception)

Should say virtual and override here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:95
> +    void set(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef argument, JSValueRef*)

Should say virtual and override here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:111
> +    ~CallbackArgumentOfClass()

Should say virtual here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:116
> +    void set(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef argument, JSValueRef* exception)

Should say virtual and override here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:140
> +class CallbackArgumentNSNumber : public CallbackArgument {
> +public:

Same comment applies many, many other places: There is no reason that the override of the virtual function named set needs to be public. I would have made it private.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:141
> +    void set(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef argument, JSValueRef* exception)

Should say virtual and override here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:150
> +    void set(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef argument, JSValueRef* exception)

Should say virtual and override here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:159
> +    void set(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef argument, JSValueRef* exception)

Should say virtual and override here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:168
> +    void set(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef argument, JSValueRef* exception)

Should say virtual and override here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:177
> +    void set(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef argument, JSValueRef* exception)

Should say virtual and override here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:193
> +        m_allocation = (char*)malloc(size + alignment);

Should use static_cast.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:203
> +    void set(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef argument, JSValueRef*)

Should say virtual and override here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:222
> +    CallbackArgumentBlockCallback(JSBlockAdaptor* adaptor)
> +        : m_adaptor(adaptor)
> +    {
> +    }

Not retaining the passed-in JSBlockAdaptor is unusual. The caller does things correctly, but I worry that it’s easy to get this wrong. One way to avoid this is to use a private constructor and a named construction function named “adopt” to make it clear this adopts a reference count. Or at least name the argument in a way that tries to make that clear. Or even have the class name signal this adoption.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:229
> +    void set(NSInvocation* invocation, NSInteger argumentNumber, JSContext* context, JSValueRef argument, JSValueRef* exception)

Should say virtual and override here.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:263
> +        return nil;

Not sure nil is right here because I don’t think ResultType is a Objective-C object pointer type.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:320
> +    virtual JSValueRef get(NSInvocation*, JSContext* context, JSValueRef*)

Should be marked override.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:328
> +    virtual JSValueRef get(NSInvocation* invocation, JSContext* context, JSValueRef*)

Should be marked override.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:339
> +    virtual JSValueRef get(NSInvocation* invocation, JSContext* context, JSValueRef*)

Should be marked override.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:343
> +        return JSValueMakeNumber(contextInternalContext(context), (double)value);

Should be static_cast. Can we get away without a typecast?

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:357
> +class CallbackResultStruct : public CallbackResult {

Code here is so similar to other classes I have to wonder if there’s a way to share more code.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:366
> +        m_allocation = (char*)malloc(size + alignment);

Should be static_cast.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:492
> +    delete (ObjCCallbackFunction*)JSObjectGetPrivate(object);

static_cast?

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:498
> +    // (1) We don't want to support the C-API's confusing drops-locks-once policy - should only drop locks if we can do so recursivey.

Typo: recursivey

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:499
> +    // (2) We're caling some JSC internals that require us to be on the 'inside' - e.g. createTypeError.

Typo: caling

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:500
> +    // (3) We need to be locked (per context would be fine) against conflicting usgae of the ObjCCallbackFunction's NSInvocation.

Typo: usgae

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:539
> +    static JSClassRef classRef = 0;
> +
> +    if (!classRef) {
> +        JSClassDefinition definition;
> +        definition = kJSClassDefinitionEmpty;
> +        definition.className = "Function";
> +        definition.finalize = objCCallbackFunctionFinalize;
> +        definition.callAsFunction = objCCallbackFunctionCallAsFunction;
> +        classRef = JSClassCreate(&definition);
> +    }

More elegant to do this in a separate function and write:

    static JSClassRef classRef = createObjCCallbackFunctionClass();

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:560
> +        }
> +        [m_invocation setTarget:target];
> +    }
> +    case CallbackClassMethod:
> +        firstArgument = 2;
> +        break;

I assume this falls through intentionally. I think a comment is needed to make it clear it’s not a mistake.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:591
> +    enum CheckResult {
> +        CheckResultFalse,
> +        CheckResultTrue,
> +        CheckResultUnknown
> +    };
> +    static enum CheckResult check = CheckResultUnknown;
> +    if (check == CheckResultUnknown) {
> +        id block = ^(NSString *string){ return string; };
> +        check = _Block_has_signature(block) && strstr(_Block_signature(block), "NSString") ? CheckResultTrue : CheckResultFalse;
> +    }
> +    return (bool)check;

There’s no need for the enum. You can just use a combined C++ and C syntax something like this:

    static bool containsClass = ^{
        id block = ^(NSString *string){ return string; };
        return _Block_has_signature(block) && strstr(_Block_signature(block), "NSString");
    };
    return containsClass;

This avoids a number of unpleasant code style things.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:598
> +    if (!isdigit(*position))
> +        return false;
> +    while(isdigit(*++position));

This should use isASCIIDigit, not isdigit.

The while should have a space after it before the parentheses and should use braces rather than a semicolon for greater clarity.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:613
> +        if ('@' != *(position++) || !skipNumber(position) || ':' != *(position++) || !skipNumber(position))

It’s idiomatic to write *position++ rather than *(position++)

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:617
> +        if (('@' != *(position++)) || ('?' != *(position++)) || !skipNumber(position) || (!blockSignatureContainsClass() && strchr(position, '@')))

The extra parentheses here are a bit confusing. The blockSignatureContainsClass logic is sufficiently subtle that I think we need a comment.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:674
> +    return ((ObjCCallbackFunction*)JSObjectGetPrivate(object))->wrappedBlock();

static_cast instead of C-style cast?

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:81
> +inline void forEachMethodInClass(Class cls, void (^callback)(Method method))

The argument name method isn’t needed here.

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:89
> +    unsigned count;
> +    Method* methods = class_copyMethodList(cls, &count);
> +    if (count) {
> +        for (unsigned i = 0; i < count; ++i)
> +            callback(methods[i]);
> +        free(methods);
> +    }

Why the if statement? It seems that it optimizes the case of 0 methods, which probably is not common enough that it needs optimization. And it causes the code to depend on a return value of NULL for a count of 0; it seems it would be safer to just call free unconditionally.

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:100
> +    unsigned count;
> +    struct objc_method_description* methods = protocol_copyMethodDescriptionList(protocol, isRequiredMethod, isInstanceMethod, &count);
> +    if (count) {
> +        for (unsigned i = 0; i < count; ++i)
> +            callback(methods[i].name, methods[i].types);
> +        free(methods);
> +    }

Same comment about the if statement.

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:111
> +    unsigned count;
> +    objc_property_t* properties = protocol_copyPropertyList(protocol, &count);
> +    if (count) {
> +        for (unsigned i = 0; i < count; ++i)
> +            callback(properties[i]);
> +        free(properties);
> +    }

Same comment about the if statement.

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:117
> +    NSUInteger count = 1;

NSUInteger is a strange type to choose here. I would choose size_t given what this is used for.

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:119
> +        char c = *(position++);

Idiomatic to just do *position++

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:138
> +class StringRange {
> +public:
> +    StringRange(const char* begin, const char* end) : m_ptr(strndup(begin, end - begin)) { }
> +    ~StringRange() { free(m_ptr); }
> +    operator const char*() { return m_ptr; }
> +    const char* get() { return m_ptr; }
> +
> +private:
> +    char* m_ptr;
> +};

Should mark this class noncopyable.

The member functions should be const.

> Source/JavaScriptCore/API/ObjcRuntimeExtras.h:219
> +extern "C" {
> +    const char *_protocol_getMethodTypeEncoding(Protocol *, SEL, BOOL isRequiredMethod, BOOL isInstanceMethod);
> +    id objc_initWeak(id *, id);
> +    void objc_destroyWeak(id *);
> +    bool _Block_has_signature(void *);
> +    const char * _Block_signature(void *);
> +}

Are these really needed here? Aren’t they in public system headers?

> Source/JavaScriptCore/API/tests/testapi.c:51
> +#if PLATFORM(MAC)

Seems like the wrong ifdef. Should use the “if we have the Objective-C API” ifdef, right?

> Source/JavaScriptCore/API/tests/testapi.m:137
> +    enum CheckResult {
> +        CheckResultFalse,
> +        CheckResultTrue,
> +        CheckResultUnknown
> +    };
> +    static enum CheckResult check = CheckResultUnknown;
> +    if (check == CheckResultUnknown) {
> +        id block = ^(NSString *string){ return string; };
> +        check = _Block_has_signature(block) && strstr(_Block_signature(block), "NSString") ? CheckResultTrue : CheckResultFalse;
> +    }
> +    return (bool)check;

Should rewrite as I suggested above.

> Source/JavaScriptCore/API/tests/testapi.m:147
> +        checkResult(@"2 + 2", [result toInt32] == 4);

This doesn’t check the type of the result. Same is true of a lot of other tests below. Wouldn’t it be better to check the type too?

> Source/JavaScriptCore/runtime/JSGlobalData.h:456
> +        void *m_apiData;

Same comments here as on JSGlobalObject below. I don’t like the use of “api” to mean Objective-C language binding layer and I don’t like the use of a public data member.

[Darin Adler]

Comment on attachment 181004 [details]
Removed redundant forEachProtocolImplementingProtocol

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

>> Source/JavaScriptCore/API/JSValue.mm:533
>> +        @"x":[NSNumber numberWithFloat:point.x],
> 
> Could use @(point.x) instead of [NSNumber numberWithFloat:point.x]
> 
> The explicit use of float is potentially wrong since CGFloat can be either float or double.

I guess the @(point.x) feature may not yet be in the version of clang we are using, so oops. But the point about CGFloat remains. Probably best to make a helper function that makes a number from a CGFloat that uses float or double as appropriate.

>> Source/JavaScriptCore/API/JSValue.mm:542
>> +        @"length":[NSNumber numberWithFloat:range.length]
> 
> Why float? These are integers. I suggest just @(range.location) and @(range.length).

I think the correct method to use is numberWithUnsignedInteger: since NSRange values are NSUInteger.

[Gavin Barraclough]

(In reply to comment #27)
> There’s no need for the enum. You can just use a combined C++ and C syntax something like this:
> 
>     static bool containsClass = ^{
>         id block = ^(NSString *string){ return string; };
>         return _Block_has_signature(block) && strstr(_Block_signature(block), "NSString");
>     };
>     return containsClass;
> 
> This avoids a number of unpleasant code style things.

Now that's nice.
I guess I'm not used to programming with closures in C & have not encountered this before - I like this.

[Gavin Barraclough]

Created attachment 181080 [details]
Darin review feedback part 1

Hi Darin, thanks for all the review feedback.

Here is a patch with fixes for a big bulk of the more trivial issues, will follow up with fixes/responses to the issues not yet addressed.

cheers,
G.

[Geoffrey Garen]

Comment on attachment 181080 [details]
Darin review feedback part 1

r=me

[Gavin Barraclough]

(In reply to comment #28)
> I guess the @(point.x) feature may not yet be in the version of clang we are using, so oops. But the point about CGFloat remains. Probably best to make a helper function that makes a number from a CGFloat that uses float or double as appropriate.

Actually the API is going to require latest clang anyway (for enhanced type information strings in blocks).  The @() style looks good to me, so I think I'll follow this suggestion for now.

[Gavin Barraclough]

Created attachment 181117 [details]
Darin review feedback part 2

[Geoffrey Garen]

Comment on attachment 181117 [details]
Darin review feedback part 2

r=me

[Gavin Barraclough]

Hi Darin,

Hopefully most of the issues you identified should now been fixed by the follow up patches.
There are a couple of couple of things left to follow up on.

(In reply to comment #27)
> Formatting of Objective-C type pointers here is all over the map. Our coding style says “NSInvocation *” but I don’t feel strongly about that except in public headers. Maybe we can change the WebKit coding style. I wish this patch was self-consistent about that, though. But that’s less important than other comments I made here.

I got into a bit of a mess here between the Objective-C & WebKit coding styles.  I'd attempted to resolve this the wrong way - based on the context rather than the type - so Objective-C methods used Objective-C style for Objective-C types, and C functions used C style for Objective-C types.

I've now corrected this so we're (hopefully) consistently using Objective-C style for Objective-C types everywhere.

> > Source/JavaScriptCore/API/JSContext.h:54
> > +// Instances of JSContext originating from WebKit will return a reference to the
> > +// WindowProxy object.
> 
> Interesting, but confusing. I don’t know what a WindowProxy object is.

WindowProxy is the WebIDL term for the DOMWindowShell (as opposed to the ECMA defined Global Object).

I think this method needs a well crafted comment - it clearly needs some better description, but I'd like to avoid having too in depth and wordy an exposition here. I'll undertake to revisit this.

> > Source/JavaScriptCore/API/JSValue.mm:1073
> > +            StructHandlers::iterator iter = m_structHandlers->find(WTF::String(type).impl());
> 
> Our hash tables are defined so we can find strings without creating a StringImpl just to do the search. Maybe we should do that here. Or we can just use char* for the keys instead of RefPtr<StringImpl>.
> 
> > Source/JavaScriptCore/API/JSValue.mm:1103
> > +    StructHandlers::iterator iter = m_structHandlers->find(WTF::String(encodedType).impl());
> 
> It’s a real shame that we have to allocate a StringImpl each time here. We can and should rewrite this so the map contains just char* rather than RefPtr<StringImpl>.

Yep, good point. Having looked at this with Geoff, I think the best solution is probably to use a string translator. There's a little bit of cleanup necessary to make this work (these are currently all defined in AtomicString.cpp, so at minimum will need moving to their own header). I'm going to hand this task on to whomever takes over the API work from me (probably Mark).

> > Source/JavaScriptCore/API/JSWrapperMap.mm:324
> > +        m_constructor = createObjectWithCustomBrand(context, [NSString stringWithFormat:@"%sConstructor", className], wrapperClass(), [cls retain]);
> 
> Where is the class that’s stored in the private data released?

These are being freed by wrapperFinalize (since the constructor objects are instances of wrapperClass). This could be clearer, maybe factoring out a function to construct wrapper objects that also performed the retain might be an improvement.

> > Source/JavaScriptCore/runtime/JSGlobalData.h:456
> > +        void *m_apiData;
> 
> Same comments here as on JSGlobalObject below. I don’t like the use of “api” to mean Objective-C language binding layer and I don’t like the use of a public data member.

I was lacking inspiration here! How about:
    - bindingsPrivateData(), setBindingsPrivateData()?
    - objCBindingsPrivateData(), setObjCBindingsPrivateData()?

cheers,
G

[Joseph Pecoraro]

(In reply to comment #29)
> (In reply to comment #27)
> > There’s no need for the enum. You can just use a combined C++ and C syntax something like this:
> > 
> >     static bool containsClass = ^{
> >         id block = ^(NSString *string){ return string; };
> >         return _Block_has_signature(block) && strstr(_Block_signature(block), "NSString");
> >     };
> >     return containsClass;
> > 
> > This avoids a number of unpleasant code style things.
> 
> Now that's nice.
> I guess I'm not used to programming with closures in C & have not encountered this before - I like this.

To prevent an error I've seen before, make sure you aren't assigning the block itself to the static. You want to assign the result of evaluating the block to the static!

    #include <stdio.h>
    int main()
    {
        static bool test1 = ^{ printf("Evaluated test1\n"); return false; };
        static bool test2 = ^{ printf("Evaluated test2\n"); return false; }();
        printf("test1: %s\n", test1 ? "true" : "false"); // => true, not what we expected.
        printf("test2: %s\n", test2 ? "true" : "false"); // => false, this is what we wanted.
    }

outputs:

    Evaluated test2
    test1: true
    test2: false

[Joseph Pecoraro]

Comment on attachment 181080 [details]
Darin review feedback part 1

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

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:548
> +    static bool containsClass = ^{
>          id block = ^(NSString *string){ return string; };
> -        check = _Block_has_signature(block) && strstr(_Block_signature(block), "NSString") ? CheckResultTrue : CheckResultFalse;
> -    }
> -    return (bool)check;
> +        return _Block_has_signature(block) && strstr(_Block_signature(block), "NSString");
> +    };
> +    return containsClass;

Oops, looks like I was late to the party. I think this should evaluate the block. I just verified on my machine I get different results for "containsClass = ^{...}" and "containsClass = ^{...}()".

I think blockSignatureContainsClass is now incorrectly always returning true when it should be false.

> Source/JavaScriptCore/API/tests/testapi.m:132
> +    static bool containsClass = (bool)^{
>          id block = ^(NSString *string){ return string; };
> -        check = _Block_has_signature(block) && strstr(_Block_signature(block), "NSString") ? CheckResultTrue : CheckResultFalse;
> -    }
> -    return (bool)check;
> +        return _Block_has_signature(block) && strstr(_Block_signature(block), "NSString");
> +    };
> +    return containsClass;
>  }

Ditto

[Joseph Pecoraro]

Comment on attachment 181004 [details]
Removed redundant forEachProtocolImplementingProtocol

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

Cool stuff!!! Sorry to review an old patch, but its the only place where I could read through and comment on its entirety. I avoided all of Darin's comments, and tried to check ToT before adding a comment in case things were already fixed.

> Source/JavaScriptCore/API/APIJSValue.h:43
> +// being maintained by this JSValue, and the JSValue instance may no longer by

Typo: "no longer by used" => "no longer be used"

> Source/JavaScriptCore/API/APIJSValue.h:44
> +// used as a reference to this value. For any method invoked on instance of

Typo: "invoked on instance of JSValue with a nil" probably => "invoked on instances of JSValue with a nil"

> Source/JavaScriptCore/API/APIJSValue.h:166
> +// the JavaScript langauage.

Typo: "langauage" => "language"

> Source/JavaScriptCore/API/APIJSValue.h:191
> +// Call this value as a function passing the specified "this" value and arguments.
> +- (JSValue *)callWithArguments:(NSArray *)arguments;

The comment seems out of date. There is no specified "this" value. The implementation explicitly passes 0 as the thisObject.

> Source/JavaScriptCore/API/APIJSValue.h:196
> +// value, and the specified agruments.

Typo: "agruments" => "arguments".

> Source/JavaScriptCore/API/APIJSValue.h:206
> +// Objective-C methods exported to JavaScript may have argument and/or return
> +// values of struct types if the struct if conversion to and from the struct
> +// is supported by JSValue. Support is provided for any types where JSValue

Typo: "return values of struct types if the struct if conversion to" => ???

> Source/JavaScriptCore/API/APIJSValue.h:238
> +// Convert a value to type CGRange by accessing properties named "width" and
> +// "height" from this value converting the results to double.
> +- (CGSize)toSize;

Typo: "CGRange" => "CGSize"

> Source/JavaScriptCore/API/APIJSValue.h:251
> +// An object key passed as a subscript will be converted to a JavaScipt value,

Typo: "JavaScipt" => "JavaScript"

> Source/JavaScriptCore/API/JSBlockAdaptor.h:27
> +#import <wtf/RetainPtr.h>

Looks like you can move this import to the implementation file. It's not needed here in the header.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:26
> +#include "config.h"

Everywhere you include "config.h" you use #include. They can be #import to match all other imports.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:38
> +#import "JSWrapperMap.h"
> +#import "JSValue.h"
> +#import "JSValueInternal.h"

Ordering: JSValue < JSWrapper

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:40
> +#import "wtf/OwnPtr.h"

Switch to <>s: <wtf/OwnPtr.h>

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:42
> +extern "C" {

Some extern "C" blocks in this patch are indented and others aren't. I don't know if there is a standard style.

> Source/JavaScriptCore/API/JSBlockAdaptor.mm:372
> +        // (and that were able to create a forwarding block for this signature).

Typo: "were" => "we're"

> Source/JavaScriptCore/API/JSContext.h:79
> +// assign ant uncaught exception to this property).

Typo: "ant" => "an"

> Source/JavaScriptCore/API/JSContext.h:80
> +// If a JSValue orginating from a different JSVirtualMachine this this context

Typo: "orginating" => "originating"
Typo: "JSVirtualMachine this this context" => "JSVirtualMachine than this context"

> Source/JavaScriptCore/API/JSContext.h:88
> +// occuring within a callback from JavaScript to be rethrown upon return.

Typo: "occuring" => "occurring"

> Source/JavaScriptCore/API/JSContext.h:106
> +// An object key passed as a subscript will be converted to a JavaScipt value,

Typo: "JavaScipt" => "JavaScript"

> Source/JavaScriptCore/API/JSContext.mm:119
> +        for (size_t i =0; i < count; ++i)

Style: "i =0" missing a space

> Source/JavaScriptCore/API/JSContext.mm:167
> +    [super dealloc];

I'd expect to see synthesized properties exception (retain) and exceptionHandler (copy) released here if needed as well. Or are they guaranteed to be nil (in which case it would be good to assert that)?

> Source/JavaScriptCore/API/JSExport.h:30
> +// When an JavaScript value is created from an instance of an Objective-C class

Typo: "an JavaScript" => "a JavaScript". This is the only case an was used.

> Source/JavaScriptCore/API/JSExport.h:60
> +// property being exported a JavaScript accessor property will be created on the
> +// Prototype, and for each class method exported a JavaScript function will be
> +// created on the Constructor object. For example:

This mentions that it creates accessor properties. It would be nice to mention if they are "enumerable" and "configurable" or not.

> Source/JavaScriptCore/API/JSExport.h:73
> +// since the class conforms to the MyClassJavaScriptExports protocol, and this

Typo: "MyClassJavaScriptExports" => "MyClassJavaScriptMethods"

> Source/JavaScriptCore/API/JSExport.h:96
> +//    will be created, allowing the the JavaScript function to be used in the

Typo: "the the" => "the"

> Source/JavaScriptCore/API/JSExport.h:101
> +// derives from NSString by conforms to JSExport is passed to valueWithObject:,

Typo: "derives from NSString by conforms to" => "derives from NSString  but conforms to "

> Source/JavaScriptCore/API/JSValue.mm:26
> +#include "config.h"

Normally there would be import "JSValue.h" here and a newline after it.

> Source/JavaScriptCore/API/JSValue.mm:42
> +#import "wtf/HashMap.h"
> +#import "wtf/HashSet.h"
> +#import "wtf/Vector.h"
> +#import <wtf/TCSpinLock.h>
> +#import "wtf/text/WTFString.h"

<wtf> style.

> Source/JavaScriptCore/API/JSValue.mm:306
> +    if (index != (unsigned)index)
> +        [self valueForProperty:[[JSValue valueWithDouble:index inContext:context] toString]];

Missing return!

> Source/JavaScriptCore/API/JSValue.mm:919
> +        if ([object isKindOfClass:[NSArray class]])
> +            return (ObjcContainerConvertor::Task){ object, JSObjectMakeArray(contextInternalContext(context), 0, NULL, 0), ContainerArray };

NSArray is already handled above. These 2 lines can be removed.

> Source/JavaScriptCore/API/JSValue.mm:922
> +        if ([object isKindOfClass:[NSDictionary class]])
> +            return (ObjcContainerConvertor::Task){ object, JSObjectMake(contextInternalContext(context), 0, 0), ContainerDictionary };

NSDictionary is already handled above. These 2 lines can be removed.

> Source/JavaScriptCore/API/JSWrapperMap.mm:34
> +#import "ObjCCallbackFunction.h"
> +#import "ObjcRuntimeExtras.h"

Although correct, the different capitalizations of these files is asking for trouble on a case-sensitive system! It would be good to standardize the capitalization if this wasn't already on your radar.

> Source/JavaScriptCore/API/JSWrapperMap.mm:36
> +#import "wtf/Vector.h"

<wtf> style

> Source/JavaScriptCore/API/JSWrapperMap.mm:66
> +    // Use 'index' to check for colons, if there are non, this is easy!

Typo: "non" => "none"

> Source/JavaScriptCore/API/JSWrapperMap.mm:106
> +// Make an object that is in all ways are completely vanilla JavaScript object,

Typo: This line need rephrasing.

> Source/JavaScriptCore/API/JSWrapperMap.mm:108
> +// Object.prototype.toString comversion.

Typo: "comversion" => "conversion"

> Source/JavaScriptCore/API/JSWrapperMap.mm:154
> +//  * If an accessorMap is provided, and conatins a this name, store the method in the map.

Typo: "conatins" => "contains". And the grammar here might need to change "and contains a this name"?

> Source/JavaScriptCore/API/JSWrapperMap.mm:155
> +//  * Otherwise, if the object doesn't already conatin a property with name, create it.

Typo: "conatin" => "contain"

> Source/JavaScriptCore/API/JSWrapperMap.mm:402
> +    // Skip internal classes begining with '_' - just copy link to the parent class's info.

Typo: "begining" => "beginning"

> Source/JavaScriptCore/API/JSWrapperMap.mm:425
> +    // (2) A long lived object may rack up many JSValues. When the contexts are released these will unproctect the associated JavaScript objects,

Typo: "unproctect" => "unprotect"

[Gavin Barraclough]

Hi Joe, Thanks for all the feedback.  The block thing was a genuine bug – I believe MarkH has since fixed this.  I'll follow up on all these comments.

[Joseph Pecoraro]

Yep, looks like he handled them in:
<http://webkit.org/b/106055> testapi is failing with a block-related error in the Objc API

[Joseph Pecoraro]

(In reply to comment #39)
> Hi Joe, Thanks for all the feedback.  The block thing was a genuine bug – I believe MarkH has since fixed this.  I'll follow up on all these comments.

Follow-ups for my (non typo/style) comments are:
<http://webkit.org/b/108378> Objective-C API: exceptionHandler needs to be released in JSContext dealloc
<http://webkit.org/b/108264> Objective-C API: Fix insertion of values greater than the max index allowed by the spec

Thanks!