Schetch changes required to support URLFetcher saving response to a file

Brett,
    About two weeks ago, Erik Kay talked to you and Darin about creating something like URLFetcher that saves the URL body to a file.  Before I go too far into implementing this, I want to make sure I am going down a reasonable path.  This code review sketches what I intend to do.  Please take a look and let me know if it is reasonable, or if there is a better way.

   I would like to re-use as much of URLFetcher as possible.  I replaced the string that holds the response with an abstract class that receives bytes.  Each URLFetcher::Delegate can provide an implementation that receives the bytes.  By default, a class that puts the bytes into a string is used.

   To make the diff clear, I avoided changing the name of any existing method.  To finish this work, I plan to rename URLFetcher to URLFetcherImpl and create a new class URLFetcher class that uses URLFetcherImpl to build a string, and return it as the current class does.  A new class URLFileFetcher would use URLFetcherImpl to fetch bytes, and redirect them to a file.

Sam

[Sam Kerner (Chrome), 2011-04-27 21:32:34]

This code review is being used to show an idea, and will not be committed.  See
description.

[darin (slow to review), 2011-04-28 03:25:17]

http://codereview.chromium.org/6904057/diff/1/chrome/common/net/url_fetcher.cc
File chrome/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/6904057/diff/1/chrome/common/net/url_fetcher.c...
chrome/common/net/url_fetcher.cc:35: virtual void Append(char* data, int
data_length) {
if the goal is to have an alternative implementation of this that writes to a
file, then this API is not good.  it implies that files writes will happen
synchronously on this thread.  instead, because Append is called on the IO
thread, you need to use a FileStream object to asynchronously write to the file
so that you do not block the IO thread.

however, given this interface, you may end up generating "writes" faster than
you can process them, and a large queue of pending writes might build up, which
could cause serious problems.

this API would need to have a way to signal to the caller to back off... and
then another way to signal to the caller when it is time to resume.  that sounds
complicated.

for the above reason, and because i don't think we will need any other
implementations of ResponseBodyContainerInterface, I think it would be better to
just teach URLFetcher how to optionally save data to a file.

I recommend moving the 'string' based result to a getter function on URLFetcher,
and I would add a similar getter for accessing the temp file produced when
saving to a file.  There would be a configuration option to select between these
two saving modes.  I'd also spec that the temp file gets deleted when the
URLFetcher is destroyed.  (Be sure to delete the tep file on the FILE thread
using FileUtilProxy.)

Note: URLFetcher is responsible for calling URLRequest::Read, so it can easily
implement the back off and resume logic I was talking about.

[Sam Kerner (Chrome), 2011-04-28 04:24:58]

http://codereview.chromium.org/6904057/diff/1/chrome/common/net/url_fetcher.cc
File chrome/common/net/url_fetcher.cc (right):

http://codereview.chromium.org/6904057/diff/1/chrome/common/net/url_fetcher.c...
chrome/common/net/url_fetcher.cc:35: virtual void Append(char* data, int
data_length) {
On 2011/04/28 03:25:17, darin wrote:
> if the goal is to have an alternative implementation of this that writes to a
> file, then this API is not good.  it implies that files writes will happen
> synchronously on this thread.  instead, because Append is called on the IO
> thread, you need to use a FileStream object to asynchronously write to the
file
> so that you do not block the IO thread.

My plan was to have each call to Append() send a message to the file thread to
write the data.  But you anticipated the problem I hit: 

> however, given this interface, you may end up generating "writes" faster than
> you can process them, and a large queue of pending writes might build up,
which
> could cause serious problems.
> 
> this API would need to have a way to signal to the caller to back off... and
> then another way to signal to the caller when it is time to resume.  that
sounds
> complicated.
> 
> for the above reason, and because i don't think we will need any other
> implementations of ResponseBodyContainerInterface, I think it would be better
to
> just teach URLFetcher how to optionally save data to a file.
> 
> I recommend moving the 'string' based result to a getter function on
URLFetcher,
> and I would add a similar getter for accessing the temp file produced when
> saving to a file.  There would be a configuration option to select between
these
> two saving modes.  I'd also spec that the temp file gets deleted when the
> URLFetcher is destroyed.  (Be sure to delete the tep file on the FILE thread
> using FileUtilProxy.)
> 
> Note: URLFetcher is responsible for calling URLRequest::Read, so it can easily
> implement the back off and resume logic I was talking about.

That makes sense.  Thanks for reviewing this.