Added callback to DownloadUrl() so we can find download failures.

Added callback to DownloadUrl() so we can find download failures where the download item isn't created at all.

Added observer class for browser tests, which uses the callback.

BUG=117033
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126251

[Randy Smith (Not in Mondays), 2012-03-01 16:50:11]

Ben, could you take a look at this as well?  This is more solidly in the area
that I think of as yours (the callback functionality having been added for the
extension system), but I'll do my own review here as well.

[benjhayden, 2012-03-01 19:48:53]

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
File chrome/browser/download/download_test_observer.h (right):

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_test_observer.h:205: class
DownloadTestItemCreationObserver
This doesn't appear to be used in this CL. Could it wait for the CL where it
will be used?

If I can see how it will be used, then I could see why it needs to be RCTS and
its own class instead of a static function/callback.

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_test_observer.h:233: size_t num_created() const
{ return called_back_count_; }
This method's idea of creation is different from the previous method's idea.
Could that be a problem? Maybe just comment?

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_test_observer.h:236: return callback_;
Caching the callback seems unnecessary. It isn't expensive to re-bind every
time.

[ahendrickson, 2012-03-01 21:15:01]

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
File chrome/browser/download/download_test_observer.h (right):

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_test_observer.h:205: class
DownloadTestItemCreationObserver
On 2012/03/01 19:48:53, benjhayden_chromium wrote:
> This doesn't appear to be used in this CL. Could it wait for the CL where it
> will be used?
> 
> If I can see how it will be used, then I could see why it needs to be RCTS and
> its own class instead of a static function/callback.

It was split off from CL 9426029, which uses it.

Now using the test in download_browsertest.cc.

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_test_observer.h:233: size_t num_created() const
{ return called_back_count_; }
On 2012/03/01 19:48:53, benjhayden_chromium wrote:
> This method's idea of creation is different from the previous method's idea.
> Could that be a problem? Maybe just comment?

Renamed.

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_test_observer.h:236: return callback_;
On 2012/03/01 19:48:53, benjhayden_chromium wrote:
> Caching the callback seems unnecessary. It isn't expensive to re-bind every
> time. 

It lets me make DownloadItemCreationCallback() private.

[Randy Smith (Not in Mondays), 2012-03-01 21:17:24]

High level comments (header file and class creation); will hold off on more
detailed review until we resolve these.  The basic interface to the callback
makes sense to me :-}.

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
File chrome/browser/download/download_test_observer.h (right):

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_test_observer.h:204: // or that an error
occurred and it won't be created.
Hmmm.  Good point that we now have a new need for a waiter that we didn't before
because there's another type of error.  But is there a simpler way to handle
this problem?  Specifically, would the use cases you envision for this be as
well handled by a test wrapper function that called DM::DownloadUrl with a local
callback and then called the UI message loop?  The callback would be a static
function in the file which would quit the message loop when the callback
occurred (after storing the information passed back via the callback).  I think
it would be much less code, and simpler to understand as well.

http://codereview.chromium.org/9570005/diff/3001/content/browser/download/dow...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/9570005/diff/3001/content/browser/download/dow...
content/browser/download/download_resource_handler.h:37: typedef
content::DownloadManager::OnStartedCallback OnStartedCallback;
I don't think there's any reason for this to be in the header file rather than
the .cc file, is there?  Header files should be as sparse as possible.

[ahendrickson, 2012-03-02 17:34:39]

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
File chrome/browser/download/download_test_observer.h (right):

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_test_observer.h:204: // or that an error
occurred and it won't be created.
On 2012/03/01 21:17:24, rdsmith wrote:
> Hmmm.  Good point that we now have a new need for a waiter that we didn't
before
> because there's another type of error.  But is there a simpler way to handle
> this problem?  Specifically, would the use cases you envision for this be as
> well handled by a test wrapper function that called DM::DownloadUrl with a
local
> callback and then called the UI message loop?  The callback would be a static
> function in the file which would quit the message loop when the callback
> occurred (after storing the information passed back via the callback).  I
think
> it would be much less code, and simpler to understand as well.

I'll simplify it to only using one download, but I think it's pretty easy to
understand as it is, and encapsulates the behavior nicely.

http://codereview.chromium.org/9570005/diff/3001/content/browser/download/dow...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/9570005/diff/3001/content/browser/download/dow...
content/browser/download/download_resource_handler.h:37: typedef
content::DownloadManager::OnStartedCallback OnStartedCallback;
On 2012/03/01 21:17:24, rdsmith wrote:
> I don't think there's any reason for this to be in the header file rather than
> the .cc file, is there?  Header files should be as sparse as possible.

True, but then I would have to change 5 other files to use
DownloadManager::OnStartedCallback() instead of
DownloadResourceHandler::OnStartedCallback():

chrome/browser/plugin_installer.cc
content/browser/renderer_host/buffered_resource_handler.cc
content/browser/renderer_host/render_message_filter.cc
content/browser/renderer_host/resource_dispatcher_host.cc
content/browser/renderer_host/resource_dispatcher_host.h

I will do so if you think it's worth it, but it seems to me those files
'understand' DownloadResourceHandler better than DownloadManager.

[ahendrickson, 2012-03-02 19:25:42]

PTAL

[cbentzel, 2012-03-02 20:13:05]

What is the failure you are trying to find? There should be a bug associated
with this if there is an actual problem. The CL description is too vague.

[ahendrickson, 2012-03-02 20:16:46]

On 2012/03/02 20:13:05, cbentzel wrote:
> What is the failure you are trying to find? There should be a bug associated
> with this if there is an actual problem. The CL description is too vague.

For example, a direct download that results in a 404 Not Found from the server.

[cbentzel, 2012-03-02 20:18:52]

Is this for testing use only? It doesn't look like any runtime code is providing
anything other than a null callback.

http://codereview.chromium.org/9570005/diff/8002/content/browser/download/dow...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/9570005/diff/8002/content/browser/download/dow...
content/browser/download/download_resource_handler.h:37: typedef
content::DownloadManager::OnStartedCallback OnStartedCallback;
Was this just done so you didn't have to override RDH?

[benjhayden, 2012-03-02 20:27:14]

lgtm

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
File chrome/browser/download/download_test_observer.h (right):

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_test_observer.h:236: return callback_;
On 2012/03/01 21:15:01, ahendrickson wrote:
> On 2012/03/01 19:48:53, benjhayden_chromium wrote:
> > Caching the callback seems unnecessary. It isn't expensive to re-bind every
> > time. 
> 
> It lets me make DownloadItemCreationCallback() private.

