Shift EnsureNoPendingDownloads() implementation into tests.

content/browser/download/download_browsertest.cc

 // 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.
 
 // This file contains download browser tests that are known to be runnable
 // in a pure content context.  Over time tests should be migrated here.
 
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/scoped_temp_dir.h"
@@ skipping 159 matching lines @@
 DownloadFileWithDelayFactory::DownloadFileWithDelayFactory()
     : waiting_(false) {}
 DownloadFileWithDelayFactory::~DownloadFileWithDelayFactory() {}
 
 DownloadFile* DownloadFileWithDelayFactory::CreateFile(
     DownloadCreateInfo* info,
     scoped_ptr<content::ByteStreamReader> stream,
     DownloadManager* download_manager,
     bool calculate_hash,
     const net::BoundNetLog& bound_net_log) {
-
   return new DownloadFileWithDelay(
       info, stream.Pass(), new DownloadRequestHandle(info->request_handle),
       download_manager, calculate_hash,
       scoped_ptr<content::PowerSaveBlocker>(
           new content::PowerSaveBlocker(
               content::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension,
               "Download in progress")).Pass(),
       bound_net_log, this);
 }
 
@@ skipping 26 matching lines @@
 void DownloadFileWithDelayFactory::WaitForSomeCallback() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (rename_callbacks_.empty() && detach_callbacks_.empty()) {
     waiting_ = true;
     RunMessageLoop();
     waiting_ = false;
   }
 }
 
+class CountingDownloadFile : public DownloadFileImpl {

[benjhayden, 2012/10/09 18:29:05]

Want to put this in content/public/test?

[Randy Smith (Not in Mondays), 2012/10/09 19:54:59]

Not until there's some need for it beyond this file.  (And if the need is only
within content, I'd keep it in this directory.)

+ public:
+  CountingDownloadFile(
+      const DownloadCreateInfo* info,
+      scoped_ptr<content::ByteStreamReader> stream,
+      DownloadRequestHandleInterface* request_handle,
+      scoped_refptr<content::DownloadManager> download_manager,
+      bool calculate_hash,
+      scoped_ptr<content::PowerSaveBlocker> power_save_blocker,
+      const net::BoundNetLog& bound_net_log)
+      : DownloadFileImpl(info, stream.Pass(), request_handle, download_manager,
+                         calculate_hash, power_save_blocker.Pass(),
+                         bound_net_log) {}
+
+  virtual ~CountingDownloadFile() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    active_files_--;
+  }
+
+  virtual content::DownloadInterruptReason Initialize() OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    active_files_++;
+    return DownloadFileImpl::Initialize();
+  }
+
+  static void GetNumberActiveFiles(int* result) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    *result = active_files_;
+  }
+
+  // Can be called on any thread, and will block (running message loop)
+  // until data is returned.
+  static int GetNumberActiveFilesFromFileThread() {
+    int result;

[benjhayden, 2012/10/09 18:29:05]

Can you just '= -1'? Un-initialized variables make me twitchy.

[Randy Smith (Not in Mondays), 2012/10/09 19:54:59]

Whoops.  Sure.  Done.

+    BrowserThread::PostTaskAndReply(BrowserThread::FILE, FROM_HERE,
+        base::Bind(&CountingDownloadFile::GetNumberActiveFiles, &result),
+        MessageLoop::current()->QuitClosure());
+    MessageLoop::current()->Run();
+    return result;
+  }
+
+ private:
+  static int active_files_;
+};
+
+int CountingDownloadFile::active_files_ = 0;
+
+class CountingDownloadFileFactory : public DownloadFileFactory {

[benjhayden, 2012/10/09 18:29:05]

Templatize DownloadFileFactory?

[Randy Smith (Not in Mondays), 2012/10/09 19:54:59]

Templatize?  I'm not sure what you mean?  What would I make it compile time
parameterized on?

(I suspect my answer is going to be that if there isn't a specific need for the
complexity, I won't want to put it in, but I really should understand the
complexity first :-}).

[benjhayden, 2012/10/09 20:01:47]

Ah, sorry, I was wondering if the pattern of a DFF that created a given subclass
of DF was general enough that we could avoid repeating the parameter lists.

template <class DownloadFileType>
class DownloadFileFactory {
  ...return new DownloadFileType(...);
};

typedef DownloadFileFactory<DownloadFileImpl> DownloadFileFactoryImpl;
typedef DownloadFileFactory<CountingDownloadFile> CountingDownloadFileFactory;

