Bug 195782 - Remove retain cycle from JSScript and also don't keep the cache file descriptor open so many JSScripts can be cached in a loop
Summary: Remove retain cycle from JSScript and also don't keep the cache file descript...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-14 16:59 PDT by Saam Barati
Modified: 2019-03-14 18:12 PDT (History)
14 users (show)

See Also:

Attachments
patch (15.75 KB, patch)
2019-03-14 17:22 PDT, Saam Barati 		msaboff: review+
Details | Formatted Diff | Diff
patch for landing (15.74 KB, patch)
2019-03-14 18:04 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2019-03-14 16:59:43 PDT 
This will address feedback of folks trying to use this API.
Comment 1 Saam Barati 2019-03-14 17:00:14 PDT 
<rdar://problem/48880625>
Comment 2 Saam Barati 2019-03-14 17:22:39 PDT 
Created attachment 364726 [details]
patch
Comment 3 EWS Watchlist 2019-03-14 17:24:02 PDT 
Attachment 364726 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2205:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/tests/testapi.mm:2237:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Michael Saboff 2019-03-14 17:45:54 PDT 
Comment on attachment 364726 [details]
patch

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

r=me

> Source/JavaScriptCore/API/JSScript.mm:290
> +        error = "No cache was path provided during construction of this JSScript."_s;

I know you didn't change this error message, but shouldn't it read "No cache *path was* provided during construction of this JSScript."?
Comment 5 Saam Barati 2019-03-14 17:51:39 PDT 
Comment on attachment 364726 [details]
patch

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

>> Source/JavaScriptCore/API/JSScript.mm:290
>> +        error = "No cache was path provided during construction of this JSScript."_s;
> 
> I know you didn't change this error message, but shouldn't it read "No cache *path was* provided during construction of this JSScript."?

Yeah will fix.
Comment 6 Saam Barati 2019-03-14 18:04:08 PDT 
Created attachment 364743 [details]
patch for landing
Comment 7 Saam Barati 2019-03-14 18:12:03 PDT 
landed in:
https://trac.webkit.org/changeset/242980/webkit