Bug 189652 - Use OpaqueJSString rather than JSRetainPtr inside WebKit
Summary: Use OpaqueJSString rather than JSRetainPtr inside WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-16 09:04 PDT by Darin Adler
Modified: 2018-09-23 15:15 PDT (History)
7 users (show)

See Also:

Attachments
Patch (33.13 KB, patch)
2018-09-16 12:32 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
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 Darin Adler 2018-09-16 09:04:20 PDT 
Use OpaqueJSString rather than JSRetainPtr inside WebKit
Comment 1 Darin Adler 2018-09-16 12:32:11 PDT 
Created attachment 349873 [details]
Patch
Comment 2 Darin Adler 2018-09-16 12:32:59 PDT 
Keith, this patch is a response to a comment you made in my last one about using smart pointers more consistently. I realized that inside WebKit we should use RefPtr and Ref, not JSRetainPtr, for JSStringRef.
Comment 3 Saam Barati 2018-09-17 09:29:13 PDT 
Comment on attachment 349873 [details]
Patch

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

r=me

> Source/JavaScriptCore/API/JSContext.mm:104
> +    auto sourceURLJS = OpaqueJSString::create([sourceURL absoluteString]);

This removes a null check on sourceURL. Is that intended?

> Source/JavaScriptCore/API/JSContext.mm:105
> +    JSValueRef result = JSEvaluateScript(m_context, scriptJS.get(), nullptr, sourceURLJS.get(), 0, &exceptionValue);

Ignore the above comment (I’m on my phone and can’t edit it). I knew that ObjC message send is safe on null, but I didn’t know it’s guaranteed to also return null
Comment 4 WebKit Commit Bot 2018-09-17 09:55:06 PDT 
Comment on attachment 349873 [details]
Patch

Clearing flags on attachment: 349873

Committed r236066: <https://trac.webkit.org/changeset/236066>
Comment 5 WebKit Commit Bot 2018-09-17 09:55:07 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2018-09-17 09:56:20 PDT 
<rdar://problem/44523492>
Comment 7 Darin Adler 2018-09-23 15:15:09 PDT 
Looks like I missed the JavaScriptCore/API/glib directory when making this change. The code in there still uses JSRetainPtr, not OpaqueJSString.