You can keep that method private, and you can get rid of callback_ by re-binding
every time callback() is called:

const content::DownloadManager::OnStartedCallback& callback() {
 return
base::Bind(&DownloadTestItemCreationObserver::DownloadItemCreationCallback,
this);
}

When you store the callback in a member variable, you're essentially caching it.
Every cache is a trade-off. In this case, callback_ saves a few nanoseconds but
spends a few bytes of memory and, more importantly in this case, lines of code.
Re-binding every time is a few nanoseconds slower, but saves a few bytes of
memory and, more importantly in this case, a few lines of code.

It doesn't really matter, I just didn't want you to keep thinking that I don't
know how C++ basics like private work.

[cbentzel, 2012-03-02 20:32:21]

Don't forget that you will need jam's review for the public change?

I don't really like adding yet more to the DownloadUrl public interface for the
testing case.

Are there other ways to handle this?

[cbentzel, 2012-03-02 21:00:17]

This seems fine for this particular case.

I do wish we had a better mechanism for handling the "download started callback"
but this is probably appropriate. 

I am worried, however, that we don't always invoke the callback and this could
wait forever. This is a general issue that already existed. For example, if the
DownloadUrl is for something that violates the Child Process Security Policy,
there is no return code visibile form DownloadManager::DownloadURL and the
callback is also never invoked.

http://codereview.chromium.org/9570005/diff/8002/chrome/browser/download/down...
File chrome/browser/download/download_test_observer.cc (right):

http://codereview.chromium.org/9570005/diff/8002/chrome/browser/download/down...
chrome/browser/download/download_test_observer.cc:371:
creation_info_.download_id = download_id;
Can you add a DCHECK that this is called on the UI thread?

[benjhayden, 2012-03-02 21:05:45]

On Fri, Mar 2, 2012 at 4:00 PM, <cbentzel@chromium.org> wrote:

> This seems fine for this particular case.
>
> I do wish we had a better mechanism for handling the "download started
> callback"
> but this is probably appropriate.
>
> I am worried, however, that we don't always invoke the callback and this
> could
> wait forever. This is a general issue that already existed. For example,
> if the
> DownloadUrl is for something that violates the Child Process Security
> Policy,
> there is no return code visibile form DownloadManager::DownloadURL and the
> callback is also never invoked.
>

Is there a bug for this? Repro instructions?


