Bug 67229 – ThreadableLoaderClient should support 'didDownloadData' updates

• Component: Platform
• Status: RESOLVED FIXED

[Bill Budge]

ThreadableLoaderClient should have a 'didDownloadData' method, to allow us to pass progress updates from the underlying URL loader up to AssociatedURLLoader. Modify ResourceHandleInternal, ResourceLoader, SubresourceLoader, DocumentThreadableLoader, and finally AssociatedURLLoader to pass the updates to the WebURLLoaderClient.

[Bill Budge]

Created attachment 105697 [details]
Proposed Patch

[Alexey Proskuryakov]

Does this actually have something to do with downloading? This is a whole lot of completely mysterious code being added to core classes.

+    if (m_state != ConnectionStateReceivedResponse && m_state != ConnectionStateReceivingData)
+        CRASH();

Is this intentional?

[Alexey Proskuryakov]

Comment on attachment 105697 [details]
Proposed Patch

I don't think that ResourceHandle ever downloads anything, so adding code to it that talks about downloading doesn't seem to make any sense.

[Darin Fisher (:fishd, Google)]

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

We need this change to support the Chromium WebKit API (WebURLLoader returned via WebFrame::createAssociatedURLLoader).  We need to plumb this notification through the DocumentThreadableLoader.

You can think of didDownloadData as the response equivalent of didSendData, which all ports generate to produce upload notifications.  XMLHttpRequest.responseType("blob") will need download progress events.  So, this patch seems like a good step forward.

[Brady Eidson]

This doesn't really make sense.

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.

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?

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

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

This patch tries to add a second form of didReceiveData which will just cause maintenance and code-comprehension headaches downstream.

> XMLHttpRequest.responseType("blob") will need download progress events.  So, this patch seems like a good step forward.

Except for all platforms besides Chromium, those "download progress events" already exist in the form of didReceiveData.

[Alexey Proskuryakov]

A major difference between didDownloadData and didSendData is that downloading is not related to anything that happens inside WebCore. Yet this patch introduces knowledge of unrelated functionality into ResourceLoader, SubresourceLoader and other core classes.

[Brady Eidson]

ResourceHandle is a super low-level class that arguably has too many high level semantics in it already.  This seems like a move to add more high level semantics to it.

The same holds true for (Sub)ResourceLoader, which purport to be about "resources" in the HTML/memory cache context.

Could we slow down on this and try to work out a better design before we're locked in to something that all platforms regret later?

[Darin Fisher (:fishd, Google)]

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

[Brady Eidson]

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.

[Darin Fisher (:fishd, Google)]

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

[Bill Budge]

Another solution would be to generalize this solution to make it potentially useful to all ports. What if we instead add a new 'didSendEvent' method:

void didSendEvent(const ResourceEvent&);

where ResourceEvent is like ResourceError, a simple base that can be customized by the ports.

This would let us do what we need, and provide a useful mechanism for other ports to communicate up through the loader code, without having to constantly touch this code.

I would prefer this to creating a bunch of platform-specific code here.

[Alexey Proskuryakov]

CC'ing more people who may have input.

We need a mechanism that wouldn't make clients guess what kind of callbacks their ResourceHandle is going to send, and what kind is not going to be sent. It's quite indirect for a client that wants a blob to know that it should listen for didDownloadData instead of didReceiveData.

Also, this needs to be done in a way that doesn't expose unwanted concepts to subresource loaders.

Finally, I'm still don't think that ResourceHandle is the right class to know about files. Its purpose is to abstract out platform network loading API, and network loading APIs on major platforms don't download to files bypassing memory. A WebCore loader abstraction can certainly do that (in particular, when network loading happens in another process), but it's not the job of an API wrapper.

[Bill Budge]

To clarify, in my last post I was proposing adding a new interface method to pass custom platform events through webkit. The idea is that these events contain extra information but that it is optional and platform specific, somewhat like ResourceError.

Perhaps the method could be named slightly differently to emphasize that it's optional, and can be ignored, ie. sentMessage, or sentPlatformMessage, instead of didSendEvent. I'm new to Webkit but it seems like this could be generally useful in the future and could be expanded in a way that doesn't affect loader code.

[Darin Fisher (:fishd, Google)]

