Replace the DownloadFileManager with direct ownership of DownloadFile.

Replace the DownloadFileManager with direct ownership of DownloadFile.

This CL is a re-land of http://codereview.chromium.org/10799005, which 
was reverted due to test flakiness.  

BUG=123998
R=benjhayden@chromium.org
TBR=jam@chromium.org


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

[Randy Smith (Not in Mondays), 2012-08-16 20:20:09]

John, Ben: Could you take a look at this?  PS1 of this issue is identical to the
 CL you already approved in http://codereview.chromium.org/10799005.  PS2 takes
care of the reason it was reverted (see download problems in, e.g.
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
 ).  

There was a memory bot problem that came up WRT PowerSaveBlocker that I don't
know if it was this CL or not.  I ran all the tests I think of as "download
related" locally under valgrind and did not repro.  I plan to gate landing on a
valgrind try bot run, and send a note to the memory sheriffs before landing, and
beyond that not worry about it.  Let me know if you think I should do anything
else.

[Randy Smith (Not in Mondays), 2012-08-16 20:21:37]

On 2012/08/16 20:20:09, rdsmith wrote:
> John, Ben: Could you take a look at this?  PS1 of this issue is identical to
the
>  CL you already approved in http://codereview.chromium.org/10799005.  PS2
takes
> care of the reason it was reverted (see download problems in, e.g.
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%2520ChromiumOS...
>  ).  
> 
> There was a memory bot problem that came up WRT PowerSaveBlocker that I don't
> know if it was this CL or not.  I ran all the tests I think of as "download
> related" locally under valgrind and did not repro.  I plan to gate landing on
a
> valgrind try bot run, and send a note to the memory sheriffs before landing,
and
> beyond that not worry about it.  Let me know if you think I should do anything
> else.

If you care, an example of the valgrind failure is at
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20%28...
.

[benjhayden, 2012-08-16 20:35:20]

LGTM with one request for a comment that you can ignore if you want.

http://codereview.chromium.org/10836293/diff/2001/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10836293/diff/2001/content/browser/download/do...
content/browser/download/download_item_impl.cc:123: // at the end of the
function.
Please also warn against copying callbacks containing base::Passed.

[Randy Smith (Not in Mondays), 2012-08-16 20:44:21]

http://codereview.chromium.org/10836293/diff/2001/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10836293/diff/2001/content/browser/download/do...
content/browser/download/download_item_impl.cc:123: // at the end of the
function.
On 2012/08/16 20:35:20, benjhayden_chromium wrote:
> Please also warn against copying callbacks containing base::Passed.

I apologize, but I think I'm going to take your out about ignoring the comment. 
The problem is that a warning isn't appropriate here (while these functions are
used in constructing callbacks, there's no hint of callback magic here) and
isn't really appropriate at point of use (in that the callbacks there are being
passed directly to PostTask and hence shouldn't need the warning).  I sorta want
a warning in base/callback.h, but the author didn't choose to put one in, and
there's already one in Will's writeup on constructs in chrome.  So I'm not sure
there's a good place to add it.

[Randy Smith (Not in Mondays), 2012-08-17 19:18:30]

John indicates that TBRs are fine for relanding if no reviewed code has been
changed; re-submitting to the CQ on that basis.

[commit-bot: I haz the power, 2012-08-17 19:22:36]

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

[commit-bot: I haz the power, 2012-08-17 21:11:53]

Try job failure for 10836293-2001 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-17 21:24:02]

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

[commit-bot: I haz the power, 2012-08-18 02:12:51]

Change committed as 152213