Remove DownloadFileManager in favor of direct ownership of DownloadFiles.

Remove DownloadFileManager in favor of direct ownership of DownloadFiles.

This CL is equivalent to CLs http://codereview.chromium.org/10799005 and 
http://codereview.chromium.org/10836293, which were previous attempts 
to land the same functionality.  

TBR=jam@chromium.org

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

[Randy Smith (Not in Mondays), 2012-08-21 21:52:55]

Please note that this CL is now dependent on
http://codereview.chromium.org/10831412/ (added some cancel/close tests to
SavePackage) and will need to be committed after that CL.

PS1 is identical to http://codereview.chromium.org/10836293.  PS2 adds the
integration with SavePackage necessary (I believe) to handle
http://crbug.com/143600.  I've as well changed some DCHECKs to CHECKs (PS3) so
that if this isn't the cause of 143600, we'll get information enough to figure
out what the real cause is.

Ben, could you take a look?  John, you're TBR'd as you've already LGTM'd
everything

[Randy Smith (Not in Mondays), 2012-08-21 22:01:21]

http://codereview.chromium.org/10823406/diff/3001/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10823406/diff/3001/content/browser/download/sa...
content/browser/download/save_package.cc:1391: // We're lying, but it's only a
little white lie :-J.
Fix this before landing.

[jam, 2012-08-22 01:27:18]

lgtm

[Randy Smith (Not in Mondays), 2012-08-22 18:41:22]

http://codereview.chromium.org/10823406/diff/3001/content/browser/download/sa...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10823406/diff/3001/content/browser/download/sa...
content/browser/download/save_package.cc:1391: // We're lying, but it's only a
little white lie :-J.
On 2012/08/21 22:01:21, rdsmith wrote:
> Fix this before landing.

Changed comment to explain choice--basically, it's easier for observers to get
the DownloadManager than the SavePackage, so we pretend to be from that source. 
We could use the WebContents, but that'd be harder for the current (single)
consumer who uses the source.

[benjhayden, 2012-08-22 19:07:30]

http://codereview.chromium.org/10823406/diff/12001/chrome/browser/download/sa...
File chrome/browser/download/save_page_browsertest.cc (left):

http://codereview.chromium.org/10823406/diff/12001/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:314: CheckDownloadHistory(url,
full_file_name, 1);  // a.htm is 1 file.
What happened to this?

http://codereview.chromium.org/10823406/diff/12001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10823406/diff/12001/content/browser/download/d...
content/browser/download/download_item_impl.cc:776: CHECK(download_file_.get());
Should some of these new CHECKs be DCHECKs?

http://codereview.chromium.org/10823406/diff/12001/content/browser/download/d...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/10823406/diff/12001/content/browser/download/d...
content/browser/download/download_item_impl.h:264: const bool
is_save_package_download_;
How would you feel about exposing this to DownloadManagerImpl and killing
SavePageData?

http://codereview.chromium.org/10823406/diff/12001/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10823406/diff/12001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:215: file_factory_.reset(new
content::DownloadFileFactory());
Why isn't this in the init list?

[Randy Smith (Not in Mondays), 2012-08-23 16:55:58]

My apologies for the merge combined with addressing comments.  The "real work"
I've done in this CL is:
* Waiting for persisted in the save_page_browsertest.cc (and checking history
afterwards).
* Replacing SavePageData with the boolean.
* Adding some probing of calls to the DownloadFileFactory in the
DownloadManagerImpl unit test (& adding a DFF to the DMI constructor).

PTAL.

http://codereview.chromium.org/10823406/diff/12001/chrome/browser/download/sa...
File chrome/browser/download/save_page_browsertest.cc (left):

http://codereview.chromium.org/10823406/diff/12001/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:314: CheckDownloadHistory(url,
full_file_name, 1);  // a.htm is 1 file.
On 2012/08/22 19:07:31, benjhayden_chromium wrote:
> What happened to this?

Restored after adding a "wait for persisted".

http://codereview.chromium.org/10823406/diff/12001/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10823406/diff/12001/content/browser/download/d...
content/browser/download/download_item_impl.cc:776: CHECK(download_file_.get());
On 2012/08/22 19:07:31, benjhayden_chromium wrote:
> Should some of these new CHECKs be DCHECKs?

So the context here is http://crbug.com/143600--that's what happened the last
time I landed.  I *think* that the save package fiddling will have fixed that,
but that's assuming root cause is save package cancels.  If it isn't, I want
more data, and I want it without pissing off kareng more :-} (by leaving a
crasher in canary).  These CHECKs will give me that data.  If in a couple of
days I don't have any crashes, I'll make them DCHECKs again, which change will
be very low risk in terms of canary crashers.  WDYT?

http://codereview.chromium.org/10823406/diff/12001/content/browser/download/d...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/10823406/diff/12001/content/browser/download/d...
content/browser/download/download_item_impl.h:264: const bool
is_save_package_download_;
On 2012/08/22 19:07:31, benjhayden_chromium wrote:
> How would you feel about exposing this to DownloadManagerImpl and killing
> SavePageData?

I think I'll skip answering the psychological question and just say that that's
a better plan than what's in the code now, and I can't think of a better plan
than it.  Done.

(I will say that the more I work in the save package code, the more I want it
gone :-J.)

http://codereview.chromium.org/10823406/diff/12001/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10823406/diff/12001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:215: file_factory_.reset(new
content::DownloadFileFactory());
On 2012/08/22 19:07:31, benjhayden_chromium wrote:
> Why isn't this in the init list?

My initial response was going to be "because no one would use it" (the only
current use of this is from test_error_file_injector.cc, which doesn't construct
a DMI) but I realized that the DMI unit test *should* use it, so I added that. 
Done.

[benjhayden, 2012-08-23 20:01:42]

lgtm

[commit-bot: I haz the power, 2012-08-24 13:43:30]

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

[commit-bot: I haz the power, 2012-08-24 16:06:14]

Change committed as 153221