New cancel test (includes change in DownloadItem::Complete meaning and

notification interface).

* Changed DownloadItem::COMPLETE state to mean "really all done" with download (used to mean "all data received").  This isn't a very large change, since we recently put in another change to dangerous downloads where we didn't recognize all data received until we had been accepted by the user, which means that there aren't any blocking points between all data received and COMPLETE.
* Removed DownloadItem::Observer::OnDownloadFileCompleted() notification.  It's now redundant with the COMPLETE state, and the COMPLETE state works better for synchronization (because you can test the download item for it as opposed to having to be an observer when the notification is sent).
* Added a basic test for cancel functionality in the downloads system.

Also shifted MarkAsComplete to be used only by SavePackage, and make sure that the final transition occurs for drag-n-drop downloads (which don't need to go through name finalization).

BUG=78183
TEST=All known download tests, manual drag-and-drop and dangerous accept/discard pre/post data received.

Review URL: http://codereview.chromium.org/6720061

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

23 files changed, 379 insertions, 177 deletions

diff --git a/chrome/browser/automation/automation_provider_observers.cc b/chrome/browser/automation/automation_provider_observers.cc
index c9a47a7..15beb50 100644
--- a/chrome/browser/automation/automation_provider_observers.cc
+++ b/chrome/browser/automation/automation_provider_observers.cc
@@ -1452,11 +1452,9 @@ void AutomationProviderDownloadItemObserver::OnDownloadUpdated(
     download->Cancel(true);
     RemoveAndCleanupOnLastEntry(download);
   }
-}
 
-void AutomationProviderDownloadItemObserver::OnDownloadFileCompleted(
-      DownloadItem* download) {
-  RemoveAndCleanupOnLastEntry(download);
+  if (download->IsComplete())
+    RemoveAndCleanupOnLastEntry(download);
 }
 
 // We don't want to send multiple messages, as the behavior is undefined.
@@ -1528,10 +1526,6 @@ void AutomationProviderDownloadUpdatedObserver::OnDownloadOpened(
   delete this;
 }
 
