Make DownloadHistory observe manager, items

content/browser/download/download_manager_impl.cc

Index: content/browser/download/download_manager_impl.cc
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 315d6439e2f7da52743977e550c6213437a49228..7f09eaaa3451f54e3854f38e56c9f5723e5a3d0b 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -32,7 +32,6 @@
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_manager_delegate.h"
-#include "content/public/browser/download_persistent_store_info.h"
 #include "content/public/browser/download_url_parameters.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
@@ -47,7 +46,6 @@
 using content::BrowserThread;
 using content::DownloadId;
 using content::DownloadItem;
-using content::DownloadPersistentStoreInfo;
 using content::ResourceDispatcherHostImpl;
 using content::WebContents;
 
@@ -169,9 +167,29 @@ class DownloadItemFactoryImpl : public content::DownloadItemFactory {
     virtual DownloadItemImpl* CreatePersistedItem(
         DownloadItemImplDelegate* delegate,
         content::DownloadId download_id,
-        const content::DownloadPersistentStoreInfo& info,
+        const FilePath& path,
+        const GURL& url,
+        const GURL& referrer_url,
+        const base::Time& start_time,
+        const base::Time& end_time,
+        int64 received_bytes,
+        int64 total_bytes,
+        DownloadItem::DownloadState state,
+        bool opened,
         const net::BoundNetLog& bound_net_log) OVERRIDE {
-      return new DownloadItemImpl(delegate, download_id, info, bound_net_log);
+      return new DownloadItemImpl(
+          delegate,
+          download_id,
+          path,
+          url,
+          referrer_url,
+          start_time,
+          end_time,
+          received_bytes,
+          total_bytes,
+          state,
+          opened,
+          bound_net_log);
     }
 
     virtual DownloadItemImpl* CreateActiveItem(
@@ -311,8 +329,6 @@ void DownloadManagerImpl::Shutdown() {
       download->Delete(DownloadItem::DELETE_DUE_TO_BROWSER_SHUTDOWN);
     } else if (download->IsPartialDownload()) {
       download->Cancel(false);
-      if (delegate_)
-        delegate_->UpdateItemInPersistentStore(download);
     }
   }
 
@@ -340,9 +356,7 @@ void DownloadManagerImpl::SearchDownloads(const string16& query,
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
     DownloadItemImpl* download_item = it->second;
-    // TODO(benjhayden): Don't check IsPersisted().
     if (!download_item->IsTemporary() &&
-        download_item->IsPersisted() &&
         download_item->MatchesQuery(query_lower)) {
       result->push_back(download_item);
     }
@@ -426,14 +440,18 @@ void DownloadManagerImpl::OnDownloadTargetDetermined(
     content::DownloadDangerType danger_type,
     const FilePath& intermediate_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadMap::iterator download_iter = active_downloads_.find(download_id);
-  if (download_iter != active_downloads_.end()) {
-    // Once DownloadItem::OnDownloadTargetDetermined() is called, we expect a
-    // DownloadRenamedToIntermediateName() callback. This is necessary for the
-    // download to proceed.
-    download_iter->second->OnDownloadTargetDetermined(
-        target_path, disposition, danger_type, intermediate_path);
-  }
+  DownloadMap::iterator download_iter = downloads_.find(download_id);
+  if (download_iter == downloads_.end())
+    return;
+
+  // Once DownloadItem::OnDownloadTargetDetermined() is called, we expect a
+  // DownloadRenamedToIntermediateName() callback. This is necessary for the
+  // download to proceed.
+  DownloadItemImpl* item = download_iter->second;
+  item->OnDownloadTargetDetermined(
+      target_path, disposition, danger_type, intermediate_path);
+  ShowDownloadInBrowser(item);
+  NotifyModelChanged();
 }
 
 void DownloadManagerImpl::CheckForHistoryFilesRemoval() {
@@ -441,8 +459,7 @@ void DownloadManagerImpl::CheckForHistoryFilesRemoval() {
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
     DownloadItemImpl* item = it->second;
-    if (item->IsPersisted())
-      CheckForFileRemoval(item);
+    CheckForFileRemoval(item);
   }
 }
 
@@ -510,29 +527,28 @@ DownloadItemImpl* DownloadManagerImpl::CreateSavePackageDownloadItem(
     DownloadItem::Observer* observer) {
   net::BoundNetLog bound_net_log =
       net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
-  DownloadItemImpl* download = factory_->CreateSavePageItem(
+  DownloadItemImpl* download_item = factory_->CreateSavePageItem(
       this,
       main_file_path,
       page_url,
       GetNextId(),
       mime_type,
       bound_net_log);
-
-  download->AddObserver(observer);
-
-  DCHECK(!ContainsKey(downloads_, download->GetId()));
-  downloads_[download->GetId()] = download;
-  DCHECK(!SavePageData::Get(download));
-  new SavePageData(download);
-  DCHECK(SavePageData::Get(download));
-
-  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
-
-  // Will notify the observer in the callback.
-  if (delegate_)
-    delegate_->AddItemToPersistentStore(download);
-
-  return download;
+  download_item->AddObserver(observer);
+  DCHECK(!ContainsKey(downloads_, download_item->GetId()));
+  downloads_[download_item->GetId()] = download_item;
+  DCHECK(!SavePageData::Get(download_item));
+  new SavePageData(download_item);
+  DCHECK(SavePageData::Get(download_item));
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(
+      this, download_item));
+  OnDownloadTargetDetermined(
+      download_item->GetId(),
+      main_file_path,
+      DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+      content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+      main_file_path);
+  return download_item;
 }
 
 void DownloadManagerImpl::UpdateDownload(int32 download_id,
@@ -545,8 +561,6 @@ void DownloadManagerImpl::UpdateDownload(int32 download_id,
     DownloadItemImpl* download = it->second;
     if (download->IsInProgress()) {
       download->UpdateProgress(bytes_so_far, bytes_per_sec, hash_state);
-      if (delegate_)
-        delegate_->UpdateItemInPersistentStore(download);
     }
   }
 }
@@ -575,12 +589,6 @@ void DownloadManagerImpl::AssertStateConsistent(
 
   int64 state = download->GetState();
   base::debug::Alias(&state);
-  if (ContainsKey(active_downloads_, download->GetId())) {
-    if (download->IsPersisted())
-      CHECK_EQ(DownloadItem::IN_PROGRESS, download->GetState());
-    if (DownloadItem::IN_PROGRESS != download->GetState())
-      CHECK_EQ(DownloadItem::kUninitializedHandle, download->GetDbHandle());
-  }
   if (DownloadItem::IN_PROGRESS == download->GetState())
     CHECK(ContainsKey(active_downloads_, download->GetId()));
 }
@@ -602,10 +610,14 @@ bool DownloadManagerImpl::IsDownloadReadyForCompletion(
   if (active_downloads_.count(download->GetId()) == 0)
     return false;
 
-  // If the download hasn't been inserted into the history system
-  // (which occurs strictly after file name determination, intermediate
-  // file rename, and UI display) then it's not ready for completion.
-  if (!download->IsPersisted())
+  // If the intermediate filename rename hasn't occurred, don't complete the
+  // download.
+  if (download->GetTargetFilePath().DirName() !=
+      download->GetFullPath().DirName())
+    return false;
+
+  // If the target filename hasn't been determined, don't complete the download.

[Randy Smith (Not in Mondays), 2012/09/13 19:53:19]

So this is an example of the issue we were talking about yesterday.  If we can't
have a simple test as to whether we're at the right place in the state diagram,
I want to re-visit the question of whether we have the right/enough states. 
What I said yesterday is "If we can't just test the target filename, revisit",
but I think I'm going to back off from that and say that this logic is
(effectively) on the inside of the download item, so we can rely on inside
knowledge of what's going on.  In which case, just testing the FullPath for null
should work.  Do you agree?

(I'll note that this suggestion makes me a little ootsy.  The question we
*really* want to ask here is "Where are we in the main download cascade?" and
we're using the full file path nullity as a proxy for that.  I believe that it's
true that the full file path will never be non null when we haven't gotten to
this place in the download cascade, but it's not quite as direct a question as,
well, say, checking the state variable :-J.)

[benjhayden, 2012/09/21 20:45:46]

Can I let you fix this when you put DownloadItem in the driver's seat?

[Randy Smith (Not in Mondays), 2012/09/24 18:03:25]

Is there a reason why you can't just check to see if GetFullPath() is non-empty?

(Very welcome to move into DI and make a new state instead.)

[benjhayden, 2012/11/02 17:21:36]

This method moved into DII. I added comments there about why these are the
necessary conditions, but for convenience,
(!GetTargetFilePath().empty()) is checked in ReadyForDownloadCompletionDone()
(target_path_.DirName() == current_path_.DirName()) is checked in NeedsRename(),
called from ReadyForDownloadCompletionDone.

I would not object to replacing these conditions with a new state, but that
seems to me like a job for another CL. Push back if you feel strongly.

[Randy Smith (Not in Mondays), 2012/11/02 23:31:24]

I'm ok with putting in a new state in another CL, but I'm still confused about
why we need the check for target_path_.DirName() == current_path_.DirName().  I
believe that if current_path_ is non-NULL, it will have the same directory as
the target path, so we can just make that a DCHECK.  

To my mind, the question we're really asking here is "where are we in the
DownloadItem cascade?".  The "right" way to do that is via a state--ok, not in
this CL.  But having two tests implies that the two conditions race with each
other, as opposed to one always and forever occurring after the other and us
testing on the second one.  Do you know of a reason why we can't replace those
two tests with !target_path.empty()?

[benjhayden, 2012/11/06 20:01:13]

I seem to remember that there is a rare case where this can happen, but I don't
remember exactly where. I took out this condition and no tests failed, so I'll
leave it out until we find a case where it might happen.

Asanka, do you happen to know when !target_path_.empty() &&
target_path.DirName() != current_path_.DirName() if ever?

+  if (download->GetTargetName().empty())
     return false;
 
   return true;
@@ -639,7 +651,6 @@ void DownloadManagerImpl::MaybeCompleteDownload(
   DCHECK(download->IsInProgress());
   DCHECK_NE(DownloadItem::DANGEROUS, download->GetSafetyState());
   DCHECK(download->AllDataSaved());
-  DCHECK(download->IsPersisted());
 
   // Give the delegate a chance to override.  It's ok to keep re-setting the
   // delegate's |complete_callback| cb as long as there isn't another call-point
@@ -654,8 +665,6 @@ void DownloadManagerImpl::MaybeCompleteDownload(
   VLOG(20) << __FUNCTION__ << "()" << " executing: download = "
            << download->DebugString(false);
 
-  if (delegate_)
-    delegate_->UpdateItemInPersistentStore(download);
   download->OnDownloadCompleting();
 }
 
@@ -667,8 +676,6 @@ void DownloadManagerImpl::MaybeCompleteDownloadById(int download_id) {
 void DownloadManagerImpl::DownloadCompleted(DownloadItemImpl* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(download);
-  if (delegate_)
-    delegate_->UpdateItemInPersistentStore(download);
   active_downloads_.erase(download->GetId());
   AssertStateConsistent(download);
 }
@@ -708,14 +715,7 @@ void DownloadManagerImpl::OnDownloadInterrupted(
 void DownloadManagerImpl::RemoveFromActiveList(DownloadItemImpl* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(download);
-
-  // Clean up will happen when the history system create callback runs if we
-  // don't have a valid db_handle yet.
-  if (download->IsPersisted()) {
-    active_downloads_.erase(download->GetId());
-    if (delegate_)
-      delegate_->UpdateItemInPersistentStore(download);
-  }
+  active_downloads_.erase(download->GetId());
 }
 
 int DownloadManagerImpl::RemoveDownloadItems(
@@ -742,14 +742,6 @@ void DownloadManagerImpl::DownloadRemoved(DownloadItemImpl* download) {
       downloads_.find(download->GetId()) == downloads_.end())
     return;
 
-  // TODO(benjhayden,rdsmith): Remove this.
-  if (!download->IsPersisted())
-    return;
-
-  // Make history update.
-  if (delegate_)
-    delegate_->RemoveItemFromPersistentStore(download);
-
   // Remove from our tables and delete.
   int downloads_count =
       RemoveDownloadItems(DownloadItemImplVector(1, download));
@@ -758,16 +750,12 @@ void DownloadManagerImpl::DownloadRemoved(DownloadItemImpl* download) {
 
 int DownloadManagerImpl::RemoveDownloadsBetween(base::Time remove_begin,
                                                 base::Time remove_end) {
-  if (delegate_)
-    delegate_->RemoveItemsFromPersistentStoreBetween(remove_begin, remove_end);
-
   DownloadItemImplVector pending_deletes;
   for (DownloadMap::const_iterator it = downloads_.begin();
       it != downloads_.end();
       ++it) {
     DownloadItemImpl* download = it->second;
-    if (download->IsPersisted() &&
-        download->GetStartTime() >= remove_begin &&
+    if (download->GetStartTime() >= remove_begin &&
         (remove_end.is_null() || download->GetStartTime() < remove_end) &&
         (download->IsComplete() || download->IsCancelled())) {
       AssertStateConsistent(download);
@@ -814,94 +802,35 @@ void DownloadManagerImpl::RemoveObserver(Observer* observer) {
 
 // Operations posted to us from the history service ----------------------------
 
-// The history service has retrieved all download entries. 'entries' contains
-// 'DownloadPersistentStoreInfo's in sorted order (by ascending start_time).
-void DownloadManagerImpl::OnPersistentStoreQueryComplete(
-    std::vector<DownloadPersistentStoreInfo>* entries) {
-  history_size_ = entries->size();
-  for (size_t i = 0; i < entries->size(); ++i) {
-    int64 db_handle = entries->at(i).db_handle;
-    base::debug::Alias(&db_handle);
-
-    net::BoundNetLog bound_net_log =
-        net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
-    DownloadItemImpl* download = factory_->CreatePersistedItem(
-        this, GetNextId(), entries->at(i), bound_net_log);
-    DCHECK(!ContainsKey(downloads_, download->GetId()));
-    downloads_[download->GetId()] = download;
-    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
-    VLOG(20) << __FUNCTION__ << "()" << i << ">"
-             << " download = " << download->DebugString(true);
-  }
-  NotifyModelChanged();
-  CheckForHistoryFilesRemoval();
-}
-
-void DownloadManagerImpl::AddDownloadItemToHistory(DownloadItemImpl* download,
-                                                   int64 db_handle) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK_NE(DownloadItem::kUninitializedHandle, db_handle);
-  DCHECK(!download->IsPersisted());
-  download->SetDbHandle(db_handle);
-  download->SetIsPersisted();
-
-  download_stats::RecordHistorySize(history_size_);
-  // Not counting |download|.
-  ++history_size_;
-
-  // Show in the appropriate browser UI.
-  // This includes buttons to save or cancel, for a dangerous download.
-  ShowDownloadInBrowser(download);
-
-  // Inform interested objects about the new download.
+DownloadItem* DownloadManagerImpl::CreateDownloadItem(
+    const FilePath& path,
+    const GURL& url,
+    const GURL& referrer_url,
+    const base::Time& start_time,
+    const base::Time& end_time,
+    int64 received_bytes,
+    int64 total_bytes,
+    DownloadItem::DownloadState state,
+    bool opened) {
+  DownloadItemImpl* item = factory_->CreatePersistedItem(
+      this,
+      GetNextId(),
+      path,
+      url,
+      referrer_url,
+      start_time,
+      end_time,
+      received_bytes,
+      total_bytes,
+      state,
+      opened,
+      net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD));
+  DCHECK(!ContainsKey(downloads_, item->GetId()));
+  downloads_[item->GetId()] = item;
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, item));
+  VLOG(20) << __FUNCTION__ << "() download = " << item->DebugString(true);
   NotifyModelChanged();
-}
-
-
-void DownloadManagerImpl::OnItemAddedToPersistentStore(int32 download_id,
-                                                       int64 db_handle) {
-  // It's valid that we don't find a matching item, i.e. on shutdown.
-  if (!ContainsKey(downloads_, download_id))
-    return;
-
-  DownloadItemImpl* item = downloads_[download_id];
-  AddDownloadItemToHistory(item, db_handle);
-  if (SavePageData::Get(item)) {
-    OnSavePageItemAddedToPersistentStore(item);
-  } else {
-    OnDownloadItemAddedToPersistentStore(item);
-  }
-}
-
-// Once the new DownloadItem has been committed to the persistent store,
-// associate it with its db_handle (TODO(benjhayden) merge db_handle with id),
-// show it in the browser (TODO(benjhayden) the ui should observe us instead),
-// and notify observers (TODO(benjhayden) observers should be able to see the
-// item when it's created so they can observe it directly. Are there any
-// clients that actually need to know when the item is added to the history?).
-void DownloadManagerImpl::OnDownloadItemAddedToPersistentStore(
-    DownloadItemImpl* item) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  VLOG(20) << __FUNCTION__ << "()" << " db_handle = " << item->GetDbHandle()
-           << " download_id = " << item->GetId()
-           << " download = " << item->DebugString(true);
-
-  // If the download is still in progress, try to complete it.
-  //
-  // Otherwise, download has been cancelled or interrupted before we've
-  // received the DB handle.  We post one final message to the history
-  // service so that it can be properly in sync with the DownloadItem's
-  // completion status, and also inform any observers so that they get
-  // more than just the start notification.
-  if (item->IsInProgress()) {
-    MaybeCompleteDownload(item);
-  } else {
-    DCHECK(item->IsCancelled());
-    active_downloads_.erase(item->GetId());
-    if (delegate_)
-      delegate_->UpdateItemInPersistentStore(item);
-    item->UpdateObservers();
-  }
+  return item;
 }
 
 void DownloadManagerImpl::ShowDownloadInBrowser(DownloadItemImpl* download) {
@@ -937,8 +866,7 @@ void DownloadManagerImpl::NotifyModelChanged() {
 }
 
 DownloadItem* DownloadManagerImpl::GetDownloadItem(int download_id) {
-  DownloadItem* download = GetDownload(download_id);
-  return (download && download->IsPersisted()) ? download : NULL;
+  return GetDownload(download_id);
 }
 
 DownloadItem* DownloadManagerImpl::GetDownload(int download_id) {
@@ -992,45 +920,16 @@ void DownloadManagerImpl::AssertContainersConsistent() const {
 #endif
 }
 
-// SavePackage will call SavePageDownloadFinished upon completion/cancellation.
-// The history callback will call OnSavePageItemAddedToPersistentStore.
-// If the download finishes before the history callback,
-// OnSavePageItemAddedToPersistentStore calls SavePageDownloadFinished, ensuring
-// that the history event is update regardless of the order in which these two
-// events complete.
-// If something removes the download item from the download manager (Remove,
-// Shutdown) the result will be that the SavePage system will not be able to
-// properly update the download item (which no longer exists) or the download
-// history, but the action will complete properly anyway.  This may lead to the
-// history entry being wrong on a reload of chrome (specifically in the case of
-// Initiation -> History Callback -> Removal -> Completion), but there's no way
-// to solve that without canceling on Remove (which would then update the DB).
-
-void DownloadManagerImpl::OnSavePageItemAddedToPersistentStore(
-    DownloadItemImpl* item) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // Finalize this download if it finished before the history callback.
-  if (!item->IsInProgress())
-    SavePageDownloadFinished(item);
-}
-
 void DownloadManagerImpl::SavePageDownloadFinished(
     content::DownloadItem* download) {
-  if (download->IsPersisted()) {
-    if (delegate_)
-      delegate_->UpdateItemInPersistentStore(download);
-    if (download->IsComplete())
-      content::NotificationService::current()->Notify(
-          content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
-          content::Source<DownloadManager>(this),
-          content::Details<DownloadItem>(download));
-  }
+  if (download->IsComplete())
+    content::NotificationService::current()->Notify(
+        content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
+        content::Source<DownloadManager>(this),
+        content::Details<DownloadItem>(download));
 }
 
 void DownloadManagerImpl::DownloadOpened(DownloadItemImpl* download) {
-  if (delegate_)
-    delegate_->UpdateItemInPersistentStore(download);
   int num_unopened = 0;
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
@@ -1042,28 +941,10 @@ void DownloadManagerImpl::DownloadOpened(DownloadItemImpl* download) {
   download_stats::RecordOpensOutstanding(num_unopened);
 }
 
+// TODO(benjhayden) MaybeCompleteDownload should be moved into DownloadItemImpl.
 void DownloadManagerImpl::DownloadRenamedToIntermediateName(
     DownloadItemImpl* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // download->GetFullPath() is only expected to be meaningful after this
-  // callback is received. Therefore we can now add the download to a persistent
-  // store. If the rename failed, we receive an OnDownloadInterrupted() call
-  // before we receive the DownloadRenamedToIntermediateName() call.
-  if (delegate_) {
-    delegate_->AddItemToPersistentStore(download);
-  } else {
-    OnItemAddedToPersistentStore(download->GetId(),
-                                 DownloadItem::kUninitializedHandle);
-  }
-}
-
-void DownloadManagerImpl::DownloadRenamedToFinalName(
-    DownloadItemImpl* download) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // If the rename failed, we receive an OnDownloadInterrupted() call before we
-  // receive the DownloadRenamedToFinalName() call.
-  if (delegate_) {
-    delegate_->UpdatePathForItemInPersistentStore(
-        download, download->GetFullPath());
-  }
+  download->UpdateObservers();
+  MaybeCompleteDownload(download);
 }