Use ByteStream in downloads system to decouple source and sink.

Use ByteStream in downloads system to decouple source and sink.

BUG=123192
BUG=93006
BUG=111588
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140328

[Randy Smith (Not in Mondays), 2012-05-19 15:29:56]

[CCing many people who I think might be interested in this CL.  ]

Ben: I think this CL is ready for initial review.  The major thing it doesn't
include is histograms; I want to put some thought into exactly what behavior I
want from those.  They'll come in their own PS, to make them easy for review.  I
also want to go over the testing again and possibly tweak things--you're welcome
to delay the test review until I've done that.

I haven't included the switch back away from IOBuffer in this CL, mostly because
it's something that can be pulled into another CL.  If you'd like it here, just
say so and I'll put it in.

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/ba...
File content/browser/download/base_file.cc (left):

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/ba...
content/browser/download/base_file.cc:279: "byte_count", data_len)));
For downloads, this was redundant with logging occurring now in
download_file_impl.cc, and I don't think we've found it useful to date for
SavePackage problems.  It didn't seem worth the extra effort to put in parallel
logging for SavePackage for a bit of logging that was only recently inserted for
downloads.

http://codereview.chromium.org/10392111/diff/4001/content/test/net/url_reques...
File content/test/net/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/10392111/diff/4001/content/test/net/url_reques...
content/test/net/url_request_slow_download_job.cc:189: buffer_ = NULL;
This has already landed, and will disappear from this CL when I next merge.

[Randy Smith (Not in Mondays), 2012-05-19 15:31:16]

Sorry, should have said for people other than Ben: This CL is dependent on
http://codereview.chromium.org/10244001/ (creation of the ByteStream classes).

[Randy Smith (Not in Mondays), 2012-05-21 01:59:50]

Added histograms/statistics in PS4.  PS3 comments should be read for context.

[benjhayden, 2012-05-21 13:44:14]

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/do...
content/browser/download/download_file_impl.cc:69: // Unretained is safe because
the callback is nulled in the
Is there any particular reason not to use a weak pointer?

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/do...
content/browser/download/download_file_impl.cc:170: base::TimeDelta
delta(base::TimeDelta::FromMilliseconds(1000));
kConstant, please.

Also, why 1s?

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/do...
content/browser/download/download_file_impl.cc:174: state =
input_pipe_->Read(&incoming_data, &length);
Would you mind changing |length| to |incoming_data_size| so that it isn't
confused with |total_length| or |num_buffers|?

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/do...
content/browser/download/download_file_impl.cc:202: // If we're stopping to
yield the thread, post a task so we come back.
Why is it necessary to yield the thread?

[benjhayden, 2012-05-21 14:36:44]

http://codereview.chromium.org/10392111/diff/8001/content/browser/download/do...
File content/browser/download/download_file_unittest.cc (right):

http://codereview.chromium.org/10392111/diff/8001/content/browser/download/do...
content/browser/download/download_file_unittest.cc:1: 
Unnecessary blank line

http://codereview.chromium.org/10392111/diff/8001/content/browser/download/do...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10392111/diff/8001/content/browser/download/do...
content/browser/download/download_resource_handler.cc:220: if (!read_buffer_) {
Could Darin review the ResourceHandler-related parts of this cl?

[Randy Smith (Not in Mondays), 2012-05-23 03:33:14]

Darin: Could you review download_resource_handler.* for good ResourceHandler
protocol?  I'm specifically interested in your opinion on use of the new pausing
interface, but I'm also interested in your thoughts on the lack of assumptions
I'm making about what routines can be called when with regard to making sure any
started_cb is actually called.  Specifically, I'm guarding against the class
being destroyed without OnResponseStarted or OnResponseCompleted being called
(which I'm pretty sure can happen) and against OnResponseCompleted being called
without OnResponseStarted being called (which I *think* I've seen happen, but
doesn't make sense to me).

Ben: Despite Darin's review, I'd still like you to review
DownloadResourceHandler, specifically around whether I'm doing anything wrong
with the callback, and around the correct passing of ownership of structures to
the rest of the download system (both DownloadCreateInfo and the pipe).

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/ba...
File content/browser/download/base_file.cc (left):

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/ba...
content/browser/download/base_file.cc:279: "byte_count", data_len)));
On 2012/05/19 15:29:57, rdsmith wrote:
> For downloads, this was redundant with logging occurring now in
> download_file_impl.cc, and I don't think we've found it useful to date for
> SavePackage problems.  It didn't seem worth the extra effort to put in
parallel
> logging for SavePackage for a bit of logging that was only recently inserted
for
> downloads.

