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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 19 19:37:10 PST 2021

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 164860: JS Modules in Workers

Attachment 420996: Patch


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

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


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

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.


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

More information about the webkit-reviews mailing list