[Webkit-unassigned] [Bug 73743] [GStreamer] webkitwebsrc: use SubResourceLoader

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73743


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |eric.carlson at apple.com




--- Comment #24 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-07-18 10:25:55 PST ---
(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (In reply to comment #20)
> > > > The platform specific code is indeed part of the ResourceHandle, the soup implementation allows the client to provide its own read buffer, which is used by the gstreamer backend to avoid buffer copies while loading media, but it's not a gstreamer feature. 
> > > 
> > > But you just said that it's a gstreamer feature.
> > 
> > It's a ResourceHandleSoup feature used by the streamer media backend.
> 
> Right.  Again, not something that adds to the argument that this belongs at a higher level than that platform specific networking layer.
> 
> > > > Each media backend tends to do things radically differently from each other, but we've successfully kept the implementation details of ours out of the general purpose, cross-platform loader code.
> > > 
> > > > The same way the mac port has platform specific support for loading the data in a CFArrayRef. 
> > > 
> > > Which is an unfortunate case and one that we hope to rectify going forward.
> > > 
> > > But also, note that this callback is *only* in SubresourceLoader, and it was not necessary to pipe it through to the entire CachedResource/CachedRawResource/CachedRawResourceClient stack of classes.
> > > 
> > > They are absolutely *not* a place for platform loading details.  It's wrong wrong wrong.
> > 
> > That was just an example, there's also a mac platform ifdef in CachedResource, tryReplaceEncodedData, that is only used by the network process. 
> 
> Actually, there is literally nothing platform dependent about that, and we have it working in generic WebKit1 implementations experimentally.  That will be upstreamed sometime soon.  The only reason it was #if PLATFORM(MAC)'ed was to keep from having to give empty implementations to other platforms.

Ok, so it's the same example as getOrCreateReadBuffer, there's nothing specific to soup, I simply added the ifdefs to avoid empty implementations in other ports.

> This new thing that was added throughout the entire SubresourceLoader and memory cache machinery is not something that is generically usable, nor does it belong in CachedResource* code at all.

Why can't it be used by other ports? Because the network platform handles the read buffer and doesn't allow API users to provide their own?

> > But anyway, as I said, I agree with you that ideally there shouldn't be platform ifdefs there. And that's why I also proposed to simply remove the #ifdefs, and leave it unimplemented in platforms that don't need it, or even add a feature flag. I think letting the client provide the read buffer is useful feature in any case.
> 
> Perhaps implementing a feature where WebCore provides a buffer to the networking layer is useful.  This is the wrong form to do it in.  We would not implement it using raw pointers and it would not be implemented in CachedResource* code.

Fair enough.

> > > > Except for the headers, all other platform specific code is in platform-specific files. I don't see how the gstreamer loader can provide the read buffer without using the CachedRawResource client. I agree that ideally, there shouldn't be platform ifdefs in those files. Another possibility is adding getOrCreateReadBuffer unconditionally, but unimplemented in other platforms, since the method itself doesn't have any platform specific types.
> > > 
> > > Let me ask this - What advantage is gained by this bug?  The bug is titled "use SubresourceLoader", but why is that important?  Didn't things work just fine when this code was in ResourceHandle?  Why is this move important to even be contemplating?
> > 
> > Phil can probably answer it better than me. I started to work on this to reduce the amount of layering violations in gstreamer media player. Using CachedResourceLoader is still a layering violation, but also used by mac port, so it would be a common point to look for a solution valid for both ports. 
> 
> Can you tell me how using CachedResourceLoader for the gstreamer media backend is a layering violation for the Mac port?  (I know that's not what you meant, but I actually have no idea what you mean here, so I'd like clarification)

Because the media player is in platform and loader code is not. See bugs #21354 and #21562.

> > It gives us more control over mixed content and security related stuff, so It should allow us to unskip some layout tests (which I forgot to do it).
> > It also allows to track the progress of the media loaded in the inspector like any other resource. 
> > And also allows us to easily support other non HTTP sources like data or blob urls, see bug #102586.
> 
> Thanks.  Those sound like good reasons to do this.  I wouldn't have had to ask if they were spelled out in the bug.

See bug description and comment #5.

> But good reasons to do this are still not a reason for doing it incorrectly.

I agree. I'll think of better alternatives, one that come to my mind in this moment would be to add a special loader for media, similar to the plugins stream loader used by PluginStream. I'm adding Eric to the CC since maybe he had other ideas.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.