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

[bugzilla-daemon at webkit.org]

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





--- Comment #8 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-08-30 16:32:50 PST ---
(In reply to comment #5)
> This doesn't really make sense.

I will try to help.  Thank you for taking the time to comment on this review.


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


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


> (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!


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


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


> 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?  Perhaps we should put didDownloadData in a #if PLATFORM(CHROMIUM) block just as you have put willCacheResponse and shouldCacheResponse in platform specific #ifdefs?

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