Shift "commit point" for when a download will no longer accept cancels.

This CL shifts the commit point for a download to just after the download 
file release has been dispatched.  The download remains IN_PROGRESS as 
far as the outside world is concerned, but at this point it is committed to 
continue to completion.

BUG=123998
R=benjhayden@chromium.org
R=asanka@chromium.org


Review URL: https://chromiumcodereview.appspot.com/10950015

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@158298 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/download/download_browsertest.cc

@@ -8,10 +8,16 @@
 #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_file_manager.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/renderer_host/resource_dispatcher_host_impl.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 +31,197 @@ 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 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,
+      // |owner| is required to outlive the DownloadFileWithDelay.
+      DownloadFileWithDelayFactory* owner);
+
+  virtual ~DownloadFileWithDelay();
+
+  // 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
+  // retrieval.
+  virtual void Detach(base::Closure callback) OVERRIDE;
+
+ private:
+  static void RenameCallbackWrapper(
+      DownloadFileWithDelayFactory* factory,
+      const RenameCompletionCallback& original_callback,
+      content::DownloadInterruptReason reason,
+      const FilePath& path);
+
+  static void DetachCallbackWrapper(
+      DownloadFileWithDelayFactory* factory,
+      const base::Closure& original_callback);
+
+  // May only be used on the UI thread.
+  DownloadFileWithDelayFactory* owner_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadFileWithDelay);
+};
+
+class DownloadFileWithDelayFactory : public DownloadFileFactory {
+ public:
+  DownloadFileWithDelayFactory();
+  virtual ~DownloadFileWithDelayFactory();
+
+  // 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;
+
+  // Must all be called on the UI thread.
+  void AddRenameCallback(base::Closure callback);
+  void AddDetachCallback(base::Closure callback);
+  void GetAllRenameCallbacks(std::vector<base::Closure>* results);
+  void GetAllDetachCallbacks(std::vector<base::Closure>* results);
+
+  // Do not return until either GetAllRenameCallbacks() or
+  // GetAllDetachCallbacks() will return a non-empty list.
+  void WaitForSomeCallback();
+
+ private:
+  std::vector<base::Closure> rename_callbacks_;
+  std::vector<base::Closure> detach_callbacks_;
+  bool waiting_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadFileWithDelayFactory);
+};
+
+DownloadFileWithDelay::DownloadFileWithDelay(
+      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,
+      DownloadFileWithDelayFactory* owner)
+    : DownloadFileImpl(info, stream.Pass(), request_handle, download_manager,
+                       calculate_hash, power_save_blocker.Pass(),
+                       bound_net_log),
+      owner_(owner) {}
+
+DownloadFileWithDelay::~DownloadFileWithDelay() {}
+
+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,
+                 base::Unretained(owner_), callback));
+}
+
+void DownloadFileWithDelay::Detach(base::Closure callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DownloadFileImpl::Detach(
+      base::Bind(DownloadFileWithDelay::DetachCallbackWrapper,
+                 base::Unretained(owner_), callback));
+}
+
+// static
+void DownloadFileWithDelay::RenameCallbackWrapper(
+    DownloadFileWithDelayFactory* factory,
+    const RenameCompletionCallback& original_callback,
+    content::DownloadInterruptReason reason,
+    const FilePath& path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  factory->AddRenameCallback(base::Bind(original_callback, reason, path));
+}
+
+// static
+void DownloadFileWithDelay::DetachCallbackWrapper(
+    DownloadFileWithDelayFactory* factory,
+    const base::Closure& original_callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  factory->AddDetachCallback(original_callback);
+}
+
+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);
+}
+
+void DownloadFileWithDelayFactory::AddRenameCallback(base::Closure callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  rename_callbacks_.push_back(callback);
+  if (waiting_)
+    MessageLoopForUI::current()->Quit();
+}
+
+void DownloadFileWithDelayFactory::AddDetachCallback(base::Closure callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  detach_callbacks_.push_back(callback);
+  if (waiting_)
+    MessageLoopForUI::current()->Quit();
+}
+
+void DownloadFileWithDelayFactory::GetAllRenameCallbacks(
+    std::vector<base::Closure>* results) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  results->swap(rename_callbacks_);
+}
+
+void DownloadFileWithDelayFactory::GetAllDetachCallbacks(
+    std::vector<base::Closure>* results) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  results->swap(detach_callbacks_);
+}
+
+void DownloadFileWithDelayFactory::WaitForSomeCallback() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (rename_callbacks_.empty() && detach_callbacks_.empty()) {
+    waiting_ = true;
+    RunMessageLoop();
+    waiting_ = false;
+  }
 }
 
 }  // namespace
@@ -114,6 +308,11 @@ class DownloadContentTest : public ContentBrowserTest {
     return true;
   }
 
