[webkit-reviews] review granted: [Bug 164860] JS Modules in Workers : [Attachment 420996] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 164860: JS Modules in Workers
https://bugs.webkit.org/show_bug.cgi?id=164860

Attachment 420996: Patch

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




--- Comment #30 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 420996
  --> https://bugs.webkit.org/attachment.cgi?id=420996
Patch

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

r=me

Do we want some more tests here? Especially for various error cases, and
various complex programs and such?

> Source/WebCore/bindings/js/JSDOMExceptionHandling.cpp:172
> +	   // FIXME: All callers to createDOMException need to pass in the
correct global object.
> +	   // For now, we're going to assume the lexicalGlobalObject. Which is
wrong in cases like this:
> +	   // frames[0].document.createElement(null, null); // throws an
exception which should have the subframe's prototypes.

maybe file a bug for this?

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:332
> +void ScriptModuleLoader::notifyFinished(ModuleScriptLoader&
moduleScriptLoader, URL&& sourceURL, Ref<DeferredPromise> promise)

nit: why not just take URL here (callers can still move into the parameter)?
That way we  know this function takes ownership of URL. I don't think this
sourceURL parameter is actually moved anywhere here, or at least there are
early return paths that don't take ownership over it.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:404
> +		   rejectToPropagateNetworkError(promise.get(),
ModuleFetchFailureKind::WasCanceled, "Importing a module script is
canceled."_s);

nit: I wonder if this error should be more like "Loading a module script was
cancelled"  or "Load for importing a module script was canceled"

> Source/WebCore/workers/WorkerGlobalScope.cpp:282
> +	   return Exception { TypeError, "importScripts cannot be used if
woerker type is \"module\""_s };

"woerker" => "worker"

> Source/WebCore/workers/WorkerOrWorkletScriptController.cpp:364
> +	       // Overwrite the detailed error with a generic error.

nit: not sure this comment is adding anything

> Source/WebCore/workers/WorkerOrWorkletScriptController.cpp:425
> +		       // Overwrite the detailed error with a generic error.

ditto

I wonder if we can share this  code  w/ above that seems to do a similar thing.