Done.

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/do...
content/browser/download/download_file_impl.cc:69: // Unretained is safe because
the callback is nulled in the
On 2012/05/21 13:44:14, benjhayden_chromium wrote:
> Is there any particular reason not to use a weak pointer?

Given that we already have the factory, no.  Done.

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/do...
content/browser/download/download_file_impl.cc:170: base::TimeDelta
delta(base::TimeDelta::FromMilliseconds(1000));
On 2012/05/21 13:44:14, benjhayden_chromium wrote:
> kConstant, please.

Done.

> Also, why 1s?

The flying monkeys delivered it (i.e. it's a completely made up number).  If
you've got preferences, I'm happy to adapt--I just wanted to put a limit on how
long we could block the file thread, having had the bad browser experience of
the file thread being blocked for the entire download.

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/do...
content/browser/download/download_file_impl.cc:174: state =
input_pipe_->Read(&incoming_data, &length);
On 2012/05/21 13:44:14, benjhayden_chromium wrote:
> Would you mind changing |length| to |incoming_data_size| so that it isn't
> confused with |total_length| or |num_buffers|?

Done.

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/do...
content/browser/download/download_file_impl.cc:202: // If we're stopping to
yield the thread, post a task so we come back.
On 2012/05/21 13:44:14, benjhayden_chromium wrote:
> Why is it necessary to yield the thread?

That's a complex question, to which the answer is "It's not, but ...".

If the ByteStream implementation is lock based (as it originally was before Will
talked me out of it), it's possible to lock the file thread for the entire
course of the download when the network bandwidth is >> than the file system
bandwidth (in my case it was a file: download from a local file system to an NFS
mounted file system).  That's not a good user experience.

Of course, the ByteStream implementation is no longer lock based.  But there's
nothing in the interface to guarantee that it's not lock based, and I could
imagine replacing it with a lock based implementation in the future (if it turns
out to be much better performing).  And I dislike having consumers dependent on
features that are not guaranteed in the interface.  So I left the yielding in. 
But I could be talked into taking it out and replacing it with a warning; I
don't like having excess code either.  I'm not certain what the right choice is
here--do you have an opinion?

http://codereview.chromium.org/10392111/diff/4001/content/test/net/url_reques...
File content/test/net/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/10392111/diff/4001/content/test/net/url_reques...
content/test/net/url_request_slow_download_job.cc:189: buffer_ = NULL;
On 2012/05/19 15:29:57, rdsmith wrote:
> This has already landed, and will disappear from this CL when I next merge.
>  

Done.

http://codereview.chromium.org/10392111/diff/8001/content/browser/download/do...
File content/browser/download/download_file_unittest.cc (right):

http://codereview.chromium.org/10392111/diff/8001/content/browser/download/do...
content/browser/download/download_file_unittest.cc:1: 
On 2012/05/21 14:36:45, benjhayden_chromium wrote:
> Unnecessary blank line

Done.

http://codereview.chromium.org/10392111/diff/8001/content/browser/download/do...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10392111/diff/8001/content/browser/download/do...
content/browser/download/download_resource_handler.cc:220: if (!read_buffer_) {
On 2012/05/21 14:36:45, benjhayden_chromium wrote:
> Could Darin review the ResourceHandler-related parts of this cl?

Sure, I'll ask him.  Even if he does, though, I'd like you to make sure I'm not
doing anything stupid with the callback/download id semantics--I'm going to be
tweaking them a little bit.

[benjhayden, 2012-05-24 15:35:56]

Probably last round.

It took me a while to get what you're doing with started_cb, and I'm still not
sure that I totally understand, so I asked for comments and some small code
changes.

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

http://codereview.chromium.org/10392111/diff/4001/content/browser/download/do...
content/browser/download/download_file_impl.cc:202: // If we're stopping to
yield the thread, post a task so we come back.
On 2012/05/23 03:33:15, rdsmith wrote:
> On 2012/05/21 13:44:14, benjhayden_chromium wrote:
> > Why is it necessary to yield the thread?
> 
> That's a complex question, to which the answer is "It's not, but ...".
> 
> If the ByteStream implementation is lock based (as it originally was before
Will
> talked me out of it), it's possible to lock the file thread for the entire
> course of the download when the network bandwidth is >> than the file system
> bandwidth (in my case it was a file: download from a local file system to an
NFS
> mounted file system).  That's not a good user experience.

I don't quite understand why the file thread doesn't lock without the ByteStream
lock.

> 
> Of course, the ByteStream implementation is no longer lock based.  But there's
> nothing in the interface to guarantee that it's not lock based, and I could
> imagine replacing it with a lock based implementation in the future (if it
turns
> out to be much better performing).  And I dislike having consumers dependent
on
> features that are not guaranteed in the interface.  So I left the yielding in.

> But I could be talked into taking it out and replacing it with a warning; I
> don't like having excess code either.  I'm not certain what the right choice
is
> here--do you have an opinion?
> 

My impression of the TaskRunner revolution was that it would allow us to spin up
a whole pool of worker threads to be shared by all of chrome's subsystems.
<slight hyperbole>Every single other long-running process at Google uses
threadpools, which is why I still chuckle and SMH when I think of Chrome's
4-thread architecture.
If TaskRunners ran on a threadpool, then we can run a risk of blocking one or
two worker threads, at least until metrics show a specific significant problem.

NodeJS has asynchronous file access APIs on all platforms. Maybe we could grab
its |fs| module.

For now, though, yielding after 1s sounds good.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:183: started_cb_));
Can you comment why you're passing started_cb_ to StartOnUIThread and nullifying
it here instead of both running it and nullifying it there?
I think I see the race condition that you're fixing, but it still looks strange.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:191:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Can you add this annotation to *all* the methods? It's super helpful.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:347: void
DownloadResourceHandler::StartOnUIThread(
If you want to avoid accessing member variables outside the IO thread, can you
make this method a static non-member function like CallStartedCBOnUIThread()?
And please comment that it's a static non-member function in order to prevent
member access because it runs off the IO thread. And annotate with the
DCHECK(BrowserThread...).

[darin (slow to review), 2012-05-24 22:01:35]

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
File content/browser/download/download_resource_handler.cc (left):

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:398: MaybeResumeRequest();
do we still need this call to MaybeResumeRequest?  lack of a download_id_ seems
to no longer be a reason for pausing the network stream.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:124:
scoped_ptr<content::ByteStreamOutput> pipe_output;
I am very concerned with this use of Input and Output terminology.  You are
using them in a grammatically correct fashion, but it is in conflict with the
way these terms are typically used in the context of streams.  Normally "Input"
is for the side with a "Read" function so that you can input data into a
program, and "Output" is for the side with a "Write" function so that you can
output data from a program.  Here you have inverted the sense, and I suspect
this will lead to confusion.  Why fight convention?

you might also consider using the terms "reader" and "writer", so you'd have
pipe_reader_->Read(...) and pipe_writer_->Write(...).  that would be fairly
conventional too.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:128: kDownloadPipeSize,
&pipe_input_, &pipe_output);
nit: you call the variables pipe_{input,output}, which suggests that the thing
that you are creating is a pipe, but the function is CreateByteStream.  Why did
you choose ByteStream instead of Pipe for the class name?

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:259: pause_count_++;
I'd call PauseRequest() here.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:332: request_ = NULL;
technically, the purpose of OnRequestClosed is to tell you when you must drop
any pointers to the URLRequest.  you can/should move this code to
OnRequestClosed instead.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:450: void
DownloadResourceHandler::MaybeResumeRequest() {
i'd probably just fold this code into ResumeRequest since that should now be the
only caller.

[Randy Smith (Not in Mondays), 2012-05-26 02:36:54]

Ben, Darin: PTAL.

Ricardo: Could you review resource_dispatcher_host_unittest.cc,
url_request_test_job.*, and (while you're there)
x509_user_cert_resource_handler.*?  The x509 stuff is just incorporating code
into the resource handler that used to be in download_buffer; the other stuff is
changes I needed to make to the RDH unit tests to get them to correctly run with
download requests in the wake of the download pause removal (the download
requests were being completed when the tests needed them to stay pending).

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
File content/browser/download/download_resource_handler.cc (left):

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:398: MaybeResumeRequest();
On 2012/05/24 22:01:36, darin wrote:
> do we still need this call to MaybeResumeRequest?  lack of a download_id_
seems
> to no longer be a reason for pausing the network stream.

I believe not.  It was in for a while because we were accessing download_id_ in
OnResponseCompleted(), but we aren't anymore.  Done.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:124:
scoped_ptr<content::ByteStreamOutput> pipe_output;
On 2012/05/24 22:01:36, darin wrote:
> I am very concerned with this use of Input and Output terminology.  You are
> using them in a grammatically correct fashion, but it is in conflict with the
> way these terms are typically used in the context of streams.  Normally
"Input"
> is for the side with a "Read" function so that you can input data into a
> program, and "Output" is for the side with a "Write" function so that you can
> output data from a program.  Here you have inverted the sense, and I suspect
> this will lead to confusion.  Why fight convention?
> 
> you might also consider using the terms "reader" and "writer", so you'd have
> pipe_reader_->Read(...) and pipe_writer_->Write(...).  that would be fairly
> conventional too.

Done.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:128: kDownloadPipeSize,
&pipe_input_, &pipe_output);
On 2012/05/24 22:01:36, darin wrote:
> nit: you call the variables pipe_{input,output}, which suggests that the thing
> that you are creating is a pipe, but the function is CreateByteStream.  Why
did
> you choose ByteStream instead of Pipe for the class name?

I didn't put a lot of thought into the naming, actually.  I think I'd rather
bring Mohammed to the Mountain, so I'll change the variables I've named based
off of "pipe" to be named based off of "stream".  Let me know what you think.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:183: started_cb_));
On 2012/05/24 15:35:57, benjhayden_chromium wrote:
> Can you comment why you're passing started_cb_ to StartOnUIThread and
nullifying
> it here instead of both running it and nullifying it there?
> I think I see the race condition that you're fixing, but it still looks
strange.

