OnDownloadStarted takes DownloadItem* instead of DownloadId

OnDownloadStarted takes DownloadItem* instead of DownloadId
PS1@r160378

Reviewers:
sky: content/public/test, content/browser/renderer_host/resource_dispatcher_host_impl.cc
binji: chrome/browser/ui/intents
bauerb: chrome/browser/plugins
finnur: chrome/browser/extensions
Done:
rdsmith: *

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

[benjhayden, 2012-10-05 19:11:36]

PTAL

[Randy Smith (Not in Mondays), 2012-10-08 19:20:52]

It's awesome when I have a "Oh, that refactor's going to be complicated"
reaction, and you go off and do it, and it's actually pretty simple :-}.  

This all looks good; there are some header files that don't look necessary on
the review level so I wanted to quiz you on them, and there's one extra bit of
refactoring I'd like you to consider, but beyond that it looks good.

http://codereview.chromium.org/11068027/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/11068027/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_browsertest.cc:15: #include
"base/stringprintf.h"
Why?

http://codereview.chromium.org/11068027/diff/1/chrome/browser/ui/intents/web_...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/11068027/diff/1/chrome/browser/ui/intents/web_...
chrome/browser/ui/intents/web_intent_picker_controller.h:22: #include
"content/public/browser/download_id.h"
Why?

http://codereview.chromium.org/11068027/diff/1/content/browser/download/downl...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/11068027/diff/1/content/browser/download/downl...
content/browser/download/download_manager_impl.cc:379: return
GetDownload(download_id.local());
I think that it would be a better design to refactor this so that
CreateDownloadItem() returns the DownloadItem so you don't need to do
GetDownload().  OTOH, I'm doing exactly that in the DFM nuke CL, and there's
really no point in duplicating the work.  So this is fine, but I wanted to call
out the code preference.

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

http://codereview.chromium.org/11068027/diff/1/content/browser/download/downl...
content/browser/download/download_resource_handler.cc:73: DownloadItem* item =
download_manager->StartDownload(
What would you think of just passing the started_cb into DM::StartDownload so
that the DRH doesn't need to know about DownloadItems?  It requires the DM to
know about the callback type, but that doesn't seem like a horrible thing.

(Note that this would suggest some refactoring so that the default calling path
in the DRH is to pass a NULL DI and assert that the error is not net::OK. 
Specifically, I think that CallStartedCB is now *always* called with a null
download (and probably always was :-})).

[benjhayden, 2012-10-08 19:49:45]

http://codereview.chromium.org/11068027/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/11068027/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_browsertest.cc:15: #include
"base/stringprintf.h"
On 2012/10/08 19:20:52, rdsmith wrote:
> Why?

L112 calls StringPrintf.
download_id.h includes base/stringprintf.h, but download_id.h is no longer
included in something that extension_browsertest.cc includes.
It looks like chrome's IWYU needs some love.
http://code.google.com/p/chromium/wiki/IncludeWhatYouUse

http://codereview.chromium.org/11068027/diff/1/chrome/browser/ui/intents/web_...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/11068027/diff/1/chrome/browser/ui/intents/web_...
chrome/browser/ui/intents/web_intent_picker_controller.h:22: #include
"content/public/browser/download_id.h"
On 2012/10/08 19:20:52, rdsmith wrote:
> Why?

This class still uses DownloadId, but download_id.h is now no longer included in
another header that this file includes.

http://codereview.chromium.org/11068027/diff/1/content/browser/download/downl...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/11068027/diff/1/content/browser/download/downl...
content/browser/download/download_manager_impl.cc:379: return
GetDownload(download_id.local());
On 2012/10/08 19:20:52, rdsmith wrote:
> I think that it would be a better design to refactor this so that
> CreateDownloadItem() returns the DownloadItem so you don't need to do
> GetDownload().  OTOH, I'm doing exactly that in the DFM nuke CL, and there's
> really no point in duplicating the work.  So this is fine, but I wanted to
call
> out the code preference.

