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 c1e983e7d4e310ce3406ac075ecb612c23c9110e..605a58c1353677f13570cef8ee49e2ad7b33d09c 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -52,26 +52,26 @@ using content::WebContents;
 namespace {
 
 // This is just used to remember which DownloadItems come from SavePage.
-class SavePageExternalData : public DownloadItem::ExternalData {
+class SavePageData : public base::SupportsUserData::Data {
  public:
   // A spoonful of syntactic sugar.
   static bool Get(DownloadItem* item) {
-    return item->GetExternalData(kKey) != NULL;
+    return item->GetUserData(kKey) != NULL;
   }
 
-  explicit SavePageExternalData(DownloadItem* item) {
-    item->SetExternalData(kKey, this);
+  explicit SavePageData(DownloadItem* item) {
+    item->SetUserData(kKey, this);
   }
 
-  virtual ~SavePageExternalData() {}
+  virtual ~SavePageData() {}
 
  private:
   static const char kKey[];
 
-  DISALLOW_COPY_AND_ASSIGN(SavePageExternalData);
+  DISALLOW_COPY_AND_ASSIGN(SavePageData);
 };
 
-const char SavePageExternalData::kKey[] = "DownloadItem SavePageExternalData";
+const char SavePageData::kKey[] = "DownloadItem SavePageData";
 
 void BeginDownload(content::DownloadUrlParameters* params) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
@@ -312,8 +312,6 @@ void DownloadManagerImpl::Shutdown() {
       download->Delete(DownloadItem::DELETE_DUE_TO_BROWSER_SHUTDOWN);
     } else if (download->IsPartialDownload()) {
       download->Cancel(false);
-      if (delegate_)
-        delegate_->UpdateItemInPersistentStore(download);
     }
   }
 
@@ -321,13 +319,9 @@ void DownloadManagerImpl::Shutdown() {
   // and all in progress downloads have been cancelled.  We can now delete
   // anything left.
 
-  // Copy downloads_ to separate container so as not to set off checks
-  // in DownloadItem destruction.
-  DownloadMap downloads_to_delete;
-  downloads_to_delete.swap(downloads_);
-
   active_downloads_.clear();
-  STLDeleteValues(&downloads_to_delete);
+  STLDeleteValues(&downloads_);
+  downloads_.clear();
 
   // We'll have nothing more to report to the observers after this point.
   observers_.Clear();
@@ -345,9 +339,7 @@ void DownloadManagerImpl::GetTemporaryDownloads(
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
     DownloadItemImpl* item = it->second;
-    // TODO(benjhayden): Don't check IsPersisted().
     if (item->IsTemporary() &&
-        item->IsPersisted() &&
         (dir_path.empty() ||
          item->GetTargetFilePath().DirName() == dir_path))
       result->push_back(item);
@@ -361,9 +353,7 @@ void DownloadManagerImpl::GetAllDownloads(
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
     DownloadItemImpl* item = it->second;
-    // TODO(benjhayden): Don't check IsPersisted().
     if (!item->IsTemporary() &&
-        item->IsPersisted() &&
         (dir_path.empty() ||
          item->GetTargetFilePath().DirName() == dir_path))
       result->push_back(item);
@@ -380,9 +370,7 @@ void DownloadManagerImpl::SearchDownloads(const string16& query,
     // Display Incognito downloads only in Incognito window, and vice versa.
     // The Incognito Downloads page will get the list of non-Incognito downloads
     // from its parent profile.
-    // TODO(benjhayden): Don't check IsPersisted().
     if (!download_item->IsTemporary() &&
-        download_item->IsPersisted() &&
         (browser_context_->IsOffTheRecord() == download_item->IsOtr()) &&
         download_item->MatchesQuery(query_lower)) {
       result->push_back(download_item);
@@ -468,13 +456,20 @@ 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();
+  if (!SavePageData::Get(item)) {
+    MaybeCompleteDownload(item);
   }
 }
 
@@ -483,8 +478,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);
   }
 }
 
@@ -553,7 +547,7 @@ 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,
@@ -561,24 +555,16 @@ DownloadItemImpl* DownloadManagerImpl::CreateSavePackageDownloadItem(
       GetNextId(),
       mime_type,
       bound_net_log);
-
-  download->AddObserver(observer);
-
-  DCHECK(!ContainsKey(downloads_, download->GetId()));
-  downloads_[download->GetId()] = download;
-  DCHECK(!SavePageExternalData::Get(download));
-  new SavePageExternalData(download);
-  DCHECK(SavePageExternalData::Get(download));
-
-  // TODO(benjhayden): Fire OnDownloadCreated for SavePackage downloads when
-  // we're comfortable with the user interacting with them.
-  // 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));
+  // TODO(benjhayden) XXX occam test showing SavePackage DownloadItems in ui
+  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(
+      this, download_item));
+  return download_item;
 }
 
 void DownloadManagerImpl::UpdateDownload(int32 download_id,
@@ -591,8 +577,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);
     }
   }
 }
