[webkit-reviews] review granted: [Bug 205294] import.meta.url: baseURL for a module script should be response URL, not request URL : [Attachment 396416] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 205294: import.meta.url: baseURL for a module script should be response
URL, not request URL
https://bugs.webkit.org/show_bug.cgi?id=205294

Attachment 396416: Patch

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




--- Comment #21 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 396416
  --> https://bugs.webkit.org/attachment.cgi?id=396416
Patch

LGTM!
Some nits below.

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

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:109
> +    ASSERT(baseURL.isValid());

Maybe ASSERT that the url is valid in
ScriptModuleLoader::responseURLFromRequestURL or in resolveModuleSpecifier

>
LayoutTests/http/wpt/html/semantics/scripting-1/the-script-element/module/modul
e-meta-url-redirect.html:11
> +let basename =
`http://${document.location.host}/WebKit/html/semantics/scripting-1/the-script-
element/module`;

Could use const here and elsewhere.

>
LayoutTests/http/wpt/html/semantics/scripting-1/the-script-element/module/modul
e-meta-url-redirect.html:13
> +    return new Promise(function (resolve, reject) {

You use (resolve, reject) => elsewhere.
It is apparently good style to limit the code executed in the function(resolve,
reject) as well.