[webkit-reviews] review granted: [Bug 125607] Test new JSContext name APIs : [Attachment 219021] [PATCH] Add Test

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 125607: Test new JSContext name APIs
https://bugs.webkit.org/show_bug.cgi?id=125607

Attachment 219021: [PATCH] Add Test
https://bugs.webkit.org/attachment.cgi?id=219021&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219021&action=review


> Source/JavaScriptCore/API/tests/testapi.c:1089
> +    JSGlobalContextRef context = JSGlobalContextCreate(0);

nullptr

> Source/JavaScriptCore/API/tests/testapi.c:1092
> +    result &= assertTrue(!str, "Default context name is NULL");

Not sure we should capitalize NULL just because there’s a C macro that we never
use by that name. I think the word null is fine without shouting.

> Source/JavaScriptCore/API/tests/testapi.mm:1203
> +	   JSContext *context = [[JSContext alloc] init];
> +	   checkResult(@"default context.name is nil", context.name == nil);
> +	   context.name = @"Foo";
> +	   checkResult(@"set context.name is expected", [context.name
isEqualToString:@"Foo"]);

Why less testing here? Why not replicate the “two names” thing you did in the C
test?

> Source/JavaScriptCore/ChangeLog:9
> +	   * API/JSContext.h:
> +	   * API/JSContextRef.h:

Really ought to say why you touched these files. Obviously not to “test new
JSContext name APIs”.

> Tools/ChangeLog:8
> +	   Test new JSContext name APIs
> +	   https://bugs.webkit.org/show_bug.cgi?id=125607
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * Scripts/run-javascriptcore-tests:

Change has nothing to do with the bug title.