Initial implementation of the ByteStream refactor.

Initial implementation of the ByteStream refactor.

This puts an abstraction barrier between DownloadResourceHandler
and DownloadFile; instead of targeting each other directly, they interact
through a ByteStream class that transfers (zero-copy) bytes, potentially
between threads.  This refactor enables substitution of different download
sources and download destinations, and potentially adding in filters 
between source and destination as well.  

BUG=123192

[Randy Smith (Not in Mondays), 2012-04-12 18:29:46]

Andy: This is a first pass at the ByteStream refactor.  I'm interested in your
thoughts as to whether I goofed anything up in the code I transferred from
download_file_manager.cc to download_file_impl.cc, since I think you know that
code pretty well.  One thing that I want to do in that space that I know I
haven't yet is get rid of DownloadFile::AppendDataToFile()--that's no longer
used from release code.  I know that I need to add unit tests for the new
functionality I put into DownloadFile.

Anyone else: This is a work in progress; I've got several changes still to make
(specifically adding unit tests for ByteStream).  I'll be adding reviewers
(almost certainly several) and probably splitting this into a couple of CLs (no
reason ByteStream can't go in on it's own).  So don't panic.  Yet :-}.

[ahendrickson, 2012-04-16 15:14:26]

This basically looks fine to me, with some nits.