Done.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:191:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2012/05/24 15:35:57, benjhayden_chromium wrote:
> Can you add this annotation to *all* the methods? It's super helpful.

I did almost all the methods; I skipped the ones where I thought flexibility
might be useful (construction, though probably not necessary), and where the
routine didn't do anything but was just there for override purposes.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:259: pause_count_++;
On 2012/05/24 22:01:36, darin wrote:
> I'd call PauseRequest() here.

Feels a little weird, since the *defer = true implementation is outside the
call, but it does collapse common code.  Done.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:332: request_ = NULL;
On 2012/05/24 22:01:36, darin wrote:
> technically, the purpose of OnRequestClosed is to tell you when you must drop
> any pointers to the URLRequest.  you can/should move this code to
> OnRequestClosed instead.

Done.  The code's a bit stale, anyway, as we no longer have any activity going
on after OnResponseCompleted() (because we no longer delay the implementation of
that call waiting for the download id).

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:347: void
DownloadResourceHandler::StartOnUIThread(
On 2012/05/24 15:35:57, benjhayden_chromium wrote:
> If you want to avoid accessing member variables outside the IO thread, can you
> make this method a static non-member function like CallStartedCBOnUIThread()?
> And please comment that it's a static non-member function in order to prevent
> member access because it runs off the IO thread. And annotate with the
> DCHECK(BrowserThread...).

Done.

http://codereview.chromium.org/10392111/diff/21001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:450: void
DownloadResourceHandler::MaybeResumeRequest() {
On 2012/05/24 22:01:36, darin wrote:
> i'd probably just fold this code into ResumeRequest since that should now be
the
> only caller.

Done.

[darin (slow to review), 2012-05-26 03:59:19]

LGTM for DRH (just nits)

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:43: static const int
kDownloadPipeSize = 100 * 1024;
nit: "pipe" -> "stream" / "byte stream"?

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:156:
scoped_ptr<content::ByteStreamOutput> stream_reader;
I know I'm being pedantic, but if stream_reader is the right name for the
variable, then {Prefix}StreamReader is probably the right name for the class. 
Just food for thought.  I know a class name is good when I like using the name
for variables of that type too ;-)

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:214:
scoped_refptr<DownloadFileManager>(download_file_manager_),
nit: I think it is OK to just change download_file_manager_ to be of type
scoped_refptr<DownloadFileManager>.

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:296: PauseRequest();
I know PauseRequest() looks odd because we are so used to
ResourceDispatcherHostImpl::PauseRequest(), but that is no longer a public
function ;-)  The DRH has its own concept of pausing the request!

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:423: last_pipe_pause_time_
= base::TimeTicks();
nit: another case where "pipe" is used instead of "byte stream" or "stream"