+  DownloadFileManager* GetDownloadFileManager() {
+    ResourceDispatcherHostImpl* rdh(ResourceDispatcherHostImpl::Get());
+    return rdh->download_file_manager();
+  }
+
  private:
   static void EnsureNoPendingDownloadJobsOnIO(bool* result) {
     if (URLRequestSlowDownloadJob::NumberOutstandingRequests())
@@ -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();
+  GetDownloadFileManager()->SetFileFactoryForTesting(
+      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->GetAllDetachCallbacks(&callbacks);
+  ASSERT_TRUE(callbacks.empty());
+  file_factory->GetAllRenameCallbacks(&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->GetAllDetachCallbacks(&callbacks);
+  ASSERT_TRUE(callbacks.empty());
+  file_factory->GetAllRenameCallbacks(&callbacks);
+  ASSERT_EQ(1u, callbacks.size());
+
+  // Cancel it.
+  std::vector<DownloadItem*> items;
+  DownloadManagerForShell(shell())->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.
+  // Setup new factory.
+  DownloadFileWithDelayFactory* file_factory =
+      new DownloadFileWithDelayFactory();
+  GetDownloadFileManager()->SetFileFactoryForTesting(
+      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->GetAllDetachCallbacks(&callbacks);
+  ASSERT_TRUE(callbacks.empty());
+  file_factory->GetAllRenameCallbacks(&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->GetAllDetachCallbacks(&callbacks);
+  ASSERT_TRUE(callbacks.empty());
+  file_factory->GetAllRenameCallbacks(&callbacks);
+  ASSERT_EQ(1u, callbacks.size());
+
+  // Call it.
+  callbacks[0].Run();
+  callbacks.clear();
+
+  // Confirm download isn't complete yet.
+  std::vector<DownloadItem*> items;
+  DownloadManagerForShell(shell())->GetAllDownloads(&items);
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, items[0]->GetState());
+
+  // Cancel the download; confirm cancel fails anyway.
+  ASSERT_EQ(1u, items.size());
+  items[0]->Cancel(true);
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, items[0]->GetState());
+  RunAllPendingInMessageLoop();
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, items[0]->GetState());
+
+  // Confirm detach callback and run it.
+  file_factory->WaitForSomeCallback();
+  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

content/browser/download/download_file.h

@@ -28,15 +28,15 @@ class CONTENT_EXPORT DownloadFile {
   // DOWNLOAD_INTERRUPT_REASON_NONE and |path| the path the rename
   // was done to.  On a failed rename, |reason| will contain the
   // error.
-  typedef base::Callback<void(content::DownloadInterruptReason reason,
+  typedef base::Callback<void(DownloadInterruptReason reason,
                               const FilePath& path)> RenameCompletionCallback;
 
   virtual ~DownloadFile() {}
 
-  // If calculate_hash is true, sha256 hash will be calculated.
   // Returns DOWNLOAD_INTERRUPT_REASON_NONE on success, or a network
-  // error code on failure.
-  virtual DownloadInterruptReason Initialize() = 0;
+  // error code on failure.  Upon completion, |callback| will be
+  // called on the UI thread as per the comment above.
+  virtual content::DownloadInterruptReason Initialize() = 0;
 
   // Rename the download file to |full_path|.  If that file exists and
   // |overwrite_existing_file| is false, |full_path| will be uniquified by
@@ -48,7 +48,8 @@ class CONTENT_EXPORT DownloadFile {
                       const RenameCompletionCallback& callback) = 0;
 
   // Detach the file so it is not deleted on destruction.
-  virtual void Detach() = 0;
+  // |callback| will be called on the UI thread after detach.
+  virtual void Detach(base::Closure callback) = 0;
 
   // Abort the download and automatically close the file.
   virtual void Cancel() = 0;

content/browser/download/download_file_factory.cc

@@ -0,0 +1,30 @@
+// 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/browser/download/download_file_factory.h"
+
+#include "content/browser/download/download_file_impl.h"
+#include "content/browser/power_save_blocker.h"
+
+namespace content {
+
+DownloadFileFactory::~DownloadFileFactory() {}
+
+DownloadFile* DownloadFileFactory::CreateFile(
+    DownloadCreateInfo* info,
+    scoped_ptr<content::ByteStreamReader> stream,
+    DownloadManager* download_manager,
+    bool calculate_hash,
+    const net::BoundNetLog& bound_net_log) {
+  return new DownloadFileImpl(
+      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 content

content/browser/download/download_file_factory.h

@@ -0,0 +1,42 @@
+// 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.
+//
+#ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_FILE_FACTORY_H_
+#define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_FILE_FACTORY_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "content/common/content_export.h"
+#include "content/browser/download/download_create_info.h"
+#include "googleurl/src/gurl.h"
+
+namespace net {
+class BoundNetLog;
+}
+
+namespace content {
+
+struct DownloadSaveInfo;
+
+class ByteStreamReader;
+class DownloadDestinationObserver;
+class DownloadFile;
+class DownloadManager;
+
+class CONTENT_EXPORT DownloadFileFactory {
+ public:
+  virtual ~DownloadFileFactory();
+
+  virtual content::DownloadFile* CreateFile(
+      DownloadCreateInfo* info,
+      scoped_ptr<content::ByteStreamReader> stream,
+      DownloadManager* download_manager,
+      bool calculate_hash,
+      const net::BoundNetLog& bound_net_log);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_FILE_FACTORY_H_

content/browser/download/download_file_impl.cc

@@ -60,17 +60,20 @@ DownloadFileImpl::~DownloadFileImpl() {
 }
 
 content::DownloadInterruptReason DownloadFileImpl::Initialize() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
   update_timer_.reset(new base::RepeatingTimer<DownloadFileImpl>());
-  net::Error result = file_.Initialize(default_download_directory_);
-  if (result != net::OK) {
+  net::Error net_result = file_.Initialize(default_download_directory_);
+  if (net_result != net::OK) {
     return content::ConvertNetErrorToInterruptReason(
-        result, content::DOWNLOAD_INTERRUPT_FROM_DISK);
+        net_result, content::DOWNLOAD_INTERRUPT_FROM_DISK);
   }
 
   stream_reader_->RegisterCallback(
       base::Bind(&DownloadFileImpl::StreamActive, weak_factory_.GetWeakPtr()));
 
   download_start_ = base::TimeTicks::Now();
+
   // Initial pull from the straw.
   StreamActive();
 
@@ -126,8 +129,17 @@ void DownloadFileImpl::Rename(const FilePath& full_path,
       base::Bind(callback, reason, new_path));
 }
 
-void DownloadFileImpl::Detach() {
+void DownloadFileImpl::Detach(base::Closure callback) {
+  // Doing the annotation here leaves a small window during
+  // which the file has the final name but hasn't been marked with the
+  // Mark Of The Web.  However, it allows anti-virus scanners on Windows
+  // to actually see the data (http://crbug.com/127999), and the Window
+  // is pretty small (round trip to the UI thread).
+  AnnotateWithSourceInformation();
+
   file_.Detach();
+
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
 }
 
 void DownloadFileImpl::Cancel() {

content/browser/download/download_file_impl.h

@@ -43,7 +43,7 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public content::DownloadFile {
   virtual void Rename(const FilePath& full_path,
                       bool overwrite_existing_file,
                       const RenameCompletionCallback& callback) OVERRIDE;
-  virtual void Detach() OVERRIDE;
+  virtual void Detach(base::Closure callback) OVERRIDE;
   virtual void Cancel() OVERRIDE;
   virtual void AnnotateWithSourceInformation() OVERRIDE;
   virtual FilePath FullPath() const OVERRIDE;

content/browser/download/download_file_manager.cc

@@ -31,43 +31,10 @@ using content::DownloadFile;
 using content::DownloadId;
 using content::DownloadManager;
 
-namespace {
-
-class DownloadFileFactoryImpl
-    : public DownloadFileManager::DownloadFileFactory {
- public:
-  DownloadFileFactoryImpl() {}
-
-  virtual content::DownloadFile* CreateFile(
-      DownloadCreateInfo* info,
-      scoped_ptr<content::ByteStreamReader> stream,
-      DownloadManager* download_manager,
-      bool calculate_hash,
-      const net::BoundNetLog& bound_net_log) OVERRIDE;
-};
-
-DownloadFile* DownloadFileFactoryImpl::CreateFile(
-    DownloadCreateInfo* info,
-    scoped_ptr<content::ByteStreamReader> stream,
-    DownloadManager* download_manager,
-    bool calculate_hash,
-    const net::BoundNetLog& bound_net_log) {
-  return new DownloadFileImpl(
-      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
-
-DownloadFileManager::DownloadFileManager(DownloadFileFactory* factory)
+DownloadFileManager::DownloadFileManager(content::DownloadFileFactory* factory)
     : download_file_factory_(factory) {
   if (download_file_factory_ == NULL)
-    download_file_factory_.reset(new DownloadFileFactoryImpl);
+    download_file_factory_.reset(new content::DownloadFileFactory);
 }
 
 DownloadFileManager::~DownloadFileManager() {
@@ -147,20 +114,9 @@ void DownloadFileManager::CompleteDownload(
            << " id = " << global_id
            << " download_file = " << download_file->DebugString();
 
-  // Done here on Windows so that anti-virus scanners invoked by
-  // the attachment service actually see the data; see
-  // http://crbug.com/127999.
-  // Done here for mac because we only want to do this once; see
-  // http://crbug.com/13120 for details.
-  // Other platforms don't currently do source annotation.
-  download_file->AnnotateWithSourceInformation();
-
-  download_file->Detach();
+  download_file->Detach(callback);
 
   EraseDownload(global_id);
-
-  // Notify our caller we've let it go.
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
 }
 
 void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {

content/browser/download/download_file_manager.h

@@ -50,6 +50,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/timer.h"
 #include "content/browser/download/download_file.h"
+#include "content/browser/download/download_file_factory.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/download_id.h"
 #include "content/public/browser/download_interrupt_reasons.h"
@@ -85,22 +86,10 @@ class CONTENT_EXPORT DownloadFileManager
   typedef content::DownloadFile::RenameCompletionCallback
       RenameCompletionCallback;
 
-  class DownloadFileFactory {
-   public:
-    virtual ~DownloadFileFactory() {}
-
-    virtual content::DownloadFile* CreateFile(
-        DownloadCreateInfo* info,
-        scoped_ptr<content::ByteStreamReader> stream,
-        content::DownloadManager* download_manager,
-        bool calculate_hash,
-        const net::BoundNetLog& bound_net_log) = 0;
-  };
-
   // Takes ownership of the factory.
   // Passing in a NULL for |factory| will cause a default
   // |DownloadFileFactory| to be used.
-  explicit DownloadFileManager(DownloadFileFactory* factory);
+  explicit DownloadFileManager(content::DownloadFileFactory* factory);
 
   // Create a download file and record it in the download file manager.
   virtual void CreateDownloadFile(
@@ -139,11 +128,12 @@ class CONTENT_EXPORT DownloadFileManager
   // Primarily for testing.
   virtual int NumberOfActiveDownloads() const;
 
-  void SetFileFactoryForTesting(scoped_ptr<DownloadFileFactory> file_factory) {
+  void SetFileFactoryForTesting(
+      scoped_ptr<content::DownloadFileFactory> file_factory) {
     download_file_factory_.reset(file_factory.release());
   }
 
-  DownloadFileFactory* GetFileFactoryForTesting() const {
+  content::DownloadFileFactory* GetFileFactoryForTesting() const {
     return download_file_factory_.get();  // Explicitly NOT a scoped_ptr.
   }
 
@@ -172,7 +162,7 @@ class CONTENT_EXPORT DownloadFileManager
   // A map of all in progress downloads.  It owns the download files.
   DownloadFileMap downloads_;
 
-  scoped_ptr<DownloadFileFactory> download_file_factory_;
+  scoped_ptr<content::DownloadFileFactory> download_file_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadFileManager);
 };

content/browser/download/download_file_manager_unittest.cc

@@ -13,6 +13,7 @@
 #include "content/browser/download/byte_stream.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_interrupt_reasons_impl.h"
+#include "content/browser/download/download_file_factory.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/mock_download_file.h"
 #include "content/public/browser/download_id.h"
@@ -48,8 +49,7 @@ class TestDownloadManager : public MockDownloadManager {
   ~TestDownloadManager() {}
 };
 
-class MockDownloadFileFactory :
-    public DownloadFileManager::DownloadFileFactory {
+class MockDownloadFileFactory : public content::DownloadFileFactory {
 
  public:
   MockDownloadFileFactory() {}
@@ -184,8 +184,10 @@ class DownloadFileManagerTest : public testing::Test {
 
   // Create a download item on the DFM.
   // |info| is the information needed to create a new download file.
-  // |id| is the download ID of the new download file.
   void CreateDownloadFile(scoped_ptr<DownloadCreateInfo> info) {
+    // Anything that isn't DOWNLOAD_INTERRUPT_REASON_NONE.
+    last_reason_ = content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED;
+
     // Mostly null out args; they'll be passed to MockDownloadFileFactory
     // to be ignored anyway.
     download_file_manager_->CreateDownloadFile(
@@ -195,8 +197,6 @@ class DownloadFileManagerTest : public testing::Test {
                    // The test jig will outlive all download files.
                    base::Unretained(this)));
 
-    // Anything that isn't DOWNLOAD_INTERRUPT_REASON_NONE.
-    last_reason_ = content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED;
     ProcessAllPendingMessages();
     EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, last_reason_);
   }
@@ -237,9 +237,7 @@ class DownloadFileManagerTest : public testing::Test {
     MockDownloadFile* file = download_file_factory_->GetExistingFile(id);
     ASSERT_TRUE(file != NULL);
 
-    EXPECT_CALL(*file, AnnotateWithSourceInformation())
-        .WillOnce(Return());
-    EXPECT_CALL(*file, Detach())
+    EXPECT_CALL(*file, Detach(_))
         .WillOnce(Return());
     int num_downloads = download_file_manager_->NumberOfActiveDownloads();
     EXPECT_LT(0, num_downloads);

content/browser/download/download_item_impl.cc

@@ -80,22 +80,6 @@ const char* DebugSafetyStateString(DownloadItem::SafetyState state) {
   };
 }
 
-const char* DebugDownloadStateString(DownloadItem::DownloadState state) {
-  switch (state) {
-    case DownloadItem::IN_PROGRESS:
-      return "IN_PROGRESS";
-    case DownloadItem::COMPLETE:
-      return "COMPLETE";
-    case DownloadItem::CANCELLED:
-      return "CANCELLED";
-    case DownloadItem::INTERRUPTED:
-      return "INTERRUPTED";
-    default:
-      NOTREACHED() << "Unknown download state " << state;
-      return "unknown";
-  };
-}
-
 // Classes to null out request handle calls (for SavePage DownloadItems, which
 // may have, e.g., Cancel() called on them without it doing anything)
 // and to DCHECK on them (for history DownloadItems, which should never have
@@ -158,7 +142,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       bytes_per_sec_(0),
       last_reason_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks()),
-      state_(info.state),
+      state_(ExternalToInternalState(info.state)),
       danger_type_(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
       start_time_(info.start_time),
       end_time_(info.end_time),
@@ -178,9 +162,9 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       bound_net_log_(bound_net_log),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   delegate_->Attach();
-  if (IsInProgress())
-    state_ = CANCELLED;
-  if (IsComplete())
+  if (state_ == IN_PROGRESS_INTERNAL)
+    state_ = CANCELLED_INTERNAL;
+  if (state_ == COMPLETE_INTERNAL)
     all_data_saved_ = true;
   Init(false /* not actively downloading */,
        download_net_logs::SRC_HISTORY_IMPORT);
@@ -213,7 +197,7 @@ DownloadItemImpl::DownloadItemImpl(
       bytes_per_sec_(0),
       last_reason_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks::Now()),
-      state_(IN_PROGRESS),
+      state_(IN_PROGRESS_INTERNAL),
       danger_type_(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
       start_time_(info.start_time),
       db_handle_(DownloadItem::kUninitializedHandle),
@@ -268,7 +252,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       bytes_per_sec_(0),
       last_reason_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks::Now()),
-      state_(IN_PROGRESS),
+      state_(IN_PROGRESS_INTERNAL),
       danger_type_(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
       start_time_(base::Time::Now()),
       db_handle_(DownloadItem::kUninitializedHandle),
@@ -339,7 +323,7 @@ void DownloadItemImpl::DangerousDownloadValidated() {
 void DownloadItemImpl::TogglePause() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  DCHECK(IsInProgress());
+  DCHECK(state_ == IN_PROGRESS_INTERNAL || state_ == COMPLETING_INTERNAL);
   if (is_paused_)
     request_handle_->ResumeRequest();
   else
@@ -356,7 +340,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
       content::DOWNLOAD_INTERRUPT_REASON_USER_SHUTDOWN;
 
   VLOG(20) << __FUNCTION__ << "() download = " << DebugString(true);
-  if (!IsPartialDownload()) {
+  if (state_ != IN_PROGRESS_INTERNAL) {
     // Small downloads might be complete before this method has
     // a chance to run.
     return;
@@ -364,7 +348,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
 
   download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
 
-  TransitionTo(CANCELLED);
+  TransitionTo(CANCELLED_INTERNAL);
   if (user_cancel)
     delegate_->DownloadStopped(this);
 }
@@ -410,7 +394,7 @@ void DownloadItemImpl::Remove() {
 void DownloadItemImpl::OpenDownload() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (IsPartialDownload()) {
+  if (state_ == IN_PROGRESS_INTERNAL) {
     // We don't honor the open_when_complete_ flag for temporary
     // downloads. Don't set it because it shows up in the UI.
     if (!IsTemporary())
@@ -418,7 +402,7 @@ void DownloadItemImpl::OpenDownload() {
     return;
   }
 
-  if (!IsComplete() || file_externally_removed_)
+  if (state_ != COMPLETE_INTERNAL || file_externally_removed_)
     return;
 
   // Ideally, we want to detect errors in opening and report them, but we
@@ -458,7 +442,7 @@ int64 DownloadItemImpl::GetDbHandle() const {
 }
 
 DownloadItem::DownloadState DownloadItemImpl::GetState() const {
-  return state_;
+  return InternalToExternalState(state_);
 }
 
 content::DownloadInterruptReason DownloadItemImpl::GetLastReason() const {
@@ -480,24 +464,24 @@ bool DownloadItemImpl::IsPersisted() const {
 // TODO(ahendrickson) -- Move |INTERRUPTED| from |IsCancelled()| to
 // |IsPartialDownload()|, when resuming interrupted downloads is implemented.
 bool DownloadItemImpl::IsPartialDownload() const {
-  return (state_ == IN_PROGRESS);
+  return InternalToExternalState(state_) == IN_PROGRESS;
 }
 
 bool DownloadItemImpl::IsInProgress() const {
-  return (state_ == IN_PROGRESS);
+  return InternalToExternalState(state_) == IN_PROGRESS;
 }
 
 bool DownloadItemImpl::IsCancelled() const {
-  return (state_ == CANCELLED) ||
-         (state_ == INTERRUPTED);
+  DownloadState external_state = InternalToExternalState(state_);
+  return  external_state == CANCELLED || external_state == INTERRUPTED;
 }
 
 bool DownloadItemImpl::IsInterrupted() const {
-  return (state_ == INTERRUPTED);
+  return InternalToExternalState(state_) == INTERRUPTED;
 }
 
 bool DownloadItemImpl::IsComplete() const {
-  return (state_ == COMPLETE);
+  return InternalToExternalState(state_) == COMPLETE;
 }
 
 const GURL& DownloadItemImpl::GetURL() const {
@@ -670,7 +654,7 @@ base::Time DownloadItemImpl::GetEndTime() const {
 }
 
 bool DownloadItemImpl::CanShowInFolder() {
-  return !IsCancelled() && !file_externally_removed_;
+  return state_ != CANCELLED_INTERNAL && !file_externally_removed_;
 }
 
 bool DownloadItemImpl::CanOpenDownload() {
@@ -750,7 +734,7 @@ std::string DownloadItemImpl::DebugString(bool verbose) const {
       base::StringPrintf("{ id = %d"
                          " state = %s",
                          download_id_.local(),
-                         DebugDownloadStateString(GetState()));
+                         DebugDownloadStateString(state_));
 
   // Construct a string of the URL chain.
   std::string url_list("<none>");
@@ -834,11 +818,11 @@ void DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) {
   // interrupts to race with cancels.
 
   // Whatever happens, the first one to hit the UI thread wins.
-  if (!IsInProgress())
+  if (state_ != IN_PROGRESS_INTERNAL)
     return;
 
   last_reason_ = reason;
-  TransitionTo(INTERRUPTED);
+  TransitionTo(INTERRUPTED_INTERNAL);
   download_stats::RecordDownloadInterrupted(
       reason, received_bytes_, total_bytes_);
   delegate_->DownloadStopped(this);
@@ -856,7 +840,7 @@ void DownloadItemImpl::UpdateProgress(int64 bytes_so_far,
                                       const std::string& hash_state) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (!IsInProgress()) {
+  if (state_ != IN_PROGRESS_INTERNAL) {
     // Ignore if we're no longer in-progress.  This can happen if we race a
     // Cancel on the UI thread with an update on the FILE thread.
     //
@@ -901,7 +885,7 @@ void DownloadItemImpl::MarkAsComplete() {
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
-  TransitionTo(COMPLETE);
+  TransitionTo(COMPLETE_INTERNAL);
 }
 
 void DownloadItemImpl::SetIsPersisted() {
@@ -986,6 +970,16 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
   // space/permission/availability constraints.
   DCHECK(intermediate_path.DirName() == target_path.DirName());
 
+  if (state_ != IN_PROGRESS_INTERNAL) {
+    // If we've been cancelled or interrupted while the target was being
+    // determined, continue the cascade with a null name.
+    // The error doesn't matter as the cause of download stoppage
+    // will already have been recorded and will be retained, but we use
+    // whatever was recorded for consistency.
+    OnDownloadRenamedToIntermediateName(last_reason_, FilePath());
+    return;
+  }
+
   // Rename to intermediate name.
   // TODO(asanka): Skip this rename if AllDataSaved() is true. This avoids a
   //               spurious rename when we can just rename to the final
@@ -1025,6 +1019,9 @@ void DownloadItemImpl::MaybeCompleteDownload() {
 void DownloadItemImpl::OnDownloadCompleting() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  if (state_ != IN_PROGRESS_INTERNAL)
+    return;
+
   VLOG(20) << __FUNCTION__ << "()"
            << " needs rename = " << NeedsRename()
            << " " << DebugString(true);
@@ -1048,6 +1045,7 @@ void DownloadItemImpl::OnDownloadCompleting() {
                    delegate_->GetDownloadFileManager(), GetGlobalId(),
                    base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
                               weak_ptr_factory_.GetWeakPtr())));
+    TransitionTo(COMPLETING_INTERNAL);
   }
 }
 
@@ -1056,6 +1054,12 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
     const FilePath& full_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // If a cancel or interrupt hit, we'll cancel the DownloadFile, which
+  // will result in deleting the file on the file thread.  So we don't
+  // care about the name having been changed.
+  if (state_ != IN_PROGRESS_INTERNAL)
+    return;
+
   VLOG(20) << __FUNCTION__ << "()"
            << " full_path = \"" << full_path.value() << "\""
            << " needed rename = " << NeedsRename()
@@ -1080,6 +1084,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
                  delegate_->GetDownloadFileManager(), GetGlobalId(),
                  base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
                             weak_ptr_factory_.GetWeakPtr())));
+  TransitionTo(COMPLETING_INTERNAL);
 }
 
 void DownloadItemImpl::OnDownloadFileReleased() {
@@ -1101,7 +1106,7 @@ void DownloadItemImpl::Completed() {
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
-  TransitionTo(COMPLETE);
+  TransitionTo(COMPLETE_INTERNAL);
   delegate_->DownloadCompleted(this);
   download_stats::RecordDownloadCompleted(start_tick_, received_bytes_);
 
@@ -1143,27 +1148,33 @@ void DownloadItemImpl::ProgressComplete(int64 bytes_so_far,
     total_bytes_ = 0;
 }
 
-void DownloadItemImpl::TransitionTo(DownloadState new_state) {
+void DownloadItemImpl::TransitionTo(DownloadInternalState new_state) {
   if (state_ == new_state)
     return;
 
-  DownloadState old_state = state_;
+  DownloadInternalState old_state = state_;
   state_ = new_state;
 
   switch (state_) {
-    case COMPLETE:
+    case COMPLETING_INTERNAL:
+      bound_net_log_.AddEvent(
+          net::NetLog::TYPE_DOWNLOAD_ITEM_COMPLETING,
+          base::Bind(&download_net_logs::ItemCompletingCallback,
+                     received_bytes_, &hash_));
+      break;
+    case COMPLETE_INTERNAL:
       bound_net_log_.AddEvent(
           net::NetLog::TYPE_DOWNLOAD_ITEM_FINISHED,
           base::Bind(&download_net_logs::ItemFinishedCallback,
-                     received_bytes_, &hash_));
+                     auto_opened_));
       break;
-    case INTERRUPTED:
+    case INTERRUPTED_INTERNAL:
       bound_net_log_.AddEvent(
           net::NetLog::TYPE_DOWNLOAD_ITEM_INTERRUPTED,
           base::Bind(&download_net_logs::ItemInterruptedCallback,
                      last_reason_, received_bytes_, &hash_state_));
       break;
-    case CANCELLED:
+    case CANCELLED_INTERNAL:
       bound_net_log_.AddEvent(
           net::NetLog::TYPE_DOWNLOAD_ITEM_CANCELED,
           base::Bind(&download_net_logs::ItemCanceledCallback,
@@ -1175,10 +1186,14 @@ void DownloadItemImpl::TransitionTo(DownloadState new_state) {
 
   VLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true);
 
-  UpdateObservers();
+  // Only update observers on user visible state changes.
+  if (InternalToExternalState(old_state) != InternalToExternalState(state_))
+    UpdateObservers();
 
-  bool is_done = (state_ != IN_PROGRESS);
-  bool was_done = (old_state != IN_PROGRESS);
+  bool is_done = (state_ != IN_PROGRESS_INTERNAL &&
+                  state_ != COMPLETING_INTERNAL);
+  bool was_done = (old_state != IN_PROGRESS_INTERNAL &&
+                   old_state != COMPLETING_INTERNAL);
   if (is_done && !was_done)
     bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE);
 }
@@ -1212,6 +1227,60 @@ void DownloadItemImpl::SetFullPath(const FilePath& new_path) {
                  &current_path_, &new_path));
 }
 
+// static
+DownloadItem::DownloadState DownloadItemImpl::InternalToExternalState(
+    DownloadInternalState internal_state) {
+  switch (internal_state) {
+    case IN_PROGRESS_INTERNAL:
+      return IN_PROGRESS;
+    case COMPLETING_INTERNAL:
+      return IN_PROGRESS;
+    case COMPLETE_INTERNAL:
+      return COMPLETE;
+    case CANCELLED_INTERNAL:
+      return CANCELLED;
+    case INTERRUPTED_INTERNAL:
+      return INTERRUPTED;
+    default:
+      NOTREACHED();
+  }
+  return MAX_DOWNLOAD_STATE;
+}
 
+// static
+DownloadItemImpl::DownloadInternalState
+DownloadItemImpl::ExternalToInternalState(
+     DownloadState external_state) {
+   switch (external_state) {
+     case IN_PROGRESS:
+       return IN_PROGRESS_INTERNAL;
+     case COMPLETE:
+       return COMPLETE_INTERNAL;
+     case CANCELLED:
+       return CANCELLED_INTERNAL;
+     case INTERRUPTED:
+       return INTERRUPTED_INTERNAL;
+     default:
+       NOTREACHED();
+   }
+   return MAX_DOWNLOAD_INTERNAL_STATE;
+ }
 
-
+const char* DownloadItemImpl::DebugDownloadStateString(
+    DownloadInternalState state) {
+  switch (state) {
+    case IN_PROGRESS_INTERNAL:
+      return "IN_PROGRESS";
+    case COMPLETING_INTERNAL:
+      return "COMPLETING";
+    case COMPLETE_INTERNAL:
+      return "COMPLETE";
+    case CANCELLED_INTERNAL:
+      return "CANCELLED";
+    case INTERRUPTED_INTERNAL:
+      return "INTERRUPTED";
+    default:
+      NOTREACHED() << "Unknown download state " << state;
+      return "unknown";
+  };
+}

content/browser/download/download_item_impl.h

@@ -207,6 +207,34 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   virtual void SetDbHandle(int64 handle);
 
  private:
+  // Fine grained states of a download.
+  enum DownloadInternalState {
+    // Unless otherwise specified, state transitions are linear forward
+    // in this list.
+
+    // Includes both before and after file name determination.
+    // TODO(rdsmith): Put in state variable for file name determination.
+    IN_PROGRESS_INTERNAL,
+
+    // Between commit point (dispatch of download file release) and
+    // completed.  Embedder may be opening the file in this state.
+    COMPLETING_INTERNAL,
+
+    // After embedder has had a chance to auto-open.  User may now open
+    // or auto-open based on extension.
+    COMPLETE_INTERNAL,
+
+    // User has cancelled the download.
+    // Only incoming transition IN_PROGRESS->
+    CANCELLED_INTERNAL,
+
+    // An error has interrupted the download.
+    // Only incoming transition IN_PROGRESS->
+    INTERRUPTED_INTERNAL,
+
+    MAX_DOWNLOAD_INTERNAL_STATE,
+  };
+
   // Normal progression of a download ------------------------------------------
 
   // These are listed in approximately chronological order.  There are also
@@ -249,13 +277,22 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
                         const std::string& final_hash);
 
   // Call to transition state; all state transitions should go through this.
-  void TransitionTo(DownloadState new_state);
+  void TransitionTo(DownloadInternalState new_state);
 
   // Set the |danger_type_| and invoke obserers if necessary.
   void SetDangerType(content::DownloadDangerType danger_type);
 
   void SetFullPath(const FilePath& new_path);
 
+  // Mapping between internal and external states.
+  static DownloadState InternalToExternalState(
+      DownloadInternalState internal_state);
+  static DownloadInternalState ExternalToInternalState(
+      DownloadState external_state);
+
+  // Debugging routines --------------------------------------------------------
+  static const char* DebugDownloadStateString(DownloadInternalState state);
+
   // The handle to the request information.  Used for operations outside the
   // download system.
   scoped_ptr<DownloadRequestHandleInterface> request_handle_;
@@ -353,7 +390,7 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
   base::TimeTicks start_tick_;
 
   // The current state of this download.
-  DownloadState state_;
+  DownloadInternalState state_;
 
   // Current danger type for the download.
   content::DownloadDangerType danger_type_;

content/browser/download/download_item_impl_unittest.cc

@@ -67,8 +67,7 @@ class MockRequestHandle : public DownloadRequestHandleInterface {
   MOCK_CONST_METHOD0(DebugString, std::string());
 };
 
-class MockDownloadFileFactory
-    : public DownloadFileManager::DownloadFileFactory {
+class MockDownloadFileFactory : public content::DownloadFileFactory {
  public:
   content::DownloadFile* CreateFile(
       DownloadCreateInfo* info,

content/browser/download/download_net_log_parameters.cc

@@ -106,9 +106,9 @@ base::Value* ItemInterruptedCallback(content::DownloadInterruptReason reason,
   return dict;
 }
 
-base::Value* ItemFinishedCallback(int64 bytes_so_far,
-                                  const std::string* final_hash,
-                                  net::NetLog::LogLevel /* log_level */) {
+base::Value* ItemCompletingCallback(int64 bytes_so_far,
+                                    const std::string* final_hash,
+                                    net::NetLog::LogLevel /* log_level */) {
   DictionaryValue* dict = new DictionaryValue();
 
   dict->SetString("bytes_so_far", base::Int64ToString(bytes_so_far));
@@ -118,6 +118,15 @@ base::Value* ItemFinishedCallback(int64 bytes_so_far,
   return dict;
 }
 
+base::Value* ItemFinishedCallback(bool auto_opened,
+                                  net::NetLog::LogLevel /* log_level */) {
+  DictionaryValue* dict = new DictionaryValue();
+
+  dict->SetString("auto_opened", auto_opened ? "yes" : "no");
+
+  return dict;
+}
+
 base::Value* ItemCanceledCallback(int64 bytes_so_far,
                                   const std::string* hash_state,
                                   net::NetLog::LogLevel /* log_level */) {

content/browser/download/download_net_log_parameters.h

@@ -46,9 +46,13 @@ base::Value* ItemInterruptedCallback(content::DownloadInterruptReason reason,
                                      const std::string* hash_state,
                                      net::NetLog::LogLevel log_level);
 
+// Returns NetLog parameters when a DownloadItem is completing.
+base::Value* ItemCompletingCallback(int64 bytes_so_far,
+                                    const std::string* final_hash,
+                                    net::NetLog::LogLevel log_level);
+
 // Returns NetLog parameters when a DownloadItem is finished.
-base::Value* ItemFinishedCallback(int64 bytes_so_far,
-                                  const std::string* final_hash,
+base::Value* ItemFinishedCallback(bool auto_opened,
                                   net::NetLog::LogLevel log_level);
 
 // Returns NetLog parameters when a DownloadItem is canceled.

content/browser/download/mock_download_file.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "content/browser/download/mock_download_file.h"
+#include "testing/gmock/include/gmock/gmock.h"
 
 using ::testing::_;
 using ::testing::Return;

content/browser/download/mock_download_file.h

@@ -32,7 +32,7 @@ class MockDownloadFile : virtual public content::DownloadFile {
   MOCK_METHOD3(Rename, void(const FilePath& full_path,
                             bool overwrite_existing_file,
                             const RenameCompletionCallback& callback));
-  MOCK_METHOD0(Detach, void());
+  MOCK_METHOD1(Detach, void(base::Closure));
   MOCK_METHOD0(Cancel, void());
   MOCK_METHOD0(Finish, void());
   MOCK_METHOD0(AnnotateWithSourceInformation, void());

content/content_browser.gypi

@@ -319,6 +319,8 @@
     'browser/download/download_create_info.cc',
     'browser/download/download_create_info.h',
     'browser/download/download_file.h',
+    'browser/download/download_file_factory.cc',
+    'browser/download/download_file_factory.h',
     'browser/download/download_file_impl.cc',
     'browser/download/download_file_impl.h',
     'browser/download/download_file_manager.cc',

content/public/test/test_file_error_injector.cc

@@ -65,12 +65,15 @@ class DownloadFileWithErrors: public DownloadFileImpl {
       content::TestFileErrorInjector::FileOperationCode code,
       content::DownloadInterruptReason original_error);
 
-  // Used in place of original rename callback to intercept with
-  // ShouldReturnError.
-  void RenameErrorCallback(
-    const RenameCompletionCallback& original_callback,
-    content::DownloadInterruptReason original_error,
-    const FilePath& path_result);
+  // 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(
+    content::TestFileErrorInjector::FileOperationCode code,
+    content::DownloadInterruptReason* output_error);
 
   // Source URL for the file being downloaded.
   GURL source_url_;
@@ -86,6 +89,18 @@ class DownloadFileWithErrors: public DownloadFileImpl {
   DestructionCallback destruction_callback_;
 };
 
+static void RenameErrorCallback(
+    const content::DownloadFile::RenameCompletionCallback original_callback,
+    content::DownloadInterruptReason overwrite_error,
+    content::DownloadInterruptReason original_error,
+    const FilePath& path_result) {
+  original_callback.Run(
+      overwrite_error,
+      overwrite_error == content::DOWNLOAD_INTERRUPT_REASON_NONE ?
+      path_result : FilePath());
+}
+
+
 DownloadFileWithErrors::DownloadFileWithErrors(
     const DownloadCreateInfo* info,
     scoped_ptr<content::ByteStreamReader> stream,
@@ -115,8 +130,8 @@ DownloadFileWithErrors::~DownloadFileWithErrors() {
 
 content::DownloadInterruptReason DownloadFileWithErrors::Initialize() {
   return ShouldReturnError(
-          content::TestFileErrorInjector::FILE_OPERATION_INITIALIZE,
-          DownloadFileImpl::Initialize());
+      content::TestFileErrorInjector::FILE_OPERATION_INITIALIZE,
+      DownloadFileImpl::Initialize());
 }
 
 content::DownloadInterruptReason DownloadFileWithErrors::AppendDataToFile(
@@ -130,48 +145,42 @@ void DownloadFileWithErrors::Rename(
     const FilePath& full_path,
     bool overwrite_existing_file,
     const RenameCompletionCallback& callback) {
-  DownloadFileImpl::Rename(
-      full_path, overwrite_existing_file,
-      base::Bind(&DownloadFileWithErrors::RenameErrorCallback,
-                 // Unretained since this'll only be called from
-                 // the DownloadFileImpl slice of the same object.
-                 base::Unretained(this), callback));
+  content::DownloadInterruptReason error_to_return =
+      content::DOWNLOAD_INTERRUPT_REASON_NONE;
+  RenameCompletionCallback callback_to_use = callback;
+
+  // Replace callback if the error needs to be overwritten.
+  if (OverwriteError(
+          content::TestFileErrorInjector::FILE_OPERATION_RENAME,
+          &error_to_return)) {
+    callback_to_use = base::Bind(&RenameErrorCallback, callback,
+                                 error_to_return);
+  }
+
+  DownloadFileImpl::Rename(full_path, overwrite_existing_file, callback_to_use);
+}
+
+bool DownloadFileWithErrors::OverwriteError(
+    content::TestFileErrorInjector::FileOperationCode code,
+    content::DownloadInterruptReason* output_error) {
+  int counter = operation_counter_[code]++;
+
+  if (code != error_info_.code)
+    return false;
+
+  if (counter != error_info_.operation_instance)
+    return false;
+
+  *output_error = error_info_.error;
+  return true;
 }
 
 content::DownloadInterruptReason DownloadFileWithErrors::ShouldReturnError(
     content::TestFileErrorInjector::FileOperationCode code,
     content::DownloadInterruptReason original_error) {
-  int counter = operation_counter_[code];
-  ++operation_counter_[code];
-
-  if (code != error_info_.code)
-    return original_error;
-
-  if (counter != error_info_.operation_instance)
-    return original_error;
-
-  VLOG(1) << " " << __FUNCTION__ << "()"
-          << " url = '" << source_url_.spec() << "'"
-          << " code = " << content::TestFileErrorInjector::DebugString(code)
-          << " (" << code << ")"
-          << " counter = " << counter
-          << " original_error = "
-          << content::InterruptReasonDebugString(original_error)
-          << " (" << original_error << ")"
-          << " new error = "
-          << content::InterruptReasonDebugString(error_info_.error)
-          << " (" << error_info_.error << ")";
-
-  return error_info_.error;
-}
-
-void DownloadFileWithErrors::RenameErrorCallback(
-    const RenameCompletionCallback& original_callback,
-    content::DownloadInterruptReason original_error,
-    const FilePath& path_result) {
-  original_callback.Run(ShouldReturnError(
-      content::TestFileErrorInjector::FILE_OPERATION_RENAME,
-      original_error), path_result);
+  content::DownloadInterruptReason output_error = original_error;
+  OverwriteError(code, &output_error);
+  return output_error;
 }
 
 }  // namespace
@@ -179,8 +188,7 @@ void DownloadFileWithErrors::RenameErrorCallback(
 namespace content {
 
 // A factory for constructing DownloadFiles that inject errors.
-class DownloadFileWithErrorsFactory
-    : public DownloadFileManager::DownloadFileFactory {
+class DownloadFileWithErrorsFactory : public content::DownloadFileFactory {
  public:
 
   DownloadFileWithErrorsFactory(
@@ -298,7 +306,7 @@ void TestFileErrorInjector::AddFactory(
   DCHECK(download_file_manager);
 
   // Convert to base class pointer, for GCC.
-  scoped_ptr<DownloadFileManager::DownloadFileFactory> plain_factory(
+  scoped_ptr<content::DownloadFileFactory> plain_factory(
       factory.release());
 
   download_file_manager->SetFileFactoryForTesting(plain_factory.Pass());
@@ -344,11 +352,11 @@ void TestFileErrorInjector::InjectErrorsOnFileThread(
   DownloadFileManager* download_file_manager = GetDownloadFileManager();
   DCHECK(download_file_manager);
 
-  DownloadFileManager::DownloadFileFactory* file_factory =
+  content::DownloadFileFactory* file_factory =
       download_file_manager->GetFileFactoryForTesting();
 
   // Validate that we still have the same factory.
-  DCHECK_EQ(static_cast<DownloadFileManager::DownloadFileFactory*>(factory),
+  DCHECK_EQ(static_cast<content::DownloadFileFactory*>(factory),
             file_factory);
 
   // We want to replace all existing injection errors.

content/shell/shell_download_manager_delegate.cc

@@ -111,7 +111,8 @@ void ShellDownloadManagerDelegate::OnDownloadPathGenerated(
   if (suppress_prompting_) {
     // Testing exit.
     callback.Run(suggested_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-                 DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, suggested_path);
+                 DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+                 suggested_path.AddExtension(FILE_PATH_LITERAL(".crdownload")));
     return;
   }
 

net/base/net_log_event_type_list.h

@@ -1584,11 +1584,17 @@ EVENT_TYPE(DOWNLOAD_ITEM_INTERRUPTED)
 //   }
 EVENT_TYPE(DOWNLOAD_ITEM_RESUMED)
 
-// This event is created when a download item is finished.
+// This event is created when a download item is completing.
 //   {
 //     "bytes_so_far": <Number of bytes received>,
 //     "final_hash": <Final hash, as a hex-encoded binary string>,
 //   }
+EVENT_TYPE(DOWNLOAD_ITEM_COMPLETING)
+
+// This event is created when a download item is finished.
+//   {
+//     "auto_opened": <Whether or not the download was auto-opened>
+//   }
 EVENT_TYPE(DOWNLOAD_ITEM_FINISHED)
 
 // This event is created when a download item is canceled.