Bug 125607 - Test new JSContext name APIs
Summary: Test new JSContext name APIs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-11 17:47 PST by Joseph Pecoraro
Modified: 2013-12-13 15:05 PST (History)
2 users (show)

See Also:


Attachments
[PATCH] Add Test (7.69 KB, patch)
2013-12-11 17:48 PST, Joseph Pecoraro
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2013-12-11 17:47:40 PST
I should have written a test when I added these APIs. Better late then never.
Comment 1 Joseph Pecoraro 2013-12-11 17:48:44 PST
Created attachment 219021 [details]
[PATCH] Add Test
Comment 2 Darin Adler 2013-12-12 00:28:06 PST
Comment on attachment 219021 [details]
[PATCH] Add Test

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.
Comment 3 Joseph Pecoraro 2013-12-13 15:05:33 PST
Landed r160494: <http://trac.webkit.org/changeset/160494>