@@ -617,23 +601,10 @@ void DownloadManagerImpl::OnResponseCompleted(int32 download_id,
 
 void DownloadManagerImpl::AssertStateConsistent(
     DownloadItemImpl* download) const {
-  if (download->GetState() == DownloadItem::REMOVING) {
-    DCHECK(!ContainsKey(downloads_, download->GetId()));
-    DCHECK(!ContainsKey(active_downloads_, download->GetId()));
-    return;
-  }
-
-  // Should be in downloads_ if we're not REMOVING.
   CHECK(ContainsKey(downloads_, download->GetId()));
 
   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()));
 }
@@ -655,12 +626,6 @@ 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())
-    return false;
-
   return true;
 }
 
@@ -692,7 +657,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
@@ -707,8 +671,6 @@ void DownloadManagerImpl::MaybeCompleteDownload(
   VLOG(20) << __FUNCTION__ << "()" << " executing: download = "
            << download->DebugString(false);
 
-  if (delegate_)
-    delegate_->UpdateItemInPersistentStore(download);
   download->OnDownloadCompleting();
 }
 
@@ -720,8 +682,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);
 }
@@ -761,14 +721,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());
 }
 
 bool DownloadManagerImpl::GenerateFileHash() {
@@ -782,20 +735,16 @@ int DownloadManagerImpl::RemoveDownloadItems(
 
   // Delete from internal maps.
   for (DownloadItemImplVector::const_iterator it = pending_deletes.begin();
-      it != pending_deletes.end();
-      ++it) {
+       it != pending_deletes.end();
+       ++it) {
     DownloadItemImpl* download = *it;
     DCHECK(download);
-    downloads_.erase(download->GetId());
+    int32 download_id = download->GetId();
+    delete download;
+    downloads_.erase(download_id);
   }
-
-  // Tell observers to refresh their views.
   NotifyModelChanged();
-
-  // Delete the download items themselves.
-  const int num_deleted = static_cast<int>(pending_deletes.size());
-  STLDeleteContainerPointers(pending_deletes.begin(), pending_deletes.end());
-  return num_deleted;
+  return static_cast<int>(pending_deletes.size());
 }
 
 void DownloadManagerImpl::DownloadRemoved(DownloadItemImpl* download) {
@@ -803,14 +752,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));
@@ -819,21 +760,16 @@ void DownloadManagerImpl::DownloadRemoved(DownloadItemImpl* download) {
 
 int DownloadManagerImpl::RemoveDownloadsBetween(base::Time remove_begin,
                                                 base::Time remove_end) {
-  if (delegate_)
-    delegate_->RemoveItemsFromPersistentStoreBetween(remove_begin, remove_end);
-
-  // All downloads visible to the user will be in the history,
-  // so scan that map.
   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);
+      download->NotifyRemoved();
       pending_deletes.push_back(download);
     }
   }
@@ -882,9 +818,6 @@ 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(
@@ -899,73 +832,6 @@ void DownloadManagerImpl::OnPersistentStoreQueryComplete(
   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.
-  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 (SavePageExternalData::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();
-  }
-}
-
 void DownloadManagerImpl::ShowDownloadInBrowser(DownloadItemImpl* download) {
   // The 'contents' may no longer exist if the user closed the contents before
   // we get this start completion event.
@@ -998,9 +864,9 @@ void DownloadManagerImpl::NotifyModelChanged() {
   FOR_EACH_OBSERVER(Observer, observers_, ModelChanged(this));
 }
 
+// TODO(benjhayden): Kill this method premailing XXX occam
 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) {
@@ -1047,45 +913,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) {
@@ -1097,28 +934,18 @@ void DownloadManagerImpl::DownloadOpened(DownloadItemImpl* download) {
   download_stats::RecordOpensOutstanding(num_unopened);
 }
 
+// TODO(benjhayden) this should be a method on DI
 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);
-  }
+  download->UpdateObservers();
 }
 
+// TODO(benjhayden) this should be a method on DI
 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();
 }