[Downloads] Stop keying TestFileErrorInjector off of URLs.

content/public/test/test_file_error_injector.cc

Index: content/public/test/test_file_error_injector.cc
diff --git a/content/public/test/test_file_error_injector.cc b/content/public/test/test_file_error_injector.cc
index 5d5d88500fcb87a19d755b529f39570a3323437f..e1ffb2394fbe3bf4354ed5039efdcc2f8cd872eb 100644
--- a/content/public/test/test_file_error_injector.cc
+++ b/content/public/test/test_file_error_injector.cc
@@ -7,6 +7,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "content/browser/download/download_file_factory.h"
@@ -25,9 +26,6 @@ namespace {
 // A class that performs file operations and injects errors.
 class DownloadFileWithErrors: public DownloadFileImpl {
  public:
-  typedef base::Callback<void(const GURL& url)> ConstructionCallback;
-  typedef base::Callback<void(const GURL& url)> DestructionCallback;
-
   DownloadFileWithErrors(const DownloadSaveInfo& save_info,
                          const base::FilePath& default_download_directory,
                          const GURL& url,
@@ -38,8 +36,8 @@ class DownloadFileWithErrors: public DownloadFileImpl {
                          const net::BoundNetLog& bound_net_log,
                          base::WeakPtr<DownloadDestinationObserver> observer,
                          const TestFileErrorInjector::FileErrorInfo& error_info,
-                         const ConstructionCallback& ctor_callback,
-                         const DestructionCallback& dtor_callback);
+                         const base::Closure& ctor_callback,
+                         const base::Closure& dtor_callback);
 
   ~DownloadFileWithErrors() override;
 
@@ -69,9 +67,6 @@ class DownloadFileWithErrors: public DownloadFileImpl {
     TestFileErrorInjector::FileOperationCode code,
     DownloadInterruptReason* output_error);
 
-  // Source URL for the file being downloaded.
-  GURL source_url_;
-
   // Our injected error.  Only one per file.
   TestFileErrorInjector::FileErrorInfo error_info_;
 
@@ -79,7 +74,7 @@ class DownloadFileWithErrors: public DownloadFileImpl {
   std::map<TestFileErrorInjector::FileOperationCode, int> operation_counter_;
 
   // Callback for destruction.
-  DestructionCallback destruction_callback_;
+  base::Closure destruction_callback_;
 };
 
 static void InitializeErrorCallback(
@@ -111,8 +106,8 @@ DownloadFileWithErrors::DownloadFileWithErrors(
     const net::BoundNetLog& bound_net_log,
     base::WeakPtr<DownloadDestinationObserver> observer,
     const TestFileErrorInjector::FileErrorInfo& error_info,
-    const ConstructionCallback& ctor_callback,
-    const DestructionCallback& dtor_callback)
+    const base::Closure& ctor_callback,
+    const base::Closure& dtor_callback)
     : DownloadFileImpl(save_info,
                        default_download_directory,
                        url,
@@ -122,7 +117,6 @@ DownloadFileWithErrors::DownloadFileWithErrors(
                        std::move(byte_stream),
                        bound_net_log,
                        observer),
-      source_url_(url),
       error_info_(error_info),
       destruction_callback_(dtor_callback) {
   // DownloadFiles are created on the UI thread and are destroyed on the FILE
@@ -131,15 +125,12 @@ DownloadFileWithErrors::DownloadFileWithErrors(
   // one (as happens during download resumption), then the DestructionCallback
   // for the old DownloadFile is run before the ConstructionCallback for the
   // next DownloadFile.
-  BrowserThread::PostTask(
-      BrowserThread::FILE,
-      FROM_HERE,
-      base::Bind(ctor_callback, source_url_));
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, ctor_callback);
 }
 
 DownloadFileWithErrors::~DownloadFileWithErrors() {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
-  destruction_callback_.Run(source_url_);
+  destruction_callback_.Run();
 }
 
 void DownloadFileWithErrors::Initialize(
@@ -257,9 +248,8 @@ DownloadInterruptReason DownloadFileWithErrors::ShouldReturnError(
 // A factory for constructing DownloadFiles that inject errors.
 class DownloadFileWithErrorsFactory : public DownloadFileFactory {
  public:
-  DownloadFileWithErrorsFactory(
-      const DownloadFileWithErrors::ConstructionCallback& ctor_callback,
-      const DownloadFileWithErrors::DestructionCallback& dtor_callback);
+  DownloadFileWithErrorsFactory(const base::Closure& ctor_callback,
+                                const base::Closure& dtor_callback);
   ~DownloadFileWithErrorsFactory() override;
 
   // DownloadFileFactory interface.
@@ -274,26 +264,22 @@ class DownloadFileWithErrorsFactory : public DownloadFileFactory {
       const net::BoundNetLog& bound_net_log,
       base::WeakPtr<DownloadDestinationObserver> observer) override;
 
-  bool AddError(
-      const TestFileErrorInjector::FileErrorInfo& error_info);
-
-  void ClearErrors();
+  bool SetErrorQueue(TestFileErrorInjector::ErrorQueue errors);
 
  private:
   // Our injected error list, mapped by URL.  One per file.
-   TestFileErrorInjector::ErrorMap injected_errors_;
+  TestFileErrorInjector::ErrorQueue injected_errors_;
 
   // Callback for creation and destruction.
-  DownloadFileWithErrors::ConstructionCallback construction_callback_;
-  DownloadFileWithErrors::DestructionCallback destruction_callback_;
+  base::Closure construction_callback_;
+  base::Closure destruction_callback_;
 };
 
 DownloadFileWithErrorsFactory::DownloadFileWithErrorsFactory(
-    const DownloadFileWithErrors::ConstructionCallback& ctor_callback,
-    const DownloadFileWithErrors::DestructionCallback& dtor_callback)
-        : construction_callback_(ctor_callback),
-          destruction_callback_(dtor_callback) {
-}
+    const base::Closure& ctor_callback,
+    const base::Closure& dtor_callback)
+    : construction_callback_(ctor_callback),
+      destruction_callback_(dtor_callback) {}
 
 DownloadFileWithErrorsFactory::~DownloadFileWithErrorsFactory() {
 }
@@ -308,40 +294,26 @@ DownloadFile* DownloadFileWithErrorsFactory::CreateFile(
     scoped_ptr<ByteStreamReader> byte_stream,
     const net::BoundNetLog& bound_net_log,
     base::WeakPtr<DownloadDestinationObserver> observer) {
-  if (injected_errors_.find(url.spec()) == injected_errors_.end()) {
-    // Have to create entry, because FileErrorInfo is not a POD type.
-    TestFileErrorInjector::FileErrorInfo err_info = {
-      url.spec(),
-      TestFileErrorInjector::FILE_OPERATION_INITIALIZE,
-      -1,
-      DOWNLOAD_INTERRUPT_REASON_NONE
-    };
-    injected_errors_[url.spec()] = err_info;
-  }
+  static TestFileErrorInjector::FileErrorInfo kNoOpErrorInfo = {
+      TestFileErrorInjector::FILE_OPERATION_INITIALIZE, -1,
+      DOWNLOAD_INTERRUPT_REASON_NONE};
+  TestFileErrorInjector::FileErrorInfo err_info =
+      injected_errors_.empty() ? kNoOpErrorInfo : injected_errors_.front();

[svaldez, 2016/03/01 18:01:49]

I might be misunderstanding the code, but don't you want pop_front here to cycle
the deque?

[asanka, 2016/03/01 18:58:49]

Indeed. None of the tests depend on that, which makes me want to just change the
interface to SetError() instead.

Y'know what, I'll do that. This seems like unnecessary complexity.

 
   return new DownloadFileWithErrors(
       save_info, default_download_directory, url, referrer_url, calculate_hash,
       std::move(file), std::move(byte_stream), bound_net_log, observer,
-      injected_errors_[url.spec()], construction_callback_,
-      destruction_callback_);
+      err_info, construction_callback_, destruction_callback_);
 }
 
-bool DownloadFileWithErrorsFactory::AddError(
-    const TestFileErrorInjector::FileErrorInfo& error_info) {
-  // Creates an empty entry if necessary.  Duplicate entries overwrite.
-  injected_errors_[error_info.url] = error_info;
-
+bool DownloadFileWithErrorsFactory::SetErrorQueue(
+    TestFileErrorInjector::ErrorQueue errors) {
+  injected_errors_ = std::move(errors);
   return true;
 }
 
-void DownloadFileWithErrorsFactory::ClearErrors() {
-  injected_errors_.clear();
-}
-
-TestFileErrorInjector::TestFileErrorInjector(
-    DownloadManager* download_manager)
-    : created_factory_(NULL),
-      // This code is only used for browser_tests, so a
+TestFileErrorInjector::TestFileErrorInjector(DownloadManager* download_manager)
+    :  // This code is only used for browser_tests, so a
       // DownloadManager is always a DownloadManagerImpl.

[asanka, 2016/03/01 16:11:38]

This is .. special. I'm going to move ownership of download files to download
item, then we'd need to rip out most of this logic anyway. So not going to
address this now.

       download_manager_(static_cast<DownloadManagerImpl*>(download_manager)) {
   // Record the value of the pointer, for later validation.
@@ -366,10 +338,9 @@ TestFileErrorInjector::~TestFileErrorInjector() {
 bool TestFileErrorInjector::AddError(const FileErrorInfo& error_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK_LE(0, error_info.operation_instance);
-  DCHECK(injected_errors_.find(error_info.url) == injected_errors_.end());
 
   // Creates an empty entry if necessary.
-  injected_errors_[error_info.url] = error_info;
+  injected_errors_.push_back(error_info);
 
   return true;
 }
@@ -382,66 +353,50 @@ void TestFileErrorInjector::ClearErrors() {
 bool TestFileErrorInjector::InjectErrors() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  ClearFoundFiles();
+  ClearTotalFileCount();
 
   DCHECK_EQ(static_cast<DownloadFileFactory*>(created_factory_),
             download_manager_->GetDownloadFileFactoryForTesting());
 
-  created_factory_->ClearErrors();
-
-  for (ErrorMap::const_iterator it = injected_errors_.begin();
-       it != injected_errors_.end(); ++it)
-    created_factory_->AddError(it->second);
-
+  created_factory_->SetErrorQueue(std::move(injected_errors_));
   return true;
 }
 
 size_t TestFileErrorInjector::CurrentFileCount() const {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  return files_.size();
+  return active_file_count_;
 }
 
 size_t TestFileErrorInjector::TotalFileCount() const {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  return found_files_.size();
-}
-
-
-bool TestFileErrorInjector::HadFile(const GURL& url) const {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  return (found_files_.find(url) != found_files_.end());
+  return total_file_count_;
 }
 
-void TestFileErrorInjector::ClearFoundFiles() {
-  found_files_.clear();
+void TestFileErrorInjector::ClearTotalFileCount() {
+  total_file_count_ = 0;
 }
 
-void TestFileErrorInjector::DownloadFileCreated(GURL url) {
+void TestFileErrorInjector::DownloadFileCreated() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(files_.find(url) == files_.end());
-
-  files_.insert(url);
-  found_files_.insert(url);
+  ++active_file_count_;
+  ++total_file_count_;
 }
 
-void TestFileErrorInjector::DestroyingDownloadFile(GURL url) {
+void TestFileErrorInjector::DestroyingDownloadFile() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(files_.find(url) != files_.end());
-
-  files_.erase(url);
+  --active_file_count_;
 }
 
-void TestFileErrorInjector::RecordDownloadFileConstruction(const GURL& url) {
+void TestFileErrorInjector::RecordDownloadFileConstruction() {
   BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&TestFileErrorInjector::DownloadFileCreated, this, url));
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&TestFileErrorInjector::DownloadFileCreated, this));
 }
 
-void TestFileErrorInjector::RecordDownloadFileDestruction(const GURL& url) {
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-      base::Bind(&TestFileErrorInjector::DestroyingDownloadFile, this, url));
+void TestFileErrorInjector::RecordDownloadFileDestruction() {
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&TestFileErrorInjector::DestroyingDownloadFile, this));
 }
 
 // static