Replace the DownloadFileManager with direct ownership of DownloadFileImpl

content/browser/download/download_browsertest.cc

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 DCHECK(IsOnThread...) pattern?

[Randy Smith (Not in Mondays), 2012/09/13 20:15:12]

I don't consider the two equivalent; one's interface, and one's implementation. 
However, I noted that that's about as weak an argument as I've personally ever
heard, because a) these are private methods, b) everything is in the same file,
and c) this is test code.  So done.  But it's still not equivalent :-}.

+  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 the value used (indirected
through to get the underlying object) on the UI thread.  Is there a way I could
make this more clear?

+  // 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 these methods inline with the
class definition?

[Randy Smith (Not in Mondays), 2012/09/13 20:15:12]

I started out doing that.  The problem is that the two classes refer to each
other, so each's definition section needs the other's declaration section before
it.  Result->Split.  But if there's some way to do this in C++ inline, I agree
with you--it would be nicer.

+    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.

(The argument for 'while' is that it guarantees the state for the caller of
WaitForSomeCallback, i.e. no other message loop tasks stole the callbacks.  But
if someone's stealing the callbacks that code's probably pretty messed up, so
I'm good either way.)

+    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