(In reply to comment #12)
> We need a mechanism that wouldn't make clients guess what kind of callbacks
> their ResourceHandle is going to send, and what kind is not going to be sent. 

As I mention above, there is no guessing happening here.  The mechanism is
quite explicit because the client who cares about didDownloadData would have
set downloadAsFile to true.


> It's quite indirect for a client that wants a blob to know that it should 
> listen for didDownloadData instead of didReceiveData.

Maybe I made a mistake in bringing up XMLHttpRequest.responseType("Blob").
The mechanism at the ResourceHandle level is not about Blobs.  It is about
producing files.


> Also, this needs to be done in a way that doesn't expose unwanted concepts to 
> subresource loaders.

I don't understand what this means.  It would help if you could be more specific.


> Finally, I'm still don't think that ResourceHandle is the right class to know 
> about files. Its purpose is to abstract out platform network loading API, and 
> network loading APIs on major platforms don't download to files bypassing 
> memory.

How do you define "major platforms"?  Gecko's network stack and Chromium's
network stack each support this concept, and they are part of some very
popular platforms.

[Brady Eidson]

(In reply to comment #14)
> (In reply to comment #12)
> > We need a mechanism that wouldn't make clients guess what kind of callbacks
> > their ResourceHandle is going to send, and what kind is not going to be sent. 
> 
> As I mention above, there is no guessing happening here.  The mechanism is
> quite explicit because the client who cares about didDownloadData would have
> set downloadAsFile to true.

I don't see where this "downloadAsFile" concept is...?

[Darin Fisher (:fishd, Google)]

(In reply to comment #15)
> I don't see where this "downloadAsFile" concept is...?

It exists on ResourceRequest as "downloadToFile".  Sorry for the typo.  Please see platform/network/chromium/ResourceRequest.h.

[Bill Budge]

Created attachment 105938 [details]
Proposed Patch

This patch is the same as the previous one, but uses #ifdef PLATFORM(CHROMIUM) to restrict new 'didDownloadData' methods to Chromium port only.

[Alexey Proskuryakov]

Can contributors working on other ports ignore Chromium breakage when working on network loading now (like it is the case with v8 bindings)?

Otherwise, adding this even under #ifdef adds burden on other ports that I personally consider excessive.

[Darin Fisher (:fishd, Google)]

Comment on attachment 105938 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105938&action=review

> Source/WebCore/loader/ResourceLoader.cpp:577
> +#if PLATFORM(CHROMIUM)

As discussed in person, I think we should move these method implementations to
a separate file named ResourceLoaderChromium.cpp.  It should live in loader/chromium/.
See loader/mac/ResourceLoaderCFNet.cpp for example.

Same goes for the other loader/*.cpp files in this patch.

[Darin Fisher (:fishd, Google)]

(In reply to comment #18)
> Can contributors working on other ports ignore Chromium breakage when working on network loading now (like it is the case with v8 bindings)?
> 
> Otherwise, adding this even under #ifdef adds burden on other ports that I personally consider excessive.

I don't understand why you view this as an *excessive* burden.  It is simply connecting up some notifications that only the Chromium implementation of ResourceHandle will generate.  No other port needs to worry about didDownloadData.

There is already convention for routing port specific notifications through these interfaces.  Under USE(CFNETWORK), ResourceLoader has willCacheResponse() and shouldCacheResponse() methods, and under HAVE(CFNETWORK_DATA_ARRAY_CALLBACK), SubresourceLoader has supportsDataArray() and didReceiveDataArray() methods.

To be clear, I don't like #ifdefs very much, and I would like loader code to be cross-platform.  I definitely would encourage work that moves us in that direction.  However, it seems unfair to require a higher standard of PLATFORM(CHROMIUM) than what exists for USE(CFNETWORK).

I believe that Bill's patch, with the changes I suggested, gets us to a point where PLATFORM(CHROMIUM) is not unlike USE(CFNETWORK) in terms of the ugliness that it imposes on loader/.  Furthermore, I believe we've identified an opportunity with good use cases that might inspire better abstractions for encapsulating port-specific differences.  That sounds like something we can work on.

[Alexey Proskuryakov]

Data arrays are not conceptually different from what every port uses. It's just a different data format for efficiency, but you are still getting response data as normal.

WillCache/ShouldCache are a bit more unusual, but they still don't go beyond what every low level network loading API is concerned with.

I'm not exactly sure what your proposal is. Is it just like Bill's latest patch, but with part of implementation being in .cpp files, I'm not sure if it's much progress over the first version. Anyone refactoring resource loading would still have the alien concept of "downloading" in mind unless they are allowed to ignore it and break Chromium.

[Bill Budge]

Created attachment 106178 [details]
Proposed Patch

Here's the version of the patch with Chromium-specific implementation code segregated in separate files in case anyone wants to look at it.

[Darin Fisher (:fishd, Google)]

Comment on attachment 106178 [details]
Proposed Patch

This change looks like a clean implementation of the proposed solution.  R=me on that basis.  CQ- since there is still debate about the solution.

[Darin Fisher (:fishd, Google)]

(In reply to comment #21)
> Data arrays are not conceptually different from what every port uses. It's just a different data format for efficiency, but you are still getting response data as normal.
> 
> WillCache/ShouldCache are a bit more unusual, but they still don't go beyond what every low level network loading API is concerned with.

Maybe you are biased toward the port you are most familiar with?  I could not tell you what those functions are for unless I spent some time studying the implementation code.  I would not feel comfortable refactoring those without reaching out to someone who is familiar with the CFNETWORK port, and that seems like the metric that matters most here.

> I'm not exactly sure what your proposal is. Is it just like Bill's latest patch, but with part of implementation being in .cpp files, I'm not sure if it's much progress over the first version.

From comment #9, it would appear that Brady is not against this approach.  He at least seems to favor this over the original approach.

We are open to alternative solutions.  Simply asking us not to do this, when we have established that it is a reasonable and valuable thing for a network stack to implement (see comment #10, search for: "the purpose of this design is to enable optimizations"), does not seem constructive to me.  If you do not have a constructive alternative solution, then please at least allow us to proceed with what appears to be a par-for-the-course solution.  Thanks!

[Alexey Proskuryakov]

While downloading is arguably a valuable thing for a network stack to implement, it's not a good thing for a platform network API wrapper to implement. Its goal is nothing more than to abstract away CFNetwork, NSURLConnection, Win32 APIs, etc.

Just like ResourceHandle does not have a method to render an HTTP response to a bitmap, it should not have other higher level engine functionality.

To proceed with this patch, I would really like to have Maciej weigh in. He is on vacation this week.

[Darin Fisher (:fishd, Google)]

(In reply to comment #25)
> While downloading is arguably a valuable thing for a network stack to implement, it's not a good thing for a platform network API wrapper to implement. Its goal is nothing more than to abstract away CFNetwork, NSURLConnection, Win32 APIs, etc.

I don't agree.  As I've been saying, a network stack may provide a way to directly reference a file from the HTTP disk cache.  How can this be done by a higher layer?


> Just like ResourceHandle does not have a method to render an HTTP response to a bitmap, it should not have other higher level engine functionality.

I don't understand the relevance of this example.


> To proceed with this patch, I would really like to have Maciej weigh in. He is on vacation this week.

I am very happy to receive further input.  This change is required to fix a bug that we consider a release blocker.  I would like to therefore commit it and make revisions once we have received further design input.

[Bill Budge]

Created attachment 106219 [details]
Proposed Patch

Fixed incorrect copyright headers in new files.

[Brady Eidson]

(In reply to comment #26)
> (In reply to comment #25)
> > While downloading is arguably a valuable thing for a network stack to implement, it's not a good thing for a platform network API wrapper to implement. Its goal is nothing more than to abstract away CFNetwork, NSURLConnection, Win32 APIs, etc.
> 
> I don't agree.  As I've been saying, a network stack may provide a way to directly reference a file from the HTTP disk cache.  How can this be done by a higher layer?

But it's demonstrable that at least 2 major network stacks don't *have* files in their HTTP disk cache...

[Darin Fisher (:fishd, Google)]

(In reply to comment #28)
> But it's demonstrable that at least 2 major network stacks don't *have* files in their HTTP disk cache...

I never said it was required.  I said it could be a valuable optimization.

(By the way, Mozilla's HTTP cache will only actually create a separate file if necessary: if the file is very large or if the user has requested to access the stream as a file.)

[Darin Fisher (:fishd, Google)]

Comment on attachment 106219 [details]
Proposed Patch

R=me, thanks for fixing the license headers.

[Alexey Proskuryakov]

> I don't agree.  As I've been saying, a network stack may provide a way to directly reference a file from the HTTP disk cache.  How can this be done by a higher layer?

This is quite different from what's being done here. A platform specific method to access a file in cache would certainly belong to ResourceHandle if deemed necessary, but I don't think that it would involve generic downloading progress callbacks.

I think that the patch has improved in that this code is no longer going to confuse people using ResourceHandle, although it is still not very good for people working on it. I would like to see a refactoring non-network parts of ResourceHandle into a separate class, as discussed before, and as we again discussed on IRC now. Given the apparent time constraints, it will have to be done later.

[Alexey Proskuryakov]

> no longer going to confuse people using ResourceHandle

More precisely, using the loader stack.

[Darin Fisher (:fishd, Google)]

(In reply to comment #31)
> > I don't agree.  As I've been saying, a network stack may provide a way to directly reference a file from the HTTP disk cache.  How can this be done by a higher layer?
> 
> This is quite different from what's being done here. A platform specific method to access a file in cache would certainly belong to ResourceHandle if deemed necessary, but I don't think that it would involve generic downloading progress callbacks.

Just for completeness, the idea is to provide an API that either transparently downloads a file or vends a file from the HTTP cache.  In the first case, progress events are quite important.  In the latter case, progress events are not necessary.


> I think that the patch has improved in that this code is no longer going to confuse people using ResourceHandle, although it is still not very good for people working on it. I would like to see a refactoring non-network parts of ResourceHandle into a separate class, as discussed before, and as we again discussed on IRC now. Given the apparent time constraints, it will have to be done later.

Thanks for understanding.  CQ+'ing this patch now.  Very happy to take ongoing feedback for improving this code.

[WebKit Review Bot]

Comment on attachment 106219 [details]
Proposed Patch

Clearing flags on attachment: 106219

Committed r94466: <http://trac.webkit.org/changeset/94466>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.