Bug 102586 – [GTK] media/video-src-blob.html

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

The test is failing since it has been introduced in r134802.
http://trac.webkit.org/changeset/134802

Here's the diff:
--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/media/video-src-blob-expected.txt 
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/media/video-src-blob-actual.txt 
@@ -1,5 +1,5 @@
 This tests the ability of the <video> element to load blob URLs. In the browser, select a video file: 
 EVENT(change)
-EVENT(loadedmetadata)
+EVENT(error) TEST(false) FAIL
 END OF TEST

[Carlos Garcia Campos]

Now that we use the SubresourceLoder to load videos this should be easy to fix.

[Carlos Garcia Campos]

Created attachment 206897 [details]
Patch

[Martin Robinson]

Comment on attachment 206897 [details]
Patch

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

Looks good to me. Great work! Just a couple questions...

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:465
> +        DataBufferingPolicy bufferingPolicy = url.protocolIs("blob") ? BufferData : DoNotBufferData;
> +        CachedResourceRequest cacheRequest(request, ResourceLoaderOptions(SendCallbacks, DoNotSniffContent, bufferingPolicy, DoNotAllowStoredCredentials, DoNotAskClientForCrossOriginCredentials, DoSecurityCheck, UseDefaultOriginRestrictionsForType));
>          priv->resource = loader->requestRawResource(cacheRequest);

Why is it useful to buffer data that is already in memory on the client side?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:605
> +    if (!url.isValid() || (!url.protocolIsInHTTPFamily() && !url.protocolIs("blob"))) {

I think this could be a helper. Something like urlIsValidAndHasSupportedProtocol or just urlHasSupportedProtocol.

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 206897 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206897&action=review
> 
> Looks good to me. Great work! Just a couple questions...

Thanks!

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:465
> > +        DataBufferingPolicy bufferingPolicy = url.protocolIs("blob") ? BufferData : DoNotBufferData;
> > +        CachedResourceRequest cacheRequest(request, ResourceLoaderOptions(SendCallbacks, DoNotSniffContent, bufferingPolicy, DoNotAllowStoredCredentials, DoNotAskClientForCrossOriginCredentials, DoSecurityCheck, UseDefaultOriginRestrictionsForType));
> >          priv->resource = loader->requestRawResource(cacheRequest);
> 
> Why is it useful to buffer data that is already in memory on the client side?

Because when buffered it can be cached, I think. I followed what apple does, TBH.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:605
> > +    if (!url.isValid() || (!url.protocolIsInHTTPFamily() && !url.protocolIs("blob"))) {
> 
> I think this could be a helper. Something like urlIsValidAndHasSupportedProtocol or just urlHasSupportedProtocol.

I did it indeed :-) but in the end it looked too much code for a simple check and moved back to the current patch, I'll add the helper function since it makes the code easier to read.

[Philippe Normand]

LGTM but yeah that helper function would be good to have indeed.

[Martin Robinson]

Comment on attachment 206897 [details]
Patch

Looks good. Consider the helper when landing. :)

[Carlos Garcia Campos]

Committed r157091: <http://trac.webkit.org/changeset/157091>