diff options
32 files changed, 2326 insertions, 1091 deletions
diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc index df7b449..888704f 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.cc +++ b/chrome/browser/download/chrome_download_manager_delegate.cc @@ -175,8 +175,36 @@ void ChromeDownloadManagerDelegate::ChooseDownloadPath( } FilePath ChromeDownloadManagerDelegate::GetIntermediatePath( - const FilePath& suggested_path) { - return download_util::GetCrDownloadPath(suggested_path); + const DownloadItem& download, + bool* ok_to_overwrite) { + if (download.GetDangerType() == + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS) { + // The intermediate path could be a placeholder file that we created to + // ensure that we won't reuse the same name for multiple automatic + // downloads. We need to overwrite it. + *ok_to_overwrite = true; + return download_util::GetCrDownloadPath(download.GetTargetFilePath()); + } else { + FilePath::StringType file_name; + FilePath dir = download.GetTargetFilePath().DirName(); +#if defined(OS_WIN) + string16 unconfirmed_prefix = + l10n_util::GetStringUTF16(IDS_DOWNLOAD_UNCONFIRMED_PREFIX); +#else + std::string unconfirmed_prefix = + l10n_util::GetStringUTF8(IDS_DOWNLOAD_UNCONFIRMED_PREFIX); +#endif + base::SStringPrintf( + &file_name, + unconfirmed_prefix.append( + FILE_PATH_LITERAL(" %d.crdownload")).c_str(), + base::RandInt(0, 1000000)); + + // We may end up with a conflict if there are multiple active dangerous + // downloads and we are unlucky. So we don't allow overwriting. + *ok_to_overwrite = false; + return dir.Append(file_name); + } } WebContents* ChromeDownloadManagerDelegate:: @@ -193,7 +221,6 @@ WebContents* ChromeDownloadManagerDelegate:: #endif } - bool ChromeDownloadManagerDelegate::ShouldOpenFileBasedOnExtension( const FilePath& path) { FilePath::StringType extension = path.Extension(); @@ -443,17 +470,52 @@ void ChromeDownloadManagerDelegate::ChooseSavePath( #endif } -#if defined(ENABLE_SAFE_BROWSING) DownloadProtectionService* ChromeDownloadManagerDelegate::GetDownloadProtectionService() { +#if defined(ENABLE_SAFE_BROWSING) SafeBrowsingService* sb_service = g_browser_process->safe_browsing_service(); if (sb_service && sb_service->download_protection_service() && profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) { return sb_service->download_protection_service(); } +#endif return NULL; } -#endif + +// TODO(phajdan.jr): This is apparently not being exercised in tests. +bool ChromeDownloadManagerDelegate::IsDangerousFile( + const DownloadItem& download, + const FilePath& suggested_path, + bool visited_referrer_before) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + + // Anything loaded directly from the address bar is OK. + if (download.GetTransitionType() & content::PAGE_TRANSITION_FROM_ADDRESS_BAR) + return false; + + // Extensions that are not from the gallery are considered dangerous. + // When off-store install is disabled we skip this, since in this case, we + // will not offer to install the extension. + if (extensions::switch_utils::IsOffStoreInstallEnabled() && + download_crx_util::IsExtensionDownload(download) && + !WebstoreInstaller::GetAssociatedApproval(download)) { + return true; + } + + // Anything the user has marked auto-open is OK if it's user-initiated. + if (ShouldOpenFileBasedOnExtension(suggested_path) && + download.HasUserGesture()) + return false; + + // "Allow on user gesture" is OK when we have a user gesture and the hosting + // page has been visited before today. + download_util::DownloadDangerLevel danger_level = + download_util::GetFileDangerLevel(suggested_path.BaseName()); + if (danger_level == download_util::AllowOnUserGesture) + return !download.HasUserGesture() || !visited_referrer_before; + + return danger_level == download_util::Dangerous; +} void ChromeDownloadManagerDelegate::CheckDownloadUrlDone( int32 download_id, @@ -466,13 +528,14 @@ void ChromeDownloadManagerDelegate::CheckDownloadUrlDone( VLOG(2) << __FUNCTION__ << "() download = " << download->DebugString(false) << " verdict = " << result; + content::DownloadDangerType danger_type = download->GetDangerType(); if (result != DownloadProtectionService::SAFE) - download->SetDangerType(content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL); + danger_type = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL; download_history_->CheckVisitedReferrerBefore( download_id, download->GetReferrerUrl(), base::Bind(&ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone, - base::Unretained(this))); + base::Unretained(this), download_id, danger_type)); } void ChromeDownloadManagerDelegate::CheckClientDownloadDone( @@ -492,10 +555,12 @@ void ChromeDownloadManagerDelegate::CheckClientDownloadDone( // Do nothing. break; case DownloadProtectionService::DANGEROUS: - item->SetDangerType(content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT); + item->OnContentCheckCompleted( + content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT); break; case DownloadProtectionService::UNCOMMON: - item->SetDangerType(content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT); + item->OnContentCheckCompleted( + content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT); break; } } @@ -527,6 +592,7 @@ void ChromeDownloadManagerDelegate::Observe( void ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone( int32 download_id, + content::DownloadDangerType danger_type, bool visited_referrer_before) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); @@ -535,14 +601,16 @@ void ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone( if (!download) return; + bool should_prompt = (download->GetTargetDisposition() == + DownloadItem::TARGET_DISPOSITION_PROMPT); + bool is_forced_path = !download->GetForcedFilePath().empty(); + FilePath suggested_path; + // Check whether this download is for an extension install or not. // Allow extensions to be explicitly saved. - DownloadStateInfo state = download->GetStateInfo(); - - if (state.force_file_name.empty()) { + if (!is_forced_path) { FilePath generated_name; - download_util::GenerateFileNameFromRequest(*download, - &generated_name); + download_util::GenerateFileNameFromRequest(*download, &generated_name); // Freeze the user's preference for showing a Save As dialog. We're going // to bounce around a bunch of threads and we don't want to worry about race @@ -556,69 +624,78 @@ void ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone( // opened, don't bother asking where to keep it. if (!download_crx_util::IsExtensionDownload(*download) && !ShouldOpenFileBasedOnExtension(generated_name)) - state.prompt_user_for_save_location = true; - } - if (download_prefs_->IsDownloadPathManaged()) { - state.prompt_user_for_save_location = false; + should_prompt = true; } + if (download_prefs_->IsDownloadPathManaged()) + should_prompt = false; // Determine the proper path for a download, by either one of the following: // 1) using the default download directory. // 2) prompting the user. - if (state.prompt_user_for_save_location && - !download_manager_->LastDownloadPath().empty()) { - state.suggested_path = download_manager_->LastDownloadPath(); - } else { - state.suggested_path = download_prefs_->download_path(); - } - state.suggested_path = state.suggested_path.Append(generated_name); + FilePath target_directory; + if (should_prompt && !download_manager_->LastDownloadPath().empty()) + target_directory = download_manager_->LastDownloadPath(); + else + target_directory = download_prefs_->download_path(); + suggested_path = target_directory.Append(generated_name); } else { - state.suggested_path = state.force_file_name; + DCHECK(!should_prompt); + suggested_path = download->GetForcedFilePath(); } // If the download hasn't already been marked dangerous (could be // DANGEROUS_URL), check if it is a dangerous file. - if (state.danger == content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS) { - if (!state.prompt_user_for_save_location && - state.force_file_name.empty() && - IsDangerousFile(*download, state, visited_referrer_before)) { - state.danger = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE; + if (danger_type == content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS) { + if (!should_prompt && !is_forced_path && + IsDangerousFile(*download, suggested_path, visited_referrer_before)) { + danger_type = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE; } #if defined(ENABLE_SAFE_BROWSING) DownloadProtectionService* service = GetDownloadProtectionService(); - // Return false if this type of files is handled by the enhanced - // SafeBrowsing download protection. + // If this type of files is handled by the enhanced SafeBrowsing download + // protection, mark it as potentially dangerous content until we are done + // with scanning it. if (service && service->enabled()) { DownloadProtectionService::DownloadInfo info = DownloadProtectionService::DownloadInfo::FromDownloadItem(*download); - info.target_file = state.suggested_path; + info.target_file = suggested_path; // TODO(noelutz): if the user changes the extension name in the UI to // something like .exe SafeBrowsing will currently *not* check if the // download is malicious. if (service->IsSupportedDownload(info)) - state.danger = content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT; + danger_type = content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT; } #endif + } else { + // Currently we only expect this case. + DCHECK_EQ(content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL, danger_type); } - // We need to move over to the download thread because we don't want to stat - // the suggested path on the UI thread. - // We can only access preferences on the UI thread, so check the download path - // now and pass the value to the FILE thread. + // We need to move over to the FILE thread because we don't want to stat the + // suggested path on the UI thread. We can only access preferences on the UI + // thread, so check the download path now and pass the value to the FILE + // thread. BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, base::Bind(&ChromeDownloadManagerDelegate::CheckIfSuggestedPathExists, - this, download->GetId(), state, + this, download->GetId(), suggested_path, should_prompt, + is_forced_path, danger_type, download_prefs_->download_path())); } void ChromeDownloadManagerDelegate::CheckIfSuggestedPathExists( int32 download_id, - DownloadStateInfo state, + const FilePath& unverified_path, + bool should_prompt, + bool is_forced_path, + content::DownloadDangerType danger_type, const FilePath& default_path) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + // Suggested target path. Start with |unverified_path|. + FilePath target_path(unverified_path); + // Make sure the default download directory exists. // TODO(phajdan.jr): only create the directory when we're sure the user // is going to save there and not to another directory of his choice. @@ -626,127 +703,102 @@ void ChromeDownloadManagerDelegate::CheckIfSuggestedPathExists( // Check writability of the suggested path. If we can't write to it, default // to the user's "My Documents" directory. We'll prompt them in this case. - FilePath dir = state.suggested_path.DirName(); - FilePath filename = state.suggested_path.BaseName(); + FilePath dir = target_path.DirName(); + FilePath filename = target_path.BaseName(); if (!file_util::PathIsWritable(dir)) { VLOG(1) << "Unable to write to directory \"" << dir.value() << "\""; - state.prompt_user_for_save_location = true; + should_prompt = true; PathService::Get(chrome::DIR_USER_DOCUMENTS, &dir); - state.suggested_path = dir.Append(filename); + target_path = dir.Append(filename); } - // If the download is possibly dangerous, we'll use a temporary name for it. - if (state.danger != content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS) { - state.target_name = FilePath(state.suggested_path).BaseName(); - // Create a temporary file to hold the file until the user approves its - // download. - FilePath::StringType file_name; - FilePath path; -#if defined(OS_WIN) - string16 unconfirmed_prefix = - l10n_util::GetStringUTF16(IDS_DOWNLOAD_UNCONFIRMED_PREFIX); -#else - std::string unconfirmed_prefix = - l10n_util::GetStringUTF8(IDS_DOWNLOAD_UNCONFIRMED_PREFIX); -#endif - - while (path.empty()) { - base::SStringPrintf( - &file_name, - unconfirmed_prefix.append( - FILE_PATH_LITERAL(" %d.crdownload")).c_str(), - base::RandInt(0, 100000)); - path = dir.Append(file_name); - if (file_util::PathExists(path)) - path = FilePath(); - } - state.suggested_path = path; - } else { - // Do not add the path uniquifier if we are saving to a specific path as in - // the drag-out case. - if (state.force_file_name.empty()) { - state.path_uniquifier = download_util::GetUniquePathNumberWithCrDownload( - state.suggested_path); - } - // We know the final path, build it if necessary. - if (state.path_uniquifier > 0) { - state.suggested_path = state.suggested_path.InsertBeforeExtensionASCII( - StringPrintf(" (%d)", state.path_uniquifier)); - // Setting path_uniquifier to 0 to make sure we don't try to unique it - // later on. - state.path_uniquifier = 0; - } else if (state.path_uniquifier == -1) { - // We failed to find a unique path. We have to prompt the user. + // Decide how we want to handle this download: + + // - Uniquify: If a file exists in the target path, uniquify the path by + // appending a uniquifier ("foo.txt" -> "foo (1).txt"). + // - Overwrite: Overwrite the target path if it exists. + // - Marker: Create a placeholder file to avoid using the same filename for + // another download. (http://crbug.com/3662) + // + // Download type | Uniquify | Overwrite | Marker | + // -------------------+----------+-----------+--------+ + // Forced (Safe) | No | Yes | No + // Forced (Dangerous) | No | Yes | No + // Prompt (Safe) | Yes | Yes | No + // Prompt (Dangerous) | Yes | Yes | No + // Auto (Safe) | Yes | Yes | Yes + // Auto (Dangerous) | No | No | No + bool should_uniquify = + (!is_forced_path && + (danger_type == content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS || + should_prompt)); + bool should_overwrite = + (should_uniquify || is_forced_path); + bool should_create_marker = (should_uniquify && !should_prompt); + + if (should_uniquify) { + // When we uniquify, ideally we should exclude files that are: + // 1. Already existing in the file system. + // 2. All in-progress downloads for which we are overwriting the target. + // + // Of the 6 categories above for which we overwrite the target, we only + // exclude "Auto (Safe)" reliably. "Prompt (Safe)" downloads will be + // excluded if the conflicting download has proceeded past + // RenameInProgressDownloadFile. Ditto for "Forced" downloads. None of the + // "Dangerous" types are excluded. + // TODO(asanka): Fix this. + int uniquifier = + download_util::GetUniquePathNumberWithCrDownload(target_path); + + if (uniquifier > 0) { + target_path = target_path.InsertBeforeExtensionASCII( + StringPrintf(" (%d)", uniquifier)); + } else if (uniquifier == -1) { VLOG(1) << "Unable to find a unique path for suggested path \"" - << state.suggested_path.value() << "\""; - state.prompt_user_for_save_location = true; + << target_path.value() << "\""; + should_prompt = true; } } - // Create an empty file at the suggested path so that we don't allocate the - // same "non-existant" path to multiple downloads. - // See: http://code.google.com/p/chromium/issues/detail?id=3662 - if (!state.prompt_user_for_save_location && - state.force_file_name.empty()) { - if (state.danger != content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS) - file_util::WriteFile(state.suggested_path, "", 0); - else - file_util::WriteFile(download_util::GetCrDownloadPath( - state.suggested_path), "", 0); - } + // Create a marker file at the .crdownload path. For the cases that we use + // markers this is all we need for GetUniquePathNumberWithCrDownload() to + // reliably avoid conflicts. Note that this requires GetIntermediatePath() to + // overwrite this marker when the download proceeds. + if (should_create_marker) + file_util::WriteFile(download_util::GetCrDownloadPath(target_path), "", 0); + + DownloadItem::TargetDisposition disposition; + if (should_prompt) + disposition = DownloadItem::TARGET_DISPOSITION_PROMPT; + else if (should_overwrite) + disposition = DownloadItem::TARGET_DISPOSITION_OVERWRITE; + else + disposition = DownloadItem::TARGET_DISPOSITION_UNIQUIFY; BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, base::Bind(&ChromeDownloadManagerDelegate::OnPathExistenceAvailable, - this, download_id, state)); + this, download_id, target_path, disposition, danger_type)); } void ChromeDownloadManagerDelegate::OnPathExistenceAvailable( int32 download_id, - const DownloadStateInfo& new_state) { + const FilePath& target_path, + DownloadItem::TargetDisposition disposition, + content::DownloadDangerType danger_type) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DownloadItem* download = download_manager_->GetActiveDownloadItem(download_id); + // TODO(asanka): If the download was cancelled here, we need to manually + // cleanup the placeholder file we created in + // CheckIfSuggestedPathExists(). Or avoid creating a marker file + // in the first place. if (!download) return; - download->SetFileCheckResults(new_state); + download->OnTargetPathDetermined(target_path, disposition, danger_type); download_manager_->RestartDownload(download_id); } -// TODO(phajdan.jr): This is apparently not being exercised in tests. -bool ChromeDownloadManagerDelegate::IsDangerousFile( - const DownloadItem& download, - const DownloadStateInfo& state, - bool visited_referrer_before) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - - // Anything loaded directly from the address bar is OK. - if (state.transition_type & content::PAGE_TRANSITION_FROM_ADDRESS_BAR) - return false; - - // Extensions that are not from the gallery are considered dangerous. - // When off-store install is disabled we skip this, since in this case, we - // will cancel the install later. - if (extensions::switch_utils::IsOffStoreInstallEnabled() && - download_crx_util::IsExtensionDownload(download) && - !WebstoreInstaller::GetAssociatedApproval(download)) { - return true; - } - - // Anything the user has marked auto-open is OK if it's user-initiated. - if (ShouldOpenFileBasedOnExtension(state.suggested_path) && - state.has_user_gesture) - return false; - - // "Allow on user gesture" is OK when we have a user gesture and the hosting - // page has been visited before today. - download_util::DownloadDangerLevel danger_level = - download_util::GetFileDangerLevel(state.suggested_path.BaseName()); - if (danger_level == download_util::AllowOnUserGesture) - return !state.has_user_gesture || !visited_referrer_before; - - return danger_level == download_util::Dangerous; -} - void ChromeDownloadManagerDelegate::OnItemAddedToPersistentStore( int32 download_id, int64 db_handle) { // It's not immediately obvious, but HistoryBackend::CreateDownload() can diff --git a/chrome/browser/download/chrome_download_manager_delegate.h b/chrome/browser/download/chrome_download_manager_delegate.h index bb6b160..76e73f1 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.h +++ b/chrome/browser/download/chrome_download_manager_delegate.h @@ -12,6 +12,8 @@ #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" #include "chrome/browser/safe_browsing/download_protection_service.h" +#include "content/public/browser/download_danger_type.h" +#include "content/public/browser/download_item.h" #include "content/public/browser/download_manager_delegate.h" #include "content/public/browser/notification_observer.h" #include "content/public/browser/notification_registrar.h" @@ -20,10 +22,8 @@ class CrxInstaller; class DownloadHistory; class DownloadPrefs; class Profile; -struct DownloadStateInfo; namespace content { -class DownloadItem; class DownloadManager; } @@ -58,7 +58,8 @@ class ChromeDownloadManagerDelegate virtual void ChooseDownloadPath(content::WebContents* web_contents, const FilePath& suggested_path, int32 download_id) OVERRIDE; - virtual FilePath GetIntermediatePath(const FilePath& suggested_path) OVERRIDE; + virtual FilePath GetIntermediatePath(const content::DownloadItem& item, + bool* ok_to_overwrite) OVERRIDE; virtual content::WebContents* GetAlternativeWebContentsToNotifyForDownload() OVERRIDE; virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path) OVERRIDE; @@ -95,6 +96,19 @@ class ChromeDownloadManagerDelegate // So that test classes can inherit from this for override purposes. virtual ~ChromeDownloadManagerDelegate(); + // Returns the SafeBrowsing download protection service if it's + // enabled. Returns NULL otherwise. Protected virtual for testing. + virtual safe_browsing::DownloadProtectionService* + GetDownloadProtectionService(); + + // Returns true if this download should show the "dangerous file" warning. + // Various factors are considered, such as the type of the file, whether a + // user action initiated the download, and whether the user has explicitly + // marked the file type as "auto open". Protected virtual for testing. + virtual bool IsDangerousFile(const content::DownloadItem& download, + const FilePath& suggested_path, + bool visited_referrer_before); + // So that test classes that inherit from this for override purposes // can call back into the DownloadManager. scoped_refptr<content::DownloadManager> download_manager_; @@ -107,10 +121,6 @@ class ChromeDownloadManagerDelegate const content::NotificationSource& source, const content::NotificationDetails& details) OVERRIDE; - // Returns the SafeBrowsing download protection service if it's - // enabled. Returns NULL otherwise. - safe_browsing::DownloadProtectionService* GetDownloadProtectionService(); - // Callback function after url is checked with safebrowsing service. void CheckDownloadUrlDone( int32 download_id, @@ -122,29 +132,35 @@ class ChromeDownloadManagerDelegate safe_browsing::DownloadProtectionService::DownloadCheckResult result); // Callback function after we check whether the referrer URL has been visited - // before today. + // before today. Determines the danger state of the download based on the file + // type and |visited_referrer_before|. Generates a target path for the + // download. Invokes |CheckIfSuggestedPathExists| on the FILE thread to check + // the target path. void CheckVisitedReferrerBeforeDone(int32 download_id, + content::DownloadDangerType danger_type, bool visited_referrer_before); - // Called on the download thread to check whether the suggested file path - // exists. We don't check if the file exists on the UI thread to avoid UI - // stalls from interacting with the file system. + // Called on the FILE thread to check whether the suggested file path exists. + // We don't check if the file exists on the UI thread to avoid UI stalls from + // interacting with the file system. Creates the default download directory + // specified in |default_path| if it doesn't exist. Uniquifies |unverified + // path| if necessary. The verified path is then passed down to + // |OnPathExistenceAvailable|. void CheckIfSuggestedPathExists(int32 download_id, - DownloadStateInfo state, + const FilePath& unverified_path, + bool should_prompt, + bool is_forced_path, + content::DownloadDangerType danger_type, const FilePath& default_path); // Called on the UI thread once it's determined whether the suggested file - // path exists. - void OnPathExistenceAvailable(int32 download_id, - const DownloadStateInfo& new_state); - - // Returns true if this download should show the "dangerous file" warning. - // Various factors are considered, such as the type of the file, whether a - // user action initiated the download, and whether the user has explicitly - // marked the file type as "auto open". - bool IsDangerousFile(const content::DownloadItem& download, - const DownloadStateInfo& state, - bool visited_referrer_before); + // path exists. Updates the download identified by |download_id| with the + // |target_path|, |target_disposition| and |danger_type|. + void OnPathExistenceAvailable( + int32 download_id, + const FilePath& target_path, + content::DownloadItem::TargetDisposition disposition, + content::DownloadDangerType danger_type); // Callback from history system. void OnItemAddedToPersistentStore(int32 download_id, int64 db_handle); diff --git a/chrome/browser/download/chrome_download_manager_delegate_unittest.cc b/chrome/browser/download/chrome_download_manager_delegate_unittest.cc new file mode 100644 index 0000000..a655798 --- /dev/null +++ b/chrome/browser/download/chrome_download_manager_delegate_unittest.cc @@ -0,0 +1,1010 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/file_path.h" +#include "base/file_util.h" +#include "base/message_loop.h" +#include "base/scoped_temp_dir.h" +#include "base/string_util.h" +#include "base/value_conversions.h" +#include "chrome/browser/download/chrome_download_manager_delegate.h" +#include "chrome/browser/download/download_prefs.h" +#include "chrome/browser/download/download_util.h" +#include "chrome/browser/prefs/pref_service.h" +#include "chrome/browser/safe_browsing/download_protection_service.h" +#include "chrome/common/extensions/extension.h" +#include "chrome/common/extensions/extension_switch_utils.h" +#include "chrome/common/pref_names.h" +#include "chrome/test/base/testing_pref_service.h" +#include "chrome/test/base/testing_profile.h" +#include "content/test/mock_download_item.h" +#include "content/test/mock_download_manager.h" +#include "content/test/test_browser_thread.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +using content::DownloadItem; +using safe_browsing::DownloadProtectionService; +using ::testing::AtMost; +using ::testing::Invoke; +using ::testing::Return; +using ::testing::ReturnPointee; +using ::testing::ReturnRef; +using ::testing::ReturnRefOfCopy; +using ::testing::WithArg; +using ::testing::_; + +// Google Mock action that posts a task to the current message loop that invokes +// the first argument of the mocked method as a callback. Said argument must be +// a base::Callback<void(ParamType)>. |result| must be of |ParamType| and is +// bound as that parameter. +// Example: +// class FooClass { +// public: +// virtual void Foo(base::Callback<void(bool)> callback); +// }; +// ... +// EXPECT_CALL(mock_fooclass_instance, Foo(callback)) +// .WillOnce(ScheduleCallback(false)); +ACTION_P(ScheduleCallback, result) { + MessageLoop::current()->PostTask(FROM_HERE, base::Bind(arg0, result)); +} + +// Matches a safe_browsing::DownloadProtectionService::DownloadInfo that has +// |url| as the first URL in the |download_url_chain|. +// Example: +// EXPECT_CALL(Foo(InfoMatchinURL(url))) +MATCHER_P(InfoMatchingURL, url, "DownloadInfo matching URL " + url.spec()) { + return url == arg.download_url_chain.front(); +} + +namespace { + +// Used with DownloadTestCase. Indicates the type of test case. The expectations +// for the test is set based on the type. +enum TestCaseType { + SAVE_AS, + AUTOMATIC, + FORCED // Requires that forced_file_path be non-empty. +}; + +// Used with DownloadTestCase. Indicates whether the a file should be +// overwritten. +enum TestCaseExpectOverwrite { + EXPECT_OVERWRITE, + EXPECT_NO_OVERWRITE +}; + +// Used with DownloadTestCase. Type of intermediate filename to expect. +enum TestCaseExpectIntermediate { + EXPECT_CRDOWNLOAD, // Expect path/to/target.crdownload + EXPECT_UNCONFIRMED // Expect path/to/Unconfirmed xxx.crdownload +}; + +// A marker is a file created on the file system to "reserve" a filename. We do +// this for a limited number of cases to prevent in-progress downloads from +// getting overwritten by new downloads. +enum TestCaseExpectMarker { + EXPECT_NO_MARKER, + EXPECT_INTERMEDIATE_MARKER +}; + +// Typical download test case. Used with +// ChromeDownloadManagerDelegateTest::RunTestCase(). +struct DownloadTestCase { + // Type of test. + TestCaseType test_type; + + // The |danger_type| value is used to determine the behavior of + // DownloadProtectionService::IsSupportedDownload(), GetUrlCheckResult() and + // well as set expectations for GetDangerType() as necessary for flagging the + // download with as a dangerous download of type |danger_type|. + content::DownloadDangerType danger_type; + + // Value of GetURL() + const char* url; + + // Value of GetMimeType() + const char* mime_type; + + // Should be non-empty if |test_type| == FORCED. Value of GetForcedFilePath(). + const FilePath::CharType* forced_file_path; + + // Expected final download path. Specified relative to the test download path. + const FilePath::CharType* expected_target_path; + + // Expected target disposition. + DownloadItem::TargetDisposition expected_disposition; + + // Type of intermediate path to expect. + TestCaseExpectIntermediate expected_intermediate; + + // Whether we should overwrite the intermediate path. + TestCaseExpectOverwrite expected_overwrite_intermediate; + + // Whether a marker file should be expected. + TestCaseExpectMarker expected_marker; +}; + +// DownloadProtectionService with mock methods. Since the SafeBrowsingService is +// set to NULL, it is not safe to call any non-mocked methods other than +// SetEnabled() and enabled(). +class TestDownloadProtectionService + : public safe_browsing::DownloadProtectionService { + public: + TestDownloadProtectionService() + : safe_browsing::DownloadProtectionService(NULL, NULL) {} + MOCK_METHOD2(CheckClientDownload, + void(const DownloadProtectionService::DownloadInfo&, + const DownloadProtectionService::CheckDownloadCallback&)); + MOCK_METHOD2(CheckDownloadUrl, + void(const DownloadProtectionService::DownloadInfo&, + const DownloadProtectionService::CheckDownloadCallback&)); + MOCK_CONST_METHOD1(IsSupportedDownload, + bool(const DownloadProtectionService::DownloadInfo&)); +}; + +// Subclass of the ChromeDownloadManagerDelegate that uses a mock +// DownloadProtectionService and IsDangerousFile. +class TestChromeDownloadManagerDelegate : public ChromeDownloadManagerDelegate { + public: + explicit TestChromeDownloadManagerDelegate(Profile* profile) + : ChromeDownloadManagerDelegate(profile), + download_protection_service_(new TestDownloadProtectionService()) { + download_protection_service_->SetEnabled(true); + } + virtual safe_browsing::DownloadProtectionService* + GetDownloadProtectionService() OVERRIDE { + return download_protection_service_.get(); + } + virtual bool IsDangerousFile(const DownloadItem& download, + const FilePath& suggested_path, + bool visited_referrer_before) OVERRIDE { + // The implementaion of ChromeDownloadManagerDelegate::IsDangerousFile() is + // sensitive to a number of external factors (e.g. whether off-store + // extension installs are allowed, whether a given extension download is + // approved, whether the user wants files of a given type to be opened + // automatically etc...). We should test these specifically, but for other + // tests, we keep the IsDangerousFile() test simple so as not to make the + // tests flaky. + return suggested_path.MatchesExtension(FILE_PATH_LITERAL(".jar")) || + suggested_path.MatchesExtension(FILE_PATH_LITERAL(".exe")); + } + + // A TestDownloadProtectionService* is convenient for setting up mocks. + TestDownloadProtectionService* test_download_protection_service() { + return download_protection_service_.get(); + } + private: + ~TestChromeDownloadManagerDelegate() {} + scoped_ptr<TestDownloadProtectionService> download_protection_service_; +}; + +class ChromeDownloadManagerDelegateTest : public ::testing::Test { + public: + ChromeDownloadManagerDelegateTest(); + + // ::testing::Test + virtual void SetUp() OVERRIDE; + + // Verifies and clears test expectations for |delegate_| and + // |download_manager_|. + void VerifyAndClearExpectations(); + + // Creates MockDownloadItem and sets up default expectations. + content::MockDownloadItem* CreateActiveDownloadItem(int32 id); + + // Sets the AutoOpenBasedOnExtension user preference for |path|. + void EnableAutoOpenBasedOnExtension(const FilePath& path); + + // Given the relative path |path|, returns the full path under the temporary + // downloads directory. + FilePath GetPathInDownloadDir(const FilePath::StringType& path); + + // Run |test_case| using |download_id| as the ID for the download. + // |download_id| is typically the test case number as well, for convenience. + void RunTestCase(const DownloadTestCase& test_case, int32 download_id); + + // Set the kDownloadDefaultDirectory user preference to |path|. + void SetDefaultDownloadPath(const FilePath& path); + + // Set the kDownloadDefaultDirectory managed preference to |path|. + void SetManagedDownloadPath(const FilePath& path); + + // Set the kPromptForDownload user preference to |prompt|. + void SetPromptForDownload(bool prompt); + + // Verifies that the intermediate path in |intermediate| is the path that is + // expected for |target| given the intermediate path type in |expectation|. + void VerifyIntermediatePath(TestCaseExpectIntermediate expectation, + const FilePath& target, + const FilePath& intermediate); + + const FilePath& default_download_path() const; + TestChromeDownloadManagerDelegate* delegate(); + content::MockDownloadManager* download_manager(); + DownloadPrefs* download_prefs(); + void set_clean_marker_files(bool clean); + + private: + bool clean_marker_files_; + MessageLoopForUI message_loop_; + TestingPrefService* pref_service_; + ScopedTempDir test_download_dir_; + TestingProfile profile_; + content::TestBrowserThread ui_thread_; + content::TestBrowserThread file_thread_; + scoped_refptr<content::MockDownloadManager> download_manager_; + scoped_refptr<TestChromeDownloadManagerDelegate> delegate_; +}; + +ChromeDownloadManagerDelegateTest::ChromeDownloadManagerDelegateTest() + : clean_marker_files_(true), + ui_thread_(content::BrowserThread::UI, &message_loop_), + file_thread_(content::BrowserThread::FILE, &message_loop_), + download_manager_(new content::MockDownloadManager), + delegate_(new TestChromeDownloadManagerDelegate(&profile_)) { + delegate_->SetDownloadManager(download_manager_.get()); + pref_service_ = profile_.GetTestingPrefService(); +} + +void ChromeDownloadManagerDelegateTest::SetUp() { + ASSERT_TRUE(test_download_dir_.CreateUniqueTempDir()); + SetDefaultDownloadPath(test_download_dir_.path()); +} + +void ChromeDownloadManagerDelegateTest::VerifyAndClearExpectations() { + ::testing::Mock::VerifyAndClearExpectations(delegate_); + ::testing::Mock::VerifyAndClearExpectations(download_manager_); +} + +content::MockDownloadItem* + ChromeDownloadManagerDelegateTest::CreateActiveDownloadItem(int32 id) { + content::MockDownloadItem* item = + new ::testing::NiceMock<content::MockDownloadItem>(); + ON_CALL(*item, GetFullPath()) + .WillByDefault(ReturnRefOfCopy(FilePath())); + ON_CALL(*item, GetHash()) + .WillByDefault(ReturnRefOfCopy(std::string())); + ON_CALL(*item, GetReferrerUrl()) + .WillByDefault(ReturnRefOfCopy(GURL())); + ON_CALL(*item, GetTransitionType()) + .WillByDefault(Return(content::PAGE_TRANSITION_LINK)); + ON_CALL(*item, HasUserGesture()) + .WillByDefault(Return(false)); + ON_CALL(*item, IsDangerous()) + .WillByDefault(Return(false)); + ON_CALL(*item, IsTemporary()) + .WillByDefault(Return(false)); + EXPECT_CALL(*item, GetId()) + .WillRepeatedly(Return(id)); + EXPECT_CALL(*download_manager_, GetActiveDownloadItem(id)) + .WillRepeatedly(Return(item)); + return item; +} + +void ChromeDownloadManagerDelegateTest::EnableAutoOpenBasedOnExtension( + const FilePath& path) { + EXPECT_TRUE( + delegate_->download_prefs()->EnableAutoOpenBasedOnExtension(path)); +} + +FilePath ChromeDownloadManagerDelegateTest::GetPathInDownloadDir( + const FilePath::StringType& relative_path) { + if (relative_path.empty()) + return FilePath(); + FilePath full_path(test_download_dir_.path().Append(relative_path)); + return full_path.NormalizePathSeparators(); +} + +void ChromeDownloadManagerDelegateTest::RunTestCase( + const DownloadTestCase& test_case, + int32 download_id) { + scoped_ptr<content::MockDownloadItem> item( + CreateActiveDownloadItem(download_id)); + SCOPED_TRACE( + testing::Message() << "Running test for download id " << download_id); + + // SetUp DownloadItem + GURL download_url(test_case.url); + std::vector<GURL> url_chain; + url_chain.push_back(download_url); + FilePath forced_file_path(GetPathInDownloadDir(test_case.forced_file_path)); + EXPECT_CALL(*item, GetURL()) + .WillRepeatedly(ReturnRef(download_url)); + EXPECT_CALL(*item, GetUrlChain()) + .WillRepeatedly(ReturnRef(url_chain)); + EXPECT_CALL(*item, GetForcedFilePath()) + .WillRepeatedly(ReturnRef(forced_file_path)); + EXPECT_CALL(*item, GetMimeType()) + .WillRepeatedly(Return(test_case.mime_type)); + + // Results of SafeBrowsing URL check. + DownloadProtectionService::DownloadCheckResult url_check_result = + (test_case.danger_type == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL) ? + DownloadProtectionService::DANGEROUS : + DownloadProtectionService::SAFE; + EXPECT_CALL(*delegate_->test_download_protection_service(), + CheckDownloadUrl(InfoMatchingURL(download_url), _)) + .WillOnce(WithArg<1>(ScheduleCallback(url_check_result))); + + // Downloads that are flagged as DANGEROUS_URL aren't checked for dangerous + // content. So we never end up calling IsSupportedDownload for them. + if (test_case.danger_type != content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL) { + bool maybe_dangerous = + (test_case.danger_type == + content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT); + EXPECT_CALL(*delegate_->test_download_protection_service(), + IsSupportedDownload(InfoMatchingURL(download_url))) + .WillOnce(Return(maybe_dangerous)); + } + + // Expectations for filename determination results. + FilePath expected_target_path( + GetPathInDownloadDir(test_case.expected_target_path)); + { + ::testing::Sequence s1, s2, s3; + DownloadItem::TargetDisposition initial_disposition = + (test_case.test_type == SAVE_AS) ? + DownloadItem::TARGET_DISPOSITION_PROMPT : + DownloadItem::TARGET_DISPOSITION_OVERWRITE; + EXPECT_CALL(*item, GetTargetFilePath()) + .InSequence(s1) + .WillRepeatedly(ReturnRefOfCopy(FilePath())); + EXPECT_CALL(*item, GetTargetDisposition()) + .InSequence(s2) + .WillRepeatedly(Return(initial_disposition)); + EXPECT_CALL(*item, GetDangerType()) + .InSequence(s3) + .WillRepeatedly(Return(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS)); + EXPECT_CALL(*item, OnTargetPathDetermined(expected_target_path, + test_case.expected_disposition, + test_case.danger_type)) + .InSequence(s1, s2, s3); + EXPECT_CALL(*item, GetTargetFilePath()) + .InSequence(s1) + .WillRepeatedly(ReturnRef(expected_target_path)); + EXPECT_CALL(*item, GetTargetDisposition()) + .InSequence(s2) + .WillRepeatedly(Return(test_case.expected_disposition)); + EXPECT_CALL(*item, GetDangerType()) + .InSequence(s3) + .WillRepeatedly(Return(test_case.danger_type)); + } + + // RestartDownload() should be called at this point. + EXPECT_CALL(*download_manager_, RestartDownload(download_id)); + EXPECT_CALL(*download_manager_, LastDownloadPath()) + .WillRepeatedly(Return(FilePath())); + + // Kick off the test. + EXPECT_FALSE(delegate_->ShouldStartDownload(download_id)); + message_loop_.RunAllPending(); + + // Now query the intermediate path. + EXPECT_CALL(*item, GetDangerType()) + .WillOnce(Return(test_case.danger_type)); + bool ok_to_overwrite = false; + FilePath intermediate_path = + delegate_->GetIntermediatePath(*item, &ok_to_overwrite); + EXPECT_EQ((test_case.expected_overwrite_intermediate == EXPECT_OVERWRITE), + ok_to_overwrite); + VerifyIntermediatePath(test_case.expected_intermediate, + GetPathInDownloadDir(test_case.expected_target_path), + intermediate_path); + + EXPECT_EQ((test_case.expected_marker == EXPECT_INTERMEDIATE_MARKER), + file_util::PathExists(intermediate_path)); + // Remove the intermediate marker since our mocks won't do anything with it. + if (clean_marker_files_ && + file_util::PathExists(intermediate_path) && + !file_util::DirectoryExists(intermediate_path)) + file_util::Delete(intermediate_path, false); + VerifyAndClearExpectations(); +} + +void ChromeDownloadManagerDelegateTest::SetDefaultDownloadPath( + const FilePath& path) { + pref_service_->SetFilePath(prefs::kDownloadDefaultDirectory, path); +} + +void ChromeDownloadManagerDelegateTest::SetManagedDownloadPath( + const FilePath& path) { + pref_service_->SetManagedPref(prefs::kDownloadDefaultDirectory, + base::CreateFilePathValue(path)); +} + +void ChromeDownloadManagerDelegateTest::SetPromptForDownload(bool prompt) { + pref_service_->SetBoolean(prefs::kPromptForDownload, prompt); +} + +void ChromeDownloadManagerDelegateTest::VerifyIntermediatePath( + TestCaseExpectIntermediate expectation, + const FilePath& target, + const FilePath& intermediate) { + if (expectation == EXPECT_CRDOWNLOAD) { + EXPECT_EQ(download_util::GetCrDownloadPath(target).value(), + intermediate.value()); + } else { + // The paths (in English) look like: /path/Unconfirmed xxx.crdownload. + // Of this, we only check that the path is: + // 1. Not "/path/target.crdownload", + // 2. Points to the same directory as the target. + // 3. Has extension ".crdownload". + EXPECT_NE(download_util::GetCrDownloadPath(target).value(), + intermediate.value()); + EXPECT_EQ(target.DirName().value(), + intermediate.DirName().value()); + EXPECT_TRUE(intermediate.MatchesExtension( + FILE_PATH_LITERAL(".crdownload"))); + } +} + +const FilePath& ChromeDownloadManagerDelegateTest::default_download_path() + const { + return test_download_dir_.path(); +} + +TestChromeDownloadManagerDelegate* + ChromeDownloadManagerDelegateTest::delegate() { + return delegate_.get(); +} + +content::MockDownloadManager* + ChromeDownloadManagerDelegateTest::download_manager() { + return download_manager_.get(); +} + +DownloadPrefs* ChromeDownloadManagerDelegateTest::download_prefs() { + return delegate_->download_prefs(); +} + +void ChromeDownloadManagerDelegateTest::set_clean_marker_files(bool clean) { + clean_marker_files_ = clean; +} + +} // namespace + +TEST_F(ChromeDownloadManagerDelegateTest, ShouldStartDownload_Invalid) { + // Invalid download ID shouldn't do anything. + EXPECT_CALL(*download_manager(), GetActiveDownloadItem(-1)) + .WillOnce(Return(reinterpret_cast<DownloadItem*>(NULL))); + EXPECT_FALSE(delegate()->ShouldStartDownload(-1)); +} + +TEST_F(ChromeDownloadManagerDelegateTest, StartDownload_Basic) { + const DownloadTestCase kBasicTestCases[] = { + { + // 0: Automatic Safe + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/foo.txt", "text/plain", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_INTERMEDIATE_MARKER + }, + + { + // 1: Save_As Safe + SAVE_AS, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/foo.txt", "text/plain", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.txt"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 2: Automatic Dangerous + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, + "http://example.com/foo.exe", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.exe"), + DownloadItem::TARGET_DISPOSITION_UNIQUIFY, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 3: Save_As Dangerous. For this test case, .jar is considered to be one + // of the file types supported by safe browsing. + SAVE_AS, + content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, + "http://example.com/foo.exe", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.exe"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 4 Forced Safe + FORCED, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/foo.txt", "", + FILE_PATH_LITERAL("forced-foo.txt"), + + FILE_PATH_LITERAL("forced-foo.txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 5: Forced Dangerous. As above. .jar is considered to be one of the file + // types supportred by safe browsing. + FORCED, + content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, + "http://example.com/foo.exe", "", + FILE_PATH_LITERAL("forced-foo.exe"), + + FILE_PATH_LITERAL("forced-foo.exe"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + }; + + for (unsigned i = 0; i < arraysize(kBasicTestCases); ++i) + RunTestCase(kBasicTestCases[i], i); +} + +// The SafeBrowsing URL check is performed early. Make sure that a download item +// that has been marked as DANGEROUS_URL behaves correctly. +TEST_F(ChromeDownloadManagerDelegateTest, StartDownload_DangerousURL) { + const DownloadTestCase kDangerousURLTestCases[] = { + { + // 0: Automatic Dangerous URL + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL, + "http://phishing.example.com/foo.txt", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.txt"), + DownloadItem::TARGET_DISPOSITION_UNIQUIFY, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 1: Save As Dangerous URL + SAVE_AS, + content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL, + "http://phishing.example.com/foo.txt", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.txt"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 2: Forced Dangerous URL + FORCED, + content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL, + "http://phishing.example.com/foo.txt", "", + FILE_PATH_LITERAL("forced-foo.txt"), + + FILE_PATH_LITERAL("forced-foo.txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 3: Automatic Dangerous URL + Dangerous file. Dangerous URL takes + // precendence. + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL, + "http://phishing.example.com/foo.jar", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.jar"), + DownloadItem::TARGET_DISPOSITION_UNIQUIFY, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 4: Save As Dangerous URL + Dangerous file + SAVE_AS, + content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL, + "http://phishing.example.com/foo.jar", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.jar"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 5: Forced Dangerous URL + Dangerous file + FORCED, + content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL, + "http://phishing.example.com/foo.jar", "", + FILE_PATH_LITERAL("forced-foo.jar"), + + FILE_PATH_LITERAL("forced-foo.jar"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + }; + + for (unsigned i = 0; i < arraysize(kDangerousURLTestCases); ++i) + RunTestCase(kDangerousURLTestCases[i], i); +} + +// These test cases are run with "Prompt for download" user preference set to +// true. Even with the preference set, some of these downloads should not cause +// a prompt to appear. +TEST_F(ChromeDownloadManagerDelegateTest, StartDownload_PromptAlways) { + const DownloadTestCase kPromptingTestCases[] = { + { + // 0: Safe Automatic - Should prompt because of "Prompt for download" + // preference setting. + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/foo.txt", "text/plain", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.txt"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 1: Dangerous Automatic - Should prompt due to "Prompt for download" + // preference setting. + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, + "http://example.com/foo.exe", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.exe"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 2: Automatic Browser Extension download. - Shouldn't prompt for browser + // extension downloads even if "Prompt for download" preference is set. + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/foo.crx", + extensions::Extension::kMimeType, + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.crx"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_INTERMEDIATE_MARKER + }, + + { + // 3: Automatic User Script - Shouldn't prompt for user script downloads + // even if "Prompt for download" preference is set. + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/foo.user.js", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.user.js"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_INTERMEDIATE_MARKER + }, + + { + // 4: Automatic - The filename extension is marked as one that we will + // open automatically. Shouldn't prompt. + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/foo.dummy", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.dummy"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_INTERMEDIATE_MARKER + }, + }; + + SetPromptForDownload(true); + EnableAutoOpenBasedOnExtension(FilePath(FILE_PATH_LITERAL("dummy.dummy"))); + for (unsigned i = 0; i < arraysize(kPromptingTestCases); ++i) + RunTestCase(kPromptingTestCases[i], i); +} + +// If the download path is managed, then we don't show any prompts. +// Note that if the download path is managed, then PromptForDownload() is false. +TEST_F(ChromeDownloadManagerDelegateTest, StartDownload_ManagedPath) { + const DownloadTestCase kManagedPathTestCases[] = { + { + // 0: Automatic Safe + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/foo.txt", "text/plain", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_INTERMEDIATE_MARKER + }, + + { + // 1: Save_As Safe + SAVE_AS, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/foo.txt", "text/plain", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("foo.txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_INTERMEDIATE_MARKER + }, + }; + + SetManagedDownloadPath(default_download_path()); + ASSERT_TRUE(download_prefs()->IsDownloadPathManaged()); + for (unsigned i = 0; i < arraysize(kManagedPathTestCases); ++i) + RunTestCase(kManagedPathTestCases[i], i); +} + +// If the intermediate file already exists (for safe downloads), or if the +// target file already exists, then the suggested target path should be +// uniquified. +TEST_F(ChromeDownloadManagerDelegateTest, StartDownload_ConflictingFiles) { + // Create some target files and an intermediate file for use with the test + // cases that follow: + ASSERT_TRUE(file_util::IsDirectoryEmpty(default_download_path())); + ASSERT_EQ(0, file_util::WriteFile( + GetPathInDownloadDir(FILE_PATH_LITERAL("exists.txt")), "", 0)); + ASSERT_EQ(0, file_util::WriteFile( + GetPathInDownloadDir(FILE_PATH_LITERAL("exists.jar")), "", 0)); + ASSERT_EQ(0, file_util::WriteFile( + GetPathInDownloadDir(FILE_PATH_LITERAL("exists.exe")), "", 0)); + ASSERT_EQ(0, file_util::WriteFile( + GetPathInDownloadDir(FILE_PATH_LITERAL("exists.png.crdownload")), "", 0)); + + const DownloadTestCase kConflictingFilesTestCases[] = { + { + // 0: Automatic Safe + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/exists.txt", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("exists (1).txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_INTERMEDIATE_MARKER + }, + + { + // 1: Save_As Safe + SAVE_AS, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/exists.txt", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("exists (1).txt"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 2: Automatic Dangerous + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE, + "http://example.com/exists.jar", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("exists.jar"), + DownloadItem::TARGET_DISPOSITION_UNIQUIFY, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 3: Save_As Dangerous + SAVE_AS, + content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, + "http://example.com/exists.exe", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("exists (1).exe"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 4: Automatic Maybe_Dangerous + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, + "http://example.com/exists.exe", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("exists.exe"), + DownloadItem::TARGET_DISPOSITION_UNIQUIFY, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 5: Save_As Maybe_Dangerous + SAVE_AS, + content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, + "http://example.com/exists.exe", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("exists (1).exe"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECT_UNCONFIRMED, + EXPECT_NO_OVERWRITE, + EXPECT_NO_MARKER + }, + + { + // 6: Automatic Safe - Intermediate path exists + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/exists.png", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("exists (1).png"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_INTERMEDIATE_MARKER + }, + + { + // 7: Save_As Safe - Intermediate path exists + SAVE_AS, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/exists.png", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("exists (1).png"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_NO_MARKER + }, + }; + + for (unsigned i = 0; i < arraysize(kConflictingFilesTestCases); ++i) + RunTestCase(kConflictingFilesTestCases[i], i); +} + +// Check that multiple downloads that are in-progress at the same time don't +// trample each other. This is only done in a few limited cases currently. +TEST_F(ChromeDownloadManagerDelegateTest, StartDownload_ConflictingDownloads) { + const DownloadTestCase kConflictingDownloadsTestCases[] = { + { + // 0: Initial download. + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/exists.txt", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("exists.txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_INTERMEDIATE_MARKER + }, + + { + // 1: Should uniquify around the previous download. + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/exists.txt", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("exists (1).txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_INTERMEDIATE_MARKER + }, + + { + // 2: Uniquifies around both previous downloads. + AUTOMATIC, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, + "http://example.com/exists.txt", "", + FILE_PATH_LITERAL(""), + + FILE_PATH_LITERAL("exists (2).txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECT_CRDOWNLOAD, + EXPECT_OVERWRITE, + EXPECT_INTERMEDIATE_MARKER + }, + }; + + set_clean_marker_files(false); + for (unsigned i = 0; i < arraysize(kConflictingDownloadsTestCases); ++i) + RunTestCase(kConflictingDownloadsTestCases[i], i); +} + +// TODO(asanka): Add more tests. +// * Default download path is not writable. +// * Download path doesn't exist. +// * IsDangerousFile(). +// * Filename generation. diff --git a/chrome/browser/download/download_crx_util.cc b/chrome/browser/download/download_crx_util.cc index 6b7c33d..bdfa411 100644 --- a/chrome/browser/download/download_crx_util.cc +++ b/chrome/browser/download/download_crx_util.cc @@ -88,7 +88,8 @@ scoped_refptr<CrxInstaller> OpenChromeExtension( } bool IsExtensionDownload(const DownloadItem& download_item) { - if (download_item.PromptUserForSaveLocation()) + if (download_item.GetTargetDisposition() == + DownloadItem::TARGET_DISPOSITION_PROMPT) return false; if (download_item.GetMimeType() == extensions::Extension::kMimeType || diff --git a/chrome/browser/download/download_extension_test.cc b/chrome/browser/download/download_extension_test.cc index ec21ffd..02e2c4e 100644 --- a/chrome/browser/download/download_extension_test.cc +++ b/chrome/browser/download/download_extension_test.cc @@ -55,7 +55,8 @@ class DownloadExtensionTest : public InProcessBrowserTest { // as CANCELLED. DownloadItem::DownloadState state; - // Danger type for the download. + // Danger type for the download. Only use DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS + // and DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT. content::DownloadDangerType danger_type; }; @@ -113,7 +114,9 @@ class DownloadExtensionTest : public InProcessBrowserTest { for (size_t i = 0; i < count; ++i) { if (history_info[i].danger_type != content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS) { - items->at(i)->SetDangerType(history_info[i].danger_type); + EXPECT_EQ(content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT, + history_info[i].danger_type); + items->at(i)->OnContentCheckCompleted(history_info[i].danger_type); } } return true; diff --git a/chrome/browser/download/download_history.cc b/chrome/browser/download/download_history.cc index 1176dad..aa01860 100644 --- a/chrome/browser/download/download_history.cc +++ b/chrome/browser/download/download_history.cc @@ -54,11 +54,11 @@ void DownloadHistory::CheckVisitedReferrerBefore( hs->GetVisibleVisitCountToHost(referrer_url, &history_consumer_, base::Bind(&DownloadHistory::OnGotVisitCountToHost, base::Unretained(this))); - visited_before_requests_[handle] = std::make_pair(download_id, callback); + visited_before_requests_[handle] = callback; return; } } - callback.Run(download_id, false); + callback.Run(false); } void DownloadHistory::AddEntry( @@ -139,9 +139,8 @@ void DownloadHistory::OnGotVisitCountToHost(HistoryService::Handle handle, VisitedBeforeRequestsMap::iterator request = visited_before_requests_.find(handle); DCHECK(request != visited_before_requests_.end()); - int32 download_id = request->second.first; - VisitedBeforeDoneCallback callback = request->second.second; + VisitedBeforeDoneCallback callback = request->second; visited_before_requests_.erase(request); - callback.Run(download_id, found_visits && count && + callback.Run(found_visits && count && (first_visit.LocalMidnight() < base::Time::Now().LocalMidnight())); } diff --git a/chrome/browser/download/download_history.h b/chrome/browser/download/download_history.h index a78aaf2..777dec5 100644 --- a/chrome/browser/download/download_history.h +++ b/chrome/browser/download/download_history.h @@ -1,4 +1,4 @@ -// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -26,7 +26,7 @@ class DownloadItem; // Interacts with the HistoryService on behalf of the download subsystem. class DownloadHistory { public: - typedef base::Callback<void(int32, bool)> VisitedBeforeDoneCallback; + typedef base::Callback<void(bool)> VisitedBeforeDoneCallback; explicit DownloadHistory(Profile* profile); ~DownloadHistory(); @@ -67,8 +67,7 @@ class DownloadHistory { int64 GetNextFakeDbHandle(); private: - typedef std::map<HistoryService::Handle, - std::pair<int32, VisitedBeforeDoneCallback> > + typedef std::map<HistoryService::Handle, VisitedBeforeDoneCallback> VisitedBeforeRequestsMap; void OnGotVisitCountToHost(HistoryService::Handle handle, diff --git a/chrome/browser/download/download_item_model_unittest.cc b/chrome/browser/download/download_item_model_unittest.cc index 8a4bea3..21f989c 100644 --- a/chrome/browser/download/download_item_model_unittest.cc +++ b/chrome/browser/download/download_item_model_unittest.cc @@ -55,8 +55,6 @@ class DownloadItemModelTest : public testing::Test { ON_CALL(item_, GetTotalBytes()).WillByDefault(Return(2048)); ON_CALL(item_, IsInProgress()).WillByDefault(Return(false)); ON_CALL(item_, TimeRemaining(_)).WillByDefault(Return(false)); - ON_CALL(item_, PromptUserForSaveLocation()) - .WillByDefault(Return(false)); ON_CALL(item_, GetMimeType()).WillByDefault(Return("text/html")); ON_CALL(item_, AllDataSaved()).WillByDefault(Return(false)); ON_CALL(item_, GetOpenWhenComplete()).WillByDefault(Return(false)); @@ -66,6 +64,9 @@ class DownloadItemModelTest : public testing::Test { ON_CALL(item_, GetURL()).WillByDefault(ReturnRefOfCopy(url)); ON_CALL(item_, GetFileNameToReportUser()) .WillByDefault(Return(FilePath(FILE_PATH_LITERAL("foo.bar")))); + ON_CALL(item_, GetTargetDisposition()) + .WillByDefault( + Return(content::DownloadItem::TARGET_DISPOSITION_OVERWRITE)); } void SetupInterruptedDownloadItem(content::DownloadInterruptReason reason) { diff --git a/chrome/browser/safe_browsing/download_protection_service.cc b/chrome/browser/safe_browsing/download_protection_service.cc index c87ae89..963ab84 100644 --- a/chrome/browser/safe_browsing/download_protection_service.cc +++ b/chrome/browser/safe_browsing/download_protection_service.cc @@ -202,7 +202,7 @@ DownloadProtectionService::DownloadInfo::FromDownloadItem( download_info.referrer_url = item.GetReferrerUrl(); download_info.total_bytes = item.GetTotalBytes(); download_info.remote_address = item.GetRemoteAddress(); - download_info.user_initiated = item.GetStateInfo().has_user_gesture; + download_info.user_initiated = item.HasUserGesture(); return download_info; } diff --git a/chrome/browser/safe_browsing/download_protection_service.h b/chrome/browser/safe_browsing/download_protection_service.h index 03d8427..07ecd4c 100644 --- a/chrome/browser/safe_browsing/download_protection_service.h +++ b/chrome/browser/safe_browsing/download_protection_service.h @@ -94,7 +94,7 @@ class DownloadProtectionService { // Returns true iff the download specified by |info| should be scanned by // CheckClientDownload() for malicious content. - bool IsSupportedDownload(const DownloadInfo& info) const; + virtual bool IsSupportedDownload(const DownloadInfo& info) const; // Display more information to the user regarding the download specified by // |info|. This method is invoked when the user requests more information diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 52af5a8..c13a4b3 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -1152,6 +1152,7 @@ 'browser/cookies_tree_model_unittest.cc', 'browser/custom_handlers/protocol_handler_registry_unittest.cc', 'browser/diagnostics/diagnostics_model_unittest.cc', + 'browser/download/chrome_download_manager_delegate_unittest.cc', 'browser/download/download_item_model_unittest.cc', 'browser/download/download_request_infobar_delegate_unittest.cc', 'browser/download/download_request_limiter_unittest.cc', diff --git a/content/browser/download/download_file_manager.cc b/content/browser/download/download_file_manager.cc index a9ab8ff..40f1acb 100644 --- a/content/browser/download/download_file_manager.cc +++ b/content/browser/download/download_file_manager.cc @@ -335,25 +335,43 @@ void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) { // There are 2 possible rename cases where this method can be called: // 1. tmp -> foo.crdownload (not final, safe) // 2. tmp-> Unconfirmed.xxx.crdownload (not final, dangerous) +// TODO(asanka): Merge with RenameCompletingDownloadFile() and move +// uniquification logic into DownloadFile. void DownloadFileManager::RenameInProgressDownloadFile( - DownloadId global_id, const FilePath& full_path) { + DownloadId global_id, + const FilePath& full_path, + bool overwrite_existing_file, + const RenameCompletionCallback& callback) { VLOG(20) << __FUNCTION__ << "()" << " id = " << global_id << " full_path = \"" << full_path.value() << "\""; DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); DownloadFile* download_file = GetDownloadFile(global_id); - if (!download_file) + if (!download_file) { + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, + base::Bind(callback, FilePath())); return; + } VLOG(20) << __FUNCTION__ << "()" << " download_file = " << download_file->DebugString(); + FilePath new_path(full_path); + if (!overwrite_existing_file) { + int uniquifier = + file_util::GetUniquePathNumber(new_path, FILE_PATH_LITERAL("")); + if (uniquifier > 0) { + new_path = new_path.InsertBeforeExtensionASCII( + StringPrintf(" (%d)", uniquifier)); + } + } - net::Error rename_error = download_file->Rename(full_path); + net::Error rename_error = download_file->Rename(new_path); if (net::OK != rename_error) { - // Error. Between the time the UI thread generated 'full_path' to the time - // this code runs, something happened that prevents us from renaming. CancelDownloadOnRename(global_id, rename_error); + new_path.clear(); } + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, + base::Bind(callback, new_path)); } // The DownloadManager in the UI thread has provided a final name for the @@ -366,23 +384,23 @@ void DownloadFileManager::RenameInProgressDownloadFile( void DownloadFileManager::RenameCompletingDownloadFile( DownloadId global_id, const FilePath& full_path, - bool overwrite_existing_file) { + bool overwrite_existing_file, + const RenameCompletionCallback& callback) { VLOG(20) << __FUNCTION__ << "()" << " id = " << global_id << " overwrite_existing_file = " << overwrite_existing_file << " full_path = \"" << full_path.value() << "\""; DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); DownloadFile* download_file = GetDownloadFile(global_id); - if (!download_file) + if (!download_file) { + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, + base::Bind(callback, FilePath())); return; - - DCHECK(download_file->GetDownloadManager()); - DownloadManager* download_manager = download_file->GetDownloadManager(); + } VLOG(20) << __FUNCTION__ << "()" << " download_file = " << download_file->DebugString(); - int uniquifier = 0; FilePath new_path = full_path; if (!overwrite_existing_file) { // Make our name unique at this point, as if a dangerous file is @@ -392,7 +410,7 @@ void DownloadFileManager::RenameCompletingDownloadFile( // not exists yet, so the second file gets the same name. // This should not happen in the SAFE case, and we check for that in the UI // thread. - uniquifier = + int uniquifier = file_util::GetUniquePathNumber(new_path, FILE_PATH_LITERAL("")); if (uniquifier > 0) { new_path = new_path.InsertBeforeExtensionASCII( @@ -406,23 +424,27 @@ void DownloadFileManager::RenameCompletingDownloadFile( // Error. Between the time the UI thread generated 'full_path' to the time // this code runs, something happened that prevents us from renaming. CancelDownloadOnRename(global_id, rename_error); - return; - } - + new_path.clear(); + } else { #if defined(OS_MACOSX) - // Done here because we only want to do this once; see - // http://crbug.com/13120 for details. - download_file->AnnotateWithSourceInformation(); + // Done here because we only want to do this once; see + // http://crbug.com/13120 for details. + download_file->AnnotateWithSourceInformation(); #endif + } - BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - base::Bind(&DownloadManager::OnDownloadRenamedToFinalName, - download_manager, global_id.local(), new_path, uniquifier)); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, + base::Bind(callback, new_path)); +} + +int DownloadFileManager::NumberOfActiveDownloads() const { + return downloads_.size(); } // Called only from RenameInProgressDownloadFile and // RenameCompletingDownloadFile on the FILE thread. +// TODO(asanka): Use the RenameCompletionCallback instead of a separate +// OnDownloadInterrupted call. void DownloadFileManager::CancelDownloadOnRename( DownloadId global_id, net::Error rename_error) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); diff --git a/content/browser/download/download_file_manager.h b/content/browser/download/download_file_manager.h index 255dc3d..965a93c 100644 --- a/content/browser/download/download_file_manager.h +++ b/content/browser/download/download_file_manager.h @@ -44,6 +44,7 @@ #include "base/atomic_sequence_num.h" #include "base/basictypes.h" +#include "base/callback_forward.h" #include "base/gtest_prod_util.h" #include "base/hash_tables.h" #include "base/memory/ref_counted.h" @@ -70,9 +71,15 @@ class BoundNetLog; } // Manages all in progress downloads. +// Methods are virtual to allow mocks--this class is not intended +// to be a base class. class CONTENT_EXPORT DownloadFileManager : public base::RefCountedThreadSafe<DownloadFileManager> { public: + // Callback used with RenameInProgressDownloadFile() and + // RenameCompletingDownloadFile(). + typedef base::Callback<void(const FilePath&)> RenameCompletionCallback; + class DownloadFileFactory { public: virtual ~DownloadFileFactory() {} @@ -91,56 +98,66 @@ class CONTENT_EXPORT DownloadFileManager explicit DownloadFileManager(DownloadFileFactory* factory); // Called on shutdown on the UI thread. - void Shutdown(); + virtual void Shutdown(); // Called on UI thread to make DownloadFileManager start the download. - void StartDownload(DownloadCreateInfo* info, - const DownloadRequestHandle& request_handle); + virtual void StartDownload(DownloadCreateInfo* info, + const DownloadRequestHandle& request_handle); // Handlers for notifications sent from the IO thread and run on the // FILE thread. - void UpdateDownload(content::DownloadId global_id, - content::DownloadBuffer* buffer); + virtual void UpdateDownload(content::DownloadId global_id, + content::DownloadBuffer* buffer); // |reason| is the reason for interruption, if one occurs. // |security_info| contains SSL information (cert_id, cert_status, // security_bits, ssl_connection_status), which can be used to // fine-tune the error message. It is empty if the transaction // was not performed securely. - void OnResponseCompleted(content::DownloadId global_id, - content::DownloadInterruptReason reason, - const std::string& security_info); + virtual void OnResponseCompleted(content::DownloadId global_id, + content::DownloadInterruptReason reason, + const std::string& security_info); // Handlers for notifications sent from the UI thread and run on the // FILE thread. These are both terminal actions with respect to the // download file, as far as the DownloadFileManager is concerned -- if // anything happens to the download file after they are called, it will // be ignored. - void CancelDownload(content::DownloadId id); - void CompleteDownload(content::DownloadId id); + virtual void CancelDownload(content::DownloadId id); + virtual void CompleteDownload(content::DownloadId id); // Called on FILE thread by DownloadManager at the beginning of its shutdown. - void OnDownloadManagerShutdown(content::DownloadManager* manager); - - // The DownloadManager in the UI thread has provided an intermediate - // .crdownload name for the download specified by |id|. - void RenameInProgressDownloadFile(content::DownloadId id, - const FilePath& full_path); + virtual void OnDownloadManagerShutdown(content::DownloadManager* manager); + + // The DownloadManager in the UI thread has provided an intermediate name for + // the download specified by |id|. |overwrite_existing_file| indicates whether + // any existing file at |full_path| should be overwritten. If false, adds a + // uniquifier to |full_path| and uses the resulting name as the intermediate + // path for the download. Invokes |callback| with the new path on success. If + // the rename fails, calls CancelDownloadOnRename() and invokes |callback| + // with an empty FilePath(). + virtual void RenameInProgressDownloadFile( + content::DownloadId id, + const FilePath& full_path, + bool overwrite_existing_file, + const RenameCompletionCallback& callback); // The DownloadManager in the UI thread has provided a final name for the - // download specified by |id|. - // |overwrite_existing_file| prevents uniquification, and is used for SAFE - // downloads, as the user may have decided to overwrite the file. - // Sent from the UI thread and run on the FILE thread. - void RenameCompletingDownloadFile(content::DownloadId id, - const FilePath& full_path, - bool overwrite_existing_file); + // download specified by |id|. |overwrite_existing_file| prevents + // uniquification, and is used for SAFE downloads, as the user may have + // decided to overwrite the file. Sent from the UI thread and run on the FILE + // thread. Invokes |callback| with the new path on success. If the rename + // fails, calls CancelDownloadOnRename() and invokes |callback| with an empty + // FilePath(). + virtual void RenameCompletingDownloadFile( + content::DownloadId id, + const FilePath& full_path, + bool overwrite_existing_file, + const RenameCompletionCallback& callback); // The number of downloads currently active on the DownloadFileManager. // Primarily for testing. - int NumberOfActiveDownloads() const { - return downloads_.size(); - } + virtual int NumberOfActiveDownloads() const; void SetFileFactoryForTesting(scoped_ptr<DownloadFileFactory> file_factory) { download_file_factory_.reset(file_factory.release()); @@ -150,14 +167,15 @@ class CONTENT_EXPORT DownloadFileManager return download_file_factory_.get(); // Explicitly NOT a scoped_ptr. } + protected: + virtual ~DownloadFileManager(); + private: friend class base::RefCountedThreadSafe<DownloadFileManager>; friend class DownloadFileManagerTest; friend class DownloadManagerTest; FRIEND_TEST_ALL_PREFIXES(DownloadManagerTest, StartDownload); - ~DownloadFileManager(); - // Timer helpers for updating the UI about the current progress of a download. void StartUpdateTimer(); void StopUpdateTimer(); diff --git a/content/browser/download/download_file_manager_unittest.cc b/content/browser/download/download_file_manager_unittest.cc index 15eab3a..43863c4 100644 --- a/content/browser/download/download_file_manager_unittest.cc +++ b/content/browser/download/download_file_manager_unittest.cc @@ -35,6 +35,15 @@ using ::testing::StrEq; namespace { +// MockDownloadManager with the addition of a mock callback used for testing. +class TestDownloadManager : public MockDownloadManager { + public: + MOCK_METHOD2(OnDownloadRenamed, + void(int download_id, const FilePath& full_path)); + private: + ~TestDownloadManager() {} +}; + class MockDownloadFileFactory : public DownloadFileManager::DownloadFileFactory { @@ -136,7 +145,7 @@ class DownloadFileManagerTest : public testing::Test { } virtual void SetUp() { - download_manager_ = new MockDownloadManager(); + download_manager_ = new TestDownloadManager(); request_handle_.reset(new MockDownloadRequestHandle(download_manager_)); download_file_factory_ = new MockDownloadFileFactory; download_file_manager_ = new DownloadFileManager(download_file_factory_); @@ -295,33 +304,9 @@ class DownloadFileManagerTest : public testing::Test { net::Error rename_error, RenameFileState state, RenameFileOverwrite should_overwrite) { - // Expected call sequence: - // RenameInProgressDownloadFile/RenameCompletingDownloadFile - // GetDownloadFile - // DownloadFile::Rename - // - // On Error: - // CancelDownloadOnRename - // GetDownloadFile - // DownloadFile::GetDownloadManager - // No Manager: - // DownloadFile::CancelDownloadRequest/return - // Has Manager: - // DownloadFile::BytesSoFar - // Process one message in the message loop - // DownloadManager::OnDownloadInterrupted - // - // On Success, if final rename: - // Process one message in the message loop - // DownloadManager::OnDownloadRenamedToFinalName MockDownloadFile* file = download_file_factory_->GetExistingFile(id); ASSERT_TRUE(file != NULL); - if (state == COMPLETE || rename_error != net::OK) { - EXPECT_CALL(*file, GetDownloadManager()) - .Times(AtLeast(1)); - } - EXPECT_CALL(*file, Rename(unique_path)) .Times(1) .WillOnce(Return(rename_error)); @@ -332,13 +317,24 @@ class DownloadFileManagerTest : public testing::Test { .WillRepeatedly(Return(byte_count_[id])); EXPECT_CALL(*file, GetHashState()) .Times(AtLeast(1)); + EXPECT_CALL(*file, GetDownloadManager()) + .Times(AtLeast(1)); + } else if (state == COMPLETE) { +#if defined(OS_MACOSX) + EXPECT_CALL(*file, AnnotateWithSourceInformation()); +#endif } if (state == IN_PROGRESS) { - download_file_manager_->RenameInProgressDownloadFile(id, new_path); + download_file_manager_->RenameInProgressDownloadFile( + id, new_path, (should_overwrite == OVERWRITE), + base::Bind(&TestDownloadManager::OnDownloadRenamed, + download_manager_, id.local())); } else { // state == COMPLETE download_file_manager_->RenameCompletingDownloadFile( - id, new_path, (should_overwrite == OVERWRITE)); + id, new_path, (should_overwrite == OVERWRITE), + base::Bind(&TestDownloadManager::OnDownloadRenamed, + download_manager_, id.local())); } if (rename_error != net::OK) { @@ -350,11 +346,13 @@ class DownloadFileManagerTest : public testing::Test { content::ConvertNetErrorToInterruptReason( rename_error, content::DOWNLOAD_INTERRUPT_FROM_DISK))); + EXPECT_CALL(*download_manager_, + OnDownloadRenamed(id.local(), FilePath())); ProcessAllPendingMessages(); ++error_count_[id]; - } else if (state == COMPLETE) { - EXPECT_CALL(*download_manager_, OnDownloadRenamedToFinalName( - id.local(), unique_path, _)); + } else { + EXPECT_CALL(*download_manager_, + OnDownloadRenamed(id.local(), unique_path)); ProcessAllPendingMessages(); } } @@ -438,7 +436,7 @@ class DownloadFileManagerTest : public testing::Test { } protected: - scoped_refptr<MockDownloadManager> download_manager_; + scoped_refptr<TestDownloadManager> download_manager_; scoped_ptr<MockDownloadRequestHandle> request_handle_; MockDownloadFileFactory* download_file_factory_; scoped_refptr<DownloadFileManager> download_file_manager_; @@ -560,6 +558,28 @@ TEST_F(DownloadFileManagerTest, RenameInProgress) { CleanUp(dummy_id); } +TEST_F(DownloadFileManagerTest, RenameInProgressWithUniquification) { + // Same as StartDownload, at first. + DownloadCreateInfo* info = new DownloadCreateInfo; + DownloadId dummy_id(download_manager_.get(), kDummyDownloadId); + ScopedTempDir download_dir; + ASSERT_TRUE(download_dir.CreateUniqueTempDir()); + + StartDownload(info, dummy_id); + + UpdateDownload(dummy_id, kTestData1, strlen(kTestData1), net::OK); + UpdateDownload(dummy_id, kTestData2, strlen(kTestData2), net::OK); + FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt"))); + FilePath unique_foo(foo.InsertBeforeExtension(FILE_PATH_LITERAL(" (1)"))); + ASSERT_EQ(0, file_util::WriteFile(foo, "", 0)); + RenameFile(dummy_id, foo, unique_foo, net::OK, IN_PROGRESS, DONT_OVERWRITE); + UpdateDownload(dummy_id, kTestData3, strlen(kTestData3), net::OK); + + OnResponseCompleted(dummy_id, content::DOWNLOAD_INTERRUPT_REASON_NONE, ""); + + CleanUp(dummy_id); +} + TEST_F(DownloadFileManagerTest, RenameInProgressWithError) { // Same as StartDownload, at first. DownloadCreateInfo* info = new DownloadCreateInfo; diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc index b931744..2f967a8 100644 --- a/content/browser/download/download_item_impl.cc +++ b/content/browser/download/download_item_impl.cc @@ -164,15 +164,20 @@ DownloadItemImpl::DownloadItemImpl(Delegate* delegate, const DownloadPersistentStoreInfo& info, const net::BoundNetLog& bound_net_log) : download_id_(download_id), - full_path_(info.path), + current_path_(info.path), + target_path_(info.path), + target_disposition_(TARGET_DISPOSITION_OVERWRITE), url_chain_(1, info.url), referrer_url_(info.referrer_url), + transition_type_(content::PAGE_TRANSITION_LINK), + has_user_gesture_(false), total_bytes_(info.total_bytes), received_bytes_(info.received_bytes), bytes_per_sec_(0), last_reason_(content::DOWNLOAD_INTERRUPT_REASON_NONE), start_tick_(base::TimeTicks()), state_(static_cast<DownloadState>(info.state)), + danger_type_(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS), start_time_(info.start_time), end_time_(info.end_time), db_handle_(info.db_handle), @@ -189,7 +194,8 @@ DownloadItemImpl::DownloadItemImpl(Delegate* delegate, opened_(info.opened), open_enabled_(true), delegate_delayed_complete_(false), - bound_net_log_(bound_net_log) { + bound_net_log_(bound_net_log), + ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) { delegate_->Attach(); if (IsInProgress()) state_ = CANCELLED; @@ -206,14 +212,17 @@ DownloadItemImpl::DownloadItemImpl( DownloadRequestHandleInterface* request_handle, bool is_otr, const net::BoundNetLog& bound_net_log) - : state_info_(info.save_info.file_path, - info.has_user_gesture, info.transition_type, - info.prompt_user_for_save_location), - request_handle_(request_handle), + : request_handle_(request_handle), download_id_(info.download_id), + target_disposition_( + (info.prompt_user_for_save_location) ? + TARGET_DISPOSITION_PROMPT : TARGET_DISPOSITION_OVERWRITE), url_chain_(info.url_chain), referrer_url_(info.referrer_url), suggested_filename_(UTF16ToUTF8(info.save_info.suggested_name)), + forced_file_path_(info.save_info.file_path), + transition_type_(info.transition_type), + has_user_gesture_(info.has_user_gesture), content_disposition_(info.content_disposition), mime_type_(info.mime_type), original_mime_type_(info.original_mime_type), @@ -225,6 +234,7 @@ DownloadItemImpl::DownloadItemImpl( last_reason_(content::DOWNLOAD_INTERRUPT_REASON_NONE), start_tick_(base::TimeTicks::Now()), state_(IN_PROGRESS), + danger_type_(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS), start_time_(info.start_time), db_handle_(DownloadItem::kUninitializedHandle), delegate_(delegate), @@ -240,7 +250,8 @@ DownloadItemImpl::DownloadItemImpl( opened_(false), open_enabled_(true), delegate_delayed_complete_(false), - bound_net_log_(bound_net_log) { + bound_net_log_(bound_net_log), + ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) { delegate_->Attach(); Init(true /* actively downloading */, download_net_logs::SRC_NEW_DOWNLOAD); @@ -269,9 +280,13 @@ DownloadItemImpl::DownloadItemImpl(Delegate* delegate, const net::BoundNetLog& bound_net_log) : request_handle_(new NullDownloadRequestHandle()), download_id_(download_id), - full_path_(path), + current_path_(path), + target_path_(path), + target_disposition_(TARGET_DISPOSITION_OVERWRITE), url_chain_(1, url), referrer_url_(GURL()), + transition_type_(content::PAGE_TRANSITION_LINK), + has_user_gesture_(false), mime_type_(mime_type), original_mime_type_(mime_type), total_bytes_(0), @@ -280,6 +295,7 @@ DownloadItemImpl::DownloadItemImpl(Delegate* delegate, last_reason_(content::DOWNLOAD_INTERRUPT_REASON_NONE), start_tick_(base::TimeTicks::Now()), state_(IN_PROGRESS), + danger_type_(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS), start_time_(base::Time::Now()), db_handle_(DownloadItem::kUninitializedHandle), delegate_(delegate), @@ -295,7 +311,8 @@ DownloadItemImpl::DownloadItemImpl(Delegate* delegate, opened_(false), open_enabled_(true), delegate_delayed_complete_(false), - bound_net_log_(bound_net_log) { + bound_net_log_(bound_net_log), + ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) { delegate_->Attach(); Init(true /* actively downloading */, download_net_logs::SRC_SAVE_PAGE_AS); @@ -576,26 +593,35 @@ void DownloadItemImpl::TransitionTo(DownloadState new_state) { bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, NULL); } -void DownloadItemImpl::UpdateSafetyState() { - SafetyState updated_value = state_info_.IsDangerous() ? - DownloadItem::DANGEROUS : DownloadItem::SAFE; +void DownloadItemImpl::SetDangerType(content::DownloadDangerType danger_type) { + danger_type_ = danger_type; + // Notify observers if the safety state has changed as a result of the new + // danger type. + SafetyState updated_value = IsDangerous() ? + DownloadItem::DANGEROUS : DownloadItem::SAFE; if (updated_value != safety_state_) { safety_state_ = updated_value; - bound_net_log_.AddEvent( net::NetLog::TYPE_DOWNLOAD_ITEM_SAFETY_STATE_UPDATED, make_scoped_refptr(new download_net_logs::ItemCheckedParameters( GetDangerType(), GetSafetyState()))); - UpdateObservers(); } } -void DownloadItemImpl::UpdateTarget() { +void DownloadItemImpl::SetFullPath(const FilePath& new_path) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + VLOG(20) << __FUNCTION__ << "()" + << " new_path = \"" << new_path.value() << "\"" + << " " << DebugString(true); + DCHECK(!new_path.empty()); + current_path_ = new_path; - if (state_info_.target_name.value().empty()) - state_info_.target_name = full_path_.BaseName(); + bound_net_log_.AddEvent( + net::NetLog::TYPE_DOWNLOAD_ITEM_RENAMED, + make_scoped_refptr( + new download_net_logs::ItemRenamedParameters( + current_path_.AsUTF8Unsafe(), new_path.AsUTF8Unsafe()))); } void DownloadItemImpl::Interrupted(int64 size, @@ -632,8 +658,10 @@ void DownloadItemImpl::Delete(DeleteReason reason) { NOTREACHED(); } - BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, - base::Bind(&DeleteDownloadedFile, full_path_)); + // TODO(asanka): Avoid deleting a file that is still owned by DownloadFile. + if (!current_path_.empty()) + BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, + base::Bind(&DeleteDownloadedFile, current_path_)); Remove(); // We have now been deleted. } @@ -678,32 +706,6 @@ int DownloadItemImpl::PercentComplete() const { return static_cast<int>(received_bytes_ * 100.0 / total_bytes_); } -void DownloadItemImpl::OnPathDetermined(const FilePath& path) { - full_path_ = path; - // If we prompted the user, then target_name is stale. Allow it to be - // populated by UpdateTarget(). - if (PromptUserForSaveLocation()) - state_info_.target_name.clear(); - UpdateTarget(); -} - -void DownloadItemImpl::Rename(const FilePath& full_path) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - - VLOG(20) << __FUNCTION__ << "()" - << " full_path = \"" << full_path.value() << "\"" - << " " << DebugString(true); - DCHECK(!full_path.empty()); - - bound_net_log_.AddEvent( - net::NetLog::TYPE_DOWNLOAD_ITEM_RENAMED, - make_scoped_refptr( - new download_net_logs::ItemRenamedParameters( - full_path_.AsUTF8Unsafe(), full_path.AsUTF8Unsafe()))); - - full_path_ = full_path; -} - void DownloadItemImpl::TogglePause() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); @@ -726,29 +728,32 @@ void DownloadItemImpl::OnDownloadCompleting(DownloadFileManager* file_manager) { DCHECK_NE(DANGEROUS, GetSafetyState()); DCHECK(file_manager); - // If we prompted the user for save location, then we should overwrite the - // target. Otherwise, if the danger state was NOT_DANGEROUS, we already - // uniquified the path and should overwrite. - bool should_overwrite = - (PromptUserForSaveLocation() || - GetDangerType() == content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); + // TODO(asanka): Reduce code duplication across the NeedsRename() and + // !NeedsRename() completion pathways. if (NeedsRename()) { - BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, + bool should_overwrite = + (GetTargetDisposition() != DownloadItem::TARGET_DISPOSITION_UNIQUIFY); + DownloadFileManager::RenameCompletionCallback callback = + base::Bind(&DownloadItemImpl::OnDownloadRenamedToFinalName, + weak_ptr_factory_.GetWeakPtr(), + base::Unretained(file_manager)); + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, base::Bind(&DownloadFileManager::RenameCompletingDownloadFile, - file_manager, download_id_, - GetTargetFilePath(), should_overwrite)); - return; + file_manager, GetGlobalId(), GetTargetFilePath(), + should_overwrite, callback)); + } else { + Completed(); + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, + base::Bind(&DownloadFileManager::CompleteDownload, + file_manager, download_id_)); } - - Completed(); - - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - base::Bind(&DownloadFileManager::CompleteDownload, - file_manager, download_id_)); } -void DownloadItemImpl::OnDownloadRenamedToFinalName(const FilePath& full_path) { +void DownloadItemImpl::OnDownloadRenamedToFinalName( + DownloadFileManager* file_manager, + const FilePath& full_path) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); VLOG(20) << __FUNCTION__ << "()" @@ -757,13 +762,32 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(const FilePath& full_path) { << " " << DebugString(false); DCHECK(NeedsRename()); - Rename(full_path); + if (!full_path.empty()) { + // full_path is now the current and target file path. + target_path_ = full_path; + SetFullPath(full_path); + delegate_->DownloadRenamedToFinalName(this); + + if (delegate_->ShouldOpenDownload(this)) + Completed(); + else + delegate_delayed_complete_ = true; + + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, + base::Bind(&DownloadFileManager::CompleteDownload, + file_manager, GetGlobalId())); + } +} - if (delegate_->ShouldOpenDownload(this)) { - Completed(); - } else { - delegate_delayed_complete_ = true; +void DownloadItemImpl::OnDownloadRenamedToIntermediateName( + const FilePath& full_path) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + if (!full_path.empty()) { + SetFullPath(full_path); + UpdateObservers(); } + delegate_->DownloadRenamedToIntermediateName(this); } bool DownloadItemImpl::MatchesQuery(const string16& query) const { @@ -788,35 +812,33 @@ bool DownloadItemImpl::MatchesQuery(const string16& query) const { if (base::i18n::StringSearchIgnoringCaseAndAccents(query, url_formatted)) return true; + // TODO(asanka): Change this to GetTargetFilePath() once DownloadQuery has + // been modified to work with target paths. string16 path(GetFullPath().LossyDisplayName()); return base::i18n::StringSearchIgnoringCaseAndAccents(query, path); } -void DownloadItemImpl::SetFileCheckResults(const DownloadStateInfo& state) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - - VLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true); - state_info_ = state; - VLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true); - - UpdateSafetyState(); -} - content::DownloadDangerType DownloadItemImpl::GetDangerType() const { - return state_info_.danger; -} - -void DownloadItemImpl::SetDangerType(content::DownloadDangerType danger_type) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - state_info_.danger = danger_type; - UpdateSafetyState(); + return danger_type_; } +// TODO(asanka): Unify GetSafetyState() and IsDangerous(). bool DownloadItemImpl::IsDangerous() const { - return state_info_.IsDangerous(); + // TODO(noelutz): At this point only the windows views UI supports + // warnings based on dangerous content. +#ifdef OS_WIN + return (danger_type_ == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE || + danger_type_ == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL || + danger_type_ == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT || + danger_type_ == content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT); +#else + return (danger_type_ == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE || + danger_type_ == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL); +#endif } DownloadPersistentStoreInfo DownloadItemImpl::GetPersistentStoreInfo() const { + // TODO(asanka): Persist GetTargetFilePath() as well. return DownloadPersistentStoreInfo(GetFullPath(), GetURL(), GetReferrerUrl(), @@ -843,18 +865,61 @@ content::BrowserContext* DownloadItemImpl::GetBrowserContext() const { return delegate_->GetBrowserContext(); } -FilePath DownloadItemImpl::GetTargetFilePath() const { - return full_path_.DirName().Append(state_info_.target_name); +const FilePath& DownloadItemImpl::GetTargetFilePath() const { + return target_path_; +} + +DownloadItem::TargetDisposition DownloadItemImpl::GetTargetDisposition() const { + return target_disposition_; +} + +void DownloadItemImpl::OnTargetPathDetermined( + const FilePath& target_path, + TargetDisposition disposition, + content::DownloadDangerType danger_type) { + // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved. + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + target_path_ = target_path; + target_disposition_ = disposition; + SetDangerType(danger_type); +} + +void DownloadItemImpl::OnTargetPathSelected(const FilePath& target_path) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK_EQ(TARGET_DISPOSITION_PROMPT, target_disposition_); + target_path_ = target_path; +} + +void DownloadItemImpl::OnContentCheckCompleted( + content::DownloadDangerType danger_type) { + // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved. + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(AllDataSaved()); + SetDangerType(danger_type); +} + +void DownloadItemImpl::OnIntermediatePathDetermined( + DownloadFileManager* file_manager, + const FilePath& intermediate_path, + bool ok_to_overwrite) { + DownloadFileManager::RenameCompletionCallback callback = + base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName, + weak_ptr_factory_.GetWeakPtr()); + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, + base::Bind(&DownloadFileManager::RenameInProgressDownloadFile, + file_manager, GetGlobalId(), intermediate_path, + ok_to_overwrite, callback)); +} + +const FilePath& DownloadItemImpl::GetFullPath() const { + return current_path_; } FilePath DownloadItemImpl::GetFileNameToReportUser() const { if (!display_name_.empty()) return display_name_; - if (state_info_.path_uniquifier > 0) { - return state_info_.target_name.InsertBeforeExtensionASCII( - StringPrintf(" (%d)", state_info_.path_uniquifier)); - } - return state_info_.target_name; + return target_path_.BaseName(); } void DownloadItemImpl::SetDisplayName(const FilePath& name) { @@ -863,7 +928,7 @@ void DownloadItemImpl::SetDisplayName(const FilePath& name) { FilePath DownloadItemImpl::GetUserVerifiedFilePath() const { return (safety_state_ == DownloadItem::SAFE) ? - GetTargetFilePath() : full_path_; + GetTargetFilePath() : GetFullPath(); } void DownloadItemImpl::OffThreadCancel(DownloadFileManager* file_manager) { @@ -880,17 +945,18 @@ void DownloadItemImpl::Init(bool active, download_net_logs::DownloadType download_type) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - UpdateTarget(); if (active) download_stats::RecordDownloadCount(download_stats::START_COUNT); + if (target_path_.empty()) + target_path_ = current_path_; std::string file_name; if (download_type == download_net_logs::SRC_HISTORY_IMPORT) { - // full_path_ works for History and Save As versions. - file_name = full_path_.AsUTF8Unsafe(); + // target_path_ works for History and Save As versions. + file_name = target_path_.AsUTF8Unsafe(); } else { // See if it's set programmatically. - file_name = state_info_.force_file_name.AsUTF8Unsafe(); + file_name = forced_file_path_.AsUTF8Unsafe(); // Possibly has a 'download' attribute for the anchor. if (file_name.empty()) file_name = suggested_filename_; @@ -924,6 +990,11 @@ void DownloadItemImpl::Init(bool active, VLOG(20) << __FUNCTION__ << "() " << DebugString(true); } +bool DownloadItemImpl::NeedsRename() const { + DCHECK(target_path_.DirName() == current_path_.DirName()); + return target_path_ != current_path_; +} + // TODO(ahendrickson) -- Move |INTERRUPTED| from |IsCancelled()| to // |IsPartialDownload()|, when resuming interrupted downloads is implemented. bool DownloadItemImpl::IsPartialDownload() const { @@ -985,8 +1056,8 @@ std::string DownloadItemImpl::DebugString(bool verbose) const { " last_modified = '%s'" " etag = '%s'" " url_chain = \n\t\"%s\"\n\t" - " target = \"%" PRFilePath "\"" - " full_path = \"%" PRFilePath "\"", + " full_path = \"%" PRFilePath "\"" + " target_path = \"%" PRFilePath "\"", GetDbHandle(), GetTotalBytes(), GetReceivedBytes(), @@ -997,8 +1068,8 @@ std::string DownloadItemImpl::DebugString(bool verbose) const { GetLastModifiedTime().c_str(), GetETag().c_str(), url_list.c_str(), - state_info_.target_name.value().c_str(), - GetFullPath().value().c_str()); + GetFullPath().value().c_str(), + GetTargetFilePath().value().c_str()); } else { description += base::StringPrintf(" url = \"%s\"", url_list.c_str()); } @@ -1012,10 +1083,6 @@ bool DownloadItemImpl::AllDataSaved() const { return all_data_saved_; } DownloadItem::DownloadState DownloadItemImpl::GetState() const { return state_; } -const FilePath& DownloadItemImpl::GetFullPath() const { return full_path_; } -void DownloadItemImpl::SetPathUniquifier(int uniquifier) { - state_info_.path_uniquifier = uniquifier; -} const std::vector<GURL>& DownloadItemImpl::GetUrlChain() const { return url_chain_; } @@ -1086,15 +1153,20 @@ DownloadItem::SafetyState DownloadItemImpl::GetSafetyState() const { } bool DownloadItemImpl::IsOtr() const { return is_otr_; } bool DownloadItemImpl::GetAutoOpened() { return auto_opened_; } -const FilePath& DownloadItemImpl::GetTargetName() const { - return state_info_.target_name; -} -bool DownloadItemImpl::PromptUserForSaveLocation() const { - return state_info_.prompt_user_for_save_location; +FilePath DownloadItemImpl::GetTargetName() const { + return target_path_.BaseName(); } -const FilePath& DownloadItemImpl::GetSuggestedPath() const { - return state_info_.suggested_path; +const FilePath& DownloadItemImpl::GetForcedFilePath() const { + // TODO(asanka): Get rid of GetForcedFilePath(). We should instead just + // require that clients respect GetTargetFilePath() if it is already set. + return forced_file_path_; } +bool DownloadItemImpl::HasUserGesture() const { + return has_user_gesture_; +}; +content::PageTransition DownloadItemImpl::GetTransitionType() const { + return transition_type_; +}; bool DownloadItemImpl::IsTemporary() const { return is_temporary_; } void DownloadItemImpl::SetIsTemporary(bool temporary) { is_temporary_ = temporary; @@ -1108,10 +1180,6 @@ const std::string& DownloadItemImpl::GetETag() const { return etag_; } content::DownloadInterruptReason DownloadItemImpl::GetLastReason() const { return last_reason_; } -DownloadStateInfo DownloadItemImpl::GetStateInfo() const { return state_info_; } -bool DownloadItemImpl::NeedsRename() const { - return state_info_.target_name != full_path_.BaseName(); -} void DownloadItemImpl::MockDownloadOpenForTesting() { open_enabled_ = false; } DownloadItem::ExternalData* diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h index 1bcdcd4..2b1d7c8 100644 --- a/content/browser/download/download_item_impl.h +++ b/content/browser/download/download_item_impl.h @@ -11,6 +11,7 @@ #include "base/basictypes.h" #include "base/file_path.h" #include "base/memory/scoped_ptr.h" +#include "base/memory/weak_ptr.h" #include "base/observer_list.h" #include "base/time.h" #include "base/timer.h" @@ -69,6 +70,8 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { virtual void DownloadCompleted(DownloadItem* download) = 0; virtual void DownloadOpened(DownloadItem* download) = 0; virtual void DownloadRemoved(DownloadItem* download) = 0; + virtual void DownloadRenamedToIntermediateName(DownloadItem* download) = 0; + virtual void DownloadRenamedToFinalName(DownloadItem* download) = 0; // Assert consistent state for delgate object at various transitions. virtual void AssertStateConsistent(DownloadItem* download) const = 0; @@ -137,13 +140,9 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { virtual bool TimeRemaining(base::TimeDelta* remaining) const OVERRIDE; virtual int64 CurrentSpeed() const OVERRIDE; virtual int PercentComplete() const OVERRIDE; - virtual void OnPathDetermined(const FilePath& path) OVERRIDE; virtual bool AllDataSaved() const OVERRIDE; - virtual void SetFileCheckResults(const DownloadStateInfo& state) OVERRIDE; - virtual void Rename(const FilePath& full_path) OVERRIDE; virtual void TogglePause() OVERRIDE; virtual void OnDownloadCompleting(DownloadFileManager* file_manager) OVERRIDE; - virtual void OnDownloadRenamedToFinalName(const FilePath& full_path) OVERRIDE; virtual bool MatchesQuery(const string16& query) const OVERRIDE; virtual bool IsPartialDownload() const OVERRIDE; virtual bool IsInProgress() const OVERRIDE; @@ -152,7 +151,18 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { virtual bool IsComplete() const OVERRIDE; virtual DownloadState GetState() const OVERRIDE; virtual const FilePath& GetFullPath() const OVERRIDE; - virtual void SetPathUniquifier(int uniquifier) OVERRIDE; + virtual const FilePath& GetTargetFilePath() const OVERRIDE; + virtual TargetDisposition GetTargetDisposition() const OVERRIDE; + virtual void OnTargetPathDetermined( + const FilePath& target_path, + TargetDisposition disposition, + content::DownloadDangerType danger_type) OVERRIDE; + virtual void OnTargetPathSelected(const FilePath& target_path) OVERRIDE; + virtual void OnContentCheckCompleted( + content::DownloadDangerType danger_type) OVERRIDE; + virtual void OnIntermediatePathDetermined(DownloadFileManager* file_manager, + const FilePath& path, + bool ok_to_overwrite) OVERRIDE; virtual const GURL& GetURL() const OVERRIDE; virtual const std::vector<GURL>& GetUrlChain() const OVERRIDE; virtual const GURL& GetOriginalUrl() const OVERRIDE; @@ -182,13 +192,13 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { virtual bool GetFileExternallyRemoved() const OVERRIDE; virtual SafetyState GetSafetyState() const OVERRIDE; virtual content::DownloadDangerType GetDangerType() const OVERRIDE; - virtual void SetDangerType(content::DownloadDangerType danger_type) OVERRIDE; virtual bool IsDangerous() const OVERRIDE; virtual bool GetAutoOpened() OVERRIDE; - virtual const FilePath& GetTargetName() const OVERRIDE; - virtual bool PromptUserForSaveLocation() const OVERRIDE; + virtual FilePath GetTargetName() const OVERRIDE; + virtual const FilePath& GetForcedFilePath() const OVERRIDE; + virtual bool HasUserGesture() const OVERRIDE; + virtual content::PageTransition GetTransitionType() const OVERRIDE; virtual bool IsOtr() const OVERRIDE; - virtual const FilePath& GetSuggestedPath() const OVERRIDE; virtual bool IsTemporary() const OVERRIDE; virtual void SetIsTemporary(bool temporary) OVERRIDE; virtual void SetOpened(bool opened) OVERRIDE; @@ -198,14 +208,11 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { virtual content::DownloadInterruptReason GetLastReason() const OVERRIDE; virtual content::DownloadPersistentStoreInfo GetPersistentStoreInfo() const OVERRIDE; - virtual DownloadStateInfo GetStateInfo() const OVERRIDE; virtual content::BrowserContext* GetBrowserContext() const OVERRIDE; virtual content::WebContents* GetWebContents() const OVERRIDE; - virtual FilePath GetTargetFilePath() const OVERRIDE; virtual FilePath GetFileNameToReportUser() const OVERRIDE; virtual void SetDisplayName(const FilePath& name) OVERRIDE; virtual FilePath GetUserVerifiedFilePath() const OVERRIDE; - virtual bool NeedsRename() const OVERRIDE; virtual void OffThreadCancel(DownloadFileManager* file_manager) OVERRIDE; virtual std::string DebugString(bool verbose) const OVERRIDE; virtual void MockDownloadOpenForTesting() OVERRIDE; @@ -220,6 +227,10 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { // this is. void Init(bool active, download_net_logs::DownloadType download_type); + // Returns true if the download still needs to be renamed to + // GetTargetFilePath(). + bool NeedsRename() const; + // Internal helper for maintaining consistent received and total sizes, and // hash state. void UpdateProgress(int64 bytes_so_far, const std::string& hash_state); @@ -242,20 +253,19 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { // Call to transition state; all state transitions should go through this. void TransitionTo(DownloadState new_state); - // Called when safety_state_ should be recomputed from is_dangerous_file - // and is_dangerous_url. - void UpdateSafetyState(); + // Set the |danger_type_| and invoke obserers if necessary. + void SetDangerType(content::DownloadDangerType danger_type); - // Helper function to recompute |state_info_.target_name| when - // it may have changed. (If it's non-null it should be left alone, - // otherwise updated from |full_path_|.) - void UpdateTarget(); + // Set the |current_path_| to |new_path|. + void SetFullPath(const FilePath& new_path); - // State information used by the download manager. - DownloadStateInfo state_info_; + // Callback invoked when the download has been renamed to its final name. + void OnDownloadRenamedToFinalName(DownloadFileManager* file_manager, + const FilePath& full_path); - // Display name for the download if different from the target filename. - FilePath display_name_; + // Callback invoked when the download has been renamed to its intermediate + // name. + void OnDownloadRenamedToIntermediateName(const FilePath& full_path); // The handle to the request information. Used for operations outside the // download system. @@ -264,8 +274,23 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { // Download ID assigned by DownloadResourceHandler. content::DownloadId download_id_; - // Full path to the downloaded or downloading file. - FilePath full_path_; + // Display name for the download. If this is empty, then the display name is + // considered to be |target_path_.BaseName()|. + FilePath display_name_; + + // Full path to the downloaded or downloading file. This is the path to the + // physical file, if one exists. The final target path is specified by + // |target_path_|. |current_path_| can be empty if the in-progress path hasn't + // been determined. + FilePath current_path_; + + // Target path of an in-progress download. We may be downloading to a + // temporary or intermediate file (specified by |current_path_|. Once the + // download completes, we will rename the file to |target_path_|. + FilePath target_path_; + + // Whether the target should be overwritten, uniquified or prompted for. + TargetDisposition target_disposition_; // The chain of redirects that leading up to and including the final URL. std::vector<GURL> url_chain_; @@ -278,6 +303,16 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { // http://www.whatwg.org/specs/web-apps/current-work/#downloading-hyperlinks std::string suggested_filename_; + // If non-empty, contains an externally supplied path that should be used as + // the target path. + FilePath forced_file_path_; + + // Page transition that triggerred the download. + content::PageTransition transition_type_; + + // Whether the download was triggered with a user gesture. + bool has_user_gesture_; + // Information from the request. // Content-disposition field from the header. std::string content_disposition_; @@ -331,6 +366,9 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { // The current state of this download. DownloadState state_; + // Current danger type for the download. + content::DownloadDangerType danger_type_; + // The views of this item in the download shelf and download contents. ObserverList<Observer> observers_; @@ -394,6 +432,8 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { // Net log to use for this download. const net::BoundNetLog bound_net_log_; + base::WeakPtrFactory<DownloadItemImpl> weak_ptr_factory_; + DISALLOW_COPY_AND_ASSIGN(DownloadItemImpl); }; diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc index d4c62a0..0700f88 100644 --- a/content/browser/download/download_item_impl_unittest.cc +++ b/content/browser/download/download_item_impl_unittest.cc @@ -6,6 +6,7 @@ #include "base/stl_util.h" #include "base/threading/thread.h" #include "content/browser/download/download_create_info.h" +#include "content/browser/download/download_file_manager.h" #include "content/browser/download/download_item_impl.h" #include "content/browser/download/download_request_handle.h" #include "content/public/browser/download_id.h" @@ -21,6 +22,9 @@ using content::DownloadItem; using content::DownloadManager; using content::MockDownloadItem; using content::WebContents; +using ::testing::_; +using ::testing::AllOf; +using ::testing::Property; DownloadId::Domain kValidDownloadItemIdDomain = "valid DownloadId::Domain"; @@ -36,6 +40,8 @@ class MockDelegate : public DownloadItemImpl::Delegate { MOCK_METHOD1(DownloadCompleted, void(DownloadItem* download)); MOCK_METHOD1(DownloadOpened, void(DownloadItem* download)); MOCK_METHOD1(DownloadRemoved, void(DownloadItem* download)); + MOCK_METHOD1(DownloadRenamedToIntermediateName, void(DownloadItem* download)); + MOCK_METHOD1(DownloadRenamedToFinalName, void(DownloadItem* download)); MOCK_CONST_METHOD1(AssertStateConsistent, void(DownloadItem* download)); }; @@ -49,8 +55,58 @@ class MockRequestHandle : public DownloadRequestHandleInterface { MOCK_CONST_METHOD0(DebugString, std::string()); }; +class MockDownloadFileFactory + : public DownloadFileManager::DownloadFileFactory { + public: + MOCK_METHOD5(CreateFile, + content::DownloadFile*(DownloadCreateInfo*, + const DownloadRequestHandle&, + DownloadManager*, + bool, + const net::BoundNetLog&)); +}; + +class MockDownloadFileManager : public DownloadFileManager { + public: + MockDownloadFileManager(); + MOCK_METHOD0(Shutdown, void()); + MOCK_METHOD2(StartDownload, + void(DownloadCreateInfo*, const DownloadRequestHandle&)); + MOCK_METHOD2(UpdateDownload, void(DownloadId, content::DownloadBuffer*)); + MOCK_METHOD3(OnResponseCompleted, + void(DownloadId, content::DownloadInterruptReason, + const std::string&)); + MOCK_METHOD1(CancelDownload, void(DownloadId)); + MOCK_METHOD1(CompleteDownload, void(DownloadId)); + MOCK_METHOD1(OnDownloadManagerShutdown, void(DownloadManager*)); + MOCK_METHOD4(RenameInProgressDownloadFile, + void(DownloadId, const FilePath&, bool, + const RenameCompletionCallback&)); + MOCK_METHOD4(RenameCompletingDownloadFile, + void(DownloadId, const FilePath&, bool, + const RenameCompletionCallback&)); + MOCK_CONST_METHOD0(NumberOfActiveDownloads, int()); + private: + ~MockDownloadFileManager() {} +}; + +// Schedules a task to invoke the RenameCompletionCallback with |new_path| on +// the UI thread. Should only be used as the action for +// MockDownloadFileManager::Rename*DownloadFile as follows: +// EXPECT_CALL(mock_download_file_manager, +// RenameInProgressDownloadFile(_,_,_,_)) +// .WillOnce(ScheduleRenameCallback(new_path)); +ACTION_P(ScheduleRenameCallback, new_path) { + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, + base::Bind(arg3, new_path)); +} + +MockDownloadFileManager::MockDownloadFileManager() + : DownloadFileManager(new MockDownloadFileFactory) { } +} // namespace + class DownloadItemTest : public testing::Test { public: class MockObserver : public DownloadItem::Observer { @@ -78,7 +134,8 @@ class DownloadItemTest : public testing::Test { }; DownloadItemTest() - : ui_thread_(BrowserThread::UI, &loop_) { + : ui_thread_(BrowserThread::UI, &loop_), + file_thread_(BrowserThread::FILE, &loop_) { } ~DownloadItemTest() { @@ -125,10 +182,18 @@ class DownloadItemTest : public testing::Test { delete item; } + void RunAllPendingInMessageLoops() { + loop_.RunAllPending(); + } + + MockDelegate* mock_delegate() { + return &delegate_; + } + private: MessageLoopForUI loop_; - // UI thread. - content::TestBrowserThread ui_thread_; + content::TestBrowserThread ui_thread_; // UI thread + content::TestBrowserThread file_thread_; // FILE thread testing::NiceMock<MockDelegate> delegate_; std::set<DownloadItem*> allocated_downloads_; }; @@ -149,7 +214,6 @@ const FilePath::CharType kDummyPath[] = FILE_PATH_LITERAL("/testpath"); // void ShowDownloadInShell(); // void CompleteDelayedDownload(); // void OnDownloadCompleting(DownloadFileManager* file_manager); -// void OnDownloadRenamedToFinalName(const FilePath& full_path); // set_* mutators TEST_F(DownloadItemTest, NotificationAfterUpdate) { @@ -219,56 +283,110 @@ TEST_F(DownloadItemTest, NotificationAfterRemove) { ASSERT_TRUE(observer.CheckUpdated()); } -TEST_F(DownloadItemTest, NotificationAfterSetFileCheckResults) { - // Setting to safe should not trigger any notifications +TEST_F(DownloadItemTest, NotificationAfterOnTargetPathDetermined) { DownloadItem* safe_item = CreateDownloadItem(DownloadItem::IN_PROGRESS); MockObserver safe_observer(safe_item); - DownloadStateInfo state = safe_item->GetStateInfo();; - state.danger = content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS; - safe_item->SetFileCheckResults(state); - ASSERT_FALSE(safe_observer.CheckUpdated()); + // Calling OnTargetPathDetermined does not trigger notification if danger type + // is NOT_DANGEROUS. + safe_item->OnTargetPathDetermined( + FilePath(kDummyPath), DownloadItem::TARGET_DISPOSITION_OVERWRITE, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); + EXPECT_FALSE(safe_observer.CheckUpdated()); + + DownloadItem* dangerous_item = CreateDownloadItem(DownloadItem::IN_PROGRESS); + MockObserver dangerous_observer(dangerous_item); + + // Calling OnTargetPathDetermined does trigger notification if danger type + // anything other than NOT_DANGEROUS. + dangerous_item->OnTargetPathDetermined( + FilePath(kDummyPath), DownloadItem::TARGET_DISPOSITION_OVERWRITE, + content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE); + EXPECT_TRUE(dangerous_observer.CheckUpdated()); +} - // Setting to unsafe url or unsafe file should trigger notification +TEST_F(DownloadItemTest, NotificationAfterOnTargetPathSelected) { + DownloadItem* item = CreateDownloadItem(DownloadItem::IN_PROGRESS); + MockObserver observer(item); + + item->OnTargetPathDetermined( + FilePath(kDummyPath), DownloadItem::TARGET_DISPOSITION_PROMPT, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); + item->OnTargetPathSelected(FilePath(kDummyPath)); + EXPECT_FALSE(observer.CheckUpdated()); +} + +TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) { + // Setting to NOT_DANGEROUS does not trigger a notification. + DownloadItem* safe_item = CreateDownloadItem(DownloadItem::IN_PROGRESS); + MockObserver safe_observer(safe_item); + + safe_item->OnTargetPathDetermined( + FilePath(kDummyPath), DownloadItem::TARGET_DISPOSITION_OVERWRITE, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); + EXPECT_FALSE(safe_observer.CheckUpdated()); + safe_item->OnAllDataSaved(1, ""); + EXPECT_TRUE(safe_observer.CheckUpdated()); + safe_item->OnContentCheckCompleted( + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); + EXPECT_FALSE(safe_observer.CheckUpdated()); + + // Setting to unsafe url or unsafe file should trigger a notification. DownloadItem* unsafeurl_item = CreateDownloadItem(DownloadItem::IN_PROGRESS); MockObserver unsafeurl_observer(unsafeurl_item); - state = unsafeurl_item->GetStateInfo();; - state.danger = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL; - unsafeurl_item->SetFileCheckResults(state); - ASSERT_TRUE(unsafeurl_observer.CheckUpdated()); + unsafeurl_item->OnTargetPathDetermined( + FilePath(kDummyPath), DownloadItem::TARGET_DISPOSITION_OVERWRITE, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); + EXPECT_FALSE(unsafeurl_observer.CheckUpdated()); + unsafeurl_item->OnAllDataSaved(1, ""); + EXPECT_TRUE(unsafeurl_observer.CheckUpdated()); + unsafeurl_item->OnContentCheckCompleted( + content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL); + EXPECT_TRUE(unsafeurl_observer.CheckUpdated()); unsafeurl_item->DangerousDownloadValidated(); - ASSERT_TRUE(unsafeurl_observer.CheckUpdated()); + EXPECT_TRUE(unsafeurl_observer.CheckUpdated()); DownloadItem* unsafefile_item = CreateDownloadItem(DownloadItem::IN_PROGRESS); MockObserver unsafefile_observer(unsafefile_item); - state = unsafefile_item->GetStateInfo();; - state.danger = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE; - unsafefile_item->SetFileCheckResults(state); - ASSERT_TRUE(unsafefile_observer.CheckUpdated()); + unsafefile_item->OnTargetPathDetermined( + FilePath(kDummyPath), DownloadItem::TARGET_DISPOSITION_OVERWRITE, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); + EXPECT_FALSE(unsafefile_observer.CheckUpdated()); + unsafefile_item->OnAllDataSaved(1, ""); + EXPECT_TRUE(unsafefile_observer.CheckUpdated()); + unsafefile_item->OnContentCheckCompleted( + content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE); + EXPECT_TRUE(unsafefile_observer.CheckUpdated()); unsafefile_item->DangerousDownloadValidated(); - ASSERT_TRUE(unsafefile_observer.CheckUpdated()); -} - -TEST_F(DownloadItemTest, NotificationAfterOnPathDetermined) { - DownloadItem* item = CreateDownloadItem(DownloadItem::IN_PROGRESS); - MockObserver observer(item); - - // Calling OnPathDetermined does not trigger notification - item->OnPathDetermined(FilePath(kDummyPath)); - ASSERT_FALSE(observer.CheckUpdated()); + EXPECT_TRUE(unsafefile_observer.CheckUpdated()); } -TEST_F(DownloadItemTest, NotificationAfterRename) { +// DownloadItemImpl::OnIntermediatePathDetermined will schedule a task to run +// DownloadFileManager::RenameInProgressDownloadFile(). Once the rename +// completes, DownloadItemImpl receives a notification with the new file +// name. Check that observers are updated when the new filename is available and +// not before. +TEST_F(DownloadItemTest, NotificationAfterOnIntermediatePathDetermined) { DownloadItem* item = CreateDownloadItem(DownloadItem::IN_PROGRESS); MockObserver observer(item); - - // Calling Rename does not trigger notification - item->Rename(FilePath(kDummyPath)); - ASSERT_FALSE(observer.CheckUpdated()); + FilePath intermediate_path(kDummyPath); + FilePath new_intermediate_path(intermediate_path.AppendASCII("foo")); + scoped_refptr<MockDownloadFileManager> file_manager( + new MockDownloadFileManager); + EXPECT_CALL(*file_manager.get(), + RenameInProgressDownloadFile(_,intermediate_path,false,_)) + .WillOnce(ScheduleRenameCallback(new_intermediate_path)); + + item->OnIntermediatePathDetermined(file_manager.get(), intermediate_path, + false /* ok_to_overwrite */); + EXPECT_FALSE(observer.CheckUpdated()); + RunAllPendingInMessageLoops(); + EXPECT_TRUE(observer.CheckUpdated()); + EXPECT_EQ(new_intermediate_path, item->GetFullPath()); } TEST_F(DownloadItemTest, NotificationAfterTogglePause) { @@ -284,14 +402,11 @@ TEST_F(DownloadItemTest, NotificationAfterTogglePause) { TEST_F(DownloadItemTest, DisplayName) { DownloadItem* item = CreateDownloadItem(DownloadItem::IN_PROGRESS); - DownloadStateInfo info = item->GetStateInfo(); - info.target_name = FilePath(FILE_PATH_LITERAL("foo.bar")); - item->SetFileCheckResults(info); + item->OnTargetPathDetermined(FilePath(kDummyPath).AppendASCII("foo.bar"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); EXPECT_EQ(FILE_PATH_LITERAL("foo.bar"), item->GetFileNameToReportUser().value()); - item->SetPathUniquifier(1); - EXPECT_EQ(FILE_PATH_LITERAL("foo (1).bar"), - item->GetFileNameToReportUser().value()); item->SetDisplayName(FilePath(FILE_PATH_LITERAL("new.name"))); EXPECT_EQ(FILE_PATH_LITERAL("new.name"), item->GetFileNameToReportUser().value()); @@ -364,6 +479,60 @@ TEST_F(DownloadItemTest, ExternalData) { EXPECT_EQ(3, destructor_called); } +// Test that the delegate is invoked after the download file is renamed. +// Delegate::DownloadRenamedToIntermediateName() should be invoked when the +// download is renamed to the intermediate name. +// Delegate::DownloadRenamedToFinalName() should be invoked after the final +// rename. +TEST_F(DownloadItemTest, CallbackAfterRenameToIntermediateName) { + DownloadItem* item = CreateDownloadItem(DownloadItem::IN_PROGRESS); + FilePath intermediate_path(kDummyPath); + FilePath new_intermediate_path(intermediate_path.AppendASCII("foo")); + FilePath final_path(intermediate_path.AppendASCII("bar")); + scoped_refptr<MockDownloadFileManager> file_manager( + new MockDownloadFileManager); + EXPECT_CALL(*file_manager.get(), + RenameInProgressDownloadFile(item->GetGlobalId(), + intermediate_path, false, _)) + .WillOnce(ScheduleRenameCallback(new_intermediate_path)); + // DownloadItemImpl should invoke this callback on the delegate once the + // download is renamed to the intermediate name. Also check that GetFullPath() + // returns the intermediate path at the time of the call. + EXPECT_CALL(*mock_delegate(), + DownloadRenamedToIntermediateName( + AllOf(item, + Property(&DownloadItem::GetFullPath, + new_intermediate_path)))); + item->OnTargetPathDetermined(final_path, + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); + item->OnIntermediatePathDetermined(file_manager.get(), intermediate_path, + false /* ok_to_overwrite */); + RunAllPendingInMessageLoops(); + // All the callbacks should have happened by now. + ::testing::Mock::VerifyAndClearExpectations(file_manager.get()); + ::testing::Mock::VerifyAndClearExpectations(mock_delegate()); + + EXPECT_CALL(*file_manager.get(), + RenameCompletingDownloadFile(item->GetGlobalId(), + final_path, true, _)) + .WillOnce(ScheduleRenameCallback(final_path)); + EXPECT_CALL(*file_manager.get(), CompleteDownload(item->GetGlobalId())); + // DownloadItemImpl should invoke this callback on the delegate after the + // final rename has completed. Also check that GetFullPath() and + // GetTargetFilePath() return the final path at the time of the call. + EXPECT_CALL(*mock_delegate(), + DownloadRenamedToFinalName( + AllOf(item, + Property(&DownloadItem::GetFullPath, final_path), + Property(&DownloadItem::GetTargetFilePath, + final_path)))); + item->OnDownloadCompleting(file_manager.get()); + RunAllPendingInMessageLoops(); + ::testing::Mock::VerifyAndClearExpectations(file_manager.get()); + ::testing::Mock::VerifyAndClearExpectations(mock_delegate()); +} + TEST(MockDownloadItem, Compiles) { MockDownloadItem mock_item; } diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc index aae7f8a..3296daa 100644 --- a/content/browser/download/download_manager_impl.cc +++ b/content/browser/download/download_manager_impl.cc @@ -259,7 +259,8 @@ void DownloadManagerImpl::GetTemporaryDownloads( for (DownloadMap::iterator it = history_downloads_.begin(); it != history_downloads_.end(); ++it) { if (it->second->IsTemporary() && - (dir_path.empty() || it->second->GetFullPath().DirName() == dir_path)) + (dir_path.empty() || + it->second->GetTargetFilePath().DirName() == dir_path)) result->push_back(it->second); } } @@ -271,7 +272,8 @@ void DownloadManagerImpl::GetAllDownloads( for (DownloadMap::iterator it = history_downloads_.begin(); it != history_downloads_.end(); ++it) { if (!it->second->IsTemporary() && - (dir_path.empty() || it->second->GetFullPath().DirName() == dir_path)) + (dir_path.empty() || + it->second->GetTargetFilePath().DirName() == dir_path)) result->push_back(it->second); } } @@ -377,29 +379,18 @@ void DownloadManagerImpl::RestartDownload(int32 download_id) { VLOG(20) << __FUNCTION__ << "()" << " download = " << download->DebugString(true); - FilePath suggested_path = download->GetSuggestedPath(); - - if (download->PromptUserForSaveLocation()) { + if (download->GetTargetDisposition() == + DownloadItem::TARGET_DISPOSITION_PROMPT) { // We must ask the user for the place to put the download. WebContents* contents = download->GetWebContents(); - FilePath target_path; - // If |download| is a potentially dangerous file, then |suggested_path| - // contains the intermediate name instead of the final download - // filename. The latter is GetTargetName(). - if (download->GetDangerType() != - content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS) - target_path = suggested_path.DirName().Append(download->GetTargetName()); - else - target_path = suggested_path; - - delegate_->ChooseDownloadPath(contents, target_path, + delegate_->ChooseDownloadPath(contents, download->GetTargetFilePath(), download_id); FOR_EACH_OBSERVER(Observer, observers_, SelectFileDialogDisplayed(this, download_id)); } else { - // No prompting for download, just continue with the suggested name. - ContinueDownloadWithPath(download, suggested_path); + // No prompting for download, just continue with the current target path. + OnTargetPathAvailable(download); } } @@ -458,13 +449,9 @@ DownloadItem* DownloadManagerImpl::CreateSavePackageDownloadItem( return download; } -// For non-safe downloads with no prompting, |chosen_file| is the intermediate -// path for saving the in-progress download. The final target filename for these -// is |download->GetTargetName()|. For all other downloads (non-safe downloads -// for which we have prompted for a save location, and all safe downloads), -// |chosen_file| is the final target download path. -void DownloadManagerImpl::ContinueDownloadWithPath( - DownloadItem* download, const FilePath& chosen_file) { +// The target path for the download item is now valid. We proceed with the +// determination of an intermediate path. +void DownloadManagerImpl::OnTargetPathAvailable(DownloadItem* download) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DCHECK(download); @@ -473,46 +460,24 @@ void DownloadManagerImpl::ContinueDownloadWithPath( DCHECK(ContainsKey(downloads_, download)); DCHECK(ContainsKey(active_downloads_, download_id)); - // Make sure the initial file name is set only once. - DCHECK(download->GetFullPath().empty()); - download->OnPathDetermined(chosen_file); - VLOG(20) << __FUNCTION__ << "()" << " download = " << download->DebugString(true); // Rename to intermediate name. - FilePath download_path; - if (download->GetDangerType() != - content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS) { - if (download->PromptUserForSaveLocation()) { - // When we prompt the user, we overwrite the FullPath with what the user - // wanted to use. Construct a file path using the previously determined - // intermediate filename and the new path. - // TODO(asanka): This can trample an in-progress download in the new - // target directory if it was using the same intermediate name. - FilePath file_name = download->GetSuggestedPath().BaseName(); - download_path = download->GetFullPath().DirName().Append(file_name); - } else { - // The download's name is already set to an intermediate name, so no need - // to override. - download_path = download->GetFullPath(); - } - } else { - // The download is a safe download. We need to rename it to its - // intermediate path. The final name after user confirmation will be set - // from DownloadItem::OnDownloadCompleting. - download_path = delegate_->GetIntermediatePath(download->GetFullPath()); - } - - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - base::Bind(&DownloadFileManager::RenameInProgressDownloadFile, - file_manager_, download->GetGlobalId(), - download_path)); - - download->Rename(download_path); - - delegate_->AddItemToPersistentStore(download); + // TODO(asanka): Skip this rename if download->AllDataSaved() is true. This + // avoids a spurious rename when we can just rename to the final + // filename. Unnecessary renames may cause bugs like + // http://crbug.com/74187. + bool ok_to_overwrite = true; + FilePath intermediate_path = + delegate_->GetIntermediatePath(*download, &ok_to_overwrite); + // We want the intermediate and target paths to refer to the same directory so + // that they are both on the same device and subject to same + // space/permission/availability constraints. + DCHECK(intermediate_path.DirName() == + download->GetTargetFilePath().DirName()); + download->OnIntermediatePathDetermined(file_manager_, intermediate_path, + ok_to_overwrite); } void DownloadManagerImpl::UpdateDownload(int32 download_id, @@ -650,8 +615,6 @@ void DownloadManagerImpl::MaybeCompleteDownload(DownloadItem* download) { << download->DebugString(false); delegate_->UpdateItemInPersistentStore(download); - - // Finish the download. download->OnDownloadCompleting(file_manager_); } @@ -669,38 +632,6 @@ void DownloadManagerImpl::DownloadCompleted(DownloadItem* download) { AssertStateConsistent(download); } -void DownloadManagerImpl::OnDownloadRenamedToFinalName( - int download_id, - const FilePath& full_path, - int uniquifier) { - VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id - << " full_path = \"" << full_path.value() << "\"" - << " uniquifier = " << uniquifier; - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - - DownloadItem* item = GetDownloadItem(download_id); - if (!item) - return; - - if (item->GetDangerType() == content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS || - item->PromptUserForSaveLocation()) { - DCHECK_EQ(0, uniquifier) - << "We should not uniquify user supplied filenames or safe filenames " - "that have already been uniquified."; - } - - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - base::Bind(&DownloadFileManager::CompleteDownload, - file_manager_, item->GetGlobalId())); - - if (uniquifier) - item->SetPathUniquifier(uniquifier); - - item->OnDownloadRenamedToFinalName(full_path); - delegate_->UpdatePathForItemInPersistentStore(item, full_path); -} - void DownloadManagerImpl::CancelDownload(int32 download_id) { DownloadItem* download = GetActiveDownload(download_id); // A cancel at the right time could remove the download from the @@ -882,20 +813,22 @@ void DownloadManagerImpl::RemoveObserver(Observer* observer) { void DownloadManagerImpl::FileSelected(const FilePath& path, int32 download_id) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(!path.empty()); DownloadItem* download = GetActiveDownloadItem(download_id); if (!download) return; VLOG(20) << __FUNCTION__ << "()" << " path = \"" << path.value() << "\"" - << " download = " << download->DebugString(true); + << " download = " << download->DebugString(true); - // Retain the last directory that was picked by the user. Exclude temporary - // downloads since the path likely points at the location of a temporary file. - if (download->PromptUserForSaveLocation() && !download->IsTemporary()) + // Retain the last directory. Exclude temporary downloads since the path + // likely points at the location of a temporary file. + if (!download->IsTemporary()) last_download_path_ = path.DirName(); // Make sure the initial file name is set only once. - ContinueDownloadWithPath(download, path); + download->OnTargetPathSelected(path); + OnTargetPathAvailable(download); } void DownloadManagerImpl::FileSelectionCanceled(int32 download_id) { @@ -1174,6 +1107,23 @@ void DownloadManagerImpl::DownloadOpened(DownloadItem* download) { download_stats::RecordOpensOutstanding(num_unopened); } +void DownloadManagerImpl::DownloadRenamedToIntermediateName( + DownloadItem* download) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + // If the rename failed, we receive an OnDownloadInterrupted() call before we + // receive the DownloadRenamedToIntermediateName() call. + delegate_->AddItemToPersistentStore(download); +} + +void DownloadManagerImpl::DownloadRenamedToFinalName( + DownloadItem* download) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + // If the rename failed, we receive an OnDownloadInterrupted() call before we + // receive the DownloadRenamedToFinalName() call. + delegate_->UpdatePathForItemInPersistentStore(download, + download->GetFullPath()); +} + void DownloadManagerImpl::SetFileManagerForTesting( DownloadFileManager* file_manager) { file_manager_ = file_manager; diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h index 7f267b2..ad1a027 100644 --- a/content/browser/download/download_manager_impl.h +++ b/content/browser/download/download_manager_impl.h @@ -50,9 +50,6 @@ class CONTENT_EXPORT DownloadManagerImpl int64 size, const std::string& hash_state, content::DownloadInterruptReason reason) OVERRIDE; - virtual void OnDownloadRenamedToFinalName(int download_id, - const FilePath& full_path, - int uniquifier) OVERRIDE; virtual int RemoveDownloadsBetween(base::Time remove_begin, base::Time remove_end) OVERRIDE; virtual int RemoveDownloads(base::Time remove_begin) OVERRIDE; @@ -107,6 +104,10 @@ class CONTENT_EXPORT DownloadManagerImpl virtual void DownloadOpened( content::DownloadItem* download) OVERRIDE; virtual void DownloadRemoved(content::DownloadItem* download) OVERRIDE; + virtual void DownloadRenamedToIntermediateName( + content::DownloadItem* download) OVERRIDE; + virtual void DownloadRenamedToFinalName( + content::DownloadItem* download) OVERRIDE; virtual void AssertStateConsistent( content::DownloadItem* download) const OVERRIDE; @@ -150,8 +151,7 @@ class CONTENT_EXPORT DownloadManagerImpl // Called back after a target path for the file to be downloaded to has been // determined, either automatically based on the suggested file name, or by // the user in a Save As dialog box. - void ContinueDownloadWithPath(content::DownloadItem* download, - const FilePath& chosen_file); + void OnTargetPathAvailable(content::DownloadItem* download); // Retrieves the download from the |download_id|. // Returns NULL if the download is not active. diff --git a/content/browser/download/download_manager_impl_unittest.cc b/content/browser/download/download_manager_impl_unittest.cc index 2f3bbe8..5337374 100644 --- a/content/browser/download/download_manager_impl_unittest.cc +++ b/content/browser/download/download_manager_impl_unittest.cc @@ -102,9 +102,14 @@ class TestDownloadManagerDelegate : public content::DownloadManagerDelegate { TestDownloadManagerDelegate() : mark_content_dangerous_(false), prompt_user_for_save_location_(false), + should_complete_download_(true), download_manager_(NULL) { } + void set_download_directory(const FilePath& path) { + download_directory_ = path; + } + void set_download_manager(content::DownloadManager* dm) { download_manager_ = dm; } @@ -115,22 +120,30 @@ class TestDownloadManagerDelegate : public content::DownloadManagerDelegate { virtual bool ShouldStartDownload(int32 download_id) OVERRIDE { DownloadItem* item = download_manager_->GetActiveDownloadItem(download_id); - DownloadStateInfo state = item->GetStateInfo(); FilePath path = net::GenerateFileName(item->GetURL(), item->GetContentDisposition(), item->GetReferrerCharset(), item->GetSuggestedFilename(), item->GetMimeType(), std::string()); + DownloadItem::TargetDisposition disposition = item->GetTargetDisposition(); if (!ShouldOpenFileBasedOnExtension(path) && prompt_user_for_save_location_) - state.prompt_user_for_save_location = true; - item->SetFileCheckResults(state); + disposition = DownloadItem::TARGET_DISPOSITION_PROMPT; + item->OnTargetPathDetermined(download_directory_.Append(path), + disposition, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); return true; } - virtual FilePath GetIntermediatePath( - const FilePath& suggested_path) OVERRIDE { - return GetTempDownloadPath(suggested_path); + virtual FilePath GetIntermediatePath(const DownloadItem& item, + bool* ok_to_overwrite) OVERRIDE { + if (intermediate_path_.empty()) { + *ok_to_overwrite = true; + return GetTempDownloadPath(item.GetTargetFilePath()); + } else { + *ok_to_overwrite = overwrite_intermediate_path_; + return intermediate_path_; + } } virtual void ChooseDownloadPath(WebContents* web_contents, @@ -177,38 +190,60 @@ class TestDownloadManagerDelegate : public content::DownloadManagerDelegate { mark_content_dangerous_ = dangerous; } + void SetIntermediatePath(const FilePath& intermediate_path, + bool overwrite_intermediate_path) { + intermediate_path_ = intermediate_path; + overwrite_intermediate_path_ = overwrite_intermediate_path; + } + + void SetShouldCompleteDownload(bool value) { + should_complete_download_ = value; + } + + void InvokeDownloadCompletionCallback() { + EXPECT_FALSE(download_completion_callback_.is_null()); + download_completion_callback_.Run(); + download_completion_callback_.Reset(); + } + virtual bool ShouldCompleteDownload( DownloadItem* item, - const base::Closure& complete_callback) { + const base::Closure& complete_callback) OVERRIDE { + download_completion_callback_ = complete_callback; if (mark_content_dangerous_) { CHECK(!complete_callback.is_null()); BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, base::Bind(&TestDownloadManagerDelegate::MarkContentDangerous, - base::Unretained(this), item->GetId(), complete_callback)); + base::Unretained(this), item->GetId())); mark_content_dangerous_ = false; return false; } else { - return true; + return should_complete_download_; } } private: void MarkContentDangerous( - int32 download_id, - const base::Closure& complete_callback) { + int32 download_id) { DownloadItem* item = download_manager_->GetActiveDownloadItem(download_id); if (!item) return; - item->SetDangerType(content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT); - complete_callback.Run(); + item->OnContentCheckCompleted( + content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT); + InvokeDownloadCompletionCallback(); } + FilePath download_directory_; FilePath expected_suggested_path_; FilePath file_selection_response_; + FilePath intermediate_path_; + bool overwrite_intermediate_path_; bool mark_content_dangerous_; bool prompt_user_for_save_location_; + bool should_complete_download_; DownloadManager* download_manager_; + base::Closure download_completion_callback_; DISALLOW_COPY_AND_ASSIGN(TestDownloadManagerDelegate); }; @@ -242,6 +277,15 @@ class DownloadManagerTest : public testing::Test { message_loop_.RunAllPending(); } + // Create a temporary directory as the downloads directory. + bool CreateTempDownloadsDirectory() { + if (!scoped_download_dir_.CreateUniqueTempDir()) + return false; + download_manager_delegate_->set_download_directory( + scoped_download_dir_.path()); + return true; + } + void AddDownloadToFileManager(int id, DownloadFile* download_file) { file_manager()->downloads_[DownloadId(kValidIdDomain, id)] = download_file; @@ -249,8 +293,8 @@ class DownloadManagerTest : public testing::Test { void AddMockDownloadToFileManager(int id, MockDownloadFile* download_file) { AddDownloadToFileManager(id, download_file); - ON_CALL(*download_file, GetDownloadManager()) - .WillByDefault(Return(download_manager_)); + EXPECT_CALL(*download_file, GetDownloadManager()) + .WillRepeatedly(Return(download_manager_)); } void OnResponseCompleted(int32 download_id, int64 size, @@ -263,7 +307,10 @@ class DownloadManagerTest : public testing::Test { } void ContinueDownloadWithPath(DownloadItem* download, const FilePath& path) { - download_manager_->ContinueDownloadWithPath(download, path); + download->OnTargetPathDetermined( + path, DownloadItem::TARGET_DISPOSITION_OVERWRITE, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); + download_manager_->OnTargetPathAvailable(download); } void UpdateData(int32 id, const char* data, size_t length) { @@ -295,6 +342,12 @@ class DownloadManagerTest : public testing::Test { return download_manager_->GetActiveDownload(id); } + FilePath GetPathInDownloadsDir(const FilePath::StringType& fragment) { + DCHECK(scoped_download_dir_.IsValid()); + FilePath full_path(scoped_download_dir_.path().Append(fragment)); + return full_path.NormalizePathSeparators(); + } + protected: scoped_ptr<TestBrowserContext> browser_context; scoped_ptr<TestDownloadManagerDelegate> download_manager_delegate_; @@ -304,6 +357,7 @@ class DownloadManagerTest : public testing::Test { content::TestBrowserThread ui_thread_; content::TestBrowserThread file_thread_; scoped_refptr<content::DownloadBuffer> download_buffer_; + ScopedTempDir scoped_download_dir_; DownloadFileManager* file_manager() { if (!file_manager_) { @@ -329,9 +383,10 @@ class DownloadFileWithErrors : public DownloadFileImpl { virtual ~DownloadFileWithErrors() {} // BaseFile delegated functions. - virtual net::Error Initialize(); - virtual net::Error AppendDataToFile(const char* data, size_t data_len); - virtual net::Error Rename(const FilePath& full_path); + virtual net::Error Initialize() OVERRIDE; + virtual net::Error AppendDataToFile(const char* data, + size_t data_len) OVERRIDE; + virtual net::Error Rename(const FilePath& full_path) OVERRIDE; void set_forced_error(net::Error error) { forced_error_ = error; } void clear_forced_error() { forced_error_ = net::OK; } @@ -427,35 +482,6 @@ const struct { true, }, }; -const struct { - FilePath::StringType suggested_path; - content::DownloadDangerType danger; - bool finish_before_rename; - int expected_rename_count; -} kDownloadRenameCases[] = { - // Safe download, download finishes BEFORE file name determined. - // Renamed twice (linear path through UI). temp file does not need - // to be deleted. - { FILE_PATH_LITERAL("foo.zip"), content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, - true, 2, }, - // Potentially dangerous download (e.g., file is dangerous), download finishes - // BEFORE file name determined. Needs to be renamed only once. - { FILE_PATH_LITERAL("Unconfirmed xxx.temp"), - content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, true, 1, }, - { FILE_PATH_LITERAL("Unconfirmed xxx.temp"), - content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE, true, 1, }, - // Safe download, download finishes AFTER file name determined. - // Needs to be renamed twice. - { FILE_PATH_LITERAL("foo.zip"), content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, - false, 2, }, - // Potentially dangerous download, download finishes AFTER file name - // determined. Needs to be renamed only once. - { FILE_PATH_LITERAL("Unconfirmed xxx.temp"), - content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, false, 1, }, - { FILE_PATH_LITERAL("Unconfirmed xxx.temp"), - content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE, false, 1, }, -}; - // This is an observer that records what download IDs have opened a select // file dialog. class SelectFileObserver : public DownloadManager::Observer { @@ -510,12 +536,12 @@ class ItemObserver : public DownloadItem::Observer { private: // DownloadItem::Observer methods - virtual void OnDownloadUpdated(DownloadItem* download) { + virtual void OnDownloadUpdated(DownloadItem* download) OVERRIDE { DCHECK_EQ(tracked_, download); states_hit_ |= (1 << download->GetState()); was_updated_ = true; } - virtual void OnDownloadOpened(DownloadItem* download) { + virtual void OnDownloadOpened(DownloadItem* download) OVERRIDE{ DCHECK_EQ(tracked_, download); states_hit_ |= (1 << download->GetState()); was_opened_ = true; @@ -531,7 +557,9 @@ class ItemObserver : public DownloadItem::Observer { TEST_F(DownloadManagerTest, MAYBE_StartDownload) { content::TestBrowserThread io_thread(BrowserThread::IO, &message_loop_); + ASSERT_TRUE(CreateTempDownloadsDirectory()); for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kStartDownloadCases); ++i) { + DVLOG(1) << "Test case " << i; download_manager_delegate_->set_prompt_user_for_save_location( kStartDownloadCases[i].prompt_for_download); @@ -569,373 +597,288 @@ TEST_F(DownloadManagerTest, MAYBE_StartDownload) { namespace { -enum PromptForSaveLocation { - DONT_PROMPT, - PROMPT +enum OverwriteDownloadPath { + DONT_OVERWRITE, + OVERWRITE }; -enum ValidateDangerousDownload { - DONT_VALIDATE, - VALIDATE +enum ResponseCompletionTime { + COMPLETES_BEFORE_RENAME, + COMPLETES_AFTER_RENAME }; -// Test cases to be used with DownloadFilenameTest. The paths that are used in -// test cases can contain "$dl" and "$alt" tokens which are replaced by a -// default download path, and an alternate download path in -// ExpandFilenameTestPath() below. +// Test cases to be used with DownloadFilenameTest. The paths are relative to +// the temporary downloads directory used for testing. const struct DownloadFilenameTestCase { - - // Fields to be set in DownloadStateInfo when calling SetFileCheckResults(). - const FilePath::CharType* suggested_path; - const FilePath::CharType* target_name; - PromptForSaveLocation prompt_user_for_save_location; - content::DownloadDangerType danger_type; + // DownloadItem properties prior to calling RestartDownload(). + const FilePath::CharType* target_path; + DownloadItem::TargetDisposition target_disposition; // If we receive a ChooseDownloadPath() call to prompt the user for a download - // location, |prompt_path| is the expected prompt path. The - // TestDownloadManagerDelegate will respond with |final_path|. If |final_path| - // is empty, then the file choose dialog be cancelled. - const FilePath::CharType* prompt_path; + // location, |expected_prompt_path| is the expected prompt path. The + // TestDownloadManagerDelegate will respond with |chosen_path|. If + // |chosen_path| is empty, then the file choose dialog be cancelled. + const FilePath::CharType* expected_prompt_path; + const FilePath::CharType* chosen_path; + + // Response to GetIntermediatePath(). If |intermediate_path| is empty, then a + // temporary path is constructed with + // GetTempDownloadPath(item->GetTargetFilePath()). + const FilePath::CharType* intermediate_path; + OverwriteDownloadPath overwrite_intermediate_path; + + // Determines when OnResponseCompleted() is called. If this is + // COMPLETES_BEFORE_RENAME, then the response completes before the filename is + // determines. + ResponseCompletionTime completion; // The expected intermediate path for the download. - const FilePath::CharType* intermediate_path; + const FilePath::CharType* expected_intermediate_path; // The expected final path for the download. - const FilePath::CharType* final_path; - - // If this is a dangerous download, then we will either validate the download - // or delete it depending on the value of |validate_dangerous_download|. - ValidateDangerousDownload validate_dangerous_download; + const FilePath::CharType* expected_final_path; } kDownloadFilenameTestCases[] = { + +#define TARGET FILE_PATH_LITERAL +#define INTERMEDIATE FILE_PATH_LITERAL +#define EXPECTED_PROMPT FILE_PATH_LITERAL +#define PROMPT_RESPONSE FILE_PATH_LITERAL +#define EXPECTED_INTERMEDIATE FILE_PATH_LITERAL +#define EXPECTED_FINAL FILE_PATH_LITERAL + { - // 0: A safe file is downloaded with no prompting. - FILE_PATH_LITERAL("$dl/foo.txt"), - FILE_PATH_LITERAL(""), - DONT_PROMPT, - content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, - FILE_PATH_LITERAL(""), - FILE_PATH_LITERAL("$dl/foo.txt.temp"), - FILE_PATH_LITERAL("$dl/foo.txt"), - DONT_VALIDATE - }, - { - // 1: A safe file is downloaded with prompting. - FILE_PATH_LITERAL("$dl/foo.txt"), - FILE_PATH_LITERAL(""), - PROMPT, - content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, - FILE_PATH_LITERAL("$dl/foo.txt"), - FILE_PATH_LITERAL("$dl/foo.txt.temp"), - FILE_PATH_LITERAL("$dl/foo.txt"), - DONT_VALIDATE - }, - { - // 2: A safe file is downloaded. The filename is changed before the dialog - // completes. - FILE_PATH_LITERAL("$dl/foo.txt"), - FILE_PATH_LITERAL(""), - PROMPT, - content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, - FILE_PATH_LITERAL("$dl/foo.txt"), - FILE_PATH_LITERAL("$dl/bar.txt.temp"), - FILE_PATH_LITERAL("$dl/bar.txt"), - DONT_VALIDATE - }, - { - // 3: A safe file is downloaded. The download path is changed before the - // dialog completes. - FILE_PATH_LITERAL("$dl/foo.txt"), - FILE_PATH_LITERAL(""), - PROMPT, - content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, - FILE_PATH_LITERAL("$dl/foo.txt"), - FILE_PATH_LITERAL("$alt/bar.txt.temp"), - FILE_PATH_LITERAL("$alt/bar.txt"), - DONT_VALIDATE - }, - { - // 4: Potentially dangerous content. - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("foo.exe"), - DONT_PROMPT, - content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, - FILE_PATH_LITERAL(""), - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("$dl/foo.exe"), - DONT_VALIDATE - }, - { - // 5: Potentially dangerous content. Uses "Save as." - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("foo.exe"), - PROMPT, - content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, - FILE_PATH_LITERAL("$dl/foo.exe"), - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("$dl/foo.exe"), - DONT_VALIDATE + // 0: No prompting. + TARGET("foo.txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECTED_PROMPT(""), + PROMPT_RESPONSE(""), + + INTERMEDIATE("foo.txt.intermediate"), + OVERWRITE, + + COMPLETES_AFTER_RENAME, + EXPECTED_INTERMEDIATE("foo.txt.intermediate"), + EXPECTED_FINAL("foo.txt"), }, { - // 6: Potentially dangerous content. Uses "Save as." The download filename - // is changed before the dialog completes. - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("foo.exe"), - PROMPT, - content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, - FILE_PATH_LITERAL("$dl/foo.exe"), - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("$dl/bar.exe"), - DONT_VALIDATE + // 1: With prompting. No rename. + TARGET("foo.txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECTED_PROMPT("foo.txt"), + PROMPT_RESPONSE("foo.txt"), + + INTERMEDIATE("foo.txt.intermediate"), + OVERWRITE, + + COMPLETES_AFTER_RENAME, + EXPECTED_INTERMEDIATE("foo.txt.intermediate"), + EXPECTED_FINAL("foo.txt"), }, { - // 7: Potentially dangerous content. Uses "Save as." The download directory - // is changed before the dialog completes. - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("foo.exe"), - PROMPT, - content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, - FILE_PATH_LITERAL("$dl/foo.exe"), - FILE_PATH_LITERAL("$alt/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("$alt/bar.exe"), - DONT_VALIDATE + // 2: With prompting. Download is renamed. + TARGET("foo.txt"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECTED_PROMPT("foo.txt"), + PROMPT_RESPONSE("bar.txt"), + + INTERMEDIATE("bar.txt.intermediate"), + OVERWRITE, + + COMPLETES_AFTER_RENAME, + EXPECTED_INTERMEDIATE("bar.txt.intermediate"), + EXPECTED_FINAL("bar.txt"), }, { - // 8: Dangerous content. Saved directly. - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("foo.exe"), - PROMPT, - content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL, - FILE_PATH_LITERAL(""), - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("$dl/foo.exe"), - VALIDATE + // 3: With prompting. Download path is changed. + TARGET("foo.txt"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECTED_PROMPT("foo.txt"), + PROMPT_RESPONSE("Foo/bar.txt"), + + INTERMEDIATE("Foo/bar.txt.intermediate"), + OVERWRITE, + + COMPLETES_AFTER_RENAME, + EXPECTED_INTERMEDIATE("Foo/bar.txt.intermediate"), + EXPECTED_FINAL("Foo/bar.txt"), }, { - // 9: Dangerous content. Saved directly. Not validated. - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("foo.exe"), - DONT_PROMPT, - content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL, - FILE_PATH_LITERAL(""), - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL(""), - DONT_VALIDATE + // 4: No prompting. Intermediate path exists. Doesn't overwrite + // intermediate path. + TARGET("exists.png"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECTED_PROMPT(""), + PROMPT_RESPONSE(""), + + INTERMEDIATE("exists.png.temp"), + DONT_OVERWRITE, + + COMPLETES_AFTER_RENAME, + EXPECTED_INTERMEDIATE("exists.png (1).temp"), + EXPECTED_FINAL("exists.png"), }, { - // 10: Dangerous content. Uses "Save as." The download directory is changed - // before the dialog completes. - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("foo.exe"), - PROMPT, - content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL, - FILE_PATH_LITERAL("$dl/foo.exe"), - FILE_PATH_LITERAL("$alt/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("$alt/bar.exe"), - VALIDATE + // 5: No prompting. Intermediate path exists. Overwrites. + TARGET("exists.png"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECTED_PROMPT(""), + PROMPT_RESPONSE(""), + + INTERMEDIATE("exists.png.temp"), + OVERWRITE, + + COMPLETES_AFTER_RENAME, + EXPECTED_INTERMEDIATE("exists.png.temp"), + EXPECTED_FINAL("exists.png"), }, { - // 11: A safe file is download. The target file exists, but we don't - // uniquify. Safe downloads are uniquified in ChromeDownloadManagerDelegate - // instead of DownloadManagerImpl. - FILE_PATH_LITERAL("$dl/exists.txt"), - FILE_PATH_LITERAL(""), - DONT_PROMPT, - content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, - FILE_PATH_LITERAL(""), - FILE_PATH_LITERAL("$dl/exists.txt.temp"), - FILE_PATH_LITERAL("$dl/exists.txt"), - DONT_VALIDATE + // 6: No prompting. Final path exists. Doesn't overwrite. + TARGET("exists.txt"), + DownloadItem::TARGET_DISPOSITION_UNIQUIFY, + + EXPECTED_PROMPT(""), + PROMPT_RESPONSE(""), + + INTERMEDIATE("exists.txt.temp"), + OVERWRITE, + + COMPLETES_AFTER_RENAME, + EXPECTED_INTERMEDIATE("exists.txt.temp"), + EXPECTED_FINAL("exists (1).txt"), }, { - // 12: A potentially dangerous file is download. The target file exists. The - // target path is uniquified. - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("exists.exe"), - DONT_PROMPT, - content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, - FILE_PATH_LITERAL(""), - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("$dl/exists (1).exe"), - DONT_VALIDATE + // 7: No prompting. Final path exists. Overwrites. + TARGET("exists.txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECTED_PROMPT(""), + PROMPT_RESPONSE(""), + + INTERMEDIATE("exists.txt.temp"), + OVERWRITE, + + COMPLETES_AFTER_RENAME, + EXPECTED_INTERMEDIATE("exists.txt.temp"), + EXPECTED_FINAL("exists.txt"), }, { - // 13: A dangerous file is download. The target file exists. The target path - // is uniquified. - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("exists.exe"), - DONT_PROMPT, - content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT, - FILE_PATH_LITERAL(""), - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("$dl/exists (1).exe"), - VALIDATE + // 8: No prompting. Response completes before filename determination. + TARGET("foo.txt"), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + + EXPECTED_PROMPT(""), + PROMPT_RESPONSE(""), + + INTERMEDIATE("foo.txt.intermediate"), + OVERWRITE, + + COMPLETES_BEFORE_RENAME, + EXPECTED_INTERMEDIATE("foo.txt.intermediate"), + EXPECTED_FINAL("foo.txt"), }, { - // 14: A potentially dangerous file is download with prompting. The target - // file exists. The target path is not uniquified because the filename was - // given to us by the user. - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("exists.exe"), - PROMPT, - content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT, - FILE_PATH_LITERAL("$dl/exists.exe"), - FILE_PATH_LITERAL("$dl/Unconfirmed xxx.download"), - FILE_PATH_LITERAL("$dl/exists.exe"), - DONT_VALIDATE + // 9: With prompting. Download path is changed. Response completes before + // filename determination. + TARGET("foo.txt"), + DownloadItem::TARGET_DISPOSITION_PROMPT, + + EXPECTED_PROMPT("foo.txt"), + PROMPT_RESPONSE("Foo/bar.txt"), + + INTERMEDIATE("Foo/bar.txt.intermediate"), + OVERWRITE, + + COMPLETES_BEFORE_RENAME, + EXPECTED_INTERMEDIATE("Foo/bar.txt.intermediate"), + EXPECTED_FINAL("Foo/bar.txt"), }, -}; -FilePath ExpandFilenameTestPath(const FilePath::CharType* template_path, - const FilePath& downloads_dir, - const FilePath& alternate_dir) { - FilePath::StringType path(template_path); - ReplaceSubstringsAfterOffset(&path, 0, FILE_PATH_LITERAL("$dl"), - downloads_dir.value()); - ReplaceSubstringsAfterOffset(&path, 0, FILE_PATH_LITERAL("$alt"), - alternate_dir.value()); - return FilePath(path).NormalizePathSeparators(); -} +#undef TARGET +#undef INTERMEDIATE +#undef EXPECTED_PROMPT +#undef PROMPT_RESPONSE +#undef EXPECTED_INTERMEDIATE +#undef EXPECTED_FINAL +}; } // namespace TEST_F(DownloadManagerTest, DownloadFilenameTest) { - ScopedTempDir scoped_dl_dir; - ASSERT_TRUE(scoped_dl_dir.CreateUniqueTempDir()); - - FilePath downloads_dir(scoped_dl_dir.path()); - FilePath alternate_dir(downloads_dir.Append(FILE_PATH_LITERAL("Foo"))); + ASSERT_TRUE(CreateTempDownloadsDirectory()); // We create a known file to test file uniquification. - file_util::WriteFile(downloads_dir.Append(FILE_PATH_LITERAL("exists.txt")), - "", 0); - file_util::WriteFile(downloads_dir.Append(FILE_PATH_LITERAL("exists.exe")), - "", 0); + ASSERT_TRUE(file_util::WriteFile( + GetPathInDownloadsDir(FILE_PATH_LITERAL("exists.txt")), "", 0) == 0); + ASSERT_TRUE(file_util::WriteFile( + GetPathInDownloadsDir(FILE_PATH_LITERAL("exists.png.temp")), "", 0) == 0); for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDownloadFilenameTestCases); ++i) { + const DownloadFilenameTestCase& test_case = kDownloadFilenameTestCases[i]; scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo); + + SCOPED_TRACE(testing::Message() << "Running test case " << i); info->download_id = DownloadId(kValidIdDomain, i); info->url_chain.push_back(GURL()); - MockDownloadFile* download_file(new NiceMock<MockDownloadFile>()); - FilePath suggested_path(ExpandFilenameTestPath( - kDownloadFilenameTestCases[i].suggested_path, - downloads_dir, alternate_dir)); - FilePath prompt_path(ExpandFilenameTestPath( - kDownloadFilenameTestCases[i].prompt_path, - downloads_dir, alternate_dir)); - FilePath intermediate_path(ExpandFilenameTestPath( - kDownloadFilenameTestCases[i].intermediate_path, - downloads_dir, alternate_dir)); - FilePath final_path(ExpandFilenameTestPath( - kDownloadFilenameTestCases[i].final_path, - downloads_dir, alternate_dir)); + MockDownloadFile* download_file( + new ::testing::StrictMock<MockDownloadFile>()); + FilePath target_path = GetPathInDownloadsDir(test_case.target_path); + FilePath expected_prompt_path = + GetPathInDownloadsDir(test_case.expected_prompt_path); + FilePath chosen_path = GetPathInDownloadsDir(test_case.chosen_path); + FilePath intermediate_path = + GetPathInDownloadsDir(test_case.intermediate_path); + FilePath expected_intermediate_path = + GetPathInDownloadsDir(test_case.expected_intermediate_path); + FilePath expected_final_path = + GetPathInDownloadsDir(test_case.expected_final_path); AddMockDownloadToFileManager(info->download_id.local(), download_file); - - EXPECT_CALL(*download_file, Rename(intermediate_path)) + EXPECT_CALL(*download_file, Rename(expected_intermediate_path)) .WillOnce(Return(net::OK)); - - if (!final_path.empty()) { - // If |final_path| is empty, its a signal that the download doesn't - // complete. Therefore it will only go through a single rename. - EXPECT_CALL(*download_file, Rename(final_path)) - .WillOnce(Return(net::OK)); - } + EXPECT_CALL(*download_file, Rename(expected_final_path)) + .WillOnce(Return(net::OK)); +#if defined(OS_MACOSX) + EXPECT_CALL(*download_file, AnnotateWithSourceInformation()); +#endif + EXPECT_CALL(*download_file, Detach()); download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle()); DownloadItem* download = GetActiveDownloadItem(i); ASSERT_TRUE(download != NULL); - DownloadStateInfo state = download->GetStateInfo(); - state.suggested_path = suggested_path; - state.danger = kDownloadFilenameTestCases[i].danger_type; - state.prompt_user_for_save_location = - (kDownloadFilenameTestCases[i].prompt_user_for_save_location == PROMPT); - state.target_name = FilePath(kDownloadFilenameTestCases[i].target_name); - if (state.danger == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT) { - // DANGEROUS_CONTENT will only be known once we have all the data. We let - // our TestDownloadManagerDelegate handle it. - state.danger = content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT; - download_manager_delegate_->SetMarkContentsDangerous(true); - } - download->SetFileCheckResults(state); + if (test_case.completion == COMPLETES_BEFORE_RENAME) + OnResponseCompleted(i, 1024, std::string("fake_hash")); + + download->OnTargetPathDetermined( + target_path, test_case.target_disposition, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); download_manager_delegate_->SetFileSelectionExpectation( - prompt_path, final_path); + expected_prompt_path, chosen_path); + download_manager_delegate_->SetIntermediatePath( + intermediate_path, test_case.overwrite_intermediate_path); + download_manager_delegate_->SetShouldCompleteDownload(false); download_manager_->RestartDownload(i); message_loop_.RunAllPending(); - OnResponseCompleted(i, 1024, std::string("fake_hash")); - message_loop_.RunAllPending(); - - if (download->GetSafetyState() == DownloadItem::DANGEROUS) { - if (kDownloadFilenameTestCases[i].validate_dangerous_download == VALIDATE) - download->DangerousDownloadValidated(); - else - download->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); - message_loop_.RunAllPending(); - // |download| might be deleted when we get here. - } - } -} - -TEST_F(DownloadManagerTest, DownloadRenameTest) { - using ::testing::_; - using ::testing::CreateFunctor; - using ::testing::Invoke; - using ::testing::Return; - - for (size_t i = 0; i < ARRAYSIZE_UNSAFE(kDownloadRenameCases); ++i) { - // Normally, the download system takes ownership of info, and is - // responsible for deleting it. In these unit tests, however, we - // don't call the function that deletes it, so we do so ourselves. - scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo); - info->download_id = DownloadId(kValidIdDomain, static_cast<int>(i)); - info->prompt_user_for_save_location = false; - info->url_chain.push_back(GURL()); - const FilePath new_path(kDownloadRenameCases[i].suggested_path); - - MockDownloadFile* download_file(new NiceMock<MockDownloadFile>()); - const DownloadId id = info->download_id; - ON_CALL(*download_file, GlobalId()) - .WillByDefault(ReturnRef(id)); - - AddMockDownloadToFileManager(info->download_id.local(), download_file); - - // |download_file| is owned by DownloadFileManager. - if (kDownloadRenameCases[i].expected_rename_count == 1) { - EXPECT_CALL(*download_file, Rename(new_path)) - .Times(1) - .WillOnce(Return(net::OK)); - } else { - ASSERT_EQ(2, kDownloadRenameCases[i].expected_rename_count); - FilePath temp(GetTempDownloadPath(new_path)); - EXPECT_CALL(*download_file, Rename(temp)) - .Times(1) - .WillOnce(Return(net::OK)); - EXPECT_CALL(*download_file, Rename(new_path)) - .Times(1) - .WillOnce(Return(net::OK)); - } - download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle()); - DownloadItem* download = GetActiveDownloadItem(i); - ASSERT_TRUE(download != NULL); - DownloadStateInfo state = download->GetStateInfo(); - state.danger = kDownloadRenameCases[i].danger; - download->SetFileCheckResults(state); - - if (kDownloadRenameCases[i].finish_before_rename) { + if (test_case.completion == COMPLETES_AFTER_RENAME) { OnResponseCompleted(i, 1024, std::string("fake_hash")); message_loop_.RunAllPending(); - FileSelected(new_path, i); - } else { - FileSelected(new_path, i); - message_loop_.RunAllPending(); - OnResponseCompleted(i, 1024, std::string("fake_hash")); } - // Validating the download item, so it will complete. - if (state.danger == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE) - download->DangerousDownloadValidated(); + + EXPECT_EQ(expected_intermediate_path.value(), + download->GetFullPath().value()); + download_manager_delegate_->SetShouldCompleteDownload(true); + download_manager_delegate_->InvokeDownloadCompletionCallback(); message_loop_.RunAllPending(); + EXPECT_EQ(expected_final_path.value(), + download->GetTargetFilePath().value()); } } @@ -964,8 +907,10 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) { AddMockDownloadToFileManager(info->download_id.local(), download_file); EXPECT_CALL(*download_file, Rename(cr_path)) - .Times(1) .WillOnce(Return(net::OK)); + EXPECT_CALL(*download_file, AppendDataToFile(kTestData, kTestDataLen)) + .WillOnce(Return(net::OK)); + EXPECT_CALL(*download_file, Cancel()); download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle()); @@ -975,7 +920,6 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) { scoped_ptr<ItemObserver> observer(new ItemObserver(download)); download_file->AppendDataToFile(kTestData, kTestDataLen); - ContinueDownloadWithPath(download, new_path); message_loop_.RunAllPending(); EXPECT_TRUE(GetActiveDownloadItem(0) != NULL); @@ -1107,8 +1051,10 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) { // |download_file| is owned by DownloadFileManager. EXPECT_CALL(*download_file, Rename(cr_path)) - .Times(1) .WillOnce(Return(net::OK)); + EXPECT_CALL(*download_file, AppendDataToFile(kTestData, kTestDataLen)) + .WillOnce(Return(net::OK)); + EXPECT_CALL(*download_file, Cancel()); download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle()); @@ -1150,12 +1096,9 @@ TEST_F(DownloadManagerTest, MAYBE_DownloadOverwriteTest) { using ::testing::Invoke; using ::testing::Return; - // Create a temporary directory. - ScopedTempDir temp_dir_; - ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); - + ASSERT_TRUE(CreateTempDownloadsDirectory()); // File names we're using. - const FilePath new_path(temp_dir_.path().AppendASCII("foo.txt")); + const FilePath new_path(GetPathInDownloadsDir(FILE_PATH_LITERAL("foo.txt"))); const FilePath cr_path(GetTempDownloadPath(new_path)); EXPECT_FALSE(file_util::PathExists(new_path)); @@ -1241,12 +1184,10 @@ TEST_F(DownloadManagerTest, MAYBE_DownloadRemoveTest) { using ::testing::Invoke; using ::testing::Return; - // Create a temporary directory. - ScopedTempDir temp_dir_; - ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); + ASSERT_TRUE(CreateTempDownloadsDirectory()); // File names we're using. - const FilePath new_path(temp_dir_.path().AppendASCII("foo.txt")); + const FilePath new_path(GetPathInDownloadsDir(FILE_PATH_LITERAL("foo.txt"))); const FilePath cr_path(GetTempDownloadPath(new_path)); EXPECT_FALSE(file_util::PathExists(new_path)); diff --git a/content/browser/download/download_state_info.cc b/content/browser/download/download_state_info.cc deleted file mode 100644 index b927fb0..0000000 --- a/content/browser/download/download_state_info.cc +++ /dev/null @@ -1,41 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "content/browser/download/download_state_info.h" - -#include "content/public/browser/download_item.h" - -DownloadStateInfo::DownloadStateInfo() - : path_uniquifier(0), - has_user_gesture(false), - transition_type(content::PAGE_TRANSITION_LINK), - prompt_user_for_save_location(false), - danger(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS) { -} - -DownloadStateInfo::DownloadStateInfo(const FilePath& forced_name, - bool has_user_gesture, - content::PageTransition transition_type, - bool prompt_user_for_save_location) - : path_uniquifier(0), - has_user_gesture(has_user_gesture), - transition_type(transition_type), - prompt_user_for_save_location(prompt_user_for_save_location), - danger(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS), - force_file_name(forced_name) { -} - -bool DownloadStateInfo::IsDangerous() const { - // TODO(noelutz): At this point only the windows views UI supports - // warnings based on dangerous content. -#ifdef OS_WIN - return (danger == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE || - danger == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL || - danger == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT || - danger == content::DOWNLOAD_DANGER_TYPE_UNCOMMON_CONTENT); -#else - return (danger == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE || - danger == content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL); -#endif -} diff --git a/content/browser/download/download_state_info.h b/content/browser/download/download_state_info.h deleted file mode 100644 index 2768a5f..0000000 --- a/content/browser/download/download_state_info.h +++ /dev/null @@ -1,55 +0,0 @@ -// Copyright (c) 2012 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_STATE_INFO_H_ -#define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_STATE_INFO_H_ -#pragma once - -#include "base/file_path.h" -#include "content/common/content_export.h" -#include "content/public/browser/download_danger_type.h" -#include "content/public/common/page_transition_types.h" - -// Contains information relating to the process of determining what to do with -// the download. -struct DownloadStateInfo { - DownloadStateInfo(); - DownloadStateInfo(const FilePath& forced_name, - bool has_user_gesture, - content::PageTransition transition_type, - bool prompt_user_for_save_location); - - // Indicates if the download is dangerous. - CONTENT_EXPORT bool IsDangerous() const; - - // The original name for a dangerous download, specified by the request. - FilePath target_name; - - // The path where we save the download. Typically generated. - FilePath suggested_path; - - // A number that should be added to the suggested path to make it unique. - // 0 means no number should be appended. It is eventually incorporated - // into the final file name. - int path_uniquifier; - - // True if the download is the result of user action. - bool has_user_gesture; - - content::PageTransition transition_type; - - // True if we should display the 'save as...' UI and prompt the user - // for the download location. - // False if the UI should be suppressed and the download performed to the - // default location. - bool prompt_user_for_save_location; - - content::DownloadDangerType danger; - - // If non-empty, contains an externally supplied filename that should be used - // as the target path. - FilePath force_file_name; -}; - -#endif // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_STATE_INFO_H_ diff --git a/content/content_browser.gypi b/content/content_browser.gypi index d3b37fe..98aec01 100644 --- a/content/content_browser.gypi +++ b/content/content_browser.gypi @@ -288,8 +288,6 @@ 'browser/download/download_request_handle.h', 'browser/download/download_resource_handler.cc', 'browser/download/download_resource_handler.h', - 'browser/download/download_state_info.cc', - 'browser/download/download_state_info.h', 'browser/download/download_stats.cc', 'browser/download/download_stats.h', 'browser/download/drag_download_file.cc', diff --git a/content/public/browser/DEPS b/content/public/browser/DEPS index 69de06e..26f874f 100644 --- a/content/public/browser/DEPS +++ b/content/public/browser/DEPS @@ -5,7 +5,6 @@ include_rules = [ "+content/browser/worker_host/worker_service_impl.h", # TODO(jam): remove - "+content/browser/download/download_state_info.h", "+content/browser/javascript_dialogs.h", "+content/browser/web_contents/web_contents_impl.h", "+content/browser/webui/web_ui.h", diff --git a/content/public/browser/download_item.h b/content/public/browser/download_item.h index e07444a..a76786e 100644 --- a/content/public/browser/download_item.h +++ b/content/public/browser/download_item.h @@ -20,11 +20,13 @@ #include <map> #include <string> +#include <vector> +#include "base/file_path.h" #include "base/string16.h" -#include "content/browser/download/download_state_info.h" #include "content/public/browser/download_danger_type.h" #include "content/public/browser/download_interrupt_reasons.h" +#include "content/public/common/page_transition_types.h" class DownloadFileManager; class FilePath; @@ -84,6 +86,16 @@ class CONTENT_EXPORT DownloadItem { DELETE_DUE_TO_USER_DISCARD }; + // How the final target path should be used. + enum TargetDisposition { + TARGET_DISPOSITION_OVERWRITE, // Overwrite if the target already exists. + TARGET_DISPOSITION_UNIQUIFY, // Append a uniquifier if the target already + // exists. E.g.: "foo.txt" -> "foo (1).txt" + TARGET_DISPOSITION_PROMPT // Prompt the user for the actual + // target. Implies + // TARGET_DISPOSITION_OVERWRITE. + }; + // A fake download table ID which represents a download that has started, // but is not yet in the table. static const int kUninitializedHandle; @@ -201,21 +213,9 @@ class CONTENT_EXPORT DownloadItem { // total size). virtual int PercentComplete() const = 0; - // Called when the final path has been determined. - virtual void OnPathDetermined(const FilePath& path) = 0; - // Returns true if this download has saved all of its data. virtual bool AllDataSaved() const = 0; - // Update the fields that may have changed in DownloadStateInfo as a - // result of analyzing the file and figuring out its type, location, etc. - // May only be called once. - virtual void SetFileCheckResults(const DownloadStateInfo& state) = 0; - - // Update the download's path, the actual file is renamed on the download - // thread. - virtual void Rename(const FilePath& full_path) = 0; - // Allow the user to temporarily pause a download or resume a paused download. virtual void TogglePause() = 0; @@ -224,9 +224,6 @@ class CONTENT_EXPORT DownloadItem { // DownloadItem::Completed(). virtual void OnDownloadCompleting(DownloadFileManager* file_manager) = 0; - // Called when the file name for the download is renamed to its final name. - virtual void OnDownloadRenamedToFinalName(const FilePath& full_path) = 0; - // Returns true if this item matches |query|. |query| must be lower-cased. virtual bool MatchesQuery(const string16& query) const = 0; @@ -245,14 +242,51 @@ class CONTENT_EXPORT DownloadItem { // Returns true if we have all the data and know the final file name. virtual bool IsComplete() const = 0; + // Full path to the downloaded or downloading file. This is the path to the + // physical file, if one exists. Can be empty if the in-progress path hasn't + // been determined yet. + virtual const FilePath& GetFullPath() const = 0; + + // Target path of an in-progress download. We may be downloading to a + // temporary or intermediate file (specified by |current_path_|. Once the + // download completes, we will rename the file to |target_path_|. + virtual const FilePath& GetTargetFilePath() const = 0; + + // Get the target disposition. + virtual TargetDisposition GetTargetDisposition() const = 0; + + // Called when the target path has been determined. |target_path| is the + // suggested target path. |disposition| indicates how the target path should + // be used (see TargetDisposition). |danger_type| is the danger level of + // |target_path| as determined by the caller. If |disposition| is + // TARGET_DISPOSITION_PROMPT, then OnTargetPathSelected() should be called + // subsequently with the user's selected target path. + virtual void OnTargetPathDetermined( + const FilePath& target_path, + TargetDisposition disposition, + content::DownloadDangerType danger_type) = 0; + + // This method should be called if and only if OnTargetPathDetermined() was + // called previously with disposition set to TARGET_DISPOSITION_PROMPT. + // |target_path| is the path that the user selected after being prompted for a + // target path. + virtual void OnTargetPathSelected(const FilePath& target_path) = 0; + + // Called if a check of the download contents was performed and the results of + // the test are available. This should only be called after AllDataSaved() is + // true. + virtual void OnContentCheckCompleted(DownloadDangerType danger_type) = 0; + + virtual void OnIntermediatePathDetermined(DownloadFileManager* file_manager, + const FilePath& path, + bool ok_to_overwrite) = 0; + virtual void SetIsPersisted() = 0; virtual bool IsPersisted() const = 0; // Accessors virtual const std::string& GetHash() const = 0; virtual DownloadState GetState() const = 0; - virtual const FilePath& GetFullPath() const = 0; - virtual void SetPathUniquifier(int uniquifier) = 0; virtual const GURL& GetURL() const = 0; virtual const std::vector<GURL>& GetUrlChain() const = 0; virtual const GURL& GetOriginalUrl() const = 0; @@ -280,14 +314,14 @@ class CONTENT_EXPORT DownloadItem { virtual SafetyState GetSafetyState() const = 0; // Why |safety_state_| is not SAFE. virtual DownloadDangerType GetDangerType() const = 0; - virtual void SetDangerType(DownloadDangerType danger_type) = 0; virtual bool IsDangerous() const = 0; virtual bool GetAutoOpened() = 0; - virtual const FilePath& GetTargetName() const = 0; - virtual bool PromptUserForSaveLocation() const = 0; + virtual FilePath GetTargetName() const = 0; + virtual const FilePath& GetForcedFilePath() const = 0; + virtual bool HasUserGesture() const = 0; + virtual content::PageTransition GetTransitionType() const = 0; virtual bool IsOtr() const = 0; - virtual const FilePath& GetSuggestedPath() const = 0; virtual bool IsTemporary() const = 0; virtual void SetIsTemporary(bool temporary) = 0; virtual void SetOpened(bool opened) = 0; @@ -298,13 +332,9 @@ class CONTENT_EXPORT DownloadItem { virtual DownloadInterruptReason GetLastReason() const = 0; virtual DownloadPersistentStoreInfo GetPersistentStoreInfo() const = 0; - virtual DownloadStateInfo GetStateInfo() const = 0; virtual BrowserContext* GetBrowserContext() const = 0; virtual WebContents* GetWebContents() const = 0; - // Returns the final target file path for the download. - virtual FilePath GetTargetFilePath() const = 0; - // Returns the file-name that should be reported to the user. If a display // name has been explicitly set using SetDisplayName(), this function returns // that display name. Otherwise returns the final target filename. @@ -320,9 +350,6 @@ class CONTENT_EXPORT DownloadItem { // but does not for dangerous downloads until the name is verified. virtual FilePath GetUserVerifiedFilePath() const = 0; - // Returns true if the current file name is not the final target name yet. - virtual bool NeedsRename() const = 0; - // Cancels the off-thread aspects of the download. // TODO(rdsmith): This should be private and only called from // DownloadItem::Cancel/Interrupt; it isn't now because we can't diff --git a/content/public/browser/download_manager.h b/content/public/browser/download_manager.h index b405a55..b029c28 100644 --- a/content/public/browser/download_manager.h +++ b/content/public/browser/download_manager.h @@ -145,13 +145,6 @@ class CONTENT_EXPORT DownloadManager const std::string& hash_state, DownloadInterruptReason reason) = 0; - // Called when the download is renamed to its final name. - // |uniquifier| is a number used to make unique names for the file. It is - // only valid for the DANGEROUS_BUT_VALIDATED state of the download item. - virtual void OnDownloadRenamedToFinalName(int download_id, - const FilePath& full_path, - int uniquifier) = 0; - // Remove downloads after remove_begin (inclusive) and before remove_end // (exclusive). You may pass in null Time values to do an unbounded delete // in either direction. diff --git a/content/public/browser/download_manager_delegate.cc b/content/public/browser/download_manager_delegate.cc index 52760c1..08b34d2 100644 --- a/content/public/browser/download_manager_delegate.cc +++ b/content/public/browser/download_manager_delegate.cc @@ -5,6 +5,7 @@ #include "content/public/browser/download_manager_delegate.h" #include "content/public/browser/download_id.h" +#include "content/public/browser/download_item.h" namespace content { @@ -20,8 +21,10 @@ bool DownloadManagerDelegate::ShouldStartDownload(int32 download_id) { } FilePath DownloadManagerDelegate::GetIntermediatePath( - const FilePath& suggested_path) { - return suggested_path; + const DownloadItem& item, + bool* ok_to_overwrite) { + *ok_to_overwrite = true; + return item.GetTargetFilePath(); } WebContents* DownloadManagerDelegate:: diff --git a/content/public/browser/download_manager_delegate.h b/content/public/browser/download_manager_delegate.h index 21b09024..88a8cc5 100644 --- a/content/public/browser/download_manager_delegate.h +++ b/content/public/browser/download_manager_delegate.h @@ -57,8 +57,12 @@ class CONTENT_EXPORT DownloadManagerDelegate { int32 download_id) {} // Allows the embedder to set an intermediate name for the download until it's - // complete. If the embedder doesn't want this return the suggested path. - virtual FilePath GetIntermediatePath(const FilePath& suggested_path); + // complete. The return value is the intermediate path to use. ok_to_overwrite + // should be set to true if the intermediate path should be overwritten if it + // exists. If the embedder doesn't want to set an intermediate path, it should + // return item.GetTargetFilePath(). + virtual FilePath GetIntermediatePath(const DownloadItem& item, + bool* ok_to_overwrite); // Called when the download system wants to alert a WebContents that a // download has started, but the TabConetnts has gone away. This lets an diff --git a/content/shell/shell_download_manager_delegate.cc b/content/shell/shell_download_manager_delegate.cc index 6f10b13..9a818f42 100644 --- a/content/shell/shell_download_manager_delegate.cc +++ b/content/shell/shell_download_manager_delegate.cc @@ -43,10 +43,14 @@ DownloadId ShellDownloadManagerDelegate::GetNextId() { bool ShellDownloadManagerDelegate::ShouldStartDownload(int32 download_id) { DownloadItem* download = download_manager_->GetActiveDownloadItem(download_id); - DownloadStateInfo state = download->GetStateInfo(); - if (!state.force_file_name.empty()) + if (!download->GetForcedFilePath().empty()) { + download->OnTargetPathDetermined( + download->GetForcedFilePath(), + DownloadItem::TARGET_DISPOSITION_OVERWRITE, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); return true; + } FilePath generated_name = net::GenerateFileName( download->GetURL(), @@ -56,47 +60,44 @@ bool ShellDownloadManagerDelegate::ShouldStartDownload(int32 download_id) { download->GetMimeType(), "download"); - // Since we have no download UI, show the user a dialog always. - state.prompt_user_for_save_location = true; - BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, base::Bind( &ShellDownloadManagerDelegate::GenerateFilename, - this, download_id, state, generated_name)); + this, download_id, generated_name)); return false; } void ShellDownloadManagerDelegate::GenerateFilename( int32 download_id, - DownloadStateInfo state, const FilePath& generated_name) { - if (state.suggested_path.empty()) { - state.suggested_path = download_manager_->GetBrowserContext()->GetPath(). - Append(FILE_PATH_LITERAL("Downloads")); - if (!file_util::PathExists(state.suggested_path)) - file_util::CreateDirectory(state.suggested_path); - } - - state.suggested_path = state.suggested_path.Append(generated_name); + FilePath suggested_path = download_manager_->GetBrowserContext()->GetPath(). + Append(FILE_PATH_LITERAL("Downloads")); + if (!file_util::PathExists(suggested_path)) + file_util::CreateDirectory(suggested_path); + suggested_path = suggested_path.Append(generated_name); BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, base::Bind( &ShellDownloadManagerDelegate::RestartDownload, - this, download_id, state)); + this, download_id, suggested_path)); } void ShellDownloadManagerDelegate::RestartDownload( int32 download_id, - DownloadStateInfo state) { + const FilePath& suggested_path) { DownloadItem* download = download_manager_->GetActiveDownloadItem(download_id); if (!download) return; - download->SetFileCheckResults(state); + + // Since we have no download UI, show the user a dialog always. + download->OnTargetPathDetermined(suggested_path, + DownloadItem::TARGET_DISPOSITION_PROMPT, + content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); download_manager_->RestartDownload(download_id); } diff --git a/content/shell/shell_download_manager_delegate.h b/content/shell/shell_download_manager_delegate.h index 0440f4b..a973d26 100644 --- a/content/shell/shell_download_manager_delegate.h +++ b/content/shell/shell_download_manager_delegate.h @@ -10,8 +10,6 @@ #include "base/memory/ref_counted.h" #include "content/public/browser/download_manager_delegate.h" -struct DownloadStateInfo; - namespace content { class DownloadManager; @@ -36,10 +34,9 @@ class ShellDownloadManagerDelegate virtual ~ShellDownloadManagerDelegate(); void GenerateFilename(int32 download_id, - DownloadStateInfo state, const FilePath& generated_name); void RestartDownload(int32 download_id, - DownloadStateInfo state); + const FilePath& suggested_path); DownloadManager* download_manager_; diff --git a/content/test/mock_download_item.h b/content/test/mock_download_item.h index e028096..265e044 100644 --- a/content/test/mock_download_item.h +++ b/content/test/mock_download_item.h @@ -45,22 +45,27 @@ class MockDownloadItem : public content::DownloadItem { MOCK_CONST_METHOD1(TimeRemaining, bool(base::TimeDelta*)); MOCK_CONST_METHOD0(CurrentSpeed, int64()); MOCK_CONST_METHOD0(PercentComplete, int()); - MOCK_METHOD1(OnPathDetermined, void(const FilePath&)); MOCK_CONST_METHOD0(AllDataSaved, bool()); - MOCK_METHOD1(SetFileCheckResults, void(const DownloadStateInfo&)); - MOCK_METHOD1(Rename, void(const FilePath&)); MOCK_METHOD0(TogglePause, void()); MOCK_METHOD1(OnDownloadCompleting, void(DownloadFileManager*)); - MOCK_METHOD1(OnDownloadRenamedToFinalName, void(const FilePath&)); MOCK_CONST_METHOD1(MatchesQuery, bool(const string16& query)); MOCK_CONST_METHOD0(IsPartialDownload, bool()); MOCK_CONST_METHOD0(IsInProgress, bool()); MOCK_CONST_METHOD0(IsCancelled, bool()); MOCK_CONST_METHOD0(IsInterrupted, bool()); MOCK_CONST_METHOD0(IsComplete, bool()); - MOCK_CONST_METHOD0(GetState, DownloadState()); MOCK_CONST_METHOD0(GetFullPath, const FilePath&()); - MOCK_METHOD1(SetPathUniquifier, void(int)); + MOCK_CONST_METHOD0(GetTargetFilePath, const FilePath&()); + MOCK_CONST_METHOD0(GetTargetDisposition, TargetDisposition()); + MOCK_METHOD3(OnTargetPathDetermined, void(const FilePath&, + TargetDisposition, + content::DownloadDangerType)); + MOCK_METHOD1(OnTargetPathSelected, void(const FilePath&)); + MOCK_METHOD1(OnContentCheckCompleted, void(content::DownloadDangerType)); + MOCK_METHOD3(OnIntermediatePathDetermined, void(DownloadFileManager*, + const FilePath&, + bool)); + MOCK_CONST_METHOD0(GetState, DownloadState()); MOCK_CONST_METHOD0(GetUrlChain, const std::vector<GURL>&()); MOCK_METHOD1(SetTotalBytes, void(int64)); MOCK_CONST_METHOD0(GetURL, const GURL&()); @@ -91,13 +96,13 @@ class MockDownloadItem : public content::DownloadItem { MOCK_CONST_METHOD0(GetFileExternallyRemoved, bool()); MOCK_CONST_METHOD0(GetSafetyState, SafetyState()); MOCK_CONST_METHOD0(GetDangerType, content::DownloadDangerType()); - MOCK_METHOD1(SetDangerType, void(content::DownloadDangerType)); MOCK_CONST_METHOD0(IsDangerous, bool()); MOCK_METHOD0(GetAutoOpened, bool()); - MOCK_CONST_METHOD0(GetTargetName, const FilePath&()); - MOCK_CONST_METHOD0(PromptUserForSaveLocation, bool()); + MOCK_CONST_METHOD0(GetTargetName, FilePath()); + MOCK_CONST_METHOD0(GetForcedFilePath, const FilePath&()); + MOCK_CONST_METHOD0(HasUserGesture, bool()); + MOCK_CONST_METHOD0(GetTransitionType, content::PageTransition()); MOCK_CONST_METHOD0(IsOtr, bool()); - MOCK_CONST_METHOD0(GetSuggestedPath, const FilePath&()); MOCK_CONST_METHOD0(IsTemporary, bool()); MOCK_METHOD1(SetIsTemporary, void(bool)); MOCK_METHOD1(SetOpened, void(bool)); @@ -106,14 +111,11 @@ class MockDownloadItem : public content::DownloadItem { MOCK_CONST_METHOD0(GetETag, const std::string&()); MOCK_CONST_METHOD0(GetLastReason, DownloadInterruptReason()); MOCK_CONST_METHOD0(GetPersistentStoreInfo, DownloadPersistentStoreInfo()); - MOCK_CONST_METHOD0(GetStateInfo, DownloadStateInfo()); MOCK_CONST_METHOD0(GetBrowserContext, content::BrowserContext*()); MOCK_CONST_METHOD0(GetWebContents, content::WebContents*()); - MOCK_CONST_METHOD0(GetTargetFilePath, FilePath()); MOCK_CONST_METHOD0(GetFileNameToReportUser, FilePath()); MOCK_METHOD1(SetDisplayName, void(const FilePath&)); MOCK_CONST_METHOD0(GetUserVerifiedFilePath, FilePath()); - MOCK_CONST_METHOD0(NeedsRename, bool()); MOCK_METHOD1(OffThreadCancel, void(DownloadFileManager* file_manager)); MOCK_CONST_METHOD1(DebugString, std::string(bool)); MOCK_METHOD0(MockDownloadOpenForTesting, void()); diff --git a/content/test/mock_download_manager.h b/content/test/mock_download_manager.h index 6ced362..41d92e7 100644 --- a/content/test/mock_download_manager.h +++ b/content/test/mock_download_manager.h @@ -45,9 +45,6 @@ class MockDownloadManager : public content::DownloadManager { int64 size, const std::string& hash_state, content::DownloadInterruptReason reason)); - MOCK_METHOD3(OnDownloadRenamedToFinalName, void(int download_id, - const FilePath& full_path, - int uniquifier)); MOCK_METHOD2(RemoveDownloadsBetween, int(base::Time remove_begin, base::Time remove_end)); MOCK_METHOD1(RemoveDownloads, int(base::Time remove_begin)); |