Index: content/browser/download/download_browsertest.cc |
diff --git a/content/browser/download/download_browsertest.cc b/content/browser/download/download_browsertest.cc |
index 6fab6e25e38dc4f0b6836bba8233aedb996fd2e0..95c29f86140a5307e5846198ce7dae0d7d613332 100644 |
--- a/content/browser/download/download_browsertest.cc |
+++ b/content/browser/download/download_browsertest.cc |
@@ -8,10 +8,14 @@ |
#include "base/file_path.h" |
#include "base/file_util.h" |
#include "base/scoped_temp_dir.h" |
+#include "content/browser/download/download_file_factory.h" |
+#include "content/browser/download/download_file_impl.h" |
#include "content/browser/download/download_item_impl.h" |
#include "content/browser/download/download_manager_impl.h" |
+#include "content/browser/power_save_blocker.h" |
#include "content/browser/web_contents/web_contents_impl.h" |
#include "content/public/test/download_test_observer.h" |
+#include "content/public/test/test_utils.h" |
#include "content/shell/shell.h" |
#include "content/shell/shell_browser_context.h" |
#include "content/shell/shell_download_manager_delegate.h" |
@@ -25,9 +29,204 @@ namespace content { |
namespace { |
-static DownloadManager* DownloadManagerForShell(Shell* shell) { |
- return BrowserContext::GetDownloadManager( |
- shell->web_contents()->GetBrowserContext()); |
+class DownloadFileWithDelayFactory; |
+ |
+static DownloadManagerImpl* DownloadManagerForShell(Shell* shell) { |
+ // We're in a content_browsertest; we know that the DownloadManager |
+ // is a DownloadManagerImpl. |
+ return static_cast<DownloadManagerImpl*>( |
+ BrowserContext::GetDownloadManager( |
+ shell->web_contents()->GetBrowserContext())); |
+} |
+ |
+class DownloadFileWithDelay : public DownloadFileImpl { |
+ public: |
+ DownloadFileWithDelay( |
+ const content::DownloadSaveInfo& save_info, |
+ const GURL& url, |
+ const GURL& referrer_url, |
+ int64 received_bytes, |
+ bool calculate_hash, |
+ scoped_ptr<content::ByteStreamReader> stream, |
+ const net::BoundNetLog& bound_net_log, |
+ scoped_ptr<content::PowerSaveBlocker> power_save_blocker, |
+ base::WeakPtr<content::DownloadDestinationObserver> observer, |
+ base::WeakPtr<DownloadFileWithDelayFactory> owner); |
+ |
benjhayden
2012/09/12 21:01:01
Don't you need a virtual destructor?
Randy Smith (Not in Mondays)
2012/09/13 20:15:12
Done.
|
+ // Wraps DownloadFileImpl::Rename and intercepts the return callback, |
+ // storing it in the factory that produced this object for later |
+ // retrieval. |
+ virtual void Rename(const FilePath& full_path, |
+ bool overwrite_existing_file, |
+ const RenameCompletionCallback& callback) OVERRIDE; |
+ |
+ // Wraps DownloadFileImpl::Detach and intercepts the return callback, |
+ // storing it in the factory that produced this object for later |
benjhayden
2012/09/12 21:01:01
double space
Randy Smith (Not in Mondays)
2012/09/13 20:15:12
Done.
|
+ // retrieval. |
+ virtual void Detach(base::Closure callback) OVERRIDE; |
+ |
+ private: |
+ // Must be called on the UI thread. |
+ static void RenameCallbackWrapper( |
+ DownloadFileWithDelayFactory* factory, |
+ const RenameCompletionCallback& original_callback, |
+ content::DownloadInterruptReason reason, |
+ const FilePath& path); |
+ |
+ // Must be called on the UI thread. |
benjhayden
2012/09/12 21:01:01
Instead of these comments, can you just have the D
Randy Smith (Not in Mondays)
2012/09/13 20:15:12
I don't consider the two equivalent; one's interfa
|
+ static void DetachCallbackWrapper( |
+ DownloadFileWithDelayFactory* factory, |
+ const base::Closure& original_callback); |
+ |
+ // Must only be accessed on the FILE thread; must be used only on the |
benjhayden
2012/09/12 21:01:01
What's the difference between access and use?
Randy Smith (Not in Mondays)
2012/09/13 20:15:12
The variable must be read on the FILE thread, and
|
+ // UI thread. (This object lives on the file thread, and the |
+ // DownloadFileWithDelayFactory lives on the UI thread). |
+ base::WeakPtr<DownloadFileWithDelayFactory> owner_; |
+}; |
+ |
+// All routines on this class must be called on the UI thread. |
+class DownloadFileWithDelayFactory : public DownloadFileFactory { |
+ public: |
+ DownloadFileWithDelayFactory(); |
+ virtual ~DownloadFileWithDelayFactory(); |
+ |
+ // DownloadFileFactory interface. |
+ virtual DownloadFile* CreateFile( |
+ const content::DownloadSaveInfo& save_info, |
+ GURL url, |
+ GURL referrer_url, |
+ int64 received_bytes, |
+ bool calculate_hash, |
+ scoped_ptr<content::ByteStreamReader> stream, |
+ const net::BoundNetLog& bound_net_log, |
+ base::WeakPtr<content::DownloadDestinationObserver> observer) OVERRIDE; |
+ |
+ void PostRenameCallback(base::Closure callback); |
benjhayden
2012/09/12 21:01:01
s/Post/Add/g ?
Randy Smith (Not in Mondays)
2012/09/13 20:15:12
Done.
|
+ void PostDetachCallback(base::Closure callback); |
+ void GetRenameCallbacks(std::vector<base::Closure>* results); |
benjhayden
2012/09/12 21:01:01
s/Get/GetAll/g ?
Randy Smith (Not in Mondays)
2012/09/13 20:15:12
Done.
|
+ void GetDetachCallbacks(std::vector<base::Closure>* results); |
+ |
+ // Do not return until either GetRenameCallbacks() or GetDetachCallbacks() |
+ // will return a non-empty list. |
+ void WaitForSomeCallback(); |
+ |
+ private: |
+ base::WeakPtrFactory<DownloadFileWithDelayFactory> factory_; |
benjhayden
2012/09/12 21:01:01
weak_ptr_factor_?
Randy Smith (Not in Mondays)
2012/09/13 20:15:12
Done.
|
+ std::vector<base::Closure> rename_callbacks_; |
+ std::vector<base::Closure> detach_callbacks_; |
+ bool waiting_; |
+}; |
benjhayden
2012/09/12 21:01:01
DISALLOW_COPY_AND_ASSIGN?
Randy Smith (Not in Mondays)
2012/09/13 20:15:12
Done.
|
+ |
+DownloadFileWithDelay::DownloadFileWithDelay( |
benjhayden
2012/09/12 21:01:01
Since we're in a test, would you mind defining the
Randy Smith (Not in Mondays)
2012/09/13 20:15:12
I started out doing that. The problem is that the
|
+ const content::DownloadSaveInfo& save_info, |
+ const GURL& url, |
+ const GURL& referrer_url, |
+ int64 received_bytes, |
+ bool calculate_hash, |
+ scoped_ptr<content::ByteStreamReader> stream, |
+ const net::BoundNetLog& bound_net_log, |
+ scoped_ptr<content::PowerSaveBlocker> power_save_blocker, |
+ base::WeakPtr<content::DownloadDestinationObserver> observer, |
+ base::WeakPtr<DownloadFileWithDelayFactory> owner) |
+ : DownloadFileImpl( |
+ save_info, url, referrer_url, received_bytes, calculate_hash, |
+ stream.Pass(), bound_net_log, power_save_blocker.Pass(), |
+ observer), |
+ owner_(owner) {} |
+ |
+void DownloadFileWithDelay::Rename(const FilePath& full_path, |
+ bool overwrite_existing_file, |
+ const RenameCompletionCallback& callback) { |
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); |
+ DownloadFileImpl::Rename( |
+ full_path, overwrite_existing_file, |
+ base::Bind(DownloadFileWithDelay::RenameCallbackWrapper, |
+ owner_, callback)); |
+} |
+ |
+void DownloadFileWithDelay::Detach(base::Closure callback) { |
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); |
+ DownloadFileImpl::Detach( |
+ base::Bind(DownloadFileWithDelay::DetachCallbackWrapper, |
+ owner_, callback)); |
+} |
+ |
+// static |
+void DownloadFileWithDelay::RenameCallbackWrapper( |
+ DownloadFileWithDelayFactory* factory, |
+ const RenameCompletionCallback& original_callback, |
+ content::DownloadInterruptReason reason, |
+ const FilePath& path) { |
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); |
+ factory->PostRenameCallback(base::Bind(original_callback, reason, path)); |
+} |
+ |
+// static |
+void DownloadFileWithDelay::DetachCallbackWrapper( |
+ DownloadFileWithDelayFactory* factory, |
+ const base::Closure& original_callback) { |
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); |
+ factory->PostDetachCallback(original_callback); |
+} |
+ |
+DownloadFileWithDelayFactory::DownloadFileWithDelayFactory() |
+ : factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)), |
+ waiting_(false) {} |
+DownloadFileWithDelayFactory::~DownloadFileWithDelayFactory() {} |
+ |
+DownloadFile* DownloadFileWithDelayFactory::CreateFile( |
+ const content::DownloadSaveInfo& save_info, |
+ GURL url, |
+ GURL referrer_url, |
+ int64 received_bytes, |
+ bool calculate_hash, |
+ scoped_ptr<content::ByteStreamReader> stream, |
+ const net::BoundNetLog& bound_net_log, |
+ base::WeakPtr<content::DownloadDestinationObserver> observer) { |
+ scoped_ptr<content::PowerSaveBlocker> psb( |
+ new content::PowerSaveBlocker( |
+ content::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension, |
+ "Download in progress")); |
+ return new DownloadFileWithDelay( |
+ save_info, url, referrer_url, received_bytes, calculate_hash, |
+ stream.Pass(), bound_net_log, psb.Pass(), observer, |
+ factory_.GetWeakPtr()); |
+} |
+ |
+void DownloadFileWithDelayFactory::PostRenameCallback(base::Closure callback) { |
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); |
+ rename_callbacks_.push_back(callback); |
+ if (waiting_) |
+ MessageLoopForUI::current()->Quit(); |
+} |
+ |
+void DownloadFileWithDelayFactory::PostDetachCallback(base::Closure callback) { |
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); |
+ detach_callbacks_.push_back(callback); |
+ if (waiting_) |
+ MessageLoopForUI::current()->Quit(); |
+} |
+ |
+void DownloadFileWithDelayFactory::GetRenameCallbacks( |
+ std::vector<base::Closure>* results) { |
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); |
+ results->swap(rename_callbacks_); |
+} |
+ |
+void DownloadFileWithDelayFactory::GetDetachCallbacks( |
+ std::vector<base::Closure>* results) { |
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); |
+ results->swap(detach_callbacks_); |
+} |
+ |
+void DownloadFileWithDelayFactory::WaitForSomeCallback() { |
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); |
+ |
+ while (rename_callbacks_.empty() && detach_callbacks_.empty()) { |
benjhayden
2012/09/12 21:01:01
'while' or 'if'?
Randy Smith (Not in Mondays)
2012/09/13 20:15:12
Presuming comment means you prefer 'if'; done.
(T
|
+ waiting_ = true; |
+ RunMessageLoop(); |
+ waiting_ = false; |
+ } |
} |
} // namespace |
@@ -127,18 +326,18 @@ class DownloadContentTest : public ContentBrowserTest { |
}; |
IN_PROC_BROWSER_TEST_F(DownloadContentTest, DownloadCancelled) { |
- // TODO(rdsmith): Fragile code warning! The code below relies on the |
- // DownloadTestObserverInProgress only finishing when the new download |
- // has reached the state of being entered into the history and being |
- // user-visible (that's what's required for the Remove to be valid and |
- // for the download shelf to be visible). By the pure semantics of |
- // DownloadTestObserverInProgress, that's not guaranteed; DownloadItems |
- // are created in the IN_PROGRESS state and made known to the DownloadManager |
- // immediately, so any ModelChanged event on the DownloadManager after |
- // navigation would allow the observer to return. However, the only |
- // ModelChanged() event the code will currently fire is in |
- // OnCreateDownloadEntryComplete, at which point the download item will |
- // be in the state we need. |
+ // TODO(rdsmith): Fragile code warning! The code below relies on |
+ // the DownloadTestObserverInProgress only finishing when the new |
+ // download has reached the state of being entered into the history |
+ // and being user-visible (that's what's required for the Remove to |
+ // be valid). By the pure semantics of |
+ // DownloadTestObserverInProgress, that's not guaranteed; |
+ // DownloadItems are created in the IN_PROGRESS state and made known |
+ // to the DownloadManager immediately, so any ModelChanged event on |
+ // the DownloadManager after navigation would allow the observer to |
+ // return. However, the only ModelChanged() event the code will |
+ // currently fire is in OnCreateDownloadEntryComplete, at which |
+ // point the download item will be in the state we need. |
// The right way to fix this is to create finer grained states on the |
// DownloadItem, and wait for the state that indicates the item has been |
// entered in the history and made visible in the UI. |
@@ -220,4 +419,110 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, MultiDownload) { |
file2, GetTestFilePath("download", "download-test.lib"))); |
} |
+// Try to cancel just before we release the download file, by delaying final |
+// rename callback. |
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtFinalRename) { |
+ // Setup new factory. |
+ DownloadFileWithDelayFactory* file_factory = |
+ new DownloadFileWithDelayFactory(); |
+ DownloadManagerImpl* download_manager(DownloadManagerForShell(shell())); |
+ download_manager->SetDownloadFileFactoryForTesting( |
+ scoped_ptr<content::DownloadFileFactory>(file_factory).Pass()); |
+ |
+ // Create a download |
+ FilePath file(FILE_PATH_LITERAL("download-test.lib")); |
+ NavigateToURL(shell(), URLRequestMockHTTPJob::GetMockUrl(file)); |
+ |
+ // Wait until the first (intermediate file) rename and execute the callback. |
+ file_factory->WaitForSomeCallback(); |
+ std::vector<base::Closure> callbacks; |
+ file_factory->GetDetachCallbacks(&callbacks); |
+ ASSERT_TRUE(callbacks.empty()); |
+ file_factory->GetRenameCallbacks(&callbacks); |
+ ASSERT_EQ(1u, callbacks.size()); |
+ callbacks[0].Run(); |
+ callbacks.clear(); |
+ |
+ // Wait until the second (final) rename callback is posted. |
+ file_factory->WaitForSomeCallback(); |
+ file_factory->GetDetachCallbacks(&callbacks); |
+ ASSERT_TRUE(callbacks.empty()); |
+ file_factory->GetRenameCallbacks(&callbacks); |
+ ASSERT_EQ(1u, callbacks.size()); |
+ |
+ // Cancel it. |
+ std::vector<DownloadItem*> items; |
+ download_manager->GetAllDownloads(&items); |
+ ASSERT_EQ(1u, items.size()); |
+ items[0]->Cancel(true); |
+ RunAllPendingInMessageLoop(); |
+ |
+ // Check state. |
+ EXPECT_EQ(DownloadItem::CANCELLED, items[0]->GetState()); |
+ |
+ // Run final rename callback. |
+ callbacks[0].Run(); |
+ callbacks.clear(); |
+ |
+ // Check state. |
+ EXPECT_EQ(DownloadItem::CANCELLED, items[0]->GetState()); |
+} |
+ |
+// Try to cancel just after we release the download file, by delaying |
+// release. |
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtRelease) { |
+ // Setup new factory. |
+ DownloadFileWithDelayFactory* file_factory = |
+ new DownloadFileWithDelayFactory(); |
+ DownloadManagerImpl* download_manager(DownloadManagerForShell(shell())); |
+ download_manager->SetDownloadFileFactoryForTesting( |
+ scoped_ptr<content::DownloadFileFactory>(file_factory).Pass()); |
+ |
+ // Create a download |
+ FilePath file(FILE_PATH_LITERAL("download-test.lib")); |
+ NavigateToURL(shell(), URLRequestMockHTTPJob::GetMockUrl(file)); |
+ |
+ // Wait until the first (intermediate file) rename and execute the callback. |
+ file_factory->WaitForSomeCallback(); |
+ std::vector<base::Closure> callbacks; |
+ file_factory->GetDetachCallbacks(&callbacks); |
+ ASSERT_TRUE(callbacks.empty()); |
+ file_factory->GetRenameCallbacks(&callbacks); |
+ ASSERT_EQ(1u, callbacks.size()); |
+ callbacks[0].Run(); |
+ callbacks.clear(); |
+ |
+ // Wait until the second (final) rename callback is posted. |
+ file_factory->WaitForSomeCallback(); |
+ file_factory->GetDetachCallbacks(&callbacks); |
+ ASSERT_TRUE(callbacks.empty()); |
+ file_factory->GetRenameCallbacks(&callbacks); |
+ ASSERT_EQ(1u, callbacks.size()); |
+ |
+ // Call it. |
+ callbacks[0].Run(); |
+ callbacks.clear(); |
+ |
+ // Confirm download now complete. |
+ std::vector<DownloadItem*> items; |
+ download_manager->GetAllDownloads(&items); |
+ EXPECT_EQ(DownloadItem::COMPLETE, items[0]->GetState()); |
+ |
+ // Cancel the download; confirm cancel fails. |
+ ASSERT_EQ(1u, items.size()); |
+ items[0]->Cancel(true); |
+ EXPECT_EQ(DownloadItem::COMPLETE, items[0]->GetState()); |
+ RunAllPendingInMessageLoop(); |
+ EXPECT_EQ(DownloadItem::COMPLETE, items[0]->GetState()); |
+ |
+ // Confirm detach callback and run it. |
+ file_factory->WaitForSomeCallback(); |
+ file_factory->GetRenameCallbacks(&callbacks); |
+ ASSERT_TRUE(callbacks.empty()); |
+ file_factory->GetDetachCallbacks(&callbacks); |
+ ASSERT_EQ(1u, callbacks.size()); |
+ callbacks[0].Run(); |
+ callbacks.clear(); |
+} |
+ |
} // namespace content |