Add sourceURL to _evaluateJavaScript: so the scripts appear in Web Inspector

• Component: WebKit API
• Status: RESOLVED FIXED

[Timothy Hatcher]

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.

[Timothy Hatcher]

<rdar://problem/62074376>

[Timothy Hatcher]

Created attachment 399383 [details]
Patch

[Timothy Hatcher]

Created attachment 399388 [details]
Patch

[EWS Watchlist]

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

[Devin Rousso]

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 🤔

[Timothy Hatcher]

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.

[Timothy Hatcher]

Created attachment 399391 [details]
Patch

[Timothy Hatcher]

Created attachment 399396 [details]
Patch

[Timothy Hatcher]

Created attachment 399397 [details]
Patch

[Devin Rousso]

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]`).

[Timothy Hatcher]

Created attachment 399400 [details]
Patch

[EWS]

Committed r261718: <https://trac.webkit.org/changeset/261718>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399400 [details].