Bug 175836 – [JSC API] We should support the symbol type in our C/Obj-C API

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

[JSC API] We should support the symbol type in our C API

[Keith Miller]

Created attachment 318773 [details]
Patch

[Keith Miller]

Comment on attachment 318773 [details]
Patch

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

> Source/JavaScriptCore/API/JSValueRef.h:54
> +    kJSTypeSymbol CF_ENUM_AVAILABLE(10_13, 11_0)

This is a temporary number since it's probably won't make it into these releases.

> Source/JavaScriptCore/API/JSValueRef.h:161
> +JS_EXPORT bool JSValueIsSymbol(JSContextRef ctx, JSValueRef value) CF_AVAILABLE(10_13, 11_0);

Ditto.

[Keith Miller]

Comment on attachment 318773 [details]
Patch

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

>> Source/JavaScriptCore/API/JSValueRef.h:54
>> +    kJSTypeSymbol CF_ENUM_AVAILABLE(10_13, 11_0)
> 
> This is a temporary number since it's probably won't make it into these releases.

it's => this.

[Build Bot]

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


ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  CF_ENUM_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 4 files


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

[Geoffrey Garen]

Comment on attachment 318773 [details]
Patch

Needs API review and an ObjC equivalent.

[Keith Miller]

Created attachment 318939 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:578:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:615:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:615:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:615:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  CF_ENUM_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 10 in 9 files


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

[Geoffrey Garen]

Comment on attachment 318939 [details]
Patch

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

> Source/JavaScriptCore/API/JSObjectRef.cpp:336
> +bool JSObjectHasPropertyWithKey(JSContextRef ctx, JSObjectRef object, JSValueRef key, JSValueRef* exception)

We should probably not include this function because there is no language concept for a HasOwnProperty operation that takes a key. Instead, the language has an 'in' operator and a hasOwnProperty built-in function, and both take string arguments.

Clients who want JSObjectHasPropertyWithKey can use JSObjectGetPropertyWithKey, or build a helper function on top of it.

