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, 
	* http://codereview.chromium.org/10836293,
	* https://chromiumcodereview.appspot.com/10823406, and
	* http://codereview.chromium.org/10867065

which were previous attempts to land the same functionality. 

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

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

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

Ben, PTAL.  I'd love to think hard about race conditions here (so as to minimize
the number of future re-lands) so see if you can come up with some new ones. 
Feel free to grab me if you want brainstorm help (I've done what I can, but two
people brainstorming are better than one).

[Randy Smith (Not in Mondays), 2012-08-29 20:39:15]

Ben and I talked offline; this is the result of our conversation:

+ There are two things we need from the download item:
    a) It to have provoked the download shelf to open.
    b) It to have flushed the final history update out to the DB.
  Before testing if the shelf is present or if the DB is
  correct, we need to make sure that (if the test is proceding
  properly) those events have occurred.
   
+ Download shelf present:
  ShowDownloadInBrowser
    AddDownloadItemToHistory
      OnItemAddedToPersistentStore
  -> If the item has been persisted, the shelf should be
  visible.
   
+ To see whether the item has been persisted, we need an UpdateObservers() to be
called after SetIsPersisted is called.  Currently there is no UpdateObservers()
called in the context of SetIsPersisted being called.
   
+ Database updated.  (Note that we don't actually need the DB to have actually
been updated, we need the final message that updates the DB to have been sent to
the DB thread; any queries that follow that will be behind that update, and
hence will see that result.)

  For this we need an UpdateItemInPersistentStore() *after* the final
