Bug 221255 – Add a loader for <model> resources

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Quint]

Add a loader for <model> resources

[Antoine Quint]

Created attachment 418986 [details]
Patch

[youenn fablet]

Comment on attachment 418986 [details]
Patch

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

> Source/WebCore/Modules/model-element/ModelResourceLoader.cpp:46
> +    , m_document(makeWeakPtr(document))

I am not sure m_document is needed, ContextDestructionObserver should allow you to get it as a scriptExecutionContext.

> Source/WebCore/Modules/model-element/ModelResourceLoader.cpp:48
> +    , m_crossOriginMode(crossOriginMode)

Maybe String&& crossOriginMode

> Source/WebCore/Modules/model-element/ModelResourceLoader.cpp:61
> +    m_modelElement = nullptr;

Can the model element be bound to another document during its lifetime?

> Source/WebCore/Modules/model-element/ModelResourceLoader.cpp:100
> +    auto resource = m_document->cachedResourceLoader().requestModel(WTFMove(cachedRequest)).value_or(nullptr);

What would be the destination of this request?
This is visible from service worker for instance. I guess we might need a new one? Or use Image maybe?

> Source/WebCore/Modules/model-element/ModelResourceLoader.cpp:102
> +        return nullptr;

I would try to do like DocumentThreadableLoader and log the error to the console in case of error.

> Source/WebCore/Modules/model-element/ModelResourceLoader.cpp:104
> +    Ref<ModelResource> modelResource = ModelResource::create(*this, resource);

auto.
You can also probably move resource here.

> Source/WebCore/Modules/model-element/ModelResourceLoader.cpp:105
> +    m_resources.add(modelResource.ptr());

Can we use a WeakHashSet?

> Source/WebCore/Modules/model-element/ModelResourceLoader.cpp:116
> +Ref<ModelResource> ModelResource::create(ModelResourceLoader& loader, CachedResourceHandle<CachedRawResource> resource)

CachedResourceHandle<CachedRawResource>&&

> Source/WebCore/Modules/model-element/ModelResourceLoader.cpp:118
> +    return adoptRef(*new ModelResource(loader, resource));

WTFMove(resource)

> Source/WebCore/Modules/model-element/ModelResourceLoader.cpp:121
> +ModelResource::ModelResource(ModelResourceLoader& loader, CachedResourceHandle<CachedRawResource> resource)

CachedResourceHandle<CachedRawResource>&&

> Source/WebCore/Modules/model-element/ModelResourceLoader.cpp:122
> +    : m_loader(loader)

This is dangerous, ModelResource is refable so there is a possibility it will be live longer than its ModelResourceLoader.
I would use a WeakPtr for loader.

> Source/WebCore/Modules/model-element/ModelResourceLoader.cpp:144
> +void ModelResource::responseReceived(CachedResource& resource, const ResourceResponse& response, CompletionHandler<void()>&& completionHandler)

This is sharing a lot of code with MediaResource::responseReceived.
How about trying to add a method to MediaResourceLoader called requestModelResource and rename MediaResourceLoader::requestResource as MediaResourceLoader::requestAudioVideoResource?

It seems ModelResource and MediaResource have a lot in common.
Maybe we could update MediaResource to take a type member so that the differences between model and media resources would be handled within MediaResource thanks to this type member?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:135
> +    case CachedResource::Type::ModelResource:

Unclear how much ModelResource differs from MediaResource.
Might be worth it for logging though.

[Antoine Quint]

Created attachment 418993 [details]
Patch

[Antoine Quint]

Created attachment 418994 [details]
Patch

[Antoine Quint]

Created attachment 418996 [details]
Patch

[youenn fablet]

Comment on attachment 418996 [details]
Patch

Seems good overall.
I am wondering whether we should not add a service worker test to validate the destination is model for model loads.
Not sure whether we are ready for this though.

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

> Source/WebCore/loader/FetchOptions.h:40
> +#if ENABLE(MODEL_ELEMENT)

Probably not needed.

> Source/WebCore/platform/network/ResourceRequestBase.h:418
> +#if ENABLE(MODEL_ELEMENT)

Remove it or add it in enum class Requester : uint8_t

[Antoine Quint]

Created attachment 419003 [details]
Patch

[youenn fablet]

Comment on attachment 419003 [details]
Patch

r=me once bots are happy.
Can you file a bug to keep track of adding a service worker destination test for model?

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

> Source/WebCore/loader/MediaResourceLoader.h:71
> +    FetchOptions::Destination m_destination;

Might need FetchOptions.h include for non unified builds.

> Source/WebCore/loader/cache/CachedResource.h:184
> +    bool isMainOrMediaOrIconOrRawResource() const

Sounds good. Ideally we would move the definition to an inline method in the header:
class {
    bool isMainOrMediaOrIconOrRawResource() const;
};

inline bool CachedResource::isMainOrMediaOrIconOrRawResource() const
{
    return ...
}

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:326
> +    CachedResource::Type resourceType = CachedResource::Type::ModelResource;

auto.
And should probably MediaResource here instead of ModelResource.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:397
> +            return ResourceLoadInfo::Type::Other;

Media might be better than Other here?

[Antoine Quint]

Created attachment 419011 [details]
Patch

[Sam Weinig]

Out of interest, what is the reasoning behind reusing MediaResourceLoader for this? Not saying it is wrong, just that the ChangeLog doesn't say. 

What features of the MediaResourceLoader are being used that say, just using CachedRawResource doesn't get you?

[youenn fablet]

> What features of the MediaResourceLoader are being used that say, just using
> CachedRawResource doesn't get you?

MediaResourceLoader is handling media subresource loads, it handles things like CORS, CSP, contrary to CachedRawResource which is more flexible.
As a first approximation, it seems ok to design model loading similarly to media loading.

[Antoine Quint]

Created attachment 419125 [details]
Patch

[Antoine Quint]

Committed r272313: <https://trac.webkit.org/changeset/272313>

[Radar WebKit Bug Importer]

<rdar://problem/73930776>

[Sam Weinig]

(In reply to youenn fablet from comment #11)
> > What features of the MediaResourceLoader are being used that say, just using
> > CachedRawResource doesn't get you?
> 
> MediaResourceLoader is handling media subresource loads, it handles things
> like CORS, CSP, contrary to CachedRawResource which is more flexible.
> As a first approximation, it seems ok to design model loading similarly to
> media loading.

Does <model> want the media behavior for CORS and CSP? Do we want the behavior of PlatformMediaResourceLoader for this? Really feels like using MediaResourceLoader isn't the right call for this right now.

[Sam Weinig]

(In reply to Sam Weinig from comment #15)
> (In reply to youenn fablet from comment #11)
> > > What features of the MediaResourceLoader are being used that say, just using
> > > CachedRawResource doesn't get you?
> > 
> > MediaResourceLoader is handling media subresource loads, it handles things
> > like CORS, CSP, contrary to CachedRawResource which is more flexible.
> > As a first approximation, it seems ok to design model loading similarly to
> > media loading.
> 
> Does <model> want the media behavior for CORS and CSP? Do we want the
> behavior of PlatformMediaResourceLoader for this? Really feels like using
> MediaResourceLoader isn't the right call for this right now.

Also, I'm pretty sure all cached resources support CORS and CSP, including CachedRawResource. 

For CORS, CachedResourceLoader should follow CORS rules due based on the ResourceLoaderOptions/FetchOptions passed in. 

For CSP, most of seems to be via CachedResourceLoader::allowedByContentSecurityPolicy(), where you can add a resource type for specific policy differences.