Bug 184385 – Have WorkerScriptLoader::loadAsynchronously() take a FetchOptions

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Have WorkerScriptLoader::loadAsynchronously() take a ThreadableLoaderOptions.

[Daniel Bates]

Created attachment 337425 [details]
Patch

[youenn fablet]

Comment on attachment 337425 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        to support blocking scripts with a non-JavaScript MIME type in a subsequent commit.

I do not really like to expose ThreadableLoaderOptions outside WorkerScriptLoader, this is an implementation detail and we might want to go away from it.
I do not really like to expose ResourceLoaderOptions either. Again implementation detail, in particular sendLoadCallbacks which is meaningless to WorkerScriptLoader clients.

Ideally we would just pass a FetchOptions and try to compute the other parameters inside WorkerScriptLoader using the ScriptExecutionContext.
Would it be possible to pass FetchOptions + an enum stating whether this is for a service worker job?

[Daniel Bates]

(In reply to youenn fablet from comment #2)
> Comment on attachment 337425 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337425&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        to support blocking scripts with a non-JavaScript MIME type in a subsequent commit.
> 
> I do not really like to expose ThreadableLoaderOptions outside
> WorkerScriptLoader, this is an implementation detail and we might want to go
> away from it.

I do not see the need to guard against such implementation details because WorkerScriptLoader is a facade that provides a convenient API for interacting with a ThreadableLoader. The facade nature of WorkerScriptLoader is analogous to CachedResourceLoader, which adds caching and invalidation logic around ResourceLoader. And ResourceLoaderOptions can be used by both ResourceLoader and CachedResourceLoader. (And this makes sense because CachedResourceLoader is a facade for interacting with ResourceLoader). That is, CachedResourceLoader and WorkerScriptLoader exist as conveniences. A caller does not need to make use of either one and can work with ThreadableLoader and ResourceLoader directly. Although it is preferred and more time efficient for a caller to make use of WorkerScriptLoader or CachedResourceLoader instead of re-implementing equivalent behavior plus or minus the quirk(s) the caller needs for the load.

> I do not really like to expose ResourceLoaderOptions either. Again
> implementation detail, in particular sendLoadCallbacks which is meaningless
> to WorkerScriptLoader clients.
> 

sendLoadCallbacks represents a coupling of the ResourceLoader and FrameLoader code. We should fix this up such that fields on ThreadableLoaderOptions/ResourceLoaderOptions are applicable to loading and not side effects of loading (as exemplified by sendLoadCallbacks). Fixing these issues would avoid exposing "meaningless" options. I do not see the need to fix this up before fixing this bug.

> Ideally we would just pass a FetchOptions and try to compute the other
> parameters inside WorkerScriptLoader using the ScriptExecutionContext.

Keying off of the type of ScriptExecutionContext is not sufficient for asynchronous loading using WorkerScriptLoader because both a Web Worker and Service Worker perform such loads with respect to the Document (i.e. ScriptExecutionContext::isDocument() evaluates true). This is consistent with our current approach of defining a virtual function on ThreadableLoaderClient, isServiceWorkerClient, and querying it at <<https://trac.webkit.org/browser/trunk/Source/WebCore/workers/WorkerScriptLoader.cpp?rev=230346#L99> to differentiate between an asynchronous load initiated as part of creating a Web Worker from an asynchronous load on behalf of the Service Worker machinery.

> Would it be possible to pass FetchOptions + an enum stating whether this is
> for a service worker job?

[Daniel Bates]

(In reply to youenn fablet from comment #2)
> Would it be possible to pass FetchOptions + an enum stating whether this is
> for a service worker job?

I didn't mean to ignore this question. This seems like bad programming practice as it is comparable to type checking: the enum represents the type information of the caller. It is better to pass information to an algorithm that encapsulates the behavior change than it is to perform type checking in the algorithm itself to change its behavior. Unlike the former, the latter imposes a coupling on the types. ThreadableLoaderOptions and ResourceLoaderOptions encapsulate the various behavioral changes for a load for the most part and I do not see a need to guard exposing ThreadableLoaderOptions. See the paragraph of my reply in comment 3 for more details.

[youenn fablet]

Specs are defined in terms of fetch request, not in terms of ThreadableLoader or ResourceLoader.
Ideally we would write loading client code in terms of fetch request fields only.
We are not yet there but we should keep that in mind and try to not expose additional fields.

Exposing ServiceWorkerMode is fine, FetchOptions as well.
Exposing ContentSecurityPolicyEnforcement might not be great but I guess it is there for some time so I am fine keeping it the way it is.

Your pending patch is related to FetchOptions::destination so exposing FetchOptions should be ok, right?

I am ok exposing FetchOptions + ServiceWorkerMode or FetchOptions + ServiceWorkerMode + ContentSecurityPolicyEnforcement.

[Daniel Bates]

(In reply to youenn fablet from comment #5)
> Your pending patch is related to FetchOptions::destination so exposing
> FetchOptions should be ok, right?
> 

For now, FetchOptions should be sufficient.

[Daniel Bates]

Created attachment 337440 [details]
Patch

Updated patch to pass a FetchOptions

[youenn fablet]

Comment on attachment 337440 [details]
Patch

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

> Source/WebCore/loader/ThreadableLoader.h:66
> +        ThreadableLoaderOptions(const FetchOptions&);

We could use explicit here.
In theory we should probably take a FetchOptions&& in case we set integrity in the future for instance.

> Source/WebCore/workers/WorkerScriptLoader.cpp:96
> +    // service worker mode as an argument.

I don't really understand this comment.
The service worker spec states to set the service worker mode to none in case of a service worker job.
And a service worker job could be executed from a worker/service worker context.

[Daniel Bates]

Comment on attachment 337440 [details]
Patch

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

>> Source/WebCore/loader/ThreadableLoader.h:66
>> +        ThreadableLoaderOptions(const FetchOptions&);
> 
> We could use explicit here.
> In theory we should probably take a FetchOptions&& in case we set integrity in the future for instance.

Will fix before landing.

>> Source/WebCore/workers/WorkerScriptLoader.cpp:96
>> +    // service worker mode as an argument.
> 
> I don't really understand this comment.
> The service worker spec states to set the service worker mode to none in case of a service worker job.
> And a service worker job could be executed from a worker/service worker context.

The point of the comment is to explain why we cannot use scriptExecutionContext.isServiceWorkerGlobalScope() to determine the value of serviceWorkersMode as we do in WorkerScriptLoader::loadSynchronously(). From debugging WorkerScriptLoader::loadAsynchronously() can be called from the service worker job machinery with a document script execution context.

I'll change the comment to read:

A service worker job can be executed from a worker context or document context.

[Daniel Bates]

Committed r230374: <https://trac.webkit.org/changeset/230374>

[Radar WebKit Bug Importer]

<rdar://problem/39264322>