Ya, I thought about making CreateDownloadItem return the item, but noticed that
that would require some surgery around bound_net_log, and decided to let you
handle it. :-)

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

http://codereview.chromium.org/11068027/diff/1/content/browser/download/downl...
content/browser/download/download_resource_handler.cc:73: DownloadItem* item =
download_manager->StartDownload(
On 2012/10/08 19:20:52, rdsmith wrote:
> What would you think of just passing the started_cb into DM::StartDownload so
> that the DRH doesn't need to know about DownloadItems?  It requires the DM to
> know about the callback type, but that doesn't seem like a horrible thing.
> 
> (Note that this would suggest some refactoring so that the default calling
path
> in the DRH is to pass a NULL DI and assert that the error is not net::OK. 
> Specifically, I think that CallStartedCB is now *always* called with a null
> download (and probably always was :-})).

Hm, maybe in another CL.
I have no strong objections to that, but I kind of like being able to see all of
the callers of started_cb in a single file, and DRH would still need to include
download_item.h because it uses DI::IN_PROGRESS further down.

[Randy Smith (Not in Mondays), 2012-10-08 19:50:40]

lgtm

[benjhayden, 2012-10-09 15:28:02]

sky, binji, bauerb, finnur: PTAL for ownership. Thanks!

[Finnur, 2012-10-09 15:57:51]

OWNERS LG. Just have a couple of nits for those files I looked at.

General comment:
BUG= and TEST= are missing from CL description.

http://codereview.chromium.org/11068027/diff/16004/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/11068027/diff/16004/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:651: VLOG(1) <<
__FUNCTION__ << " " << item << " " << error;
don't you still want the id here and not the pointer value?

http://codereview.chromium.org/11068027/diff/16004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/11068027/diff/16004/chrome/browser/extensions/...
chrome/browser/extensions/extension_browsertest.cc:15: #include
"base/stringprintf.h"
Was this intentional?

[Finnur, 2012-10-09 15:58:19]

Hmm... let's try that again:

OWNERS LGTM (with nits)

[binji, 2012-10-09 16:02:29]

intents lgtm

[Bernhard Bauer, 2012-10-09 16:04:25]

chrome/browser/plugins LGTM

[sky, 2012-10-09 16:16:11]

LGTM

http://codereview.chromium.org/11068027/diff/16004/content/public/test/downlo...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/11068027/diff/16004/content/public/test/downlo...
content/public/test/download_test_observer.cc:412: DownloadItem* item,
net::Error error) {
nit: when you wrap, each param on its own line.

[benjhayden, 2012-10-09 20:57:18]

CQ-ing.
Thanks, everybody, for the prompt response!

http://codereview.chromium.org/11068027/diff/16004/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

http://codereview.chromium.org/11068027/diff/16004/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api.cc:651: VLOG(1) <<
__FUNCTION__ << " " << item << " " << error;
On 2012/10/09 15:57:51, Finnur wrote:
> don't you still want the id here and not the pointer value? 

I'm not interested in the particular value of the id, just the general degree of
success.

http://codereview.chromium.org/11068027/diff/16004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_browsertest.cc (right):

http://codereview.chromium.org/11068027/diff/16004/chrome/browser/extensions/...
chrome/browser/extensions/extension_browsertest.cc:15: #include
"base/stringprintf.h"
On 2012/10/09 15:57:51, Finnur wrote:
> Was this intentional?

Yes, this file calls StringPrintf.
Some other header file X.h that is included here was including download_id.h
(which includes base/stringprintf.h) but X.h no longer includes download_id.h.
Chrome's IWYU needs some love.

http://codereview.chromium.org/11068027/diff/16004/content/public/test/downlo...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/11068027/diff/16004/content/public/test/downlo...
content/public/test/download_test_observer.cc:412: DownloadItem* item,
net::Error error) {
On 2012/10/09 16:16:11, sky wrote:
> nit: when you wrap, each param on its own line.

Done.

[commit-bot: I haz the power, 2012-10-09 20:58:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/11068027/15006

[commit-bot: I haz the power, 2012-10-09 23:17:59]

Change committed as 160971