Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(429)

Unified Diff: content/browser/download/download_manager_impl.cc

Issue 10915180: Make DownloadHistory observe manager, items (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: . Created 8 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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
benjhayden 2012/09/21 20:45:46 Can I let you fix this when you put DownloadItem i
Randy Smith (Not in Mondays) 2012/09/24 18:03:25 Is there a reason why you can't just check to see
benjhayden 2012/11/02 17:21:36 This method moved into DII. I added comments there
Randy Smith (Not in Mondays) 2012/11/02 23:31:24 I'm ok with putting in a new state in another CL,
benjhayden 2012/11/06 20:01:13 I seem to remember that there is a rare case where
+ 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);
}

Powered by Google App Engine
This is Rietveld 408576698