>
>
> http://codereview.chromium.**org/9570005/diff/8002/chrome/**
>
browser/download/download_**test_observer.cc<http://codereview.chromium.org/9570005/diff/8002/chrome/browser/download/download_test_observer.cc>
> File chrome/browser/download/**download_test_observer.cc (right):
>
> http://codereview.chromium.**org/9570005/diff/8002/chrome/**
>
browser/download/download_**test_observer.cc#newcode371<http://codereview.chromium.org/9570005/diff/8002/chrome/browser/download/download_test_observer.cc#newcode371>
> chrome/browser/download/**download_test_observer.cc:371:
> creation_info_.download_id = download_id;
> Can you add a DCHECK that this is called on the UI thread?
>
>
http://codereview.chromium.**org/9570005/<http://codereview.chromium.org/9570...
>

[cbentzel, 2012-03-02 21:11:21]

On Fri, Mar 2, 2012 at 4:05 PM, Ben Hayden <benjhayden@chromium.org> wrote:

>
>
> On Fri, Mar 2, 2012 at 4:00 PM, <cbentzel@chromium.org> wrote:
>
>> This seems fine for this particular case.
>>
>> I do wish we had a better mechanism for handling the "download started
>> callback"
>> but this is probably appropriate.
>>
>> I am worried, however, that we don't always invoke the callback and this
>> could
>> wait forever. This is a general issue that already existed. For example,
>> if the
>> DownloadUrl is for something that violates the Child Process Security
>> Policy,
>> there is no return code visibile form DownloadManager::DownloadURL and the
>> callback is also never invoked.
>>
>
> Is there a bug for this? Repro instructions?
>

It was from reviewing this current CL. Up until now callers of DownloadUrl
did not pass in callbacks which they expected to be invoked.

>
>
>>
>>
>> http://codereview.chromium.**org/9570005/diff/8002/chrome/**
>>
browser/download/download_**test_observer.cc<http://codereview.chromium.org/9570005/diff/8002/chrome/browser/download/download_test_observer.cc>
>> File chrome/browser/download/**download_test_observer.cc (right):
>>
>> http://codereview.chromium.**org/9570005/diff/8002/chrome/**
>>
browser/download/download_**test_observer.cc#newcode371<http://codereview.chromium.org/9570005/diff/8002/chrome/browser/download/download_test_observer.cc#newcode371>
>> chrome/browser/download/**download_test_observer.cc:371:
>> creation_info_.download_id = download_id;
>> Can you add a DCHECK that this is called on the UI thread?
>>
>>
http://codereview.chromium.**org/9570005/<http://codereview.chromium.org/9570...
>>
>
>

[ahendrickson, 2012-03-02 21:43:15]

On 2012/03/02 20:18:52, cbentzel wrote:
> Is this for testing use only? It doesn't look like any runtime code is
providing
> anything other than a null callback.

Randy wants to use this for other things in the future.  I'm just the first to
need it.

[ahendrickson, 2012-03-02 21:45:03]

On 2012/03/02 20:32:21, cbentzel wrote:
> Don't forget that you will need jam's review for the public change?
> 
> I don't really like adding yet more to the DownloadUrl public interface for
the
> testing case.
> 
> Are there other ways to handle this?

I think Randy said he would be able to LGTM this on his own, but I'll double
check.

This is going to be used in the future for other things, I'm told.

[ahendrickson, 2012-03-02 21:49:11]

On 2012/03/02 21:45:03, ahendrickson wrote:
> On 2012/03/02 20:32:21, cbentzel wrote:
> > Don't forget that you will need jam's review for the public change?
> > 
> > I don't really like adding yet more to the DownloadUrl public interface for
> the
> > testing case.
> > 
> > Are there other ways to handle this?
> 
> I think Randy said he would be able to LGTM this on his own, but I'll double
> check.

I know that Randy cleared it with jam before giving me the go-ahead.

> 
> This is going to be used in the future for other things, I'm told.

[ahendrickson, 2012-03-02 21:51:41]

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
File chrome/browser/download/download_test_observer.h (right):

http://codereview.chromium.org/9570005/diff/3001/chrome/browser/download/down...
chrome/browser/download/download_test_observer.h:236: return callback_;
On 2012/03/02 20:27:16, benjhayden_chromium wrote:
> On 2012/03/01 21:15:01, ahendrickson wrote:
> > On 2012/03/01 19:48:53, benjhayden_chromium wrote:
> > > Caching the callback seems unnecessary. It isn't expensive to re-bind
every
> > > time. 
> > 
> > It lets me make DownloadItemCreationCallback() private.
> 
> You can keep that method private, and you can get rid of callback_ by
re-binding
> every time callback() is called:
> 
> const content::DownloadManager::OnStartedCallback& callback() {
>  return
> base::Bind(&DownloadTestItemCreationObserver::DownloadItemCreationCallback,
> this);
> }
> 
> When you store the callback in a member variable, you're essentially caching
it.
> Every cache is a trade-off. In this case, callback_ saves a few nanoseconds
but
> spends a few bytes of memory and, more importantly in this case, lines of
code.
> Re-binding every time is a few nanoseconds slower, but saves a few bytes of
> memory and, more importantly in this case, a few lines of code.
> 
> It doesn't really matter, I just didn't want you to keep thinking that I don't
> know how C++ basics like private work.

Ah, I didn't think of that.

Changed.

http://codereview.chromium.org/9570005/diff/8002/chrome/browser/download/down...
File chrome/browser/download/download_test_observer.cc (right):

http://codereview.chromium.org/9570005/diff/8002/chrome/browser/download/down...
chrome/browser/download/download_test_observer.cc:371:
creation_info_.download_id = download_id;
On 2012/03/02 21:00:18, cbentzel wrote:
> Can you add a DCHECK that this is called on the UI thread? 

Done.

http://codereview.chromium.org/9570005/diff/8002/content/browser/download/dow...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/9570005/diff/8002/content/browser/download/dow...
content/browser/download/download_resource_handler.h:37: typedef
content::DownloadManager::OnStartedCallback OnStartedCallback;
On 2012/03/02 20:18:52, cbentzel wrote:
> Was this just done so you didn't have to override RDH?

This was done so that some code which was using RDH::OnStartedCallback didn't
have to reference DownloadManager.

[Randy Smith (Not in Mondays), 2012-03-06 16:25:19]

Quick high level response to the comment thread: This CL is in response to a
problem uncovered during testing, which is that the programmatic interface can
drop requests on the floor without any warning.  That is badly broken.  This
isn't my ideal fix, but my ideal fix (which is to guarantee that we always
create a DownloadItem, possibly in error, on any call to DownloadUrl) would take
a fair amount more work, and I'm not completely sure yet it's where I want to go
(callback interfaces might be easier for callers).  So:

* I agree with Chris that we should have a bug that describes this problem.

* I think this CL should fix the problem Chris called out (that there are other
places we can bail without calling the callback).  Let's actually fix the real
bug here--this isn't about adding an interface for testing, it's about fixing a
broken interface that we noticed during test writing.

* I'm disinclined to say that this CL should also fix all callers to actually
react to detected errors, but we should keep the issue open until those are
fixed.

[Randy Smith (Not in Mondays), 2012-03-06 16:25:55]

On 2012/03/06 16:25:19, rdsmith wrote:
> Quick high level response to the comment thread: This CL is in response to a
> problem uncovered during testing, which is that the programmatic interface can
> drop requests on the floor without any warning.  That is badly broken.  This
> isn't my ideal fix, but my ideal fix (which is to guarantee that we always
> create a DownloadItem, possibly in error, on any call to DownloadUrl) would
take
> a fair amount more work, and I'm not completely sure yet it's where I want to
go
> (callback interfaces might be easier for callers).  So:
> 
> * I agree with Chris that we should have a bug that describes this problem.
> 
> * I think this CL should fix the problem Chris called out (that there are
other
> places we can bail without calling the callback).  Let's actually fix the real
> bug here--this isn't about adding an interface for testing, it's about fixing
a
> broken interface that we noticed during test writing.
> 
> * I'm disinclined to say that this CL should also fix all callers to actually
> react to detected errors, but we should keep the issue open until those are
> fixed.

Oh, and yes I did run this interface change by John before giving Andy the
go-ahead.

[Randy Smith (Not in Mondays), 2012-03-06 21:29:14]

http://codereview.chromium.org/9570005/diff/3001/content/browser/download/dow...
File content/browser/download/download_resource_handler.h (right):

http://codereview.chromium.org/9570005/diff/3001/content/browser/download/dow...
content/browser/download/download_resource_handler.h:37: typedef
content::DownloadManager::OnStartedCallback OnStartedCallback;
On 2012/03/02 17:34:39, ahendrickson wrote:
> On 2012/03/01 21:17:24, rdsmith wrote:
> > I don't think there's any reason for this to be in the header file rather
than
> > the .cc file, is there?  Header files should be as sparse as possible.
> 
> True, but then I would have to change 5 other files to use
> DownloadManager::OnStartedCallback() instead of
> DownloadResourceHandler::OnStartedCallback():
> 
> chrome/browser/plugin_installer.cc
> content/browser/renderer_host/buffered_resource_handler.cc
> content/browser/renderer_host/render_message_filter.cc
> content/browser/renderer_host/resource_dispatcher_host.cc
> content/browser/renderer_host/resource_dispatcher_host.h
> 
> I will do so if you think it's worth it, but it seems to me those files
> 'understand' DownloadResourceHandler better than DownloadManager.

Got it--thanks for the explanation.  I'm good with this.

http://codereview.chromium.org/9570005/diff/11009/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/9570005/diff/11009/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:726:
EXPECT_EQ(download_info.show_download_item, creation_observer->started());
As I understand the DownloadTestItemCreationObserver interface, this doesn't
guarantee that an item's going to show up.  Shouldn't you check the error code
here in cases where you don't expect the item to show up, and expect success
from the creation observer when you do?

http://codereview.chromium.org/9570005/diff/11009/chrome/browser/download/dow...
File chrome/browser/download/download_test_observer.h (right):

http://codereview.chromium.org/9570005/diff/11009/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.h:210: struct CreationInfo {
Sorry to be pedantic about test code, but good class/header file habits are good
:-} :-J: This structure looks to only be relevant to the implementation, and
looking at the implementation, I'm not clear why you have it at all rather than
having the member variables separately in the class.  If you need it, could you
move it completely into the .cc file (along with the portions of the
implementation here that depend on it :-J)?  If you don't need it, can you get
rid of it?

http://codereview.chromium.org/9570005/diff/11009/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9570005/diff/11009/content/public/browser/down...
content/public/browser/download_manager.h:182: // occurs.
You should specify the calling interface to callback.  I think the only part
that isn't obvious from the signature is that DownloadId will be invalid? 
unspecified?  If net::Error is not net::OK.

(This could also be done at the typedef; that's probably a better location for
it, actually.)

[ahendrickson, 2012-03-06 22:01:36]

http://codereview.chromium.org/9570005/diff/11009/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9570005/diff/11009/content/public/browser/down...
content/public/browser/download_manager.h:182: // occurs.
On 2012/03/06 21:29:14, rdsmith wrote:
> You should specify the calling interface to callback.  I think the only part
> that isn't obvious from the signature is that DownloadId will be invalid? 
> unspecified?  If net::Error is not net::OK.
> 
> (This could also be done at the typedef; that's probably a better location for
> it, actually.)

Heh, I added that information to the typedef in the last CL.

[ahendrickson, 2012-03-07 02:58:12]

http://codereview.chromium.org/9570005/diff/11009/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/9570005/diff/11009/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:726:
EXPECT_EQ(download_info.show_download_item, creation_observer->started());
On 2012/03/06 21:29:14, rdsmith wrote:
> As I understand the DownloadTestItemCreationObserver interface, this doesn't
> guarantee that an item's going to show up.  Shouldn't you check the error code
> here in cases where you don't expect the item to show up, and expect success
> from the creation observer when you do?

Done.

http://codereview.chromium.org/9570005/diff/11009/chrome/browser/download/dow...
File chrome/browser/download/download_test_observer.h (right):

http://codereview.chromium.org/9570005/diff/11009/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.h:210: struct CreationInfo {
On 2012/03/06 21:29:14, rdsmith wrote:
> Sorry to be pedantic about test code, but good class/header file habits are
good
> :-} :-J: This structure looks to only be relevant to the implementation, and
> looking at the implementation, I'm not clear why you have it at all rather
than
> having the member variables separately in the class.  If you need it, could
you
> move it completely into the .cc file (along with the portions of the
> implementation here that depend on it :-J)?  If you don't need it, can you get
> rid of it?

Done.

[Randy Smith (Not in Mondays), 2012-03-07 21:16:56]

http://codereview.chromium.org/9570005/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/9570005/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:720:
EXPECT_EQ(download_info.show_download_item, creation_observer->started());
Don't use mean ->succeeded() here?  started() will be true if there's a
callback, even if it's in error.

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

http://codereview.chromium.org/9570005/diff/28001/content/browser/download/do...
content/browser/download/download_resource_handler.cc:44: if
(started_cb.is_null())
When you null a callback, doesn't that mean that running it does nothing?  If
so, you can get rid of this conditional (and maybe replace it with a comment if
it's confusing).

http://codereview.chromium.org/9570005/diff/28001/content/browser/download/do...
content/browser/download/download_resource_handler.cc:180:
base::Bind(&CallStartedCBOnUIThread, started_cb_, id, error));
As noted elsewhere, I think this can be simplified to avoid needing
CallStartedCBOnUIThread.

http://codereview.chromium.org/9570005/diff/28001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9570005/diff/28001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:317:
base::Bind(&CallbackOnUIThread, started_cb, net_error));
Can you just bind started_cb to content::DownloadId::Invalid(), net_error and
post that callback to the UI thread, skipping CallbackOnUIThread?

http://codereview.chromium.org/9570005/diff/28001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:975: return
CallbackAndReturn(started_cb, net::ERR_ACCESS_DENIED);
Since you've already created the resource handler at this point, I think you'll
get a double callback (one from the DRH destructor).  (If you agree and fix
this, I'd add a comment above about passing responsibility for the callback to
the DRH.)

http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
content/public/browser/download_manager.h:66: // NOTE: It is OK for the
DownloadId to be invalid if there is an error.
wording nit: "Is it OK" sounds like telling the consumer what they can and can't
do.  This is telling them what they should expect, so I think something like "If
there is an error, the DownloadId will be invalid." would be better.

http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
content/public/browser/download_manager.h:184: // occurs.
This sentence is actually underspecified, as the download can start and an error
occur later.  I'm inclined to say that once we know the DownloadItem is being
created, we can call the callback with the DownloadId and let any other errors
be the DownloadItem::Observer interface's problem.  If you agree, this
documentation should be made more specific (and you should make sure that's the
functionality that's actually supported).

[Randy Smith (Not in Mondays), 2012-03-07 21:19:16]

You may also want to pull someone else in (Ricardo?) to run their eye over the
RDH changes.

On 2012/03/07 21:16:56, rdsmith wrote:
>
http://codereview.chromium.org/9570005/diff/28001/chrome/browser/download/dow...
> File chrome/browser/download/download_browsertest.cc (right):
> 
>
http://codereview.chromium.org/9570005/diff/28001/chrome/browser/download/dow...
> chrome/browser/download/download_browsertest.cc:720:
> EXPECT_EQ(download_info.show_download_item, creation_observer->started());
> Don't use mean ->succeeded() here?  started() will be true if there's a
> callback, even if it's in error.
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/browser/download/do...
> File content/browser/download/download_resource_handler.cc (right):
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/browser/download/do...
> content/browser/download/download_resource_handler.cc:44: if
> (started_cb.is_null())
> When you null a callback, doesn't that mean that running it does nothing?  If
> so, you can get rid of this conditional (and maybe replace it with a comment
if
> it's confusing).
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/browser/download/do...
> content/browser/download/download_resource_handler.cc:180:
> base::Bind(&CallStartedCBOnUIThread, started_cb_, id, error));
> As noted elsewhere, I think this can be simplified to avoid needing
> CallStartedCBOnUIThread.
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/browser/renderer_ho...
> File content/browser/renderer_host/resource_dispatcher_host.cc (right):
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/browser/renderer_ho...
> content/browser/renderer_host/resource_dispatcher_host.cc:317:
> base::Bind(&CallbackOnUIThread, started_cb, net_error));
> Can you just bind started_cb to content::DownloadId::Invalid(), net_error and
> post that callback to the UI thread, skipping CallbackOnUIThread?
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/browser/renderer_ho...
> content/browser/renderer_host/resource_dispatcher_host.cc:975: return
> CallbackAndReturn(started_cb, net::ERR_ACCESS_DENIED);
> Since you've already created the resource handler at this point, I think
you'll
> get a double callback (one from the DRH destructor).  (If you agree and fix
> this, I'd add a comment above about passing responsibility for the callback to
> the DRH.)
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> File content/public/browser/download_manager.h (right):
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> content/public/browser/download_manager.h:66: // NOTE: It is OK for the
> DownloadId to be invalid if there is an error.
> wording nit: "Is it OK" sounds like telling the consumer what they can and
can't
> do.  This is telling them what they should expect, so I think something like
"If
> there is an error, the DownloadId will be invalid." would be better.
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> content/public/browser/download_manager.h:184: // occurs.
> This sentence is actually underspecified, as the download can start and an
error
> occur later.  I'm inclined to say that once we know the DownloadItem is being
> created, we can call the callback with the DownloadId and let any other errors
> be the DownloadItem::Observer interface's problem.  If you agree, this
> documentation should be made more specific (and you should make sure that's
the
> functionality that's actually supported).

[jstritar, 2012-03-07 21:34:39]

http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
content/public/browser/download_manager.h:184: // occurs.
On 2012/03/07 21:16:57, rdsmith wrote:
> This sentence is actually underspecified, as the download can start and an
error
> occur later.  I'm inclined to say that once we know the DownloadItem is being
> created, we can call the callback with the DownloadId and let any other errors
> be the DownloadItem::Observer interface's problem.  If you agree, this
> documentation should be made more specific (and you should make sure that's
the
> functionality that's actually supported).

What if we define a Delegate that has separate methods, where we guarantee that
one of them will be called?

class Delegate {
  public:
    void OnDownloadStarted(DownloadId id) = 0;
    void OnDownloadNotStarted(net::Error error) = 0;
};

Once OnDownloadStarted is invoked, the client can use DownloadItem::Observer.

[cbentzel, 2012-03-08 17:36:44]

On 2012/03/07 21:34:39, jstritar wrote:
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> File content/public/browser/download_manager.h (right):
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> content/public/browser/download_manager.h:184: // occurs.
> On 2012/03/07 21:16:57, rdsmith wrote:
> > This sentence is actually underspecified, as the download can start and an
> error
> > occur later.  I'm inclined to say that once we know the DownloadItem is
being
> > created, we can call the callback with the DownloadId and let any other
errors
> > be the DownloadItem::Observer interface's problem.  If you agree, this
> > documentation should be made more specific (and you should make sure that's
> the
> > functionality that's actually supported).
> 
> What if we define a Delegate that has separate methods, where we guarantee
that
> one of them will be called?
> 
> class Delegate {
>   public:
>     void OnDownloadStarted(DownloadId id) = 0;
>     void OnDownloadNotStarted(net::Error error) = 0;
> };
> 
> Once OnDownloadStarted is invoked, the client can use DownloadItem::Observer.

I generally prefer callbacks to Delegates. Much easier to deal with lifetime
management issues since they already handle weak pointers, refcounted pointers,
unretained pointers, etc.

As soon as you introduce a Delegate you have to worry about the lifetime of it
relative to the DownloadManager.

[jstritar, 2012-03-08 17:54:24]

On 2012/03/08 17:36:44, cbentzel wrote:
> On 2012/03/07 21:34:39, jstritar wrote:
> >
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> > File content/public/browser/download_manager.h (right):
> > 
> >
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> > content/public/browser/download_manager.h:184: // occurs.
> > On 2012/03/07 21:16:57, rdsmith wrote:
> > > This sentence is actually underspecified, as the download can start and an
> > error
> > > occur later.  I'm inclined to say that once we know the DownloadItem is
> being
> > > created, we can call the callback with the DownloadId and let any other
> errors
> > > be the DownloadItem::Observer interface's problem.  If you agree, this
> > > documentation should be made more specific (and you should make sure
that's
> > the
> > > functionality that's actually supported).
> > 
> > What if we define a Delegate that has separate methods, where we guarantee
> that
> > one of them will be called?
> > 
> > class Delegate {
> >   public:
> >     void OnDownloadStarted(DownloadId id) = 0;
> >     void OnDownloadNotStarted(net::Error error) = 0;
> > };
> > 
> > Once OnDownloadStarted is invoked, the client can use
DownloadItem::Observer.
> 
> I generally prefer callbacks to Delegates. Much easier to deal with lifetime
> management issues since they already handle weak pointers, refcounted
pointers,
> unretained pointers, etc.
> 
> As soon as you introduce a Delegate you have to worry about the lifetime of it
> relative to the DownloadManager.

How about we change the semantics of this callback then, so it's called exactly
once: when the download starts or fails to start.  Failures that occur after the
download start could be picked up by DownloadItem::Observer.

[Randy Smith (Not in Mondays), 2012-03-08 17:57:11]

On 2012/03/08 17:54:24, jstritar wrote:
> On 2012/03/08 17:36:44, cbentzel wrote:
> > On 2012/03/07 21:34:39, jstritar wrote:
> > >
> >
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> > > File content/public/browser/download_manager.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> > > content/public/browser/download_manager.h:184: // occurs.
> > > On 2012/03/07 21:16:57, rdsmith wrote:
> > > > This sentence is actually underspecified, as the download can start and
an
> > > error
> > > > occur later.  I'm inclined to say that once we know the DownloadItem is
> > being
> > > > created, we can call the callback with the DownloadId and let any other
> > errors
> > > > be the DownloadItem::Observer interface's problem.  If you agree, this
> > > > documentation should be made more specific (and you should make sure
> that's
> > > the
> > > > functionality that's actually supported).
> > > 
> > > What if we define a Delegate that has separate methods, where we guarantee
> > that
> > > one of them will be called?
> > > 
> > > class Delegate {
> > >   public:
> > >     void OnDownloadStarted(DownloadId id) = 0;
> > >     void OnDownloadNotStarted(net::Error error) = 0;
> > > };
> > > 
> > > Once OnDownloadStarted is invoked, the client can use
> DownloadItem::Observer.
> > 
> > I generally prefer callbacks to Delegates. Much easier to deal with lifetime
> > management issues since they already handle weak pointers, refcounted
> pointers,
> > unretained pointers, etc.
> > 
> > As soon as you introduce a Delegate you have to worry about the lifetime of
it
> > relative to the DownloadManager.
> 
> How about we change the semantics of this callback then, so it's called
exactly
> once: when the download starts or fails to start.  Failures that occur after
the
> download start could be picked up by DownloadItem::Observer.

+1 (or higher).

[cbentzel, 2012-03-08 17:57:23]

On 2012/03/08 17:54:24, jstritar wrote:
> On 2012/03/08 17:36:44, cbentzel wrote:
> > On 2012/03/07 21:34:39, jstritar wrote:
> > >
> >
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> > > File content/public/browser/download_manager.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> > > content/public/browser/download_manager.h:184: // occurs.
> > > On 2012/03/07 21:16:57, rdsmith wrote:
> > > > This sentence is actually underspecified, as the download can start and
an
> > > error
> > > > occur later.  I'm inclined to say that once we know the DownloadItem is
> > being
> > > > created, we can call the callback with the DownloadId and let any other
> > errors
> > > > be the DownloadItem::Observer interface's problem.  If you agree, this
> > > > documentation should be made more specific (and you should make sure
> that's
> > > the
> > > > functionality that's actually supported).
> > > 
> > > What if we define a Delegate that has separate methods, where we guarantee
> > that
> > > one of them will be called?
> > > 
> > > class Delegate {
> > >   public:
> > >     void OnDownloadStarted(DownloadId id) = 0;
> > >     void OnDownloadNotStarted(net::Error error) = 0;
> > > };
> > > 
> > > Once OnDownloadStarted is invoked, the client can use
> DownloadItem::Observer.
> > 
> > I generally prefer callbacks to Delegates. Much easier to deal with lifetime
> > management issues since they already handle weak pointers, refcounted
> pointers,
> > unretained pointers, etc.
> > 
> > As soon as you introduce a Delegate you have to worry about the lifetime of
it
> > relative to the DownloadManager.
> 
> How about we change the semantics of this callback then, so it's called
exactly
> once: when the download starts or fails to start.  Failures that occur after
the
> download start could be picked up by DownloadItem::Observer.

I believe that's what the semantics are here.

[cbentzel, 2012-03-08 17:58:48]

This looks fine. I'll remove myself from the reviewers list since Randy etc.
should be the ultimate authority.

http://codereview.chromium.org/9570005/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_test_observer.cc (right):

http://codereview.chromium.org/9570005/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.cc:322:
DownloadTestItemCreationObserver::~DownloadTestItemCreationObserver() {
Nit: comes after the constructor.

http://codereview.chromium.org/9570005/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.cc:350: if (waiting_ &&
(called_back_count_ > 0))
called_back_count_ will always be > 0. Do you need this?

I think you could also remove the waiting_ variable and just check is_running on
the message loop. This is fine though.

http://codereview.chromium.org/9570005/diff/28001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9570005/diff/28001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:315:
BrowserThread::PostTask(
Skip this if started_cb.is_null()?

http://codereview.chromium.org/9570005/diff/28001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/9570005/diff/28001/net/url_request/url_request...
net/url_request/url_request.h:327: const GURL& url() const {
Why was this change needed?

[jstritar, 2012-03-08 17:59:06]

Oh, in that case maybe we just need to clarify the documentation.

[cbentzel, 2012-03-08 17:59:31]

On 2012/03/08 17:57:23, cbentzel wrote:
> On 2012/03/08 17:54:24, jstritar wrote:
> > On 2012/03/08 17:36:44, cbentzel wrote:
> > > On 2012/03/07 21:34:39, jstritar wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> > > > File content/public/browser/download_manager.h (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> > > > content/public/browser/download_manager.h:184: // occurs.
> > > > On 2012/03/07 21:16:57, rdsmith wrote:
> > > > > This sentence is actually underspecified, as the download can start
and
> an
> > > > error
> > > > > occur later.  I'm inclined to say that once we know the DownloadItem
is
> > > being
> > > > > created, we can call the callback with the DownloadId and let any
other
> > > errors
> > > > > be the DownloadItem::Observer interface's problem.  If you agree, this
> > > > > documentation should be made more specific (and you should make sure
> > that's
> > > > the
> > > > > functionality that's actually supported).
> > > > 
> > > > What if we define a Delegate that has separate methods, where we
guarantee
> > > that
> > > > one of them will be called?
> > > > 
> > > > class Delegate {
> > > >   public:
> > > >     void OnDownloadStarted(DownloadId id) = 0;
> > > >     void OnDownloadNotStarted(net::Error error) = 0;
> > > > };
> > > > 
> > > > Once OnDownloadStarted is invoked, the client can use
> > DownloadItem::Observer.
> > > 
> > > I generally prefer callbacks to Delegates. Much easier to deal with
lifetime
> > > management issues since they already handle weak pointers, refcounted
> > pointers,
> > > unretained pointers, etc.
> > > 
> > > As soon as you introduce a Delegate you have to worry about the lifetime
of
> it
> > > relative to the DownloadManager.
> > 
> > How about we change the semantics of this callback then, so it's called
> exactly
> > once: when the download starts or fails to start.  Failures that occur after
> the
> > download start could be picked up by DownloadItem::Observer.
> 
> I believe that's what the semantics are here.

And if it's not [either in description or in current implementation] I  think
that those sound like desirable semantics.

[ahendrickson, 2012-03-08 21:33:07]

http://codereview.chromium.org/9570005/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/9570005/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_browsertest.cc:720:
EXPECT_EQ(download_info.show_download_item, creation_observer->started());
On 2012/03/07 21:16:57, rdsmith wrote:
> Don't use mean ->succeeded() here?  started() will be true if there's a
> callback, even if it's in error.

Done.

http://codereview.chromium.org/9570005/diff/28001/chrome/browser/download/dow...
File chrome/browser/download/download_test_observer.cc (right):

http://codereview.chromium.org/9570005/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.cc:322:
DownloadTestItemCreationObserver::~DownloadTestItemCreationObserver() {
On 2012/03/08 17:58:48, cbentzel wrote:
> Nit: comes after the constructor.

Done.

http://codereview.chromium.org/9570005/diff/28001/chrome/browser/download/dow...
chrome/browser/download/download_test_observer.cc:350: if (waiting_ &&
(called_back_count_ > 0))
On 2012/03/08 17:58:48, cbentzel wrote:
> called_back_count_ will always be > 0. Do you need this?
> 
> I think you could also remove the waiting_ variable and just check is_running
on
> the message loop. This is fine though.

Ah, this is kind of left over from when I was waiting for multiple downloads.

Removed called_back_count_ from test.

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

http://codereview.chromium.org/9570005/diff/28001/content/browser/download/do...
content/browser/download/download_resource_handler.cc:44: if
(started_cb.is_null())
On 2012/03/07 21:16:57, rdsmith wrote:
> When you null a callback, doesn't that mean that running it does nothing?  If
> so, you can get rid of this conditional (and maybe replace it with a comment
if
> it's confusing).

I tried it, and got a NULL function pointer which caused a crash.

http://codereview.chromium.org/9570005/diff/28001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9570005/diff/28001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:315:
BrowserThread::PostTask(
On 2012/03/08 17:58:48, cbentzel wrote:
> Skip this if started_cb.is_null()?

Done.

http://codereview.chromium.org/9570005/diff/28001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:317:
base::Bind(&CallbackOnUIThread, started_cb, net_error));
On 2012/03/07 21:16:57, rdsmith wrote:
> Can you just bind started_cb to content::DownloadId::Invalid(), net_error and
> post that callback to the UI thread, skipping CallbackOnUIThread?

Done.

http://codereview.chromium.org/9570005/diff/28001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:975: return
CallbackAndReturn(started_cb, net::ERR_ACCESS_DENIED);
On 2012/03/07 21:16:57, rdsmith wrote:
> Since you've already created the resource handler at this point, I think
you'll
> get a double callback (one from the DRH destructor).  (If you agree and fix
> this, I'd add a comment above about passing responsibility for the callback to
> the DRH.)

Done.

http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
content/public/browser/download_manager.h:66: // NOTE: It is OK for the
DownloadId to be invalid if there is an error.
On 2012/03/07 21:16:57, rdsmith wrote:
> wording nit: "Is it OK" sounds like telling the consumer what they can and
can't
> do.  This is telling them what they should expect, so I think something like
"If
> there is an error, the DownloadId will be invalid." would be better.  

Done.

http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
content/public/browser/download_manager.h:184: // occurs.
On 2012/03/07 21:16:57, rdsmith wrote:
> This sentence is actually underspecified, as the download can start and an
error
> occur later.  I'm inclined to say that once we know the DownloadItem is being
> created, we can call the callback with the DownloadId and let any other errors
> be the DownloadItem::Observer interface's problem.  If you agree, this
> documentation should be made more specific (and you should make sure that's
the
> functionality that's actually supported).

Done.

http://codereview.chromium.org/9570005/diff/28001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/9570005/diff/28001/net/url_request/url_request...
net/url_request/url_request.h:327: const GURL& url() const {
On 2012/03/08 17:58:48, cbentzel wrote:
> Why was this change needed?

I was getting a crash because url_chain_ was empty in some test.

[ahendrickson, 2012-03-08 21:44:12]

brettw: PTAL at chrome/browser/extensions, chrome/browser/tab_contents,
content/browser/renderer_host and content/browser/tab_contents.

I'm just adding empty callbacks to DownloadURL().

[Randy Smith (Not in Mondays), 2012-03-09 18:57:37]

On 2012/03/08 21:44:12, ahendrickson wrote:
> brettw: PTAL at chrome/browser/extensions, chrome/browser/tab_contents,
> content/browser/renderer_host and content/browser/tab_contents.
> 
> I'm just adding empty callbacks to DownloadURL().

content/browser/renderer_host includes resource_dispatcher_host.cc, and you're
adding calls to the callback there with semantic implications.

[Randy Smith (Not in Mondays), 2012-03-09 19:14:01]

Almost there--I'd like to understand the url_request.h change a bit better, and
I'm not sure we've gotten the right reviewers for url_request.h and
resource_dispatcher_host.cc, but the only thing blocking is the URL request
change.

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

http://codereview.chromium.org/9570005/diff/28001/content/browser/download/do...
content/browser/download/download_resource_handler.cc:44: if
(started_cb.is_null())
On 2012/03/08 21:33:07, ahendrickson wrote:
> On 2012/03/07 21:16:57, rdsmith wrote:
> > When you null a callback, doesn't that mean that running it does nothing? 
If
> > so, you can get rid of this conditional (and maybe replace it with a comment
> if
> > it's confusing).
> 
> I tried it, and got a NULL function pointer which caused a crash.

I spent a bit of time digging around to see if there was an already existing way
to do this and didn't find it, but I have a hard time believing there isn't
(e.g. when someone passes in a default constructed callback, we sometimes just
run it, and nothing blows up, so assigning a default constructed callback should
do it?).  But I'm ok with this for DRH; I would have objected more strongly to
the extra routine in RDH, but that's gone now :-}.

http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
content/public/browser/download_manager.h:184: // occurs.
On 2012/03/08 21:33:07, ahendrickson wrote:
> On 2012/03/07 21:16:57, rdsmith wrote:
> > This sentence is actually underspecified, as the download can start and an
> error
> > occur later.  I'm inclined to say that once we know the DownloadItem is
being
> > created, we can call the callback with the DownloadId and let any other
errors
> > be the DownloadItem::Observer interface's problem.  If you agree, this
> > documentation should be made more specific (and you should make sure that's
> the
> > functionality that's actually supported).
> 
> Done.

typo: "prevents ad download".

http://codereview.chromium.org/9570005/diff/28001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/9570005/diff/28001/net/url_request/url_request...
net/url_request/url_request.h:327: const GURL& url() const {
On 2012/03/08 21:33:07, ahendrickson wrote:
> On 2012/03/08 17:58:48, cbentzel wrote:
> > Why was this change needed?
> 
> I was getting a crash because url_chain_ was empty in some test.

Did you look at why it was empty in those tests when it wasn't before?  I
dislike making production code changes for tests, and I wouldn't have thought
that this CL would result in a change in url_chain_ contents.  And we don't
really have a url_request.h expert on the review list (excepting possibly Chris
:-}).  "If you don't understand it, it's dangerous."

http://codereview.chromium.org/9570005/diff/40024/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9570005/diff/40024/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:310:
DCHECK(!started_cb.is_null());
Why the DCHECK?  This may be a result of my not yet understanding callbacks
thoroughly, but I'm sure it's ok for people to pass in default constructed
callbacks (since you're doing it in this CL); if that translates to null, the
DCHECK shouldn't be here.  If it doesn't translate to null, the DCHECK is fine.

[ahendrickson, 2012-03-09 20:50:08]

http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
content/public/browser/download_manager.h:184: // occurs.
On 2012/03/09 19:14:02, rdsmith wrote:
> On 2012/03/08 21:33:07, ahendrickson wrote:
> > On 2012/03/07 21:16:57, rdsmith wrote:
> > > This sentence is actually underspecified, as the download can start and an
> > error
> > > occur later.  I'm inclined to say that once we know the DownloadItem is
> being
> > > created, we can call the callback with the DownloadId and let any other
> errors
> > > be the DownloadItem::Observer interface's problem.  If you agree, this
> > > documentation should be made more specific (and you should make sure
that's
> > the
> > > functionality that's actually supported).
> > 
> > Done.
> 
> typo: "prevents ad download".

Done.

http://codereview.chromium.org/9570005/diff/28001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/9570005/diff/28001/net/url_request/url_request...
net/url_request/url_request.h:327: const GURL& url() const {
On 2012/03/09 19:14:02, rdsmith wrote:
> On 2012/03/08 21:33:07, ahendrickson wrote:
> > On 2012/03/08 17:58:48, cbentzel wrote:
> > > Why was this change needed?
> > 
> > I was getting a crash because url_chain_ was empty in some test.
> 
> Did you look at why it was empty in those tests when it wasn't before?  I
> dislike making production code changes for tests, and I wouldn't have thought
> that this CL would result in a change in url_chain_ contents.  And we don't
> really have a url_request.h expert on the review list (excepting possibly
Chris
> :-}).  "If you don't understand it, it's dangerous."

No, I didn't.

I've reverted this, and will see if I can reproduce it.

http://codereview.chromium.org/9570005/diff/40024/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/9570005/diff/40024/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:310:
DCHECK(!started_cb.is_null());
On 2012/03/09 19:14:02, rdsmith wrote:
> Why the DCHECK?  This may be a result of my not yet understanding callbacks
> thoroughly, but I'm sure it's ok for people to pass in default constructed
> callbacks (since you're doing it in this CL); if that translates to null, the
> DCHECK shouldn't be here.  If it doesn't translate to null, the DCHECK is
fine.

Removed.

[cbentzel, 2012-03-09 21:15:19]

On 2012/03/09 19:14:01, rdsmith wrote:
> Almost there--I'd like to understand the url_request.h change a bit better,
and
> I'm not sure we've gotten the right reviewers for url_request.h and
> resource_dispatcher_host.cc, but the only thing blocking is the URL request
> change.
> 

Yeah - I shouldn't have mentioned "Looks fine" due to that. It sounds like you
are backing it out to understand why it was needed in the first place.

>
http://codereview.chromium.org/9570005/diff/28001/content/browser/download/do...
> File content/browser/download/download_resource_handler.cc (right):
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/browser/download/do...
> content/browser/download/download_resource_handler.cc:44: if
> (started_cb.is_null())
> On 2012/03/08 21:33:07, ahendrickson wrote:
> > On 2012/03/07 21:16:57, rdsmith wrote:
> > > When you null a callback, doesn't that mean that running it does nothing? 
> If
> > > so, you can get rid of this conditional (and maybe replace it with a
comment
> > if
> > > it's confusing).
> > 
> > I tried it, and got a NULL function pointer which caused a crash.
> 
> I spent a bit of time digging around to see if there was an already existing
way
> to do this and didn't find it, but I have a hard time believing there isn't
> (e.g. when someone passes in a default constructed callback, we sometimes just
> run it, and nothing blows up, so assigning a default constructed callback
should
> do it?).  But I'm ok with this for DRH; I would have objected more strongly to
> the extra routine in RDH, but that's gone now :-}.
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> File content/public/browser/download_manager.h (right):
> 
>
http://codereview.chromium.org/9570005/diff/28001/content/public/browser/down...
> content/public/browser/download_manager.h:184: // occurs.
> On 2012/03/08 21:33:07, ahendrickson wrote:
> > On 2012/03/07 21:16:57, rdsmith wrote:
> > > This sentence is actually underspecified, as the download can start and an
> > error
> > > occur later.  I'm inclined to say that once we know the DownloadItem is
> being
> > > created, we can call the callback with the DownloadId and let any other
> errors
> > > be the DownloadItem::Observer interface's problem.  If you agree, this
> > > documentation should be made more specific (and you should make sure
that's
> > the
> > > functionality that's actually supported).
> > 
> > Done.
> 
> typo: "prevents ad download".
> 
>
http://codereview.chromium.org/9570005/diff/28001/net/url_request/url_request.h
> File net/url_request/url_request.h (right):
> 
>
http://codereview.chromium.org/9570005/diff/28001/net/url_request/url_request...
> net/url_request/url_request.h:327: const GURL& url() const {
> On 2012/03/08 21:33:07, ahendrickson wrote:
> > On 2012/03/08 17:58:48, cbentzel wrote:
> > > Why was this change needed?
> > 
> > I was getting a crash because url_chain_ was empty in some test.
> 
> Did you look at why it was empty in those tests when it wasn't before?  I
> dislike making production code changes for tests, and I wouldn't have thought
> that this CL would result in a change in url_chain_ contents.  And we don't
> really have a url_request.h expert on the review list (excepting possibly
Chris
> :-}).  "If you don't understand it, it's dangerous."
> 
>
http://codereview.chromium.org/9570005/diff/40024/content/browser/renderer_ho...
> File content/browser/renderer_host/resource_dispatcher_host.cc (right):
> 
>
http://codereview.chromium.org/9570005/diff/40024/content/browser/renderer_ho...
> content/browser/renderer_host/resource_dispatcher_host.cc:310:
> DCHECK(!started_cb.is_null());
> Why the DCHECK?  This may be a result of my not yet understanding callbacks
> thoroughly, but I'm sure it's ok for people to pass in default constructed
> callbacks (since you're doing it in this CL); if that translates to null, the
> DCHECK shouldn't be here.  If it doesn't translate to null, the DCHECK is
fine.

[ahendrickson, 2012-03-09 21:33:42]

On 2012/03/09 21:15:19, cbentzel wrote:
> On 2012/03/09 19:14:01, rdsmith wrote:
> > Almost there--I'd like to understand the url_request.h change a bit better,
> and
> > I'm not sure we've gotten the right reviewers for url_request.h and
> > resource_dispatcher_host.cc, but the only thing blocking is the URL request
> > change.
> > 
> 
> Yeah - I shouldn't have mentioned "Looks fine" due to that. It sounds like you
> are backing it out to understand why it was needed in the first place.

That is correct.  If no test fails due to that, I'll leave it backed out.

[brettw, 2012-03-09 22:53:42]

LGTM rubberstamp, didn't check the details.

http://codereview.chromium.org/9570005/diff/54001/content/public/browser/down...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/9570005/diff/54001/content/public/browser/down...
content/public/browser/download_manager.h:185: // occurs that prevents a
download item from being created.
This should mention that it can be a null callback and what this means.

[ahendrickson, 2012-03-12 12:01:28]

rdsmith, PTAL.

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

LGTM; thanks.

[commit-bot: I haz the power, 2012-03-12 18:52:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/9570005/50005

[commit-bot: I haz the power, 2012-03-12 22:51:39]

Change committed as 126251