+ public:
+  CountingDownloadFileFactory() {}
+  virtual ~CountingDownloadFileFactory() {}
+
+  // DownloadFileFactory interface.
+  virtual content::DownloadFile* CreateFile(
+      DownloadCreateInfo* info,
+      scoped_ptr<content::ByteStreamReader> stream,
+      DownloadManager* download_manager,
+      bool calculate_hash,
+      const net::BoundNetLog& bound_net_log) OVERRIDE {
+    return new CountingDownloadFile(
+        info, stream.Pass(), new DownloadRequestHandle(info->request_handle),
+        download_manager, calculate_hash,
+        scoped_ptr<content::PowerSaveBlocker>(
+            new content::PowerSaveBlocker(
+                content::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension,
+                "Download in progress")).Pass(),
+        bound_net_log);
+  }
+};
+
 }  // namespace
 
 class DownloadContentTest : public ContentBrowserTest {
  protected:
   virtual void SetUpOnMainThread() OVERRIDE {
     ASSERT_TRUE(downloads_directory_.CreateUniqueTempDir());
 
     ShellDownloadManagerDelegate* delegate =
         static_cast<ShellDownloadManagerDelegate*>(
             shell()->web_contents()->GetBrowserContext()
@@ skipping 19 matching lines @@
   }
 
   // Create a DownloadTestObserverInProgress that will wait for the
   // specified number of downloads to start.
   DownloadTestObserver* CreateInProgressWaiter(
       Shell* shell, int num_downloads) {
     DownloadManager* download_manager = DownloadManagerForShell(shell);
     return new DownloadTestObserverInProgress(download_manager, num_downloads);
   }
 
+  // Note: Cannot be used with other alternative DownloadFileFactorys

[benjhayden, 2012/10/09 18:29:05]

How likely is it that we'd want to use Counting* with other alternative DFFs?

[Randy Smith (Not in Mondays), 2012/10/09 19:54:59]

It's my usual metric: I don't want to add complexity until it's needed. 
Currently we don't need it; we only have three DFFs at the moment, two of which
are in this file and don't appear to need to be used with each other.  

+  void SetupEnsureNoPendingDownloads() {
+    GetDownloadFileManager()->SetFileFactoryForTesting(
+        scoped_ptr<content::DownloadFileFactory>(
+            new CountingDownloadFileFactory()).Pass());
+  }
+
   bool EnsureNoPendingDownloads() {
     bool result = true;
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::Bind(&EnsureNoPendingDownloadJobsOnIO, &result));
     MessageLoop::current()->Run();
-    return result && DownloadManager::EnsureNoPendingDownloadsForTesting();
+    return result &&
+        (CountingDownloadFile::GetNumberActiveFilesFromFileThread() == 0);
   }
 
   void DownloadAndWait(Shell* shell, const GURL& url) {
     scoped_ptr<DownloadTestObserver> observer(CreateWaiter(shell, 1));
     NavigateToURL(shell, url);
     observer->WaitForFinished();
     EXPECT_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
   }
 
   // Checks that |path| is has |file_size| bytes, and matches the |value|
@@ skipping 52 matching lines @@
   // 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.
 
+  SetupEnsureNoPendingDownloads();
+
   // Create a download, wait until it's started, and confirm
   // we're in the expected state.
   scoped_ptr<DownloadTestObserver> observer(CreateInProgressWaiter(shell(), 1));
   NavigateToURL(shell(), GURL(URLRequestSlowDownloadJob::kUnknownSizeUrl));
   observer->WaitForFinished();
 
   std::vector<DownloadItem*> downloads;
   DownloadManagerForShell(shell())->GetAllDownloads(&downloads);
   ASSERT_EQ(1u, downloads.size());
   ASSERT_EQ(DownloadItem::IN_PROGRESS, downloads[0]->GetState());
 
   // Cancel the download and wait for download system quiesce.
   downloads[0]->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD);
   scoped_refptr<DownloadTestFlushObserver> flush_observer(
       new DownloadTestFlushObserver(DownloadManagerForShell(shell())));
   flush_observer->WaitForFlush();
 
   // Get the important info from other threads and check it.
   EXPECT_TRUE(EnsureNoPendingDownloads());
 }
 
 // Check that downloading multiple (in this case, 2) files does not result in
 // corrupted files.
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, MultiDownload) {
+  SetupEnsureNoPendingDownloads();
+
   // Create a download, wait until it's started, and confirm
   // we're in the expected state.
   scoped_ptr<DownloadTestObserver> observer1(
       CreateInProgressWaiter(shell(), 1));
   NavigateToURL(shell(), GURL(URLRequestSlowDownloadJob::kUnknownSizeUrl));
   observer1->WaitForFinished();
 
   std::vector<DownloadItem*> downloads;
   DownloadManagerForShell(shell())->GetAllDownloads(&downloads);
   ASSERT_EQ(1u, downloads.size());
@@ skipping 140 matching lines @@
   file_factory->GetAllRenameCallbacks(&callbacks);
   ASSERT_TRUE(callbacks.empty());
   file_factory->GetAllDetachCallbacks(&callbacks);
   ASSERT_EQ(1u, callbacks.size());
   callbacks[0].Run();
   callbacks.clear();
   EXPECT_EQ(DownloadItem::COMPLETE, items[0]->GetState());
 }
 
 }  // namespace content