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

Tue Aug 30 22:10:51 PDT 2011

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

--- Comment #10 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2011-08-30 22:10:51 PST ---
> 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...?

Hmm, I don't have that problem with the term "download".  We use the term "upload" (e.g., XMLHttpRequest can report upload progress events), and "download" just seems like the companion term that refers quite nicely to receiving data over the network.

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

If it is just about the name, then maybe we can come up with something you will like better.  To me, I think this is a great choice for a name.  It tells you more precisely what is going on.  In the Chromium port, we literally have a mode for ResourceHandle that allows it to produce a temporary file that holds the response body of the requested resource.

This API is controlled from the ResourceRequest::downloadToFile attribute, and from ResourceResponse you can get the downloadFilePath.  When you set downloadToFile to true, it means that the ResourceHandleClient will not have its didReceiveData method called because in this mode the data is not passed directly to the ResourceHandleClient.  In this mode, the client only cares to receive the downloaded file.

Again, the purpose of this design is to enable optimizations such as the one I described before where the downloaded file may just be a pointer to a file in the HTTP disk cache.  This seems like a legitimate feature for a network stack.  It may not be a feature that all network stacks possess, but it is surely one that could be emulated if need be.

I feel that this is a good foundation for implementing NPAPI Stream-As-File, and it is also no surprise a good foundation for implementing Pepper's Stream-To-File mode.  The only thing missing is a way to propagate download progress events.  That's where this bug comes in.

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

I understand why this seems bad.  Hopefully, my explanation of how the clients opt-in to this mode helps?  It seems to me that since the client is asking to load the resource in "download mode" (they set downloadToFile to true), that it is natural for them to receive events named didDownloadData(int).  It is also natural that they would not receive didReceiveData calls since in this download mode, there is no desire to receive the actual data directly.

> How do you decide which one you should listen to based on the names alone?

I think the names are helpful.  Again, "download" usually makes people think about files.  That's precisely what we are after in this case.

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

I know that kind of problem.  I'm not seeing it in this case because of how strongly "download" makes you think about files.

> 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 feel like this could create error prone code.  For example, someone might write code that simply logs the received data.  If they didn't realize that the data pointer could sometimes be null, then problems could occur.  Overloading didRecieveData for an uncommon use case seems like it will create bugs as people will not remember about this special case.  However, with didDownloadData, most people will be able to happily ignore it.  Only consumers that care about having the network stack produce a file will care about didDownloadData.

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

I now think that this would be the best solution (until other ports implement similar functionality in their network stacks).  It is only the PLATFORM(CHROMIUM) version of ResourceRequest that has the downloadToFile attribute, so it can only be Chromium specific code that would care about didDownloadData.  I hate #ifdefs, but in this case it probably is warranted.  That way someone reading DocumentThreadableLoader and SubresourceLoader, who doesn't work on Chromium, won't need to be concerned about the distinction between didRecieveData and didDownloadData.

Finally, another solution we could use is to expose another Client interface for the Chromium port's ResourceHandle.  It could be some kind of auxilary Client interface that is used to pass this didDownloadData event upwards.  We could expose a ResourceHandle getter on DocumentThreadableLoader that would then allow our AssociatedURLLoader to install this Client interface.  To be clear, I find this solution quite unsavory.  It is bypassing several layers to access the ResourceHandle directly.  It seems like it could therefore be a fragile solution.

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