[webkit-reviews] review granted: [Bug 192734] SourceProviders should use an actual URL instead of a string : [Attachment 358156] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <yusukesuzuki at slowstart.org> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 192734: SourceProviders should use an actual URL instead of a string
https://bugs.webkit.org/show_bug.cgi?id=192734

Attachment 358156: Patch

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




--- Comment #26 from Yusuke Suzuki <yusukesuzuki at slowstart.org> ---
Comment on attachment 358156
  --> https://bugs.webkit.org/attachment.cgi?id=358156
Patch

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

r=me, nice direction.

> Source/JavaScriptCore/ChangeLog:20
> +	   * jsc.cpp:

In this patch, the provider's sourceURL actually becomes URL. But in jsc shell,
we previously did not use URL. We used file path instead.
This patch does not use file URL for the jsc shell. Instead we create invalid
URL which holds a file path. Is my understanding correct?
If it is correct, it would be nice if we have this description in the
ChangeLog.

> Source/WebCore/ChangeLog:14
> +	   WTFMoving it.

Nice.

> Source/JavaScriptCore/jsc.cpp:2447
> +		   URL fileNameURL = URL({ }, (fileName));

Unnecessary parentheses around `fileName`.

> Source/JavaScriptCore/jsc.cpp:2448
> +		   promise = loadAndEvaluateModule(globalObject->globalExec(),
makeSource(stringFromUTF(scriptBuffer), SourceOrigin { absolutePath(fileName)
}, WTFMove(fileNameURL), TextPosition(), SourceProviderSourceType::Module),
jsUndefined());

Do we need this `fileNameURL` variable? Passing `URL({ }, fileName)` directly
to loadAndEvaluateModule seems fine.