> Source/JavaScriptCore/API/JSValue.h:409
> + @method
> + @abstract Access a property of a JSValue.
> + @result The JSValue for the requested property key or the JSValue <code>undefined</code>
> + if the property does not exist.
> + */
> +- (JSValue *)valueForPropertyWithKey:(JSValue *)key NS_AVAILABLE(10_13, 11_0);
> +
> +/*!
> + @method
> + @abstract Set a property key on a JSValue.
> + */
> +- (void)setValue:(id)value forPropertyWithKey:(JSValue *)key NS_AVAILABLE(10_13, 11_0);
> +
> +/*!
> + @method
> + @abstract Delete a property key from a JSValue.
> + @result YES if deletion is successful, NO otherwise.
> + */
> +- (BOOL)deletePropertyWithKey:(JSValue *)key NS_AVAILABLE(10_13, 11_0);
> +
> +/*!
> + @method
> + @abstract Check if a JSValue has a property key.
> + @discussion This method has the same function as the JavaScript operator <code>in</code>.
> + @result Returns YES if property key is present on the value.
> + */
> +- (BOOL)hasPropertyWithKey:(JSValue *)key NS_AVAILABLE(10_13, 11_0);
> +
> +/*!
> + @method
> + @abstract Define property keys with custom descriptors on JSValues.
> + @discussion This method may be used to create a data or accessor property on an object.
> + This method operates in accordance with the Object.defineProperty method in the
> + JavaScript language.
> + */
> +- (void)definePropertyWithKey:(JSValue *)key descriptor:(id)descriptor NS_AVAILABLE(10_13, 11_0);
> +
> +/*!

I believe that these APIs duplicate the APIs listed under @interface JSValue (SubscriptSupport), but in a less powerful way.

Perhaps we should consider adding just a deletePropertyForKeyedSubscript: method.

[Keith Miller]

Comment on attachment 318939 [details]
Patch

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

>> Source/JavaScriptCore/API/JSObjectRef.cpp:336
>> +bool JSObjectHasPropertyWithKey(JSContextRef ctx, JSObjectRef object, JSValueRef key, JSValueRef* exception)
> 
> We should probably not include this function because there is no language concept for a HasOwnProperty operation that takes a key. Instead, the language has an 'in' operator and a hasOwnProperty built-in function, and both take string arguments.
> 
> Clients who want JSObjectHasPropertyWithKey can use JSObjectGetPropertyWithKey, or build a helper function on top of it.

That's not true for example: 

Symbol.iterator in []
evaluates to true.

Object.hasOwnProperty(Symbol.iterator, [])
evaluates to false (Symbol.iterator is on Array.prototype).

{ toString() { return "length"; } } in []
evaluates to true.

Object.hasOwnProperty({ toString() { return "length"; } }, [])
evaluates to true.

>> Source/JavaScriptCore/API/JSValue.h:409
>> +/*!
> 
> I believe that these APIs duplicate the APIs listed under @interface JSValue (SubscriptSupport), but in a less powerful way.
> 
> Perhaps we should consider adding just a deletePropertyForKeyedSubscript: method.

I'm not sure I really understand the implications of this. Could you explain what you mean further?

What does adding a deletePropertyForKeyedSubscript mean. How would you invoke it? How does it work?

[Geoffrey Garen]

I didn't notice at first that this patch was exclusively about ToPropertyKey.

Let me step back a minute and look at where we are in trunk.

JavaScript provides these operations for get/set/has/remove:

    {Get|Set|Has|Remove}ById
        Example: o.id
    {Get|Set|Has|Remove}ByValue
        Example: o[value]
    {Get|Set|Has|Remove}ByPropertyKey [ new in ES6 ]
        Example: Reflect.get([], Symbol.iterator)

The C API currently has:

    {Get|Set|Has|Remove}ById
        JSObjectGetProperty (et al)
    {Get|Set|Has|Remove}ByValue
        *** MISSING ***
    {Get|Set|Has|Remove}ByPropertyKey [ new in ES6 ]
        *** MISSING ***

The Objective-C API currently has:

    {Get|Set|Has|Remove}ById
        -valueForProperty: (et al)
    {Get|Set|Has|Remove}ByValue
        -objectForKeyedSubscript: (et al)
    {Get|Set|Has|Remove}ByPropertyKey [ new in ES6 ]
        *** MISSING ***

The ByPropertyKey variants are strictly more powerful than the ById variants. If we were designing this API from scratch we might not provide the ById variants.

Similarly, the ByValue variants are strictly more powerful than the ByPropertyKey variants. Since we *are* designing this part of the API from scratch, we can still decide not to provide the ByPropertyKey variants.

Now, some questions about your patch:

(1) Should we add ByValue to the C API? Why or why not?

(2) Should we expose an explicit ToPropertyKey operation? Why or why not?

(3) Should we provide ByPropertyKey in addition to ByValue? Why or why not?

My intuition is that we should just provide ByValue because it's the most powerful operation and ToPropertyKey is an internal implementation detail of it and other runtime APIs.

[Keith Miller]

I'm confused, there is no byPropertyKey operation, as far as I know. The toPropertyKey function is an implementation detail of how we convert a value into something we can use to lookup properties with. Reflect.get(value, property) is basically the same as value[property] it just doesn't toObject value, unless that's what you were going for. I'm assuming that's not what you mean though since the byId versions of the functions we provide assume a JSObjectRef. The methods I provided are the same a looking up by value. Perhaps we should use that suffix rather than the WithKey suffix.

[Keith Miller]

Created attachment 319508 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  CF_ENUM_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 9 files


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

[Keith Miller]

Created attachment 319608 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  CF_ENUM_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 10 files


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

[Keith Miller]

Comment on attachment 319608 [details]
Patch

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

> Source/JavaScriptCore/API/JSValue.h:318
> +#if (defined(BUILDING_JAVASCRIPTCORE) && BUILDING_JAVASCRIPTCORE) || (defined(TARGET_OS_MAC) && TARGET_OS_MAC && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || (defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000)

I'm not sure if this is the right way to get around building internally on older OSs. We need this because if you are building JSC on Sierra/El Cap SDKs we will present the old signatures. 

The alternative to this is to keep the old implementations of these functions in JSValue.mm and not run the new tests on SDKs. I went with this version since it allows us to test our code even on non-High Sierra SDKs.

[Geoffrey Garen]

> > Source/JavaScriptCore/API/JSValue.h:318
> > +#if (defined(BUILDING_JAVASCRIPTCORE) && BUILDING_JAVASCRIPTCORE) || (defined(TARGET_OS_MAC) && TARGET_OS_MAC && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || (defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000)

Let's try just suppressing the warning in the implementation file.

I also think it would be OK to #ifdef out id-specfic tests when building for older SDKs.

[Keith Miller]

Created attachment 319642 [details]
fixed to not have macro value in public header

[Geoffrey Garen]

Comment on attachment 319642 [details]
fixed to not have macro value in public header

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

Next steps are fixing review comments, updating patch to trunk, API review, and notifying documentation about new API.

> Source/JavaScriptCore/API/JSValue.h:158
> ++ (JSValue *)valueWithNewSymbolFromDescription:(NSString *)description InContext:(JSContext *)context NS_AVAILABLE(10_13, 11_0);

Its "inContext". (Capitalization is part of the method signature so it's API.)

> Source/JavaScriptCore/API/JSValue.h:327
> +#if (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) || (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000)
> +
> +// These declarations exist to ensure compatablity when targeting older OSs.
> +- (JSValue *)valueForProperty:(NSString *)property;
> +- (void)setValue:(id)value forProperty:(NSString *)property;
> +- (BOOL)deleteProperty:(NSString *)property;
> +- (BOOL)hasProperty:(NSString *)property;
> +- (void)defineProperty:(NSString *)property descriptor:(id)descriptor;
> +
> +#else // (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) || (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000)

Can we move these to an out-of-the-way place?

To match your other comment, I would say "These declarations provide compatibility when targeting SDKs for legacy OSs. In macOS 10.13 and iOS11 and below, 'property' was an NSString *.

> Source/JavaScriptCore/API/JSValue.h:334
> +@discussion Before macOS 10.13 and iOS 11 <code>property</code> was an NSString *, this was relaxed to id so JavaScript Symbol properties could be accessed.

I would say "Corresponds to the JavaScript operation object[property]. Pass an NSString * to access a named property. Other valid properties include symbols, numbers, and stringifiable objects. In macOS 10.13 and iOS11 and below, 'property' was an NSString *. " (Same in other places.)

> Source/JavaScriptCore/API/JSValue.mm:250
> +#define USE_NSSTRING (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) || (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000)

Let's call this USE_NSSTRING_FOR_PROPERTY_KEY to avoid a name conflict from overly broad naming. We usually use "USE(X)" to mean that X is a project-wide feature. We do use NSString, so we need a more specific name.

> Source/JavaScriptCore/API/tests/testapi.cpp:105
> +//    {
> +//        APIString description("dope");
> +//        JSValueRef symbol = JSValueMakeSymbol(context, description);
> +//        ScriptResult result = evaluateScript("typeof(this) == 'symbol'", symbol);
> +//    }

Wat.

> Source/JavaScriptCore/API/tests/testapi.mm:575
>          JSContext *context = [[JSContext alloc] init];

These tests need more beef.

[Keith Miller]

Comment on attachment 319642 [details]
fixed to not have macro value in public header

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

>> Source/JavaScriptCore/API/JSValue.h:327
>> +#else // (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) || (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000)
> 
> Can we move these to an out-of-the-way place?
> 
> To match your other comment, I would say "These declarations provide compatibility when targeting SDKs for legacy OSs. In macOS 10.13 and iOS11 and below, 'property' was an NSString *.

I moved those to the bottom of the file.

>> Source/JavaScriptCore/API/JSValue.h:334
>> +@discussion Before macOS 10.13 and iOS 11 <code>property</code> was an NSString *, this was relaxed to id so JavaScript Symbol properties could be accessed.
> 
> I would say "Corresponds to the JavaScript operation object[property]. Pass an NSString * to access a named property. Other valid properties include symbols, numbers, and stringifiable objects. In macOS 10.13 and iOS11 and below, 'property' was an NSString *. " (Same in other places.)

Changed. I also updated the comment above the old declarations.

>> Source/JavaScriptCore/API/JSValue.mm:250
>> +#define USE_NSSTRING (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) || (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000)
> 
> Let's call this USE_NSSTRING_FOR_PROPERTY_KEY to avoid a name conflict from overly broad naming. We usually use "USE(X)" to mean that X is a project-wide feature. We do use NSString, so we need a more specific name.

Done.

>> Source/JavaScriptCore/API/tests/testapi.cpp:105
>> +//    }
> 
> Wat.

Yeah, I hadn't done testing yet. This patch was for API reviewing.

>> Source/JavaScriptCore/API/tests/testapi.mm:575
>>          JSContext *context = [[JSContext alloc] init];
> 
> These tests need more beef.

Indeed, that's done now.

[Keith Miller]

Created attachment 320056 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  CF_ENUM_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 11 files


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

[Keith Miller]

(In reply to Geoffrey Garen from comment #19)
> Comment on attachment 319642 [details]
> > Source/JavaScriptCore/API/JSValue.h:327
> > +#if (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) || (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000)
> > +
> > +// These declarations exist to ensure compatablity when targeting older OSs.
> > +- (JSValue *)valueForProperty:(NSString *)property;
> > +- (void)setValue:(id)value forProperty:(NSString *)property;
> > +- (BOOL)deleteProperty:(NSString *)property;
> > +- (BOOL)hasProperty:(NSString *)property;
> > +- (void)defineProperty:(NSString *)property descriptor:(id)descriptor;
> > +
> > +#else // (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101300) || (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && __IPHONE_OS_VERSION_MIN_REQUIRED < 110000)
> 
> Can we move these to an out-of-the-way place?
> 

I'm not actually sure where that should be. They need to be in the middle of the file somewhere since they are part of the interface and there is non-interface stuff at the bottom of the file. I put them below the newer API.

[Keith Miller]

Created attachment 320091 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  CF_ENUM_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 11 files


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

[Geoffrey Garen]

> I'm not actually sure where that should be. They need to be in the middle of
> the file somewhere since they are part of the interface and there is
> non-interface stuff at the bottom of the file. I put them below the newer
> API.

Objective-C allows you to add categories, which extend the interface, after the formal interface declaration. That's how our keyed subscripting declarations work, for example.

[Keith Miller]

rdar://problem/31375935

[Keith Miller]

Created attachment 320196 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  CF_ENUM_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 11 files


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

[Keith Miller]

Created attachment 320198 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  CF_ENUM_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 11 files


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

[Joseph Pecoraro]

Comment on attachment 320198 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        For whatever reason, whenever we added support for symbols in JSC we never extended such
> +        support to our C/Obj-C APIs. This patch remedies that problem.

This doesn't seem like a useful intro.

> Source/JavaScriptCore/API/JSObjectRef.cpp:339
> +    if (!ctx) {
> +        ASSERT_NOT_REACHED();

Why do we assert not reached? This is a public API, developers may call this with a nullptr. I can see it being useful to know in a debug build as a warning, but it is certainly possible to have happen.

> Source/JavaScriptCore/API/JSObjectRef.h:565
> +JS_EXPORT bool JSObjectHasPropertyKey(JSContextRef ctx, JSObjectRef object, JSValueRef propertyKey, JSValueRef* exception) CF_AVAILABLE(10_13, 11_0);

I think I convinced you earlier that we should go with a: JSC_AVAILABLE(JSC_MAC_TBA, JSC_IOS_TBA) and post-process like WebKit's processing:
Source/WebKit/postprocess-framework-headers.sh

We want to do it, because every time we add new API this happens, and now is as good a time as any!

> Source/JavaScriptCore/API/JSValue.h:325
> +@discussion Corresponds to the JavaScript operation <code>object[property]</code>. Pass an NSString * to access a named property. Other valid properties include symbols, numbers, and stringifiable objects. In macOS 10.13 and iOS11 and below, 'property' was an NSString *.

Style: "iOS11" => "iOS 11"

> Source/JavaScriptCore/API/JSValue.h:598
> + <code>setObject:object forKeyedSubscript:key</code> was an restricted to

Grammar: "was an restricted to"

> Source/JavaScriptCore/API/JSValue.h:696
> +// These declarations provide compatibility when targeting SDKs for legacy OSs. In macOS 10.13 and iOS11 and below, 'property' was an NSString *.
> +@interface JSValue (LegacyTargetSDK)

Should we mark these as DEPRECATED? See WK_API_DEPRECATED_WITH_REPLACEMENT in WebKit2.

> Source/JavaScriptCore/API/JSValue.mm:142
> +    JSStringRef string = JSStringCreateWithCFString(reinterpret_cast<CFStringRef>(description));

Style: Most code just does the toll-free bridge cast: (CFStringRef). reinterpret_cast draws the eye for little value.

> Source/JavaScriptCore/API/JSValueRef.cpp:92
> +    if (jsValue.isObject())
> +        return kJSTypeObject;
> +    ASSERT(jsValue.isSymbol());
> +    return kJSTypeSymbol;

It might make sense to keep object as the last. Its a more natural fallback in case something did go wrong earlier.

> Source/JavaScriptCore/API/tests/testapi.cpp:68
> +

Style: Drop this newline.

> Source/JavaScriptCore/API/tests/testapi.mm:597
> +        checkResult(@"Should be a created from Obc-C", symbol.isSymbol);

Typo: "Obc-C"

Could you also add:

  checkResult(@"Should toString with description", [[symbol toString] isEqualToString:@"Symbol(dope)"]);

> Source/JavaScriptCore/API/tests/testapi.mm:617
> +        checkResult(@"Seting by subscript with symbol should work", [object[iteratorSymbol] isEqual:theAnswer]);

Typo: "Seting" => "Setting" in a bunch of places.

[Keith Miller]

Comment on attachment 320198 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        support to our C/Obj-C APIs. This patch remedies that problem.
> 
> This doesn't seem like a useful intro.

It follows on the time honored tradition of software development, blaming people in the past!

I can remove it I suppose.

>> Source/JavaScriptCore/API/JSObjectRef.cpp:339
>> +        ASSERT_NOT_REACHED();
> 
> Why do we assert not reached? This is a public API, developers may call this with a nullptr. I can see it being useful to know in a debug build as a warning, but it is certainly possible to have happen.

I think we want to know if a test accidentally passes a null context.

>> Source/JavaScriptCore/API/JSObjectRef.h:565
>> +JS_EXPORT bool JSObjectHasPropertyKey(JSContextRef ctx, JSObjectRef object, JSValueRef propertyKey, JSValueRef* exception) CF_AVAILABLE(10_13, 11_0);
> 
> I think I convinced you earlier that we should go with a: JSC_AVAILABLE(JSC_MAC_TBA, JSC_IOS_TBA) and post-process like WebKit's processing:
> Source/WebKit/postprocess-framework-headers.sh
> 
> We want to do it, because every time we add new API this happens, and now is as good a time as any!

Yeah, I am still making the change I just haven't uploaded it yet.

>> Source/JavaScriptCore/API/JSValue.h:598
>> + <code>setObject:object forKeyedSubscript:key</code> was an restricted to
> 
> Grammar: "was an restricted to"

Fixed.

>> Source/JavaScriptCore/API/JSValue.h:696
>> +@interface JSValue (LegacyTargetSDK)
> 
> Should we mark these as DEPRECATED? See WK_API_DEPRECATED_WITH_REPLACEMENT in WebKit2.

These won't even be visible unless you are targeting an older SDK. The post processor will run unifdef on all the public header files.

>> Source/JavaScriptCore/API/JSValue.mm:142
>> +    JSStringRef string = JSStringCreateWithCFString(reinterpret_cast<CFStringRef>(description));
> 
> Style: Most code just does the toll-free bridge cast: (CFStringRef). reinterpret_cast draws the eye for little value.

I was just doing it with the reinterpret_cast to match the C++ style. We don't use the C style casts where there is another option I don't see why we would want to do it here?

>> Source/JavaScriptCore/API/JSValueRef.cpp:92
>> +    return kJSTypeSymbol;
> 
> It might make sense to keep object as the last. Its a more natural fallback in case something did go wrong earlier.

Yeah, that's fair I supposed. I'll fix.

>> Source/JavaScriptCore/API/tests/testapi.cpp:68
>> +
> 
> Style: Drop this newline.

I like the new line since it breaks all the arguments to the function from the head of the loop more cleanly.

>> Source/JavaScriptCore/API/tests/testapi.mm:617
>> +        checkResult(@"Seting by subscript with symbol should work", [object[iteratorSymbol] isEqual:theAnswer]);
> 
> Typo: "Seting" => "Setting" in a bunch of places.

Fixed.

[Yusuke Suzuki]

Can we get this?

[Keith Miller]

(In reply to Yusuke Suzuki from comment #34)
> Can we get this?

Yeah, I was going to get back to it when I'm done with https://bugs.webkit.org/show_bug.cgi?id=181953.

[Keith Miller]

Created attachment 338613 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "callback" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:95:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValue.h:692:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  JSC_API_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:181:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:190:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:200:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:300:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:311:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefInternal.h:53:  The parameter name "runLoop" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSBasePrivate.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:59:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:66:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:95:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 35 in 32 files


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

[Filip Pizlo]

Comment on attachment 338613 [details]
Patch

Looks sensible.  Can you land it behind a flag, disabled by default?

[Saam Barati]

Comment on attachment 338613 [details]
Patch

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

> Source/JavaScriptCore/API/JSValueRef.cpp:348
> +    RETURN_IF_EXCEPTION(scope, nullptr);

Don’t we want to eat this and inform callers that an exception occurred?

[Yusuke Suzuki]

Can we land this patch into the trunk?
Once it is introduced, I'll add a glib JSC API similar to this one :)

[Geoffrey Garen]

If there's interest now, let's land this patch as SPI, and we can take the steps to make it API separately.

[Keith Miller]

Created attachment 342228 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "callback" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:95:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValue.h:692:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  JSC_API_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:181:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:190:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:200:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:300:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:311:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefInternal.h:53:  The parameter name "runLoop" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSBasePrivate.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:64:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:71:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:100:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 35 in 34 files


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

[Keith Miller]

Created attachment 342312 [details]
Patch for landing

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "callback" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:95:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValue.h:692:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  JSC_API_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:181:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:190:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:200:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:300:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:311:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefInternal.h:53:  The parameter name "runLoop" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSBasePrivate.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:64:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:71:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:100:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 35 in 34 files


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

[Keith Miller]

Created attachment 342319 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "callback" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:95:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValue.h:692:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  JSC_API_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:181:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:190:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:200:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:300:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:311:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefInternal.h:53:  The parameter name "runLoop" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSBasePrivate.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:64:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:71:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:100:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 35 in 35 files


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

[Keith Miller]

Created attachment 342333 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "callback" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:95:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValue.h:692:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  JSC_API_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:181:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:190:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:200:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:300:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:311:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefInternal.h:53:  The parameter name "runLoop" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSBasePrivate.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:64:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:71:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:100:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 35 in 35 files


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

[Joseph Pecoraro]

*** Bug 168561 has been marked as a duplicate of this bug. ***

[Keith Miller]

Created attachment 345586 [details]
Patch for landing

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "callback" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:95:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValue.h:692:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  JSC_API_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:181:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:190:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:200:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:300:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:311:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefInternal.h:53:  The parameter name "runLoop" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSBasePrivate.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:64:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:71:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:100:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 35 in 33 files


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

[Joseph Pecoraro]

Comment on attachment 345586 [details]
Patch for landing

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

> Source/JavaScriptCore/API/JSValue.h:327
> +@discussion Corresponds to the JavaScript operation <code>object[property]</code>. Pass an NSString * to access a named property. Other valid properties include symbols, numbers, and stringifiable objects. In macOS 10.13 and iOS 11 and below, 'property' was an NSString *.
>  */
> -- (JSValue *)valueForProperty:(NSString *)property;
> +- (JSValue *)valueForProperty:(id)property;