Per the Chrome style guide:
"Omit {} for conditional or loop bodies where both the body and the header are
just one physical line each.  Otherwise, use {}."

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
File content/browser/download/byte_stream.cc (right):

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:26: if (data_size_ == 0 && byte_count >
0 && sink_task_runner_.get() != NULL)
Nit: Add braces for multi-line 'if' bodies.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:27: // Nothing actually touches the data
on this object can be
*Nothing that

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:46: if (contents_.empty() &&
sink_task_runner_.get() != NULL)
Nit:  Add braces.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:79: source_task_runner_.get() != NULL)
Nit:  Add braces.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:80: // Nothing actually touches the data
on this object can be
*Nothing that

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
File content/browser/download/byte_stream.h (right):

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:58: typedef enum { STREAM_EMPTY,
STREAM_NON_EMPTY, STREAM_COMPLETE } StreamState;
STREAM_NON_EMPTY -> STREAM_HAS_DATA ?

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:81: // and provide a status.
Indicate what status should be provided in the normal completion case?

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:95: void RegisterSourceCallback(
I think this needs a better name.  RegisterSourceCanAddCallback?

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:109: // Only valid to call if GetData()
has returned STREAM_COMPLETE.
What will this return if called too early?

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:119: void
RegisterSinkCallback(scoped_refptr<base::TaskRunner> sink_task_runner,
RegisterDataAvailableCallback?

http://codereview.chromium.org/10074001/diff/1/content/browser/download/downl...
File content/browser/download/download_file_impl.h (right):

http://codereview.chromium.org/10074001/diff/1/content/browser/download/downl...
content/browser/download/download_file_impl.h:55: void PipeActive();
I was not clear as to what this function does.  Needs a descriptive comment, and
perhaps a better name?

http://codereview.chromium.org/10074001/diff/1/content/browser/download/downl...
File content/browser/download/download_resource_handler.cc (left):

http://codereview.chromium.org/10074001/diff/1/content/browser/download/downl...
content/browser/download/download_resource_handler.cc:173: // We can't start
saving the data before we create the file on disk and have a
Nice!

http://codereview.chromium.org/10074001/diff/1/content/test/net/url_request_s...
File content/test/net/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/10074001/diff/1/content/test/net/url_request_s...
content/test/net/url_request_slow_download_job.cc:188: // Release reference
Is this related to this CL?

[Randy Smith (Not in Mondays), 2012-04-18 19:10:38]

Wow, this was a much more detailed review than I expected.  Thanks!

I didn't get a lot of comments in the area where I thought you have the most
expertise (which was the movement and transformation of code from
DownloadFileManager to DownloadFile) so I wanted to confirm that you thought
that was ok.  I think I found a bug scanning over it just now (I don't think I'm
finalizing the hash before final shipment, but I want to write a test to confirm
that that's broken and *then* fix it) but I could be wrong, and that's the area
where I think you're mostly likely to find big things.  Does that look ok to
you?

I'd welcome further comments from you if you want, but I'm going to assume you
won't be doing further review.  But if you any thoughts on the above question
(or anything else you'd like to comment on) please let me know.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
File content/browser/download/byte_stream.cc (right):

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:26: if (data_size_ == 0 && byte_count >
0 && sink_task_runner_.get() != NULL)
On 2012/04/16 15:14:27, ahendrickson wrote:
> Nit: Add braces for multi-line 'if' bodies.

Ooops.  But already done in a later PS.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:27: // Nothing actually touches the data
on this object can be
On 2012/04/16 15:14:27, ahendrickson wrote:
> *Nothing that

Done.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:46: if (contents_.empty() &&
sink_task_runner_.get() != NULL)
On 2012/04/16 15:14:27, ahendrickson wrote:
> Nit:  Add braces.

Done.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:79: source_task_runner_.get() != NULL)
On 2012/04/16 15:14:27, ahendrickson wrote:
> Nit:  Add braces.

Done.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:80: // Nothing actually touches the data
on this object can be
On 2012/04/16 15:14:27, ahendrickson wrote:
> *Nothing that

Done.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
File content/browser/download/byte_stream.h (right):

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:58: typedef enum { STREAM_EMPTY,
STREAM_NON_EMPTY, STREAM_COMPLETE } StreamState;
On 2012/04/16 15:14:27, ahendrickson wrote:
> STREAM_NON_EMPTY -> STREAM_HAS_DATA ?

Done.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:81: // and provide a status.
On 2012/04/16 15:14:27, ahendrickson wrote:
> Indicate what status should be provided in the normal completion case?

Done.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:95: void RegisterSourceCallback(
On 2012/04/16 15:14:27, ahendrickson wrote:
> I think this needs a better name.  RegisterSourceCanAddCallback?

I'd prefer not, mostly because there isn't really anything else that the source
can be notified about and it's a longer name.  How strongly do you feel?

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:109: // Only valid to call if GetData()
has returned STREAM_COMPLETE.
On 2012/04/16 15:14:27, ahendrickson wrote:
> What will this return if called too early?

Undefined.  I don't want to promise to assert, but assertion is what I generally
think of as happening when you violate an interface contract.  

(I've added a DCHECK in the code.)

http://codereview.chromium.org/10074001/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:119: void
RegisterSinkCallback(scoped_refptr<base::TaskRunner> sink_task_runner,
On 2012/04/16 15:14:27, ahendrickson wrote:
> RegisterDataAvailableCallback?

But this is also called when there's just a result code.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/downl...
File content/browser/download/download_file_impl.h (right):

http://codereview.chromium.org/10074001/diff/1/content/browser/download/downl...
content/browser/download/download_file_impl.h:55: void PipeActive();
On 2012/04/16 15:14:27, ahendrickson wrote:
> I was not clear as to what this function does.  Needs a descriptive comment,
and
> perhaps a better name?

Added comment; let me know if you still think I need to change the name.  It's
not part of the interface, so I'm not too too worried about it :-}.

http://codereview.chromium.org/10074001/diff/1/content/browser/download/downl...
File content/browser/download/download_resource_handler.cc (left):

http://codereview.chromium.org/10074001/diff/1/content/browser/download/downl...
content/browser/download/download_resource_handler.cc:173: // We can't start
saving the data before we create the file on disk and have a
On 2012/04/16 15:14:27, ahendrickson wrote:
> Nice!

:-).

http://codereview.chromium.org/10074001/diff/1/content/test/net/url_request_s...
File content/test/net/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/10074001/diff/1/content/test/net/url_request_s...
content/test/net/url_request_slow_download_job.cc:188: // Release reference
On 2012/04/16 15:14:27, ahendrickson wrote:
> Is this related to this CL?

Sorta-not-really.  It's an already existing problem I found while running tests
for the CL that I fixed in another CL.  So this should disappear in a future
merge.