Bug 198256 – Fix opensource build of testapi

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 370649 [details]
Patch

[Alexey Proskuryakov]

Looks like this regresses TestWebKitAPI._WKDownload.DownloadMonitorCancel?

[Alexey Proskuryakov]

Looking at the patch, it appears impossible. Must be a bad case of flakiness.

[Alexey Proskuryakov]

Comment on attachment 370649 [details]
Patch

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

> Source/JavaScriptCore/API/JSScript.mm:87
> -        createError([NSString stringWithFormat:@"Cache path `%s` does not contain in a valid directory", systemPath.utf8().data()], error);
> +        createError([NSString stringWithFormat:@"Cache path `%s` does not contain in a valid directory", systemPath.ascii().data()], error);

The post-commit comment was that you should use %@ instead of %s. Obviously, you cannot just use %@ as a replacement, because it expects an Objective-C object. But you cannot use ascii() either, because the path may not be all ASCII.

In this case, it's probably easiest to convert systemPath to an NSString, and then format with %@. In more performance sensitive cases, we'd probably want to stick with WTF.

[Tadeu Zagallo]

Created attachment 370672 [details]
Patch

[Tadeu Zagallo]

Comment on attachment 370649 [details]
Patch

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

>> Source/JavaScriptCore/API/JSScript.mm:87
>> +        createError([NSString stringWithFormat:@"Cache path `%s` does not contain in a valid directory", systemPath.ascii().data()], error);
> 
> The post-commit comment was that you should use %@ instead of %s. Obviously, you cannot just use %@ as a replacement, because it expects an Objective-C object. But you cannot use ascii() either, because the path may not be all ASCII.
> 
> In this case, it's probably easiest to convert systemPath to an NSString, and then format with %@. In more performance sensitive cases, we'd probably want to stick with WTF.

Oh, now I see that WTF::String has a conversion operator to NSString. I've updated the patch, thanks!

[Alexey Proskuryakov]

Comment on attachment 370672 [details]
Patch

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

> Source/JavaScriptCore/API/JSScript.mm:80
> +            createError([NSString stringWithFormat:@"Cache path `%@` already exists and is not a file", static_cast<NSString*>(systemPath)], error);

Please add a space between NSString and the star, since this is an Objective-C class name.

> Source/JavaScriptCore/API/JSScript.mm:87
> +        createError([NSString stringWithFormat:@"Cache path `%@` does not contain in a valid directory", static_cast<NSString*>(systemPath)], error);

Ditto.

> Source/JavaScriptCore/API/JSScript.mm:92
> +        createError([NSString stringWithFormat:@"Cache directory `%@` is not a directory or does not exist", static_cast<NSString*>(directory)], error);

Ditto.

[Tadeu Zagallo]

Created attachment 370708 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 370708 [details]
Patch for landing

Clearing flags on attachment: 370708

Committed r245800: <https://trac.webkit.org/changeset/245800>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51163764>

[Aakash Jain]

(In reply to Alexey Proskuryakov from comment #3)
> Looking at the patch, it appears impossible. Must be a bad case of flakiness.
https://bugs.webkit.org/show_bug.cgi?id=198288