[benjhayden, 2012-05-26 20:42:01]

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
File content/browser/download/download_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:934: void
WriteCopyToPipe(content::ByteStreamInput* pipe,
inputs before outputs

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:56: //
DownloadResourceHandler members on the IO thread.
on the UI thread, right?

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:130:
DCHECK(!on_response_started_called_);
What does this buy?

http://codereview.chromium.org/10392111/diff/28001/content/browser/renderer_h...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/10392111/diff/28001/content/browser/renderer_h...
content/browser/renderer_host/buffered_resource_handler.cc:284: return
UseAlternateResourceHandler(request_id, x509_cert_handler, defer);
Is it ok to not run next_handler_->OnResponseStarted() if
UseAlternateResourceHandler() returns true? Also if(*defer) return true.

[Randy Smith (Not in Mondays), 2012-05-28 02:51:36]

Ben & Darin's comments incorporated.  PTAL.

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
File content/browser/download/download_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:934: void
WriteCopyToPipe(content::ByteStreamInput* pipe,
On 2012/05/26 20:42:02, benjhayden_chromium wrote:
> inputs before outputs

Done, but I don't consider pipe an output in that sense (it's having stuff done
to it, but the function isn't creating a new pipe).

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
File content/browser/download/download_resource_handler.cc (right):

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:43: static const int
kDownloadPipeSize = 100 * 1024;
On 2012/05/26 03:59:19, darin wrote:
> nit: "pipe" -> "stream" / "byte stream"?

Done.

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:56: //
DownloadResourceHandler members on the IO thread.
On 2012/05/26 20:42:02, benjhayden_chromium wrote:
> on the UI thread, right?

Done.

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:130:
DCHECK(!on_response_started_called_);
On 2012/05/26 20:42:02, benjhayden_chromium wrote:
> What does this buy?

Turns out that there's a bug in BufferedResourceHandler where a function's
OnResponseStarted can be called twice.  I'm fixing it in a separate CL
(http://codereview.chromium.org/10451023 ) but merging that CL in here so I
don't run into the problem.  This was the 'test' side of the fix.

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:156:
scoped_ptr<content::ByteStreamOutput> stream_reader;
On 2012/05/26 03:59:19, darin wrote:
> I know I'm being pedantic, but if stream_reader is the right name for the
> variable, then {Prefix}StreamReader is probably the right name for the class. 
> Just food for thought.  I know a class name is good when I like using the name
> for variables of that type too ;-)

Done :-} :-J.

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:214:
scoped_refptr<DownloadFileManager>(download_file_manager_),
On 2012/05/26 03:59:19, darin wrote:
> nit: I think it is OK to just change download_file_manager_ to be of type
> scoped_refptr<DownloadFileManager>.

Done.

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:296: PauseRequest();
On 2012/05/26 03:59:19, darin wrote:
> I know PauseRequest() looks odd because we are so used to
> ResourceDispatcherHostImpl::PauseRequest(), but that is no longer a public
> function ;-)  The DRH has its own concept of pausing the request!