What happens now if developers pass non-JS-ifyable values to this?

    [o valueForProperty:@"test"]; // good
    [o valueForProperty:myJSValue]; // good
    [o valueForProperty:[NSNull null]]; // good?
    [o valueForProperty:[[MyObject alloc] init]]; // bad

I'd expect at test, at least for the last, to ensure we get expected output (undefined) and not an exception or something.

> Source/JavaScriptCore/API/tests/testapi.mm:641
> +        checkResult(@"Should be a created from Obc-C", symbol.isSymbol);

Typo: "Obc-C" => "ObjC"

> Source/JavaScriptCore/API/tests/testapi.mm:691
> +    }

I might be cool to have a test that sets `object[iteratorSymbol]` to be a ObjC block and verifies the block gets invoked and results are expected when iterating `object`. But that may be overkill.

> Source/JavaScriptCore/postprocess-headers.sh:87
> +function rewrite_version_min_required_macros() {

Nit: The style elsewhere in this file is a space before the `()`

> Source/JavaScriptCore/postprocess-headers.sh:89
> +    echo "rewrite_version_min_required_macros"
> +    echo "${1}"

Is this debugging or expected output?

[Keith Miller]

Comment on attachment 345586 [details]
Patch for landing

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

>> Source/JavaScriptCore/API/tests/testapi.mm:691
>> +    }
> 
> I might be cool to have a test that sets `object[iteratorSymbol]` to be a ObjC block and verifies the block gets invoked and results are expected when iterating `object`. But that may be overkill.