UpdateProgress.  If the download was persisted before the final UpdateProgress,
this will happen as part of the same UI task as calls the final UpdateProgress
(see SavePackage::Finish -> DownloadItem::UpdateProgress,
SavePackage::FinalizeDownloadEntry -> DownloadManager::SavePageDownloadFinished.
 
   
+ SavePackage marks the download as complete when SavePackage::Finish is called.
	  
+ There is a completion/persistence race; SavePackage will complete the download
when it is done, but the download persistence will occur at the whim of the DB
thread.
   
Conclusion: If the SavePackage has finished (-> the download is complete), and
the download is persisted, all state is in a position where it can be verified. 
However, because persistence doesn't notify observers, waiting on a download to
be persisted may wait forever.

Actions:
+ Make SetIsPersisted() update download observers.
+ Make sure to wait for the download to be persisted.

[benjhayden, 2012-08-29 21:00:22]

Yep, making DI::SetIsPersisted() update observers should handle both cases
in DownloadManagerImpl::SavePageDownloadFinished()
and DownloadManagerImpl::OnSavePageItemAddedToPersistentStore() where the
history calls back before or after completion.
It could also tickle latent races in unrelated tests that rely on different
OnDownloadUpdated behavior, but we'll fix those if/when they make their
presence known.


On Wed, Aug 29, 2012 at 4:39 PM, <rdsmith@chromium.org> wrote:

> Ben and I talked offline; this is the result of our conversation:
>
> + There are two things we need from the download item:
>     a) It to have provoked the download shelf to open.
>     b) It to have flushed the final history update out to the DB.
>   Before testing if the shelf is present or if the DB is
>   correct, we need to make sure that (if the test is proceding
>   properly) those events have occurred.
>
> + Download shelf present:
>   ShowDownloadInBrowser
>     AddDownloadItemToHistory
>       OnItemAddedToPersistentStore
>   -> If the item has been persisted, the shelf should be
>   visible.
>
> + To see whether the item has been persisted, we need an UpdateObservers()
> to be
> called after SetIsPersisted is called.  Currently there is no
> UpdateObservers()
> called in the context of SetIsPersisted being called.
>
> + Database updated.  (Note that we don't actually need the DB to have
> actually
> been updated, we need the final message that updates the DB to have been
> sent to
> the DB thread; any queries that follow that will be behind that update, and
> hence will see that result.)
>
>   For this we need an UpdateItemInPersistentStore() *after* the final
> UpdateProgress.  If the download was persisted before the final
> UpdateProgress,
> this will happen as part of the same UI task as calls the final
> UpdateProgress
> (see SavePackage::Finish -> DownloadItem::UpdateProgress,
> SavePackage::**FinalizeDownloadEntry -> DownloadManager::**
> SavePageDownloadFinished.
>
>
> + SavePackage marks the download as complete when SavePackage::Finish is
> called.
>
> + There is a completion/persistence race; SavePackage will complete the
> download
> when it is done, but the download persistence will occur at the whim of
> the DB
> thread.
>
> Conclusion: If the SavePackage has finished (-> the download is complete),
> and
> the download is persisted, all state is in a position where it can be
> verified.
> However, because persistence doesn't notify observers, waiting on a
> download to
> be persisted may wait forever.
>
> Actions:
> + Make SetIsPersisted() update download observers.
> + Make sure to wait for the download to be persisted.
>
>
>
>
http://codereview.chromium.**org/10900010/<http://codereview.chromium.org/109...
>

[Randy Smith (Not in Mondays), 2012-08-30 00:13:07]

Ben: PTAL at the main body of the patch.

craigdh@: Could you review the downloads.py change?

[craigdh, 2012-08-30 00:41:55]

On 2012/08/30 00:13:07, rdsmith wrote:
> Ben: PTAL at the main body of the patch.
> 
> craigdh@: Could you review the downloads.py change?

downloads.py: The change lgtm, but these tests don't look like they should be
PyAuto tests. I'll leave that call to John.

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

On 2012/08/30 00:41:55, craigdh wrote:
> On 2012/08/30 00:13:07, rdsmith wrote:
> > Ben: PTAL at the main body of the patch.
> > 
> > craigdh@: Could you review the downloads.py change?
> 
> downloads.py: The change lgtm, but these tests don't look like they should be
> PyAuto tests. I'll leave that call to John.

They shouldn't be, and Ben's going to work on changing them over.  But that's
not this CL.

[benjhayden, 2012-08-30 17:31:15]

LGTM, one request and one thinker.

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

http://codereview.chromium.org/10900010/diff/12001/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:89: const
std::vector<DownloadItem*>& WaitForDownloadItem() {
Could you return via pointer instead?
void WaitForDownloadItem(DownloadVector* items_seen)

http://codereview.chromium.org/10900010/diff/12001/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:251: // state, we need to wait
until both the save package operation is
Do we really need to wait for the save package finished message, or can we just
wait for the DownloadItem to be completed and persisted?
It would be nice to kill the save package finished notification. It can wait for
another CL if you don't think it would make this CL stickier.

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

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

http://codereview.chromium.org/10900010/diff/12001/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:89: const
std::vector<DownloadItem*>& WaitForDownloadItem() {
On 2012/08/30 17:31:15, benjhayden_chromium wrote:
> Could you return via pointer instead?
> void WaitForDownloadItem(DownloadVector* items_seen)

Yeah, that's probably more in line with existing custom.  Done.

http://codereview.chromium.org/10900010/diff/12001/chrome/browser/download/sa...
chrome/browser/download/save_page_browsertest.cc:251: // state, we need to wait
until both the save package operation is
On 2012/08/30 17:31:15, benjhayden_chromium wrote:
> Do we really need to wait for the save package finished message, or can we
just
> wait for the DownloadItem to be completed and persisted?
> It would be nice to kill the save package finished notification. It can wait
for
> another CL if you don't think it would make this CL stickier.

Huh.  Good point.  I think I'll say that I'd rather not do it in this CL (a
little bit of yak shaving is ok, but I'm drowning in hair here!) but it should
be a fine thing to do in a followon CL.  I'll put it on my queue.

[commit-bot: I haz the power, 2012-08-30 19:49:17]

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

[commit-bot: I haz the power, 2012-08-30 22:42:31]

Change committed as 154294