diff options
Diffstat (limited to 'chrome/browser')
| -rw-r--r-- | chrome/browser/download/download_item.cc | 32 | ||||
| -rw-r--r-- | chrome/browser/download/download_item.h | 23 | ||||
| -rw-r--r-- | chrome/browser/download/download_manager.cc | 181 | ||||
| -rw-r--r-- | chrome/browser/download/download_manager.h | 19 | ||||
| -rw-r--r-- | chrome/browser/download/download_manager_unittest.cc | 15 | ||||
| -rw-r--r-- | chrome/browser/download/save_package.cc | 3 |
6 files changed, 167 insertions, 106 deletions
diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc index cb753e8..620828b 100644 --- a/chrome/browser/download/download_item.cc +++ b/chrome/browser/download/download_item.cc @@ -27,6 +27,23 @@ #include "chrome/common/pref_names.h" #include "ui/base/l10n/l10n_util.h" +// A DownloadItem normally goes through the following states: +// * Created (when download starts) +// * Made visible to consumers (e.g. Javascript) after the +// destination file has been determined. +// * Entered into the history database. +// * Made visible in the download shelf. +// * All data is received. Note that the actual data download occurs +// in parallel with the above steps, but until those steps are +// complete, completion of the data download will be ignored. +// * Download file is renamed to its final name, and possibly +// auto-opened. +// TODO(rdsmith): This progress should be reflected in +// DownloadItem::DownloadState and a state transition table/state diagram. +// +// TODO(rdsmith): This description should be updated to reflect the cancel +// pathways. + namespace { // Update frequency (milliseconds). @@ -102,9 +119,12 @@ DownloadItem::DownloadItem(DownloadManager* download_manager, is_extension_install_(info.is_extension_install), name_finalized_(false), is_temporary_(false), + all_data_saved_(false), opened_(false) { if (state_ == IN_PROGRESS) state_ = CANCELLED; + if (state_ == COMPLETE) + all_data_saved_ = true; Init(false /* don't start progress timer */); } @@ -139,6 +159,7 @@ DownloadItem::DownloadItem(DownloadManager* download_manager, is_extension_install_(info.is_extension_install), name_finalized_(false), is_temporary_(!info.save_info.file_path.empty()), + all_data_saved_(false), opened_(false) { Init(true /* start progress timer */); } @@ -174,6 +195,7 @@ DownloadItem::DownloadItem(DownloadManager* download_manager, is_extension_install_(false), name_finalized_(false), is_temporary_(false), + all_data_saved_(false), opened_(false) { Init(true /* start progress timer */); } @@ -299,8 +321,14 @@ void DownloadItem::Cancel(bool update_history) { download_manager_->DownloadCancelled(id_); } -void DownloadItem::OnAllDataSaved(int64 size) { +void DownloadItem::MarkAsComplete() { + DCHECK(all_data_saved_); state_ = COMPLETE; +} + +void DownloadItem::OnAllDataSaved(int64 size) { + DCHECK(!all_data_saved_); + all_data_saved_ = true; UpdateSize(size); StopProgressTimer(); } @@ -324,7 +352,7 @@ void DownloadItem::Finished() { auto_opened_ = true; } - // Notify our observers that we are complete (the call to OnAllDataSaved() + // Notify our observers that we are complete (the call to MarkAsComplete() // set the state to complete but did not notify). UpdateObservers(); diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h index bc32f63..eaa9a55 100644 --- a/chrome/browser/download/download_item.h +++ b/chrome/browser/download/download_item.h @@ -40,8 +40,19 @@ class DownloadItem { public: enum DownloadState { IN_PROGRESS, + + // Note that COMPLETE indicates that the download has gotten all of its + // data, has figured out its final destination file, has been entered + // into the history store, and has been shown in the UI. The only + // operations remaining are acceptance by the user of + // a dangerous download (if dangerous), renaming the file + // to the final name, and auto-opening it (if it auto-opens). COMPLETE, + CANCELLED, + + // This state indicates that the download item is about to be destroyed, + // and observers seeing this state should release all references. REMOVING }; @@ -128,9 +139,13 @@ class DownloadItem { // when resuming a download (assuming the server supports byte ranges). void Cancel(bool update_history); - // Called when all data has been saved. + // Called when all data has been saved. Only has display effects. void OnAllDataSaved(int64 size); + // Called when ready to consider the data received and move on to the + // next stage. + void MarkAsComplete(); + // Called when the entire download operation (including renaming etc) // is finished. void Finished(); @@ -152,6 +167,9 @@ class DownloadItem { // total size). int PercentComplete() const; + // Whether or not this download has saved all of its data. + bool all_data_saved() const { return all_data_saved_; } + // Update the fields that may have changed in DownloadCreateInfo as a // result of analyzing the file and figuring out its type, location, etc. // May only be called once. @@ -340,6 +358,9 @@ class DownloadItem { // True if the item was downloaded temporarily. bool is_temporary_; + // True if we've saved all the data for the download. + bool all_data_saved_; + // Did the user open the item either directly or indirectly (such as by // setting always open files of this type)? The shelf also sets this field // when the user closes the shelf before the item has been opened but should diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc index 740177e..7e66e50 100644 --- a/chrome/browser/download/download_manager.cc +++ b/chrome/browser/download/download_manager.cc @@ -240,7 +240,7 @@ bool DownloadManager::Init(Profile* profile) { // history system of a new download. Since this method can be called while the // history service thread is still reading the persistent state, we do not // insert the new DownloadItem into 'history_downloads_' or inform our -// observers at this point. OnCreateDatabaseEntryComplete() handles that +// observers at this point. OnCreateDownloadEntryComplete() handles that // finalization of the the download creation as a callback from the // history thread. void DownloadManager::StartDownload(DownloadCreateInfo* info) { @@ -438,6 +438,8 @@ void DownloadManager::CreateDownloadItem(DownloadCreateInfo* info) { void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info, const FilePath& target_path) { + VLOG(20) << __FUNCTION__ << "()" << " info = " << info->DebugString(); + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); scoped_ptr<DownloadCreateInfo> infop(info); @@ -458,29 +460,25 @@ void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info, info->is_extension_install, info->original_name); in_progress_[info->download_id] = download; + UpdateAppIcon(); // Reflect entry into in_progress_. - bool download_finished = ContainsKey(pending_finished_downloads_, - info->download_id); - - VLOG(20) << __FUNCTION__ << "()" - << " target_path = \"" << target_path.value() << "\"" - << " download_finished = " << download_finished - << " info = " << info->DebugString() - << " download = " << download->DebugString(true); - - if (download_finished || info->is_dangerous) { - // The download has already finished or the download is not safe. - // We can now rename the file to its final name (or its tentative name - // in dangerous download cases). + // Rename to intermediate name. + if (info->is_dangerous) { + // The download is not safe. We can now rename the file to its + // tentative name using OnFinalDownloadName (the actual final + // name after user confirmation will be set in + // ProceedWithFinishedDangerousDownload). BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, NewRunnableMethod( file_manager_, &DownloadFileManager::OnFinalDownloadName, - download->id(), target_path, !info->is_dangerous, + download->id(), target_path, false, make_scoped_refptr(this))); } else { - // The download hasn't finished and it is a safe download. We need to - // rename it to its intermediate '.crdownload' path. + // The download is a safe download. We need to + // rename it to its intermediate '.crdownload' path. The final + // name after user confirmation will be set from + // DownloadItem::OnSafeDownloadFinished. FilePath download_path = download_util::GetCrDownloadPath(target_path); BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, @@ -490,17 +488,8 @@ void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info, download->Rename(download_path); } - if (download_finished) { - // If the download already completed by the time we reached this point, then - // notify observers that it did. - OnAllDataSaved(info->download_id, - pending_finished_downloads_[info->download_id]); - } - download_history_->AddEntry(*info, download, NewCallback(this, &DownloadManager::OnCreateDownloadEntryComplete)); - - UpdateAppIcon(); } void DownloadManager::UpdateDownload(int32 download_id, int64 size) { @@ -510,73 +499,93 @@ void DownloadManager::UpdateDownload(int32 download_id, int64 size) { DownloadItem* download = it->second; if (download->state() == DownloadItem::IN_PROGRESS) { download->Update(size); + UpdateAppIcon(); // Reflect size updates. download_history_->UpdateEntry(download); } } - UpdateAppIcon(); } void DownloadManager::OnAllDataSaved(int32 download_id, int64 size) { VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id << " size = " << size; - DownloadMap::iterator it = in_progress_.find(download_id); - if (it == in_progress_.end()) { - // The download is done, but the user hasn't selected a final location for - // it yet (the Save As dialog box is probably still showing), so just keep - // track of the fact that this download id is complete, when the - // DownloadItem is constructed later we'll notify its completion then. - DCHECK(!ContainsKey(pending_finished_downloads_, download_id)); - pending_finished_downloads_[download_id] = size; - VLOG(20) << __FUNCTION__ << "()" << " Added download_id = " << download_id - << " to pending_finished_downloads_"; - return; - } - // Remove the id from the list of pending ids. - PendingFinishedMap::iterator erase_it = - pending_finished_downloads_.find(download_id); - if (erase_it != pending_finished_downloads_.end()) { - pending_finished_downloads_.erase(erase_it); - VLOG(20) << __FUNCTION__ << "()" << " Removed download_id = " << download_id - << " from pending_finished_downloads_"; - } + DCHECK_EQ(1U, active_downloads_.count(download_id)); + DownloadItem* download = active_downloads_[download_id]; + download->OnAllDataSaved(size); - DownloadItem* download = it->second; + MaybeCompleteDownload(download); +} - VLOG(20) << __FUNCTION__ << "()" - << " download = " << download->DebugString(true); +bool DownloadManager::IsDownloadReadyForCompletion(DownloadItem* download) { + // If we don't have all the data, the download is not ready for + // completion. + if (!download->all_data_saved()) + return false; - download->OnAllDataSaved(size); + // If the download isn't active (e.g. has been cancelled) it's not + // ready for completion. + if (active_downloads_.count(download->id()) == 0) + return false; - // Clean up will happen when the history system create callback runs if we - // don't have a valid db_handle yet. - if (download->db_handle() != DownloadHistory::kUninitializedHandle) { - in_progress_.erase(it); - download_history_->UpdateEntry(download); - } + // 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. + return (download->db_handle() != DownloadHistory::kUninitializedHandle); +} - UpdateAppIcon(); +void DownloadManager::MaybeCompleteDownload(DownloadItem* download) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + VLOG(20) << __FUNCTION__ << "()" << " download = " + << download->DebugString(false); - // If this a dangerous download not yet validated by the user, don't do - // anything. When the user notifies us, it will trigger a call to - // ProceedWithFinishedDangerousDownload. - if (download->safety_state() == DownloadItem::DANGEROUS) { + if (!IsDownloadReadyForCompletion(download)) return; - } - if (download->safety_state() == DownloadItem::DANGEROUS_BUT_VALIDATED) { - // We first need to rename the downloaded file from its temporary name to - // its final name before we can continue. - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - NewRunnableMethod( - this, &DownloadManager::ProceedWithFinishedDangerousDownload, - download->db_handle(), - download->full_path(), download->target_name())); - return; + // TODO(rdsmith): DCHECK that we only pass through this point + // once per download. The natural way to do this is by a state + // transition on the DownloadItem. + + // Confirm we're in the proper set of states to be here; + // in in_progress_, have all data, have a history handle. + DCHECK_EQ(1u, in_progress_.count(download->id())); + DCHECK(download->all_data_saved()); + DCHECK(download->db_handle() != DownloadHistory::kUninitializedHandle); + DCHECK_EQ(1u, history_downloads_.count(download->db_handle())); + + VLOG(20) << __FUNCTION__ << "()" << " executing: download = " + << download->DebugString(false); + + // Remove the id from in_progress + in_progress_.erase(download->id()); + UpdateAppIcon(); // Reflect removal from in_progress_. + + // Final update of download item and history. + download->MarkAsComplete(); + download_history_->UpdateEntry(download); + + switch (download->safety_state()) { + case DownloadItem::DANGEROUS: + // If this a dangerous download not yet validated by the user, don't do + // anything. When the user notifies us, it will trigger a call to + // ProceedWithFinishedDangerousDownload. + return; + case DownloadItem::DANGEROUS_BUT_VALIDATED: + // The dangerous download has been validated by the user. We first + // need to rename the downloaded file from its temporary name to + // its final name. We will continue the download processing in the + // callback. + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, + NewRunnableMethod( + this, &DownloadManager::ProceedWithFinishedDangerousDownload, + download->db_handle(), + download->full_path(), download->target_name())); + return; + case DownloadItem::SAFE: + // The download is safe; just finish it. + download->OnSafeDownloadFinished(file_manager_); + return; } - - download->OnSafeDownloadFinished(file_manager_); } void DownloadManager::RemoveFromActiveList(int32 download_id) { @@ -667,13 +676,13 @@ void DownloadManager::DownloadCancelled(int32 download_id) { if (download->db_handle() != DownloadHistory::kUninitializedHandle) { in_progress_.erase(it); active_downloads_.erase(download_id); + UpdateAppIcon(); // Reflect removal from in_progress_. download_history_->UpdateEntry(download); } DownloadCancelledInternal(download_id, download->render_process_id(), download->request_id()); - UpdateAppIcon(); } void DownloadManager::DownloadCancelledInternal(int download_id, @@ -993,23 +1002,21 @@ void DownloadManager::OnCreateDownloadEntryComplete( // Inform interested objects about the new download. NotifyModelChanged(); - // If this download has been completed before we've received the db handle, + // If this download has been cancelled before we've received the DB handle, // 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. + // + // Otherwise, try to complete the download if (download->state() != DownloadItem::IN_PROGRESS) { + DCHECK_EQ(DownloadItem::CANCELLED, download->state()); in_progress_.erase(it); - // TODO(ahendrickson) -- We don't actually know whether or not we can - // remove the download item from the |active_downloads_| map, as there - // is no state in |DownloadItem::DownloadState| to indicate that the - // downloads system is done with an item. Fix this when we have a - // proper final state to check for. active_downloads_.erase(info.download_id); download_history_->UpdateEntry(download); download->UpdateObservers(); + } else { + MaybeCompleteDownload(download); } - - UpdateAppIcon(); } void DownloadManager::ShowDownloadInBrowser(const DownloadCreateInfo& info, @@ -1057,9 +1064,9 @@ DownloadItem* DownloadManager::GetDownloadItem(int id) { void DownloadManager::AssertContainersConsistent() const { #if !defined(NDEBUG) // Turn everything into sets. - DownloadSet in_progress_set, history_set; + DownloadSet active_set, history_set; const DownloadMap* input_maps[] = {&active_downloads_, &history_downloads_}; - DownloadSet* local_sets[] = {&in_progress_set, &history_set}; + DownloadSet* local_sets[] = {&active_set, &history_set}; DCHECK_EQ(ARRAYSIZE_UNSAFE(input_maps), ARRAYSIZE_UNSAFE(local_sets)); for (size_t i = 0; i < ARRAYSIZE_UNSAFE(input_maps); i++) { for (DownloadMap::const_iterator it = input_maps[i]->begin(); @@ -1069,7 +1076,7 @@ void DownloadManager::AssertContainersConsistent() const { } // Check if each set is fully present in downloads, and create a union. - const DownloadSet* all_sets[] = {&in_progress_set, &history_set, + const DownloadSet* all_sets[] = {&active_set, &history_set, &save_page_as_downloads_}; DownloadSet downloads_union; for (int i = 0; i < static_cast<int>(ARRAYSIZE_UNSAFE(all_sets)); i++) { diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h index 61b17be..c00437a 100644 --- a/chrome/browser/download/download_manager.h +++ b/chrome/browser/download/download_manager.h @@ -125,6 +125,17 @@ class DownloadManager void PauseDownload(int32 download_id, bool pause); void RemoveDownload(int64 download_handle); + // Determine if the download is ready for completion, i.e. has had + // all data received, and completed the filename determination and + // history insertion. + bool IsDownloadReadyForCompletion(DownloadItem* download); + + // If all pre-requisites have been met, complete download processing, i.e. + // do internal cleanup, file rename, and potentially auto-open. + // (Dangerous downloads still may block on user acceptance after this + // point.) + void MaybeCompleteDownload(DownloadItem* download); + // Called when the download is renamed to its final name. void DownloadRenamedToFinalName(int download_id, const FilePath& full_path); @@ -369,14 +380,6 @@ class DownloadManager // user wants us to prompt for a save location for each download. FilePath last_download_path_; - // Keep track of downloads that are completed before the user selects the - // destination, so that observers are appropriately notified of completion - // after this determination is made. - // The map is of download_id->remaining size (bytes), both of which are - // required when calling OnAllDataSaved. - typedef std::map<int32, int64> PendingFinishedMap; - PendingFinishedMap pending_finished_downloads_; - // The "Save As" dialog box used to ask the user where a file should be // saved. scoped_refptr<SelectFileDialog> select_file_dialog_; diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc index 610fb98..4b7dd8a 100644 --- a/chrome/browser/download/download_manager_unittest.cc +++ b/chrome/browser/download/download_manager_unittest.cc @@ -157,28 +157,29 @@ const struct { bool will_delete_crdownload; int expected_rename_count; } kDownloadRenameCases[] = { - // Safe download, download finishes BEFORE rename. - // Needs to be renamed only once. Crdownload file needs to be deleted. + // Safe download, download finishes BEFORE file name determined. + // Renamed twice (linear path through UI). Crdownload file does not need + // to be deleted. { FILE_PATH_LITERAL("foo.zip"), false, true, - true, - 1, }, - // Dangerous download, download finishes BEFORE rename. + false, + 2, }, + // Dangerous download, download finishes BEFORE file name determined. // Needs to be renamed only once. { FILE_PATH_LITERAL("Unconfirmed xxx.crdownload"), true, true, false, 1, }, - // Safe download, download finishes AFTER rename. + // Safe download, download finishes AFTER file name determined. // Needs to be renamed twice. { FILE_PATH_LITERAL("foo.zip"), false, false, false, 2, }, - // Dangerous download, download finishes AFTER rename. + // Dangerous download, download finishes AFTER file name determined. // Needs to be renamed only once. { FILE_PATH_LITERAL("Unconfirmed xxx.crdownload"), true, diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc index f7833d5..e2cebcc 100644 --- a/chrome/browser/download/save_package.cc +++ b/chrome/browser/download/save_package.cc @@ -687,8 +687,9 @@ void SavePackage::Finish() { save_ids)); download_->OnAllDataSaved(all_save_items_count_); + download_->MarkAsComplete(); // Notify download observers that we are complete (the call - // to OnAllDataSaved() set the state to complete but did not notify). + // to OnReadyToFinish() set the state to complete but did not notify). download_->UpdateObservers(); NotificationService::current()->Notify( |