[Webkit-unassigned] [Bug 67229] ThreadableLoaderClient should support 'didDownloadData' updates

Tue Aug 30 17:55:00 PDT 2011

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

--- Comment #9 from Brady Eidson <beidson at apple.com>  2011-08-30 17:55:00 PST ---
I've reordered things a bit to group my replies:

(In reply to comment #8)
> (In reply to comment #5)
> > WebCore doesn't download files to disk.  All platforms that implement a concept of downloading a file to disk do so in their API layers.
> 
> Yes, but at some cost.  In Chromium, the network stack lives in a central process.  The child processes, where WebKit runs, do not have access to the disk.  Therefore, the central process needs to write data to disk.  Having to make the data flow through the child process, only to have high level layers in WebKit redirect that data back up to the central process where it can be written to disk would be very costly.

I don't think anyone suggested this.  What you're saying (That the network process is the only one that should be writing to disk) and what I'm saying (that WebCore::ResourceHandle shouldn't have this didDownloadData concept added to it) are not incompatible.

> > If the Chromium loader process is doing the loading of data, and that data is for a file download, why does WebCore need to have this platform specific concept introduced to it instead of the Chromium loader process notifying the Chromium UI process directly?
> 
> We would like the signalling for downloading a resource to flow through WebCore.  Our AssociatedURLLoader is a concept much like NetscapePlugInStreamLoader in that it is stopped when window.stop() is called.  To support stream-to-file, and progress notifications associated with that, we need to pass this notification through WebCore.

...and...

> > > We need this change to support the Chromium WebKit API (WebURLLoader returned via WebFrame::createAssociatedURLLoader).  We need to plumb this notification through the DocumentThreadableLoader.
> > 
> > My understanding of Chromium architecture is not sophisticated, but isn't it actually an opportunity in the case of a file download for the loader process to bypass the WebCore process and notify the UI process directly?
> 
> This is not about the system we use to download files that have an unknown mime type.  This is about plugin stream-to-file.  In the case of plugins, requests are associated with the document just like other subresources.  Again, see NetscapePlugInStreamLoader.  We do not use NetscapePlugInStreamLoader because our plugin implementation is external to WebKit.

I think that canonically "download" means for a user to "download a file" to disk.  If this is about plugin stream loader, then the name download doesn't quite seem to fit...?

One of my main objections here (and I think one of Alexey's too) was that downloading - tradition downloading files to disk for the user to look at later - is not a concept that belongs in WebCore.  So maybe that's just a matter of clearing the name up.

However, that is definitely not the only objection...

> > (In reply to comment #4)
> > > The Chromium ResourceHandle knows how to download to disk (to a temporary file).  I believe that other ports do not need this functionality.  However, it could be generally useful.  It could help us implement XMLHttpRequest.responseType("blob").  
> > 
> > I think you're saying that this could help responseType("blob") on Chromium because Chromium's ResourceHandle already saves to temporary files.  This is not true of WebCore in general.
> 
> It could be.  It is not an unreasonable constraint.  The idea of having the network stack directly place data on disk for consumption by the renderer when it needs the data as a file is not foreign.  Mozilla's network stack does it (again for NPAPI stream-to-file).
> 
> 
> > >It could help us implement NPAPI StreamToFile better (without requiring WebCore to produce a temporary file).  (This is something that Chromium already does for NPAPI.)
> > 
> > I don't understand how this can help any non-Chromium port support this since non-Chromium ports don't have their ResourceHandles stream to files.
> 
> They could!

If they actually planned to, I would agree with this forward thinking.  But AFAIK, none of them plan to, so putting this in is of dubious benefit.

Now a few replies that contain suggestions for going forward:

> > > You can think of didDownloadData as the response equivalent of didSendData, which all ports generate to produce upload notifications. 
> > 
> > Except we already have symmetrical callbacks internal to WebCore - didSendData and didReceiveData.  
> 
> Yes, but didReceiveData passes the data along.  We could fake this in Chromium by using didReceiveData to pass an empty string if that would be helpful, but it seemed like a hack to me.

I agree that the current form of didReceiveData passes the data a long, and in this one use case the data itself is not needed.

I hate hate HATE the idea of having both a didReceiveData callback *and* a didDownloadData callback.

How does a client know when one of them is expected and not the other?  How do you decide which one you should listen to based on the names alone?  Both internal to WebCore and at WebKit API boundaries, we've had problems like this already where two callbacks seem so similar it's impossible to understand the distinctions by looking at the boundary without an understanding of the history.

Should both be called when a chunk of data is delivered?  That'd be silly.  But I suspect that if this lands, within a year, someone will assume that both should be called for the same chunk of data and we'll start down that slippery road.

One possible solution is to change didReceiveData - throughout the entire stack - to account for a mode where it is simply a notification of a number of bytes and doesn't actually include the buffer.

I don't know how I feel about this yet but it seems like a viable suggestion.

> > This patch tries to add a second form of didReceiveData which will just cause maintenance and code-comprehension headaches downstream.
> 
> I don't think about it that way.  ResourceHandle already has other notifications that are Safari specific, yet those are okay?  

It's unfortunate that these exist, but...

> Perhaps we should put didDownloadData in a #if PLATFORM(CHROMIUM) block just as you have put willCacheResponse and shouldCacheResponse in platform specific #ifdefs?

... the Safari-specific stuff is behind platform #ifdefs.  Which - at the very least - makes it clear to all other platform coders that they probably don't need to worry about them.

I agree that cruft has already been added here that isn't pleasing.

The fact of the matter is that while many concepts of "the low level network layer" are common among all the platforms, there's bound to be platform specific additions.  At least when they're guarded by #ifdefs in headers, it's clear that the other platforms don't need to worry about them.  Also they're implementations tend to be in the platform specific files; from ResourceHandle all the way through ResourceLoader and SubresourceLoader and their clients.  We've *mostly* done this with the will/shouldCacheResponse family but I see we didn't completely.  That was an oversight.

Perhaps another possible way to resolve this - as you suggested - would be to #ifdef it in the header and move the implementations to Chromium specific files.

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