Done.

[Keith Miller]

Comment on attachment 345586 [details]
Patch for landing

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

>> Source/JavaScriptCore/API/JSValue.h:327
>> +- (JSValue *)valueForProperty:(id)property;
> 
> What happens now if developers pass non-JS-ifyable values to this?
> 
>     [o valueForProperty:@"test"]; // good
>     [o valueForProperty:myJSValue]; // good
>     [o valueForProperty:[NSNull null]]; // good?
>     [o valueForProperty:[[MyObject alloc] init]]; // bad
> 
> I'd expect at test, at least for the last, to ensure we get expected output (undefined) and not an exception or something.

I added the test for the last one. This code  just calls the same method that the setValue called for the value.

So those should all work fine. If you pass a non-JSExported NSObject you will just get an empty wrapper object, which will get toPropertyKeyed when used as a property. i.e. it will return `o["[object Object]"]` in JS.

>> Source/JavaScriptCore/API/tests/testapi.mm:641
>> +        checkResult(@"Should be a created from Obc-C", symbol.isSymbol);
> 
> Typo: "Obc-C" => "ObjC"

fixed.

>> Source/JavaScriptCore/postprocess-headers.sh:87
>> +function rewrite_version_min_required_macros() {
> 
> Nit: The style elsewhere in this file is a space before the `()`

I realized this function isn't needed.

>> Source/JavaScriptCore/postprocess-headers.sh:89
>> +    echo "${1}"
> 
> Is this debugging or expected output?

Ditto.

[Keith Miller]

Created attachment 345773 [details]
Patch

[Keith Miller]

Created attachment 345777 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "callback" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:95:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:46:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  JSC_API_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:181:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:190:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:200:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:300:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:311:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefInternal.h:53:  The parameter name "runLoop" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSBasePrivate.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:64:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:71:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:100:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 35 in 33 files


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

[Keith Miller]

Created attachment 345788 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "callback" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:95:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:46:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  JSC_API_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:181:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:190:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:200:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:300:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:311:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefInternal.h:53:  The parameter name "runLoop" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSBasePrivate.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:64:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:71:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:100:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/APICast.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 36 in 35 files


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

[Keith Miller]

Created attachment 345792 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "callback" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:95:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:46:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  JSC_API_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:181:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:190:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:200:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:300:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:311:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefInternal.h:53:  The parameter name "runLoop" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSBasePrivate.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:64:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:71:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:100:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/APICast.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 36 in 35 files


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

[Keith Miller]

Created attachment 345797 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/API/JSTypedArray.h:48:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:63:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:74:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:87:  The parameter name "arrayType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:98:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:108:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:118:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:128:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:138:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:154:  The parameter name "bytesDeallocator" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:164:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSTypedArray.h:174:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:88:  The parameter name "callback" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefPrivate.h:95:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:46:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:54:  JSC_API_AVAILABLE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:161:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:181:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:190:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:200:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:300:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSValueRef.h:311:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRefInternal.h:53:  The parameter name "runLoop" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSBasePrivate.h:46:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:64:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:71:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSContextRef.h:100:  The parameter name "group" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/APICast.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:565:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:577:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:590:  The parameter name "attributes" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSObjectRef.h:602:  The parameter name "object" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 36 in 35 files


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

[Joseph Pecoraro]

Comment on attachment 345797 [details]
Patch

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

> Source/JavaScriptCore/API/tests/testapi.mm:698
> +        object[objCObject] = @(1);

Pro-tip: For integers you can just write `@1` instead of `@(1)`. For non-literals or floats you'd need to use the @(...) syntax.

Thanks for adding these additional tests, they look good!

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:10680
> +				HEADER_SEARCH_PATHS = (
> +					"\"$(JAVASCRIPTCORE_FRAMEWORKS_DIR)/JavaScriptCore.framework/PrivateHeaders\"",
> +					"$(inherited)",
> +				);

It looks like this patch modified the Xcode build variables HEADER_SEARCH_PATHS inside of the Xcode project.

This seems unnecessary since this is already in the ToolExecutable xcconfig:

    ToolExecutable.xcconfig
    52:HEADER_SEARCH_PATHS = "$(JAVASCRIPTCORE_FRAMEWORKS_DIR)/JavaScriptCore.framework/PrivateHeaders" $(inherited);

Are these changes still necessary?

> Source/JavaScriptCore/config.h:27
> +// Pick some obsenely large number because why not.

This comment seems out of date or irrelevant.

[Keith Miller]

Comment on attachment 345797 [details]
Patch

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

>> Source/JavaScriptCore/API/tests/testapi.mm:698
>> +        object[objCObject] = @(1);
> 
> Pro-tip: For integers you can just write `@1` instead of `@(1)`. For non-literals or floats you'd need to use the @(...) syntax.
> 
> Thanks for adding these additional tests, they look good!

Whaaaat?!? Changed.

>> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:10680
>> +				);
> 
> It looks like this patch modified the Xcode build variables HEADER_SEARCH_PATHS inside of the Xcode project.
> 
> This seems unnecessary since this is already in the ToolExecutable xcconfig:
> 
>     ToolExecutable.xcconfig
>     52:HEADER_SEARCH_PATHS = "$(JAVASCRIPTCORE_FRAMEWORKS_DIR)/JavaScriptCore.framework/PrivateHeaders" $(inherited);
> 
> Are these changes still necessary?

I dunno where those came from. I'll remove them all.

>> Source/JavaScriptCore/config.h:27
>> +// Pick some obsenely large number because why not.
> 
> This comment seems out of date or irrelevant.

Whoops. I changed it to "Use zero since it will be less than any possible version number".

[Keith Miller]

Created attachment 345810 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 345810 [details]
Patch for landing

Clearing flags on attachment: 345810

Committed r234227: <https://trac.webkit.org/changeset/234227>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

Windows ports get broken.

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/10867

> testapi.c.obj : error LNK2019: unresolved external symbol "int __cdecl testCAPIViaCpp(void)" (?testCAPIViaCpp@@YAHXZ) referenced in function _main [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\shell\testapiLib.vcxproj]
> C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\bin32\testapiLib.dll : fatal error LNK1120: 1 unresolved externals [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\shell\testapiLib.vcxproj]

[Fujii Hironori]

Committed r234248: <https://trac.webkit.org/changeset/234248>

[Fujii Hironori]

Filed Bug 188040 for AppleWin port.