We were missing the URL, using the frame's document URL instead of the original source URL. This caused two issues, one the error messages were in the wrong location when shown in the console. And the script would not appear in the Sources tab of the Inspector.
<rdar://problem/62074376>
Created attachment 399383 [details] Patch
Created attachment 399388 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 399388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399388&action=review r=me, but I think you missed some cases in GLib code > Source/WebCore/bindings/js/RunJavaScriptParameters.h:50 > + RunJavaScriptParameters(const String& source, const URL& sourceURL, bool runAsAsyncFunction, Optional<ArgumentWireBytesMap>&& arguments, bool forceUserGesture) I feel like we should just always use `URL&&`. > Source/WebCore/bindings/js/ScriptController.cpp:594 > + URL sourceURL = parameters.sourceURL; Do we need to pull this out into a local or can we just inline it? NIT: `auto`? > Source/WebCore/bindings/js/ScriptController.cpp:645 > + URL sourceURL = parameters.sourceURL; > + String sourceURLString = sourceURL.string(); NIT: `auto`? > Source/WebCore/bindings/js/ScriptController.cpp:-645 > - String sourceURL = jsSourceCode.provider()->url().string(); I think we can/should keep this logic so that we get the resolved `sourceURL` from the script itself, rather than what was passed in (just in case we add some logic later that mucks with it). ``` String sourceURLString = jsSourceCode.provider()->url().string(); ``` > Source/WebKit/UIProcess/API/Cocoa/WKUserScript.mm:-39 > - API::Object::constructInWrapper<API::UserScript>(self, WebCore::UserScript { WTF::String(source), API::UserScript::generateUniqueURL(), { }, { }, API::toWebCoreUserScriptInjectionTime(injectionTime), forMainFrameOnly ? WebCore::UserContentInjectedFrames::InjectInTopFrameOnly : WebCore::UserContentInjectedFrames::InjectInAllFrames, WebCore::WaitForNotificationBeforeInjecting::No }, API::ContentWorld::pageContentWorld()); I do like these changes, but are they strictly speaking necessary? > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:909 > + URL sourceURL = url; > + if (!sourceURL.isValid()) > + sourceURL = API::UserScript::generateUniqueURL(); Should we only do this if `url` is actually provided? Otherwise, we'd end up generating a `sourceURL` (which is really only used for Web Inspector) for _every_ evaluation 🤔
Comment on attachment 399388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399388&action=review >> Source/WebCore/bindings/js/RunJavaScriptParameters.h:50 >> + RunJavaScriptParameters(const String& source, const URL& sourceURL, bool runAsAsyncFunction, Optional<ArgumentWireBytesMap>&& arguments, bool forceUserGesture) > > I feel like we should just always use `URL&&`. Okay. >> Source/WebCore/bindings/js/ScriptController.cpp:594 >> + URL sourceURL = parameters.sourceURL; > > Do we need to pull this out into a local or can we just inline it? > > NIT: `auto`? Will fix. >> Source/WebCore/bindings/js/ScriptController.cpp:-645 >> - String sourceURL = jsSourceCode.provider()->url().string(); > > I think we can/should keep this logic so that we get the resolved `sourceURL` from the script itself, rather than what was passed in (just in case we add some logic later that mucks with it). > ``` > String sourceURLString = jsSourceCode.provider()->url().string(); > ``` Okay >> Source/WebKit/UIProcess/API/Cocoa/WKUserScript.mm:-39 >> - API::Object::constructInWrapper<API::UserScript>(self, WebCore::UserScript { WTF::String(source), API::UserScript::generateUniqueURL(), { }, { }, API::toWebCoreUserScriptInjectionTime(injectionTime), forMainFrameOnly ? WebCore::UserContentInjectedFrames::InjectInTopFrameOnly : WebCore::UserContentInjectedFrames::InjectInAllFrames, WebCore::WaitForNotificationBeforeInjecting::No }, API::ContentWorld::pageContentWorld()); > > I do like these changes, but are they strictly speaking necessary? I wanted to clean it up, was bothering me. >> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:909 >> + sourceURL = API::UserScript::generateUniqueURL(); > > Should we only do this if `url` is actually provided? Otherwise, we'd end up generating a `sourceURL` (which is really only used for Web Inspector) for _every_ evaluation 🤔 Yeah, I'll drop this. Good call.
Created attachment 399391 [details] Patch
Created attachment 399396 [details] Patch
Created attachment 399397 [details] Patch
Comment on attachment 399397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399397&action=review > Source/WebCore/bindings/js/ScriptController.cpp:644 > + auto sourceURL = parameters.sourceURL; > + if (!sourceURL.isValid()) { > + // FIXME: This is gross, but when setTimeout() and setInterval() are passed JS strings, the thrown errors should use the frame document URL (according to WPT). > + sourceURL = m_frame.document()->url(); > + } I think this needs to be in `ScriptController::executeScriptInWorld` right above the `evaluateInWorld` call, _not_ in `ScriptController::callInWorld` (which was added for `-[WKWebView callAsyncJavaScript:arguments:inContentWorld:completionHandler]`).
Created attachment 399400 [details] Patch
Committed r261718: <https://trac.webkit.org/changeset/261718> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399400 [details].