Bug 154117 - Make MediaResourceLoader behave more like a CachedResourceLoader.
Summary: Make MediaResourceLoader behave more like a CachedResourceLoader.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-11 10:43 PST by Jer Noble
Modified: 2016-02-12 10:13 PST (History)
4 users (show)

See Also:


Attachments
Patch (27.06 KB, patch)
2016-02-11 10:53 PST, Jer Noble
achristensen: review+
Details | Formatted Diff | Diff
Patch for landing (27.99 KB, patch)
2016-02-11 13:05 PST, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>