-void AutomationProviderDownloadUpdatedObserver::OnDownloadFileCompleted(
-    DownloadItem* download) {
-}
-
 AutomationProviderDownloadModelChangedObserver::
 AutomationProviderDownloadModelChangedObserver(
     AutomationProvider* provider,
diff --git a/chrome/browser/automation/automation_provider_observers.h b/chrome/browser/automation/automation_provider_observers.h
index b2b3410..2d71001 100644
--- a/chrome/browser/automation/automation_provider_observers.h
+++ b/chrome/browser/automation/automation_provider_observers.h
@@ -888,7 +888,6 @@ class AutomationProviderDownloadItemObserver : public DownloadItem::Observer {
   virtual ~AutomationProviderDownloadItemObserver();
 
   virtual void OnDownloadUpdated(DownloadItem* download);
-  virtual void OnDownloadFileCompleted(DownloadItem* download);
   virtual void OnDownloadOpened(DownloadItem* download);
 
  private:
@@ -915,7 +914,6 @@ class AutomationProviderDownloadUpdatedObserver
 
   virtual void OnDownloadUpdated(DownloadItem* download);
   virtual void OnDownloadOpened(DownloadItem* download);
-  virtual void OnDownloadFileCompleted(DownloadItem* download);
 
  private:
   base::WeakPtr<AutomationProvider> provider_;
diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
index b08bdd49..99440ba 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -4,11 +4,14 @@
 
 #include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_temp_dir.h"
 #include "base/path_service.h"
 #include "base/test/test_file_util.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_item.h"
+#include "chrome/browser/download/download_file_manager.h"
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_shelf.h"
@@ -24,6 +27,7 @@
 #include "chrome/common/url_constants.h"
 #include "chrome/test/in_process_browser_test.h"
 #include "chrome/test/ui_test_utils.h"
+#include "content/browser/renderer_host/resource_dispatcher_host.h"
 #include "content/common/page_transition_types.h"
 #include "net/base/net_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -38,14 +42,17 @@ namespace {
 // occurs at any point after construction.  When that state occurs, the class
 // is considered finished.  Callers may either probe for the finished state, or
 // wait on it.
+//
+// TODO(rdsmith): Detect manager going down, remove pointer to
+// DownloadManager, transition to finished.  (For right now we
+// just use a scoped_refptr<> to keep it around, but that may cause
+// timeouts on waiting if a DownloadManager::Shutdown() occurs which
+// cancels our in-progress downloads.)
 class DownloadsObserver : public DownloadManager::Observer,
                           public DownloadItem::Observer {
  public:
-  // The action which should be considered the completion of the download.
-  enum DownloadFinishedSignal { COMPLETE, FILE_RENAME };
-
-  // Create an object that will be considered completed when |wait_count|
-  // download items have entered state |download_finished_signal|.
+  // Create an object that will be considered finished when |wait_count|
+  // download items have entered state |download_finished_state|.
   // If |finish_on_select_file| is true, the object will also be
   // considered finished if the DownloadManager raises a
   // SelectFileDialogDisplayed() notification.
@@ -57,13 +64,13 @@ class DownloadsObserver : public DownloadManager::Observer,
   // to treat as completion events.
   DownloadsObserver(DownloadManager* download_manager,
                     size_t wait_count,
-                    DownloadFinishedSignal download_finished_signal,
+                    DownloadItem::DownloadState download_finished_state,
                     bool finish_on_select_file)
       : download_manager_(download_manager),
         wait_count_(wait_count),
         finished_downloads_at_construction_(0),
         waiting_(false),
-        download_finished_signal_(download_finished_signal),
+        download_finished_state_(download_finished_state),
         finish_on_select_file_(finish_on_select_file),
         select_file_dialog_seen_(false) {
     download_manager_->AddObserver(this);  // Will call initial ModelChanged().
@@ -100,12 +107,7 @@ class DownloadsObserver : public DownloadManager::Observer,
 
   // DownloadItem::Observer
   virtual void OnDownloadUpdated(DownloadItem* download) {
-    if ((download_finished_signal_ == COMPLETE) && download->IsComplete())
-      DownloadInFinalState(download);
-  }
-
-  virtual void OnDownloadFileCompleted(DownloadItem* download) {
-    if (download_finished_signal_ == FILE_RENAME)
+    if (download->state() == download_finished_state_)
       DownloadInFinalState(download);
   }
 
@@ -114,8 +116,8 @@ class DownloadsObserver : public DownloadManager::Observer,
   // DownloadManager::Observer
   virtual void ModelChanged() {
     // Regenerate DownloadItem observers.  If there are any download items
-    // in the COMPLETE state and that's our final state, note them in
-    // finished_downloads_ (done by OnDownloadUpdated).
+    // in our final state, note them in |finished_downloads_|
+    // (done by |OnDownloadUpdated()|).
     std::vector<DownloadItem*> downloads;
     download_manager_->SearchDownloads(string16(), &downloads);
 
@@ -154,9 +156,8 @@ class DownloadsObserver : public DownloadManager::Observer,
  private:
   // Called when we know that a download item is in a final state.
   // Note that this is not the same as it first transitioning in to the
-  // final state; in the case of DownloadItem::COMPLETE, multiple
-  // notifications may occur once the item is in that state.  So we
-  // keep our own track of transitions into final.
+  // final state; multiple notifications may occur once the item is in
+  // that state.  So we keep our own track of transitions into final.
   void DownloadInFinalState(DownloadItem* download) {
     if (finished_downloads_.find(download) != finished_downloads_.end()) {
       // We've already seen terminal state on this download.
@@ -175,7 +176,7 @@ class DownloadsObserver : public DownloadManager::Observer,
   }
 
   // The observed download manager.
-  DownloadManager* download_manager_;
+  scoped_refptr<DownloadManager> download_manager_;
 
   // The set of DownloadItem's that have transitioned to their finished state
   // since construction of this object.  When the size of this array
@@ -194,18 +195,18 @@ class DownloadsObserver : public DownloadManager::Observer,
   // ModelChanged().  We use |finished_downloads_| to track the incoming
   // transitions to final state we should ignore, and to track the
   // number of final state transitions that occurred between
-  // construction and return from wait.  But if the final state is the
-  // COMPLETE state, some downloads may be in it (and thus be entered
-  // into finished_downloads_) when we construct this class.  We don't
-  // want to count those;
+  // construction and return from wait.  But some downloads may be in our
+  // final state (and thus be entered into |finished_downloads_|) when we
+  // construct this class.  We don't want to count those in our transition
+  // to finished.
   int finished_downloads_at_construction_;
 
   // Whether an internal message loop has been started and must be quit upon
   // all downloads completing.
   bool waiting_;
 
-  // The action on which to consider the DownloadItem finished.
-  DownloadFinishedSignal download_finished_signal_;
+  // The state on which to consider the DownloadItem finished.
+  DownloadItem::DownloadState download_finished_state_;
 
   // True if we should transition the DownloadsObserver to finished if
   // the select file dialog comes up.
@@ -217,6 +218,178 @@ class DownloadsObserver : public DownloadManager::Observer,
   DISALLOW_COPY_AND_ASSIGN(DownloadsObserver);
 };
 
+// WaitForFlush() returns after:
+//      * There are no IN_PROGRESS download items remaining on the
+//        DownloadManager.
+//      * There have been two round trip messages through the file and
+//        IO threads.
+// This almost certainly means that a Download cancel has propagated through
+// the system.
+class DownloadsFlushObserver
+    : public DownloadManager::Observer,
+      public DownloadItem::Observer,
+      public base::RefCountedThreadSafe<DownloadsFlushObserver> {
+ public:
+  explicit DownloadsFlushObserver(DownloadManager* download_manager)
+      : download_manager_(download_manager),
+        waiting_for_zero_inprogress_(true) { }
+
+  void WaitForFlush() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    download_manager_->AddObserver(this);
+    ui_test_utils::RunMessageLoop();
+  }
+
+  // DownloadsManager observer methods.
+  virtual void ModelChanged() {
+    // Model has changed, so there may be more DownloadItems to observe.
+    CheckDownloadsInProgress(true);
+  }
+
+  // DownloadItem observer methods.
+  virtual void OnDownloadUpdated(DownloadItem* download) {
+    // No change in DownloadItem set on manager.
+    CheckDownloadsInProgress(false);
+  }
+  virtual void OnDownloadOpened(DownloadItem* download) {}
+
+ protected:
+  friend class base::RefCountedThreadSafe<DownloadsFlushObserver>;
+
+  virtual ~DownloadsFlushObserver() {
+    download_manager_->RemoveObserver(this);
+    for (std::set<DownloadItem*>::iterator it = downloads_observed_.begin();
+         it != downloads_observed_.end(); it++) {
+      (*it)->RemoveObserver(this);
+    }
+  }
+
+ private:
+  // If we're waiting for that flush point, check the number
+  // of downloads in the IN_PROGRESS state and take appropriate
+  // action.  If requested, also observes all downloads while iterating.
+  void CheckDownloadsInProgress(bool observe_downloads) {
+    if (waiting_for_zero_inprogress_) {
+      int count = 0;
+
+      std::vector<DownloadItem*> downloads;
+      download_manager_->SearchDownloads(string16(), &downloads);
+      std::vector<DownloadItem*>::iterator it = downloads.begin();
+      for (; it != downloads.end(); ++it) {
+        if ((*it)->state() == DownloadItem::IN_PROGRESS)
+          count++;
+        if (observe_downloads) {
+          if (downloads_observed_.find(*it) == downloads_observed_.end()) {
+            (*it)->AddObserver(this);
+          }
+          // Download items are forever, and we don't want to make
+          // assumptions about future state transitions, so once we
+          // start observing them, we don't stop until destruction.
+        }
+      }
+
+      if (count == 0) {
+        waiting_for_zero_inprogress_ = false;
+        // Stop observing DownloadItems.  We maintain the observation
+        // of DownloadManager so that we don't have to independently track
+        // whether we are observing it for conditional destruction.
+        for (std::set<DownloadItem*>::iterator it = downloads_observed_.begin();
+             it != downloads_observed_.end(); it++) {
+          (*it)->RemoveObserver(this);
+        }
+        downloads_observed_.clear();
+
+        // Trigger next step.  We need to go past the IO thread twice, as
+        // there's a self-task posting in the IO thread cancel path.
+        BrowserThread::PostTask(
+            BrowserThread::FILE, FROM_HERE,
+            NewRunnableMethod(this,
+                              &DownloadsFlushObserver::PingFileThread, 2));
+      }
+    }
+  }
+
+  void PingFileThread(int cycle) {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        NewRunnableMethod(this, &DownloadsFlushObserver::PingIOThread,
+                          cycle));
+  }
+
+  void PingIOThread(int cycle) {
+    if (--cycle) {
+      BrowserThread::PostTask(
+          BrowserThread::UI, FROM_HERE,
+          NewRunnableMethod(this, &DownloadsFlushObserver::PingFileThread,
+                            cycle));
+    } else {
+      BrowserThread::PostTask(
+          BrowserThread::UI, FROM_HERE, new MessageLoop::QuitTask());
+    }
+  }
+
+  DownloadManager* download_manager_;
+  std::set<DownloadItem*> downloads_observed_;
+  bool waiting_for_zero_inprogress_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadsFlushObserver);
+};
+
+// Collect the information from FILE and IO threads needed for the Cancel
+// Test, specifically the number of outstanding requests on the
+// ResourceDispatcherHost and the number of pending downloads on the
+// DownloadFileManager.
+class CancelTestDataCollector
+    : public base::RefCountedThreadSafe<CancelTestDataCollector> {
+ public:
+  CancelTestDataCollector()
+      : resource_dispatcher_host_(
+          g_browser_process->resource_dispatcher_host()),
+        rdh_pending_requests_(0),
+        dfm_pending_downloads_(0) { }
+
+  void WaitForDataCollected() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        NewRunnableMethod(this, &CancelTestDataCollector::IOInfoCollector));
+    ui_test_utils::RunMessageLoop();
+  }
+
+  int rdh_pending_requests() { return rdh_pending_requests_; }
+  int dfm_pending_downloads() { return dfm_pending_downloads_; }
+
+ protected:
+  friend class base::RefCountedThreadSafe<CancelTestDataCollector>;
+
+  virtual ~CancelTestDataCollector() {}
+
+ private:
+
+  void IOInfoCollector() {
+    download_file_manager_ = resource_dispatcher_host_->download_file_manager();
+    rdh_pending_requests_ = resource_dispatcher_host_->pending_requests();
+    BrowserThread::PostTask(
+        BrowserThread::FILE, FROM_HERE,
+        NewRunnableMethod(this, &CancelTestDataCollector::FileInfoCollector));
+  }
+
+  void FileInfoCollector() {
+    dfm_pending_downloads_ = download_file_manager_->NumberOfActiveDownloads();
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE, new MessageLoop::QuitTask());
+  }
+
+  ResourceDispatcherHost* resource_dispatcher_host_;
+  DownloadFileManager* download_file_manager_;
+  int rdh_pending_requests_;
+  int dfm_pending_downloads_;
+
+  DISALLOW_COPY_AND_ASSIGN(CancelTestDataCollector);
+};
+
+}  // namespace
+
 class DownloadTest : public InProcessBrowserTest {
  public:
   enum SelectExpectation {
@@ -282,21 +455,35 @@ class DownloadTest : public InProcessBrowserTest {
     return download_mananger->download_prefs()->download_path();
   }
 
+  // Create a DownloadsObserver that will wait for the
+  // specified number of downloads to finish.
   DownloadsObserver* CreateWaiter(Browser* browser, int num_downloads) {
     DownloadManager* download_manager =
         browser->profile()->GetDownloadManager();
     return new DownloadsObserver(
         download_manager, num_downloads,
-        DownloadsObserver::FILE_RENAME,  // Really done
+        DownloadItem::COMPLETE,          // Really done
         true);                           // Bail on select file
   }
 
+  // Create a DownloadsObserver that will wait for the
+  // specified number of downloads to start.
+  DownloadsObserver* CreateInProgressWaiter(Browser* browser,
+                                            int num_downloads) {
+    DownloadManager* download_manager =
+        browser->profile()->GetDownloadManager();
+    return new DownloadsObserver(
+        download_manager, num_downloads,
+        DownloadItem::IN_PROGRESS,      // Has started
+        true);                          // Bail on select file
+  }
+
   // Download |url|, then wait for the download to finish.
   // |disposition| indicates where the navigation occurs (current tab, new
   // foreground tab, etc).
   // |expectation| indicates whether or not a Select File dialog should be
   // open when the download is finished, or if we don't care.
-  // If the dialog appears, the test completes.  The only effect |expectation|
+  // If the dialog appears, the routine exits.  The only effect |expectation|
   // has is whether or not the test succeeds.
   // |browser_test_flags| indicate what to wait for, and is an OR of 0 or more
   // values in the ui_test_utils::BrowserTestWaitFlags enum.
@@ -411,7 +598,7 @@ class DownloadTest : public InProcessBrowserTest {
     //  - Iterate over browser->window()->GetDownloadShelf()'s members
     //    to see if any match the status text we want.  Start with the last one.
 
-    // Complete sending the request.  We do this by loading a second URL in a
+    // Allow the request to finish.  We do this by loading a second URL in a
     // separate tab.
     GURL finish_url(URLRequestSlowDownloadJob::kFinishDownloadUrl);
     ui_test_utils::NavigateToURLWithDisposition(
@@ -950,4 +1137,60 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, NewWindow) {
   CheckDownload(browser(), file, file);
 }
 
-}  // namespace
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) {
+  ASSERT_TRUE(InitialSetup(false));
+  EXPECT_EQ(1, browser()->tab_count());
+
+  // TODO(rdsmith): Fragile code warning!  The code below relies on the
+  // DownloadsObserver 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
+  // DownloadsObserver, 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.
+
+  // Create a download, wait until it's started, and confirm
+  // we're in the expected state.
+  scoped_ptr<DownloadsObserver> observer(CreateInProgressWaiter(browser(), 1));
+  ui_test_utils::NavigateToURL(
+      browser(), GURL(URLRequestSlowDownloadJob::kUnknownSizeUrl));
+  observer->WaitForFinished();
+
+  std::vector<DownloadItem*> downloads;
+  browser()->profile()->GetDownloadManager()->SearchDownloads(
+      string16(), &downloads);
+  ASSERT_EQ(1u, downloads.size());
+  ASSERT_EQ(DownloadItem::IN_PROGRESS, downloads[0]->state());
+  EXPECT_TRUE(IsDownloadUIVisible(browser()));
+
+  // Cancel the download and wait for download system quiesce.
+  downloads[0]->Remove(/* delete_on_disk = */ true);
+  scoped_refptr<DownloadsFlushObserver> flush_observer(
+      new DownloadsFlushObserver(browser()->profile()->GetDownloadManager()));
+  flush_observer->WaitForFlush();
+
+  // Get the important info from other threads and check it.
+  scoped_refptr<CancelTestDataCollector> info(new CancelTestDataCollector());
+  info->WaitForDataCollected();
+  EXPECT_EQ(0, info->rdh_pending_requests());
+  EXPECT_EQ(0, info->dfm_pending_downloads());
+
+  // Using "DownloadItem::Remove" follows the discard dangerous download path,
+  // which completely removes the browser from the shelf and closes the shelf
+  // if it was there.
+#if defined(OS_CHROMEOS)
+  // Except under ChromeOS in which case if we've brought up the file
+  // picker panel, it stays.
+  EXPECT_TRUE(IsDownloadUIVisible(browser()));
+#else
+  EXPECT_FALSE(IsDownloadUIVisible(browser()));
+#endif
+}
diff --git a/chrome/browser/download/download_file_manager.cc b/chrome/browser/download/download_file_manager.cc
index 33e81fe..2522a98 100644
--- a/chrome/browser/download/download_file_manager.cc
+++ b/chrome/browser/download/download_file_manager.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -324,12 +324,13 @@ void DownloadFileManager::RenameInProgressDownloadFile(
 }
 
 // The DownloadManager in the UI thread has provided a final name for the
-// download specified by 'id'. Rename the completed download.
+// download specified by 'id'. Rename the download that's in the process
+// of completing.
 //
 // There are 2 possible rename cases where this method can be called:
 // 1. foo.crdownload -> foo (final, safe)
 // 2. Unconfirmed.xxx.crdownload -> xxx (final, validated)
-void DownloadFileManager::RenameFinishedDownloadFile(
+void DownloadFileManager::RenameCompletingDownloadFile(
     int id, const FilePath& full_path, bool overwrite_existing_file) {
   VLOG(20) << __FUNCTION__ << "()" << " id = " << id
            << " overwrite_existing_file = " << overwrite_existing_file
@@ -382,8 +383,8 @@ void DownloadFileManager::RenameFinishedDownloadFile(
           new_path, uniquifier));
 }
 
-// Called only from RenameInProgressDownloadFile and RenameFinishedDownloadFile
-// on the FILE thread.
+// Called only from RenameInProgressDownloadFile and
+// RenameCompletingDownloadFile on the FILE thread.
 void DownloadFileManager::CancelDownloadOnRename(int id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
diff --git a/chrome/browser/download/download_file_manager.h b/chrome/browser/download/download_file_manager.h
index 868ba50..1d96b5d 100644
--- a/chrome/browser/download/download_file_manager.h
+++ b/chrome/browser/download/download_file_manager.h
@@ -110,9 +110,15 @@ class DownloadFileManager
   // |overwrite_existing_file| prevents uniquification, and is used for SAFE
   // downloads, as the user may have decided to overwrite the file.
   // Sent from the UI thread and run on the FILE thread.
-  void RenameFinishedDownloadFile(int id,
-                                  const FilePath& full_path,
-                                  bool overwrite_existing_file);
+  void RenameCompletingDownloadFile(int id,
+                                    const FilePath& full_path,
+                                    bool overwrite_existing_file);
+
+  // The number of downloads currently active on the DownloadFileManager.
+  // Primarily for testing.
+  int NumberOfActiveDownloads() const {
+    return downloads_.size();
+  }
 
  private:
   friend class base::RefCountedThreadSafe<DownloadFileManager>;
@@ -143,7 +149,7 @@ class DownloadFileManager
   DownloadFile* GetDownloadFile(int id);
 
   // Called only from RenameInProgressDownloadFile and
-  // RenameFinishedDownloadFile on the FILE thread.
+  // RenameCompletingDownloadFile on the FILE thread.
   void CancelDownloadOnRename(int id);
 
   // Erases the download file with the given the download |id| and removes
diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index b6f9c3b..99b6b9d 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -33,7 +33,7 @@
 //        destination file has been determined.
 //      * Entered into the history database.
 //      * Made visible in the download shelf.
-//      * All data is received.  Note that the actual data download occurs
+//      * All data is saved.  Note that the actual data download occurs
 //        in parallel with the above steps, but until those steps are
 //        complete, completion of the data download will be ignored.
 //      * Download file is renamed to its final name, and possibly
@@ -246,10 +246,6 @@ void DownloadItem::UpdateObservers() {
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadUpdated(this));
 }
 
-void DownloadItem::NotifyObserversDownloadFileCompleted() {
-  FOR_EACH_OBSERVER(Observer, observers_, OnDownloadFileCompleted(this));
-}
-
 bool DownloadItem::CanOpenDownload() {
   return !Extension::IsExtension(target_name_);
 }
@@ -357,6 +353,7 @@ void DownloadItem::Cancel(bool update_history) {
 void DownloadItem::MarkAsComplete() {
   DCHECK(all_data_saved_);
   state_ = COMPLETE;
+  UpdateObservers();
 }
 
 void DownloadItem::OnAllDataSaved(int64 size) {
@@ -366,7 +363,7 @@ void DownloadItem::OnAllDataSaved(int64 size) {
   StopProgressTimer();
 }
 
-void DownloadItem::Finished() {
+void DownloadItem::Completed() {
   VLOG(20) << " " << __FUNCTION__ << "() "
            << DebugString(false);
 
@@ -390,16 +387,13 @@ void DownloadItem::Finished() {
     auto_opened_ = true;
   }
 
-  // Notify our observers that we are complete (the call to MarkAsComplete()
-  // set the state to complete but did not notify).
-  UpdateObservers();
-
   // The download file is meant to be completed if both the filename is
   // finalized and the file data is downloaded. The ordering of these two
   // actions is indeterministic. Thus, if the filename is not finalized yet,
   // delay the notification.
   if (name_finalized()) {
-    NotifyObserversDownloadFileCompleted();
+    state_ = COMPLETE;
+    UpdateObservers();
     download_manager_->RemoveFromActiveList(id());
   }
 }
@@ -474,17 +468,15 @@ void DownloadItem::OnNameFinalized() {
            << DebugString(true);
   name_finalized_ = true;
 
-  // The download file is meant to be completed if both the filename is
-  // finalized and the file data is downloaded. The ordering of these two
-  // actions is indeterministic. Thus, if we are still in downloading the
-  // file, delay the notification.
-  if (IsComplete()) {
-    NotifyObserversDownloadFileCompleted();
-    download_manager_->RemoveFromActiveList(id());
-  }
+  // We can't reach this point in the code without having received all the
+  // data, so it's safe to move to the COMPLETE state.
+  DCHECK(all_data_saved_);
+  state_ = COMPLETE;
+  UpdateObservers();
+  download_manager_->RemoveFromActiveList(id());
 }
 
-void DownloadItem::OnDownloadFinished(DownloadFileManager* file_manager) {
+void DownloadItem::OnDownloadCompleting(DownloadFileManager* file_manager) {
   VLOG(20) << " " << __FUNCTION__ << "() "
            << " needs rename = " << NeedsRename()
            << " " << DebugString(true);
@@ -495,12 +487,14 @@ void DownloadItem::OnDownloadFinished(DownloadFileManager* file_manager) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         NewRunnableMethod(
-            file_manager, &DownloadFileManager::RenameFinishedDownloadFile,
+            file_manager, &DownloadFileManager::RenameCompletingDownloadFile,
             id(), GetTargetFilePath(), safety_state() == SAFE));
     return;
+  } else {
+    name_finalized_ = true;
   }
 
-  Finished();
+  Completed();
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
@@ -518,9 +512,7 @@ void DownloadItem::OnDownloadRenamedToFinalName(const FilePath& full_path) {
   Rename(full_path);
   OnNameFinalized();
 
-  // This was called from OnDownloadFinished; continue to call
-  // DownloadFinished.
-  Finished();
+  Completed();
 }
 
 bool DownloadItem::MatchesQuery(const string16& query) const {
diff --git a/chrome/browser/download/download_item.h b/chrome/browser/download/download_item.h
index fd81f02..e90dbf51 100644
--- a/chrome/browser/download/download_item.h
+++ b/chrome/browser/download/download_item.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 //
@@ -39,16 +39,13 @@ struct DownloadCreateInfo;
 class DownloadItem {
  public:
   enum DownloadState {
+    // Download is actively progressing.
     IN_PROGRESS,
 
-    // Note that COMPLETE indicates that the download has gotten all of its
-    // data, has figured out its final destination file, has been entered
-    // into the history store, and has been shown in the UI.  The only
-    // operations remaining are acceptance by the user of
-    // a dangerous download (if dangerous), renaming the file
-    // to the final name, and auto-opening it (if it auto-opens).
+    // Download is completely finished.
     COMPLETE,
 
+    // Download has been cancelled.
     CANCELLED,
 
     // This state indicates that the download item is about to be destroyed,
@@ -82,9 +79,6 @@ class DownloadItem {
    public:
     virtual void OnDownloadUpdated(DownloadItem* download) = 0;
 
-    // Called when a downloaded file has been completed.
-    virtual void OnDownloadFileCompleted(DownloadItem* download) = 0;
-
     // Called when a downloaded file has been opened.
     virtual void OnDownloadOpened(DownloadItem* download) = 0;
 
@@ -115,9 +109,6 @@ class DownloadItem {
   // Notifies our observers periodically.
   void UpdateObservers();
 
-  // Notifies our observers the downloaded file has been completed.
-  void NotifyObserversDownloadFileCompleted();
-
   // Whether it is OK to open this download.
   bool CanOpenDownload();
 
@@ -156,14 +147,10 @@ class DownloadItem {
   // Called when all data has been saved.  Only has display effects.
   void OnAllDataSaved(int64 size);
 
-  // Called when ready to consider the data received and move on to the
-  // next stage.
+  // Called by external code (SavePackage) using the DownloadItem interface
+  // to display progress when the DownloadItem should be considered complete.
   void MarkAsComplete();
 
-  // Called when the entire download operation (including renaming etc)
-  // is finished.
-  void Finished();
-
   // Download operation had an error.
   // |size| is the amount of data received so far, and |os_error| is the error
   // code that the operation received.
@@ -210,10 +197,10 @@ class DownloadItem {
   // Called when the name of the download is finalized.
   void OnNameFinalized();
 
-  // Called when the download is finished.
+  // Called when the download is ready to complete.
   // This may perform final rename if necessary and will eventually call
-  // DownloadManager::DownloadFinished().
-  void OnDownloadFinished(DownloadFileManager* file_manager);
+  // DownloadItem::Completed().
+  void OnDownloadCompleting(DownloadFileManager* file_manager);
 
   // Called when the file name for the download is renamed to its final name.
   void OnDownloadRenamedToFinalName(const FilePath& full_path);
@@ -297,6 +284,10 @@ class DownloadItem {
   // Internal helper for maintaining consistent received and total sizes.
   void UpdateSize(int64 size);
 
+  // Called when the entire download operation (including renaming etc)
+  // is completed.
+  void Completed();
+
   // Start/stop sending periodic updates to our observers
   void StartProgressTimer();
   void StopProgressTimer();
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index ccedecf..3d49a46 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -91,7 +91,7 @@ void DownloadManager::Shutdown() {
     it++;
 
     if (download->safety_state() == DownloadItem::DANGEROUS &&
-        (download->IsPartialDownload() || download->IsComplete())) {
+        download->IsPartialDownload()) {
       // The user hasn't accepted it, so we need to remove it
       // from the disk.  This may or may not result in it being
       // removed from the DownloadManager queues and deleted
@@ -505,7 +505,7 @@ void DownloadManager::AttachDownloadItem(DownloadCreateInfo* info) {
     // The download is a safe download.  We need to
     // rename it to its intermediate '.crdownload' path.  The final
     // name after user confirmation will be set from
-    // DownloadItem::OnDownloadFinished.
+    // DownloadItem::OnDownloadCompleting.
     download_path = download_util::GetCrDownloadPath(info->path);
   }
 
@@ -644,11 +644,10 @@ void DownloadManager::MaybeCompleteDownload(DownloadItem* download) {
   UpdateAppIcon();  // Reflect removal from in_progress_.
 
   // Final update of download item and history.
-  download->MarkAsComplete();
   download_history_->UpdateEntry(download);
 
   // Finish the download.
-  download->OnDownloadFinished(file_manager_);
+  download->OnDownloadCompleting(file_manager_);
 }
 
 void DownloadManager::RemoveFromActiveList(int32 download_id) {
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index 3baab3e..84b8399 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -99,12 +99,12 @@ class DownloadManager
                              std::vector<DownloadItem*>* result);
 
   // Return all non-temporary downloads in the specified directory that are
-  // are in progress or have finished.
+  // are in progress or have completed.
   void GetAllDownloads(const FilePath& dir_path,
                        std::vector<DownloadItem*>* result);
 
   // Return all non-temporary downloads in the specified directory that are
-  // either in-progress or finished but still waiting for user confirmation.
+  // in-progress (including dangerous downloads waiting for user confirmation).
   void GetCurrentDownloads(const FilePath& dir_path,
                            std::vector<DownloadItem*>* result);
 
@@ -130,7 +130,7 @@ class DownloadManager
   void RemoveDownload(int64 download_handle);
 
   // Determine if the download is ready for completion, i.e. has had
-  // all data received, and completed the filename determination and
+  // all data saved, and completed the filename determination and
   // history insertion.
   bool IsDownloadReadyForCompletion(DownloadItem* download);
 
@@ -347,13 +347,14 @@ class DownloadManager
   //
   // When a download is created through a user action, the corresponding
   // DownloadItem* is placed in |active_downloads_| and remains there until the
-  // download has finished.  It is also placed in |in_progress_| and remains
-  // there until it has received a valid handle from the history system. Once
-  // it has a valid handle, the DownloadItem* is placed in the
-  // |history_downloads_| map.  When the download is complete, it is removed
-  // from |in_progress_|.  Downloads from past sessions read from a
-  // persisted state from the history system are placed directly into
-  // |history_downloads_| since they have valid handles in the history system.
+  // download is in a terminal state (COMPLETE or CANCELLED).  It is also
+  // placed in |in_progress_| and remains there until it has received a
+  // valid handle from the history system. Once it has a valid handle, the
+  // DownloadItem* is placed in the |history_downloads_| map.  When the
+  // download reaches a terminal state, it is removed from |in_progress_|.
+  // Downloads from past sessions read from a persisted state from the
+  // history system are placed directly into |history_downloads_| since
+  // they have valid handles in the history system.
   typedef std::set<DownloadItem*> DownloadSet;
   typedef base::hash_map<int64, DownloadItem*> DownloadMap;
 
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index baff0b2..9b92a497 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -253,7 +253,7 @@ class ItemObserver : public DownloadItem::Observer {
  public:
   explicit ItemObserver(DownloadItem* tracked)
       : tracked_(tracked), states_hit_(0),
-        was_updated_(false), was_completed_(false), was_opened_(false) {
+        was_updated_(false), was_opened_(false) {
     DCHECK(tracked_);
     tracked_->AddObserver(this);
     // Record the initial state.
@@ -267,7 +267,6 @@ class ItemObserver : public DownloadItem::Observer {
     return (1 << state) & states_hit_;
   }
   bool was_updated() const { return was_updated_; }
-  bool was_completed() const { return was_completed_; }
   bool was_opened() const { return was_opened_; }
 
  private:
@@ -277,11 +276,6 @@ class ItemObserver : public DownloadItem::Observer {
     states_hit_ |= (1 << download->state());
     was_updated_ = true;
   }
-  virtual void OnDownloadFileCompleted(DownloadItem* download) {
-    DCHECK_EQ(tracked_, download);
-    states_hit_ |= (1 << download->state());
-    was_completed_ = true;
-  }
   virtual void OnDownloadOpened(DownloadItem* download) {
     DCHECK_EQ(tracked_, download);
     states_hit_ |= (1 << download->state());
@@ -291,7 +285,6 @@ class ItemObserver : public DownloadItem::Observer {
   DownloadItem* tracked_;
   int states_hit_;
   bool was_updated_;
-  bool was_completed_;
   bool was_opened_;
 };
 
@@ -450,7 +443,6 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
   EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
-  EXPECT_FALSE(observer->was_completed());
   EXPECT_FALSE(observer->was_opened());
 
   download->Cancel(true);
@@ -462,7 +454,6 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
   EXPECT_FALSE(observer->hit_state(DownloadItem::CANCELLED));
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
-  EXPECT_FALSE(observer->was_completed());
   EXPECT_FALSE(observer->was_opened());
 }
 
@@ -516,7 +507,6 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) {
   EXPECT_FALSE(observer->hit_state(DownloadItem::COMPLETE));
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
-  EXPECT_FALSE(observer->was_completed());
   EXPECT_FALSE(observer->was_opened());
 
   EXPECT_FALSE(file_util::PathExists(new_path));
@@ -597,7 +587,6 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
   EXPECT_TRUE(observer->hit_state(DownloadItem::COMPLETE));
   EXPECT_FALSE(observer->hit_state(DownloadItem::REMOVING));
   EXPECT_TRUE(observer->was_updated());
-  EXPECT_TRUE(observer->was_completed());
   EXPECT_FALSE(observer->was_opened());
   EXPECT_EQ(DownloadItem::COMPLETE, download->state());
 
diff --git a/chrome/browser/download/drag_download_file.cc b/chrome/browser/download/drag_download_file.cc
index e047100..261e3ec 100644
--- a/chrome/browser/download/drag_download_file.cc
+++ b/chrome/browser/download/drag_download_file.cc
@@ -167,23 +167,17 @@ void DragDownloadFile::ModelChanged() {
 
 void DragDownloadFile::OnDownloadUpdated(DownloadItem* download) {
   AssertCurrentlyOnUIThread();
-
   if (download->IsCancelled()) {
     download->RemoveObserver(this);
     download_manager_->RemoveObserver(this);
 
     DownloadCompleted(false);
+  } else if (download->IsComplete()) {
+    download->RemoveObserver(this);
+    download_manager_->RemoveObserver(this);
+    DownloadCompleted(true);
   }
-}
-
-void DragDownloadFile::OnDownloadFileCompleted(DownloadItem* download) {
-  AssertCurrentlyOnUIThread();
-  DCHECK(download->IsComplete());
-
-  download->RemoveObserver(this);
-  download_manager_->RemoveObserver(this);
-
-  DownloadCompleted(true);
+  // Ignore other states.
 }
 
 void DragDownloadFile::AssertCurrentlyOnDragThread() {
diff --git a/chrome/browser/download/drag_download_file.h b/chrome/browser/download/drag_download_file.h
index 01c73cb..581f16d 100644
--- a/chrome/browser/download/drag_download_file.h
+++ b/chrome/browser/download/drag_download_file.h
@@ -56,7 +56,6 @@ class DragDownloadFile : public ui::DownloadFileProvider,
   // DownloadItem::Observer methods.
   // Called on UI thread.
   virtual void OnDownloadUpdated(DownloadItem* download);
-  virtual void OnDownloadFileCompleted(DownloadItem* download);
   virtual void OnDownloadOpened(DownloadItem* download) { }
 
  private:
diff --git a/chrome/browser/download/mock_download_manager.h b/chrome/browser/download/mock_download_manager.h
index 270ae0f..410bdc30 100644
--- a/chrome/browser/download/mock_download_manager.h
+++ b/chrome/browser/download/mock_download_manager.h
@@ -1,9 +1,9 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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 MOCK_DOWNLOAD_MANAGER_H_
-#define MOCK_DOWNLOAD_MANAGER_H_
+#ifndef CHROME_BROWSER_DOWNLOAD_MOCK_DOWNLOAD_MANAGER_H_
+#define CHROME_BROWSER_DOWNLOAD_MOCK_DOWNLOAD_MANAGER_H_
 #pragma once
 
 #include "chrome/browser/download/download_manager.h"
@@ -19,7 +19,6 @@ class MockDownloadManager : public DownloadManager {
 
   // Override some functions.
   virtual void UpdateHistoryForDownload(DownloadItem*) { }
-  virtual void ContinueDownloadFinished(DownloadItem*) { }
 };
 
-#endif  // MOCK_DOWNLOAD_MANAGER_H_
+#endif  // CHROME_BROWSER_DOWNLOAD_MOCK_DOWNLOAD_MANAGER_H_
diff --git a/chrome/browser/download/save_package.cc b/chrome/browser/download/save_package.cc
index 9c54006..c120e83 100644
--- a/chrome/browser/download/save_package.cc
+++ b/chrome/browser/download/save_package.cc
@@ -744,9 +744,6 @@ void SavePackage::Finish() {
 
   download_->OnAllDataSaved(all_save_items_count_);
   download_->MarkAsComplete();
-  // Notify download observers that we are complete (the call
-  // to OnReadyToFinish() set the state to complete but did not notify).
-  download_->UpdateObservers();
 
   NotificationService::current()->Notify(
       NotificationType::SAVE_PACKAGE_SUCCESSFULLY_FINISHED,
diff --git a/chrome/browser/ui/cocoa/download/download_item_mac.h b/chrome/browser/ui/cocoa/download/download_item_mac.h
index a15f670..24d9c4a 100644
--- a/chrome/browser/ui/cocoa/download/download_item_mac.h
+++ b/chrome/browser/ui/cocoa/download/download_item_mac.h
@@ -38,7 +38,6 @@ class DownloadItemMac : DownloadItem::Observer {
   // DownloadItem::Observer implementation
   virtual void OnDownloadUpdated(DownloadItem* download);
   virtual void OnDownloadOpened(DownloadItem* download);
-  virtual void OnDownloadFileCompleted(DownloadItem* download) { }
 
   BaseDownloadItemModel* download_model() { return download_model_.get(); }
 
diff --git a/chrome/browser/ui/gtk/download/download_item_gtk.cc b/chrome/browser/ui/gtk/download/download_item_gtk.cc
index a957edd..f99fbeb 100644
--- a/chrome/browser/ui/gtk/download/download_item_gtk.cc
+++ b/chrome/browser/ui/gtk/download/download_item_gtk.cc
@@ -176,7 +176,8 @@ DownloadItemGtk::DownloadItemGtk(DownloadShelfGtk* parent_shelf,
       complete_animation_(this),
       icon_small_(NULL),
       icon_large_(NULL),
-      creation_time_(base::Time::Now()) {
+      creation_time_(base::Time::Now()),
+      download_complete_(false) {
   LoadIcon();
 
   body_.Own(gtk_button_new());
@@ -393,6 +394,11 @@ void DownloadItemGtk::OnDownloadUpdated(DownloadItem* download) {
       complete_animation_.Show();
       break;
     case DownloadItem::COMPLETE:
+      if (download_complete_)
+        // We've already handled the completion specific actions; skip
+        // doing them again.
+        break;
+
       if (download->auto_opened()) {
         parent_shelf_->RemoveDownloadItem(this);  // This will delete us!
         return;
@@ -403,6 +409,7 @@ void DownloadItemGtk::OnDownloadUpdated(DownloadItem* download) {
       DownloadItemDrag::SetSource(body_.get(), get_download(), icon_large_);
 
       complete_animation_.Show();
+      download_complete_ = true;
       break;
     case DownloadItem::IN_PROGRESS:
       get_download()->is_paused() ?
diff --git a/chrome/browser/ui/gtk/download/download_item_gtk.h b/chrome/browser/ui/gtk/download/download_item_gtk.h
index 6af9f6c..a8f21b4 100644
--- a/chrome/browser/ui/gtk/download/download_item_gtk.h
+++ b/chrome/browser/ui/gtk/download/download_item_gtk.h
@@ -49,7 +49,6 @@ class DownloadItemGtk : public DownloadItem::Observer,
 
   // DownloadItem::Observer implementation.
   virtual void OnDownloadUpdated(DownloadItem* download);
-  virtual void OnDownloadFileCompleted(DownloadItem* download) { }
   virtual void OnDownloadOpened(DownloadItem* download) { }
 
   // ui::AnimationDelegate implementation.
@@ -233,6 +232,10 @@ class DownloadItemGtk : public DownloadItem::Observer,
 
   // For canceling an in progress icon request.
   CancelableRequestConsumerT<int, 0> icon_consumer_;
+
+  // Indicates when the download has completed, so we don't redo
+  // on-completion actions.
+  bool download_complete_;
 };
 
 #endif  // CHROME_BROWSER_UI_GTK_DOWNLOAD_DOWNLOAD_ITEM_GTK_H_
diff --git a/chrome/browser/ui/views/download/download_item_view.h b/chrome/browser/ui/views/download/download_item_view.h
index d15bc00..295d52d 100644
--- a/chrome/browser/ui/views/download/download_item_view.h
+++ b/chrome/browser/ui/views/download/download_item_view.h
@@ -74,7 +74,6 @@ class DownloadItemView : public views::ButtonListener,
 
   // DownloadObserver method
   virtual void OnDownloadUpdated(DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadFileCompleted(DownloadItem* download) OVERRIDE { }
   virtual void OnDownloadOpened(DownloadItem* download) OVERRIDE;
 
   // Overridden from views::View:
diff --git a/chrome/browser/ui/webui/active_downloads_ui.cc b/chrome/browser/ui/webui/active_downloads_ui.cc
index 580193b..b37c280 100644
--- a/chrome/browser/ui/webui/active_downloads_ui.cc
+++ b/chrome/browser/ui/webui/active_downloads_ui.cc
@@ -115,7 +115,6 @@ class ActiveDownloadsHandler
 
   // DownloadItem::Observer interface.
   virtual void OnDownloadUpdated(DownloadItem* item);
-  virtual void OnDownloadFileCompleted(DownloadItem* item);
   virtual void OnDownloadOpened(DownloadItem* item) { }
 
   // DownloadManager::Observer interface.
@@ -704,10 +703,6 @@ void ActiveDownloadsHandler::OnDownloadUpdated(DownloadItem* item) {
       *download_util::CreateDownloadItemValue(item, id));
 }
 
-void ActiveDownloadsHandler::OnDownloadFileCompleted(DownloadItem* item) {
-  OnDownloadUpdated(item);
-}
-
 void ActiveDownloadsHandler::DeleteFile(const FilePath& path, TaskProxy* task) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   if (!file_util::Delete(path, true)) {
diff --git a/chrome/browser/ui/webui/chromeos/imageburner_ui.cc b/chrome/browser/ui/webui/chromeos/imageburner_ui.cc
index 30a6a96..1d66791 100644
--- a/chrome/browser/ui/webui/chromeos/imageburner_ui.cc
+++ b/chrome/browser/ui/webui/chromeos/imageburner_ui.cc
@@ -244,19 +244,18 @@ void ImageBurnHandler::ProgressUpdated(chromeos::BurnLibrary* object,
 }
 
 void ImageBurnHandler::OnDownloadUpdated(DownloadItem* download) {
-  if (download->IsPartialDownload()) {
-    scoped_ptr<DictionaryValue> result_value(
-        download_util::CreateDownloadItemValue(download, 0));
-    web_ui_->CallJavascriptFunction("downloadUpdated", *result_value);
+  if (download->IsCancelled()) {
+    DownloadCompleted(false);  // Should stop observation.
+    DCHECK(!download_item_observer_added_);
+  } else if (download->IsComplete()) {
+    zip_image_file_path_ = download->full_path();
+    DownloadCompleted(true);  // Should stop observation.
+    DCHECK(!download_item_observer_added_);
+  } else if (download->IsPartialDownload()) {
+      scoped_ptr<DictionaryValue> result_value(
+          download_util::CreateDownloadItemValue(download, 0));
+      web_ui_->CallJavascriptFunction("downloadUpdated", *result_value);
   }
-  if (download->IsCancelled())
-    DownloadCompleted(false);
-}
-
-void ImageBurnHandler::OnDownloadFileCompleted(DownloadItem* download) {
-  DCHECK(download->IsComplete());
-  zip_image_file_path_ = download->full_path();
-  DownloadCompleted(true);
 }
 
 void ImageBurnHandler::OnDownloadOpened(DownloadItem* download) {
@@ -590,18 +589,19 @@ void ImageBurnResourceManager::OnDownloadUpdated(DownloadItem* download) {
   if (download->IsCancelled()) {
     image_url_.reset();
     ConfigFileFetched(false);
-  }
-}
 
-void ImageBurnResourceManager::OnDownloadFileCompleted(DownloadItem* download) {
-  DCHECK(download->IsComplete());
-  std::string image_url;
-  if (file_util::ReadFileToString(config_file_path_, &image_url)) {
-    image_url_.reset(new GURL(std::string(kImageBaseURL) + image_url));
-    ConfigFileFetched(true);
-  } else {
-    image_url_.reset();
-    ConfigFileFetched(false);
+    // ConfigFileFetched should remove observer.
+    DCHECK(!download_item_observer_added_);
+    DCHECK(active_download_item_ == NULL);
+  } else if (download->IsComplete()) {
+    std::string image_url;
+    if (file_util::ReadFileToString(config_file_path_, &image_url)) {
+      image_url_.reset(new GURL(std::string(kImageBaseURL) + image_url));
+      ConfigFileFetched(true);
+    } else {
+      image_url_.reset();
+      ConfigFileFetched(false);
+    }
   }
 }
 
diff --git a/chrome/browser/ui/webui/chromeos/imageburner_ui.h b/chrome/browser/ui/webui/chromeos/imageburner_ui.h
index 3a64f84..4dfe9e1 100644
--- a/chrome/browser/ui/webui/chromeos/imageburner_ui.h
+++ b/chrome/browser/ui/webui/chromeos/imageburner_ui.h
@@ -99,7 +99,6 @@ class ImageBurnResourceManager
 
   // DownloadItem::Observer interface
   virtual void OnDownloadUpdated(DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadFileCompleted(DownloadItem* download) OVERRIDE;
   virtual void OnDownloadOpened(DownloadItem* download) OVERRIDE {}
 
   // DownloadManager::Observer interface
@@ -192,7 +191,6 @@ class ImageBurnHandler : public WebUIMessageHandler,
 
   // DownloadItem::Observer interface.
   virtual void OnDownloadUpdated(DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadFileCompleted(DownloadItem* download) OVERRIDE;
   virtual void OnDownloadOpened(DownloadItem* download) OVERRIDE;
 
   // DownloadManager::Observer interface.
diff --git a/chrome/browser/ui/webui/downloads_dom_handler.h b/chrome/browser/ui/webui/downloads_dom_handler.h
index 3778015..bda09ec 100644
--- a/chrome/browser/ui/webui/downloads_dom_handler.h
+++ b/chrome/browser/ui/webui/downloads_dom_handler.h
@@ -31,7 +31,6 @@ class DownloadsDOMHandler : public WebUIMessageHandler,
 
   // DownloadItem::Observer interface
   virtual void OnDownloadUpdated(DownloadItem* download);
-  virtual void OnDownloadFileCompleted(DownloadItem* download) { }
   virtual void OnDownloadOpened(DownloadItem* download) { }
 
   // DownloadManager::Observer interface
diff --git a/chrome/test/ui_test_utils.cc b/chrome/test/ui_test_utils.cc
index b58eea9..6ab4429 100644
--- a/chrome/test/ui_test_utils.cc
+++ b/chrome/test/ui_test_utils.cc
@@ -207,7 +207,6 @@ class DownloadsCompleteObserver : public DownloadManager::Observer,
       CheckAllDownloadsComplete();
   }
 
-  virtual void OnDownloadFileCompleted(DownloadItem* download) { }
   virtual void OnDownloadOpened(DownloadItem* download) {}
 
   // DownloadManager::Observer