Make the SavePackage use of the fake download item cleaner around cancellation.

Make the SavePackage use of the fake download item cleaner around cancellation.

This involves making sure not to touch the download item after it's canceled, 
moving the notification of SavePackage complete out of DownloadManagerImpl 
(so it's not dependent on persistence race completion), and updating the
SavePageBrowserTest to wait for persistence as well as completion before
testing history. 

R=benjhayden@chromium.org


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

[Randy Smith (Not in Mondays), 2012-09-09 23:56:37]

Ben: PTAL?  You've seen this before, it's just the SavePackage cleanups pulled
out of the DownloadFileManager nuking CL.

[benjhayden, 2012-09-10 14:39:58]

Trybots? Run these tests 100 times locally?

http://codereview.chromium.org/10916201/diff/1/chrome/browser/download/save_p...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10916201/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:242: DCHECK_EQ(1u,
items.size());
CHECK instead of DCHECK?

http://codereview.chromium.org/10916201/diff/1/content/browser/download/save_...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10916201/diff/1/content/browser/download/save_...
content/browser/download/save_package.cc:1389:
content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
Did we talk about not using this? I thought we had figured out that we didn't
really need this.

[Randy Smith (Not in Mondays), 2012-09-10 17:53:16]

> Trybots? 

Sorry, not sure how I missed those.  In progress.

> Run these tests 100 times locally?

Originally did 30x, but happy to do 100x.  In progress.

http://codereview.chromium.org/10916201/diff/1/chrome/browser/download/save_p...
File chrome/browser/download/save_page_browsertest.cc (right):

http://codereview.chromium.org/10916201/diff/1/chrome/browser/download/save_p...
chrome/browser/download/save_page_browsertest.cc:242: DCHECK_EQ(1u,
items.size());
On 2012/09/10 14:39:58, benjhayden_chromium wrote:
> CHECK instead of DCHECK?

I think of both CHECKs and DCHECKs as being discouraged during tests (something
went wrong, why not just fail the test?) so I rewrote this to actually do the
"right thing" and return a boolean, and assert on it in the calling test.

http://codereview.chromium.org/10916201/diff/1/content/browser/download/save_...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10916201/diff/1/content/browser/download/save_...
content/browser/download/save_package.cc:1389:
content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
On 2012/09/10 14:39:58, benjhayden_chromium wrote:
> Did we talk about not using this? I thought we had figured out that we didn't
> really need this.

Looking at the code, I'd rather not make the change:
* There are locations using the notification outside of save_page_browsertest.cc
(automation providers, browser_encoding_browsertest).
* There isn't an easy way to get the DownloadItem associated with a save page;
you have to rely on nothing else going on so it's the only one showing up on the
download manager.  (And two of the uses *in* save_page_browsertest.cc don't
currently do that.)
* Making the switch relies for save page termination on code that's known to be
fragile, has in the past been flaky, has TODOs all over it, and is scheduled to
be nuked soon.

[benjhayden, 2012-09-10 18:45:38]

LGTM when trybots are happy.

[commit-bot: I haz the power, 2012-09-12 17:32:51]

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

[commit-bot: I haz the power, 2012-09-12 20:27:34]

Change committed as 156367