| Summary: | Fix opensource build of testapi | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tadeu Zagallo <tzagallo> | ||||||||
| Component: | JavaScriptCore | Assignee: | Tadeu Zagallo <tzagallo> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | aakash_jain, ap, commit-queue, ews-watchlist, fpizlo, jbedard, keith_miller, mark.lam, msaboff, rmorisset, saam, webkit-bug-importer, ysuzuki | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Tadeu Zagallo
2019-05-26 07:24:22 PDT
Created attachment 370649 [details]
Patch
Looks like this regresses TestWebKitAPI._WKDownload.DownloadMonitorCancel? Looking at the patch, it appears impossible. Must be a bad case of flakiness. 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. Created attachment 370672 [details]
Patch
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 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. Created attachment 370708 [details]
Patch for landing
Comment on attachment 370708 [details] Patch for landing Clearing flags on attachment: 370708 Committed r245800: <https://trac.webkit.org/changeset/245800> All reviewed patches have been landed. Closing bug. (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 |