Bug 198256 - Fix opensource build of testapi
Summary: Fix opensource build of testapi
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-26 07:24 PDT by Tadeu Zagallo
Modified: 2019-05-28 02:47 PDT (History)
13 users (show)

See Also:


Attachments
Patch (4.09 KB, patch)
2019-05-26 07:40 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (4.13 KB, patch)
2019-05-26 23:02 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch for landing (4.14 KB, patch)
2019-05-27 14:22 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-05-26 07:24:22 PDT
...
Comment 1 Tadeu Zagallo 2019-05-26 07:40:53 PDT
Created attachment 370649 [details]
Patch
Comment 2 Alexey Proskuryakov 2019-05-26 20:43:59 PDT
Looks like this regresses TestWebKitAPI._WKDownload.DownloadMonitorCancel?
Comment 3 Alexey Proskuryakov 2019-05-26 20:44:54 PDT
Looking at the patch, it appears impossible. Must be a bad case of flakiness.
Comment 4 Alexey Proskuryakov 2019-05-26 20:54:44 PDT
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.
Comment 5 Tadeu Zagallo 2019-05-26 23:02:49 PDT
Created attachment 370672 [details]
Patch
Comment 6 Tadeu Zagallo 2019-05-26 23:08:51 PDT
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!
Comment 7 Alexey Proskuryakov 2019-05-27 13:09:06 PDT
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.
Comment 8 Tadeu Zagallo 2019-05-27 14:22:08 PDT
Created attachment 370708 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2019-05-27 14:51:46 PDT
Comment on attachment 370708 [details]
Patch for landing

Clearing flags on attachment: 370708

Committed r245800: <https://trac.webkit.org/changeset/245800>
Comment 10 WebKit Commit Bot 2019-05-27 14:51:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-05-27 14:52:21 PDT
<rdar://problem/51163764>
Comment 12 Aakash Jain 2019-05-28 02:47:44 PDT
(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