[Downloads] Stop keying TestFileErrorInjector off of URLs.

content/public/test/test_file_error_injector.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.
 
 #include "content/public/test/test_file_error_injector.h"
 
 #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"
 #include "content/browser/download/download_file_impl.h"
 #include "content/browser/download/download_interrupt_reasons_impl.h"
 #include "content/browser/download/download_manager_impl.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "url/gurl.h"
 
 namespace content {
 class ByteStreamReader;
 
 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,
                          const GURL& referrer_url,
                          bool calculate_hash,
                          base::File file,
                          scoped_ptr<ByteStreamReader> byte_stream,
                          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;
 
   void Initialize(const InitializeCallback& callback) override;
 
   // DownloadFile interface.
   DownloadInterruptReason AppendDataToFile(const char* data,
                                            size_t data_len) override;
   void RenameAndUniquify(const base::FilePath& full_path,
                          const RenameCompletionCallback& callback) override;
   void RenameAndAnnotate(const base::FilePath& full_path,
                          const RenameCompletionCallback& callback) override;
 
  private:
   // Error generating helper.
   DownloadInterruptReason ShouldReturnError(
       TestFileErrorInjector::FileOperationCode code,
       DownloadInterruptReason original_error);
 
   // Determine whether to overwrite an operation with the given code
   // with a substitute error; if returns true, |*original_error| is
   // written with the error to use for overwriting.
   // NOTE: This routine changes state; specifically, it increases the
   // operations counts for the specified code.  It should only be called
   // once per operation.
   bool OverwriteError(
     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_;
 
   // Count per operation.  0-based.
   std::map<TestFileErrorInjector::FileOperationCode, int> operation_counter_;
 
   // Callback for destruction.
-  DestructionCallback destruction_callback_;
+  base::Closure destruction_callback_;
 };
 
 static void InitializeErrorCallback(
     const DownloadFile::InitializeCallback original_callback,
     DownloadInterruptReason overwrite_error,
     DownloadInterruptReason original_error) {
   original_callback.Run(overwrite_error);
 }
 
 static void RenameErrorCallback(
@@ skipping 11 matching lines @@
     const DownloadSaveInfo& save_info,
     const base::FilePath& default_download_directory,
     const GURL& url,
     const GURL& referrer_url,
     bool calculate_hash,
     base::File file,
     scoped_ptr<ByteStreamReader> byte_stream,
     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,
                        referrer_url,
                        calculate_hash,
                        std::move(file),
                        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
   // thread. Schedule the ConstructionCallback on the FILE thread so that if a
   // DownloadItem schedules a DownloadFile to be destroyed and creates another
   // 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(
     const InitializeCallback& callback) {
   DownloadInterruptReason error_to_return = DOWNLOAD_INTERRUPT_REASON_NONE;
   InitializeCallback callback_to_use = callback;
 
   // Replace callback if the error needs to be overwritten.
   if (OverwriteError(
           TestFileErrorInjector::FILE_OPERATION_INITIALIZE,
@@ skipping 97 matching lines @@
   DownloadInterruptReason output_error = original_error;
   OverwriteError(code, &output_error);
   return output_error;
 }
 
 }  // namespace
 
 // 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.
   DownloadFile* CreateFile(
       const DownloadSaveInfo& save_info,
       const base::FilePath& default_download_directory,
       const GURL& url,
       const GURL& referrer_url,
       bool calculate_hash,
       base::File file,
       scoped_ptr<ByteStreamReader> byte_stream,
       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() {
 }
 
 DownloadFile* DownloadFileWithErrorsFactory::CreateFile(
     const DownloadSaveInfo& save_info,
     const base::FilePath& default_download_directory,
     const GURL& url,
     const GURL& referrer_url,
     bool calculate_hash,
     base::File file,
     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.
   created_factory_ =
       new DownloadFileWithErrorsFactory(
           base::Bind(&TestFileErrorInjector::RecordDownloadFileConstruction,
                      this),
           base::Bind(&TestFileErrorInjector::RecordDownloadFileDestruction,
                      this));
 
   // We will transfer ownership of the factory to the download manager.
   scoped_ptr<DownloadFileFactory> download_file_factory(
       created_factory_);
 
   download_manager_->SetDownloadFileFactoryForTesting(
       std::move(download_file_factory));
 }
 
 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;
 }
 
 void TestFileErrorInjector::ClearErrors() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   injected_errors_.clear();
 }
 
 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();
+  return total_file_count_;
 }
 
-
-bool TestFileErrorInjector::HadFile(const GURL& url) const {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
-  return (found_files_.find(url) != found_files_.end());
+void TestFileErrorInjector::ClearTotalFileCount() {
+  total_file_count_ = 0;
 }
 
-void TestFileErrorInjector::ClearFoundFiles() {
-  found_files_.clear();
+void TestFileErrorInjector::DownloadFileCreated() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  ++active_file_count_;
+  ++total_file_count_;
 }
 
-void TestFileErrorInjector::DownloadFileCreated(GURL url) {
+void TestFileErrorInjector::DestroyingDownloadFile() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(files_.find(url) == files_.end());
-
-  files_.insert(url);
-  found_files_.insert(url);
+  --active_file_count_;
 }
 
-void TestFileErrorInjector::DestroyingDownloadFile(GURL url) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(files_.find(url) != files_.end());
-
-  files_.erase(url);
+void TestFileErrorInjector::RecordDownloadFileConstruction() {
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&TestFileErrorInjector::DownloadFileCreated, this));
 }
 
-void TestFileErrorInjector::RecordDownloadFileConstruction(const GURL& url) {
+void TestFileErrorInjector::RecordDownloadFileDestruction() {
   BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&TestFileErrorInjector::DownloadFileCreated, this, url));
-}
-
-void TestFileErrorInjector::RecordDownloadFileDestruction(const GURL& url) {
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-      base::Bind(&TestFileErrorInjector::DestroyingDownloadFile, this, url));
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&TestFileErrorInjector::DestroyingDownloadFile, this));
 }
 
 // static
 scoped_refptr<TestFileErrorInjector> TestFileErrorInjector::Create(
     DownloadManager* download_manager) {
   static bool visited = false;
   DCHECK(!visited);  // Only allowed to be called once.
   visited = true;
 
   scoped_refptr<TestFileErrorInjector> single_injector(
@@ skipping 14 matching lines @@
     case FILE_OPERATION_RENAME_ANNOTATE:
       return "RENAME_ANNOTATE";
     default:
       break;
   }
 
   return "Unknown";
 }
 
 }  // namespace content