[webkit-reviews] review granted: [Bug 211904] Add sourceURL to _evaluateJavaScript: so the scripts appear in Web Inspector : [Attachment 399388] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 211904: Add sourceURL to _evaluateJavaScript: so the scripts appear in Web
Inspector
https://bugs.webkit.org/show_bug.cgi?id=211904

Attachment 399388: Patch

https://bugs.webkit.org/attachment.cgi?id=399388&action=review




--- Comment #5 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 399388
  --> https://bugs.webkit.org/attachment.cgi?id=399388
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 ��