Bug 154117

Summary:

Make MediaResourceLoader behave more like a CachedResourceLoader.

Product:

WebKit

Reporter:

Jer Noble <jer.noble>

Component:

New Bugs

Assignee:

Jer Noble <jer.noble>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, cgarcia, commit-queue, pnormand

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	achristensen: review+
Patch for landing	none

Description Jer Noble 2016-02-11 10:43:50 PST

Make MediaResourceLoader behave more like a CachedResourceLoader.

Comment 1 Jer Noble 2016-02-11 10:53:22 PST

Created attachment 271067 [details]
Patch

Comment 2 Alex Christensen 2016-02-11 11:44:44 PST

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.

Comment 3 Jer Noble 2016-02-11 12:58:06 PST

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.

Comment 4 Jer Noble 2016-02-11 13:05:29 PST

Created attachment 271076 [details]
Patch for landing

Comment 5 WebKit Commit Bot 2016-02-11 14:34:37 PST

Comment on attachment 271076 [details]
Patch for landing

Clearing flags on attachment: 271076

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