Summary: | Add sourceURL to _evaluateJavaScript: so the scripts appear in Web Inspector | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Timothy Hatcher <timothy> | ||||||||||||||
Component: | WebKit API | Assignee: | Timothy Hatcher <timothy> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | beidson, berto, cgarcia, ews-watchlist, gustavo, hi, thorton, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Timothy Hatcher
2020-05-14 10:06:01 PDT
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]. |