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

[bugzilla-daemon at webkit.org]

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





--- Comment #26 from Brady Eidson <beidson at apple.com>  2013-07-18 10:47:26 PST ---
(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > (In reply to comment #21)
> > > 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.

No.  It's actually a generic feature of any CachedResource.

What this patch crammed into multiple CachedResource* classes was a something that doesn't belong in the memory cache at all.

And - if it did - it wouldn't be implemented with raw pointers like this.

> 
> > 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?

Because other ports want the right design:
- Loader code (the interface between networking and the notion of per-page subresources) does not belong in the memory cache.
- We try to avoid raw pointers for buffers whenever humanly possible.  That this method is a raw pointer + buffer seems indicative of it's SOUP-specificness.

> > 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.

And we're not going to untangle that mess by moving platform specific bits out of the platform directory and in to loader code

> > > 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.

Some of what you mention is there.  Sorry for missing that

I think a lot of people tend to look at bug titles before anything else.  Maybe the title should be less cryptic and more descriptive.

> > 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.

Let's add Jer also.

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