Done.

http://codereview.chromium.org/10392111/diff/28001/content/browser/download/d...
content/browser/download/download_resource_handler.cc:423: last_pipe_pause_time_
= base::TimeTicks();
On 2012/05/26 03:59:19, darin wrote:
> nit: another case where "pipe" is used instead of "byte stream" or "stream"

Done.

http://codereview.chromium.org/10392111/diff/28001/content/browser/renderer_h...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/10392111/diff/28001/content/browser/renderer_h...
content/browser/renderer_host/buffered_resource_handler.cc:284: return
UseAlternateResourceHandler(request_id, x509_cert_handler, defer);
On 2012/05/26 20:42:02, benjhayden_chromium wrote:
> Is it ok to not run next_handler_->OnResponseStarted() if
> UseAlternateResourceHandler() returns true? Also if(*defer) return true.

UseAlternateResourceHandler() takes responsibility when you call it--if you
look, it actually starts using tracking variables as to whether
OnResponseStarted() has been run.

My apologies; this should never have shown up in this CL since it's part of
http://codereview.chromium.org/10451023.  I should have done the proper magic to
do the diff from that CL.  I'll try to make sure this disappears in a future
patch set.  (Not saying you're not welcome to comments on that CL if you *want*
to--just do it on that CL, and you have a small window since I've already gotten
Darin's LGTM :-}.)

[benjhayden, 2012-05-29 17:40:37]

LGTM, one nit

http://codereview.chromium.org/10392111/diff/36006/content/browser/download/d...
File content/browser/download/download_file_impl.h (right):

http://codereview.chromium.org/10392111/diff/36006/content/browser/download/d...
content/browser/download/download_file_impl.h:61: protected:                    
        // For test class overrides.
Please put the comment on the next line.

[rvargas (doing something else), 2012-05-29 18:13:36]

net and renderer_host LGTM

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:268: // This
class is a variation on URLRequestTestJob in that it only
nit: remove "only" ?

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:270: class
URLRequestTestDelayedCompleteJob : public net::URLRequestTestJob {
...DelayedCompletion... ? DelayCompletion ?

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:272:
URLRequestTestDelayedCompleteJob(net::URLRequest* request)
explicit

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:273: :
net::URLRequestTestJob(request) { }
nit: no space between {} (same thing below)

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:277: const
std::string& response_headers,
nit: indent three more spaces

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:281: request,
response_headers, response_data, auto_advance) { }
nit: arguments start on the previous line

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:1305:
response_data.resize(1025, ' ');
It seems quite fragile. Hopefully the failure will be clear when someone is
refactoring that logic.

http://codereview.chromium.org/10392111/diff/36006/net/url_request/url_reques...
File net/url_request/url_request_test_job.cc (right):

http://codereview.chromium.org/10392111/diff/36006/net/url_request/url_reques...
net/url_request/url_request_test_job.cc:251: // Make any nested read calls
return IO_PENDING.
This actually requires nested read calls for it to work. Maybe add that
somewhere to this class documentation. (although that would be documenting a
feature that is not directly supported by this class... but the derived class
(and related tests) should be aware of it).

[rvargas (doing something else), 2012-05-29 18:32:13]

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:284: bool
FinalRoundIOPending() { return true; }
I forgot about this one:
nit: virtual.

[Randy Smith (Not in Mondays), 2012-05-30 00:22:31]

I've incorporated comments.  Ricardo, if you'd be willing to do one more run on
URLRequestTestJob, I'd be grateful; I thought about your comment and tweaked the
semantics I was exporting a bit, and I'd like your opinion on it.

http://codereview.chromium.org/10392111/diff/36006/content/browser/download/d...
File content/browser/download/download_file_impl.h (right):

http://codereview.chromium.org/10392111/diff/36006/content/browser/download/d...
content/browser/download/download_file_impl.h:61: protected:                    
        // For test class overrides.
On 2012/05/29 17:40:37, benjhayden_chromium wrote:
> Please put the comment on the next line.

Done.

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:268: // This
class is a variation on URLRequestTestJob in that it only
On 2012/05/29 18:13:36, rvargas wrote:
> nit: remove "only" ?

Done.

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:270: class
URLRequestTestDelayedCompleteJob : public net::URLRequestTestJob {
On 2012/05/29 18:13:36, rvargas wrote:
> ...DelayedCompletion... ? DelayCompletion ?

Went with URLRequestTestDelayedCompletionJob; let me know if that's not ok.

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:272:
URLRequestTestDelayedCompleteJob(net::URLRequest* request)
On 2012/05/29 18:13:36, rvargas wrote:
> explicit

Whoops; thank you.

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:273: :
net::URLRequestTestJob(request) { }
On 2012/05/29 18:13:36, rvargas wrote:
> nit: no space between {} (same thing below)

Done.

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:277: const
std::string& response_headers,
On 2012/05/29 18:13:36, rvargas wrote:
> nit: indent three more spaces

Done; sorry.

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:281: request,
response_headers, response_data, auto_advance) { }
On 2012/05/29 18:13:36, rvargas wrote:
> nit: arguments start on the previous line

Done.

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:284: bool
FinalRoundIOPending() { return true; }
On 2012/05/29 18:32:13, rvargas wrote:
> I forgot about this one:
> nit: virtual.

Also added OVERRIDE, now that you got me thinking in that direction.  Done.

http://codereview.chromium.org/10392111/diff/36006/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:1305:
response_data.resize(1025, ' ');
On 2012/05/29 18:13:36, rvargas wrote:
> It seems quite fragile. Hopefully the failure will be clear when someone is
> refactoring that logic.

Completely fair, but I'm not coming up with a good way around it.  I guess the
failure mode would be that the DownloadResourceHandler is never invoked, and we
could put some tracking in to make sure that actually happens.  If it does
happen in the current situation it won't be a download request, so the ignoring
of the cancel test will fail.  I'm not sure how obvious it'll be why, though
:-{.  

Having written the above, I think I'll amplify this comment.  I can't think of
anything better to do, though.

http://codereview.chromium.org/10392111/diff/36006/net/url_request/url_reques...
File net/url_request/url_request_test_job.cc (right):

http://codereview.chromium.org/10392111/diff/36006/net/url_request/url_reques...
net/url_request/url_request_test_job.cc:251: // Make any nested read calls
return IO_PENDING.
On 2012/05/29 18:13:36, rvargas wrote:
> This actually requires nested read calls for it to work. Maybe add that
> somewhere to this class documentation. (although that would be documenting a
> feature that is not directly supported by this class... but the derived class
> (and related tests) should be aware of it).

I renamed the upcall and changed it's semantics to be defined in terms of the
state transition; let me know if you like this better.

[rvargas (doing something else), 2012-05-30 00:49:01]

LGTM

http://codereview.chromium.org/10392111/diff/26002/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/10392111/diff/26002/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:278: const
std::string& response_headers,
nit: indent two more spaces

http://codereview.chromium.org/10392111/diff/26002/net/url_request/url_reques...
File net/url_request/url_request_test_job.cc (right):

http://codereview.chromium.org/10392111/diff/26002/net/url_request/url_reques...
net/url_request/url_request_test_job.cc:251: // Make all future results return
io pending until the next
nit: results -> something else. "Operations" maybe?

[Randy Smith (Not in Mondays), 2012-05-30 19:36:42]

Hopefully final reviewer-relevant upload.  Ben, could you take a look at my
changes to download_file_manager_unittest.cc and download_stats.cc?

* DFM unit tests: I deleted a lot of tests now that data flow and completion is
completely a function of the download file.

* download_stats.cc: I made smaller some of my ranges for bandwidth so that I
get better granularity.

I think this is about ready to submit, but I'm going to do one more round of try
jobs (as it turned out my local test list hadn't included the download file
manager tests :-{) and one more leak tryjob run (I saw a weird valgrind leak,
but it was in a test that was crashing and I couldn't reproduce it in my sandbox
after I fixed the crash).

http://codereview.chromium.org/10392111/diff/26002/content/browser/renderer_h...
File content/browser/renderer_host/resource_dispatcher_host_unittest.cc (right):

http://codereview.chromium.org/10392111/diff/26002/content/browser/renderer_h...
content/browser/renderer_host/resource_dispatcher_host_unittest.cc:278: const
std::string& response_headers,
On 2012/05/30 00:49:03, rvargas wrote:
> nit: indent two more spaces

Done.

http://codereview.chromium.org/10392111/diff/26002/net/url_request/url_reques...
File net/url_request/url_request_test_job.cc (right):

http://codereview.chromium.org/10392111/diff/26002/net/url_request/url_reques...
net/url_request/url_request_test_job.cc:251: // Make all future results return
io pending until the next
On 2012/05/30 00:49:03, rvargas wrote:
> nit: results -> something else. "Operations" maybe?

I went for "reads", which seemed most precise.

[benjhayden, 2012-05-30 20:22:32]

LGTM, one nit

http://codereview.chromium.org/10392111/diff/31039/content/browser/download/d...
File content/browser/download/download_stats.cc (right):

http://codereview.chromium.org/10392111/diff/31039/content/browser/download/d...
content/browser/download/download_stats.cc:319: "Download.BandwidthNetwork",
Include units (looks like Bps?) in case you decide to add bps or KBps?
Also, network bandwidth is usually bits-per-second rather than bytes, so it
would be good to at least comment that you're breaking tradition.

[commit-bot: I haz the power, 2012-06-04 16:30:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10392111/48002

[commit-bot: I haz the power, 2012-06-04 16:31:02]

Failed to apply patch for content/browser/download/download_file_unittest.cc:
While running patch -p1 --forward --force;
patching file content/browser/download/download_file_unittest.cc
Hunk #1 FAILED at 6.
1 out of 10 hunks FAILED -- saving rejects to file
content/browser/download/download_file_unittest.cc.rej

[commit-bot: I haz the power, 2012-06-04 16:59:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10392111/63001

[commit-bot: I haz the power, 2012-06-04 18:21:01]

Change committed as 140328