Bug 154117 – Make MediaResourceLoader behave more like a CachedResourceLoader.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Make MediaResourceLoader behave more like a CachedResourceLoader.

[Jer Noble]

Created attachment 271067 [details]
Patch

[Alex Christensen]

Comment on attachment 271067 [details]
Patch

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

r=me

> Source/WebCore/loader/MediaResourceLoader.cpp:68
> +    RefPtr<MediaResource> mediaResource = MediaResource::create(*this, resource);

MediaResource::create returns a Ref.  This should be Ref or auto.

> Source/WebCore/loader/MediaResourceLoader.cpp:74
> +void MediaResourceLoader::removeResource(MediaResource* mediaResource)

I think the parameter should be a MediaResource& to indicate that it cannot be null, even though we would be taking the address of it.

> Source/WebCore/loader/MediaResourceLoader.cpp:89
> +    resource->addClient(this);

ASSERT(resource)

> Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h:47
> +    virtual void redirectReceived(PlatformMediaResource&, ResourceRequest&, const ResourceResponse&) { }
> +    virtual void dataSent(PlatformMediaResource&, unsigned long long, unsigned long long) { }

These are added and unused.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:129
>      RefPtr<PlatformMediaResourceLoader> loader;
> +    RefPtr<PlatformMediaResource> resource;

This doesn't change existing behavior in the GStreamer code, but you might want to add a FIXME comment saying the media element should own the loader, which would make loading more efficient.

[Jer Noble]

Comment on attachment 271067 [details]
Patch

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

>> Source/WebCore/loader/MediaResourceLoader.cpp:68
>> +    RefPtr<MediaResource> mediaResource = MediaResource::create(*this, resource);
> 
> MediaResource::create returns a Ref.  This should be Ref or auto.

Ok.

>> Source/WebCore/loader/MediaResourceLoader.cpp:74
>> +void MediaResourceLoader::removeResource(MediaResource* mediaResource)
> 
> I think the parameter should be a MediaResource& to indicate that it cannot be null, even though we would be taking the address of it.

Ok.

>> Source/WebCore/loader/MediaResourceLoader.cpp:89
>> +    resource->addClient(this);
> 
> ASSERT(resource)

Ok.

>> Source/WebCore/platform/graphics/PlatformMediaResourceLoader.h:47
>> +    virtual void dataSent(PlatformMediaResource&, unsigned long long, unsigned long long) { }
> 
> These are added and unused.

I'll add implementations to MediaResource.

[Jer Noble]

Created attachment 271076 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 271076 [details]
Patch for landing

Clearing flags on attachment: 271076

Committed r196442: <http://trac.webkit.org/changeset/196442>