Revert 58196 - GTTF: Clean up DownloadFileManager...

chrome/browser/download/download_file_manager.cc

Index: chrome/browser/download/download_file_manager.cc
===================================================================
--- chrome/browser/download/download_file_manager.cc	(revision 58717)
+++ chrome/browser/download/download_file_manager.cc	(working copy)
@@ -35,21 +35,6 @@
 // cause it to become unresponsive (in milliseconds).
 const int kUpdatePeriodMs = 500;
 
-DownloadManager* DownloadManagerForRenderViewHost(int render_process_id,
-                                                  int render_view_id) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
-
-  TabContents* contents = tab_util::GetTabContentsByID(render_process_id,
-                                                       render_view_id);
-  if (contents) {
-    Profile* profile = contents->profile();
-    if (profile)
-      return profile->GetDownloadManager();
-  }
-
-  return NULL;
-}
-
 }  // namespace
 
 DownloadFileManager::DownloadFileManager(ResourceDispatcherHost* rdh)
@@ -58,29 +43,37 @@
 }
 
 DownloadFileManager::~DownloadFileManager() {
+  // Check for clean shutdown.
   DCHECK(downloads_.empty());
 }
 
+// Called during the browser shutdown process to clean up any state (open files,
+// timers) that live on the download_thread_.
 void DownloadFileManager::Shutdown() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  StopUpdateTimer();
   ChromeThread::PostTask(
       ChromeThread::FILE, FROM_HERE,
       NewRunnableMethod(this, &DownloadFileManager::OnShutdown));
 }
 
+// Cease download thread operations.
 void DownloadFileManager::OnShutdown() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-  StopUpdateTimer();
   STLDeleteValues(&downloads_);
 }
 
-void DownloadFileManager::CreateDownloadFile(
-    DownloadCreateInfo* info, DownloadManager* download_manager) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+// Notifications sent from the download thread and run on the UI thread.
 
-  scoped_ptr<DownloadFile> download_file(
-      new DownloadFile(info, download_manager));
-  if (!download_file->Initialize()) {
+// Lookup the DownloadManager for this TabContents' profile and inform it of
+// a new download.
+// TODO(paulg): When implementing download restart via the Downloads tab,
+//              there will be no 'render_process_id' or 'render_view_id'.
+void DownloadFileManager::OnStartDownload(DownloadCreateInfo* info) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  DownloadManager* manager = DownloadManagerFromRenderIds(info->child_id,
+                                                          info->render_view_id);
+  if (!manager) {
     ChromeThread::PostTask(
         ChromeThread::IO, FROM_HERE,
         NewRunnableFunction(&download_util::CancelDownloadRequest,
@@ -91,26 +84,58 @@
     return;
   }
 
-  DCHECK(GetDownloadFile(info->download_id) == NULL);
-  downloads_[info->download_id] = download_file.release();
-  // TODO(phajdan.jr): fix the duplication of path info below.
-  info->path = info->save_info.file_path;
-
   StartUpdateTimer();
 
-  ChromeThread::PostTask(
-      ChromeThread::UI, FROM_HERE,
-      NewRunnableMethod(download_manager,
-                        &DownloadManager::StartDownload, info));
+  // Add the download manager to our request maps for future updates. We want to
+  // be able to cancel all in progress downloads when a DownloadManager is
+  // deleted, such as when a profile is closed. We also want to be able to look
+  // up the DownloadManager associated with a given request without having to
+  // rely on using tab information, since a tab may be closed while a download
+  // initiated from that tab is still in progress.
+  DownloadRequests& downloads = requests_[manager];
+  downloads.insert(info->download_id);
+
+  // TODO(paulg): The manager will exist when restarts are implemented.
+  DownloadManagerMap::iterator dit = managers_.find(info->download_id);
+  if (dit == managers_.end())
+    managers_[info->download_id] = manager;
+  else
+    NOTREACHED();
+
+  // StartDownload will clean up |info|.
+  manager->StartDownload(info);
 }
 
+// Update the Download Manager with the finish state, and remove the request
+// tracking entries.
+void DownloadFileManager::OnDownloadFinished(int id,
+                                             int64 bytes_so_far) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  DownloadManager* manager = GetDownloadManager(id);
+  if (manager)
+    manager->DownloadFinished(id, bytes_so_far);
+  RemoveDownload(id, manager);
+  RemoveDownloadFromUIProgress(id);
+}
+
+// Lookup one in-progress download.
 DownloadFile* DownloadFileManager::GetDownloadFile(int id) {
   DownloadFileMap::iterator it = downloads_.find(id);
   return it == downloads_.end() ? NULL : it->second;
 }
 
+// The UI progress is updated on the file thread and removed on the UI thread.
+void DownloadFileManager::RemoveDownloadFromUIProgress(int id) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  AutoLock lock(progress_lock_);
+  if (ui_progress_.find(id) != ui_progress_.end())
+    ui_progress_.erase(id);
+}
+
+// Throttle updates to the UI thread by only posting update notifications at a
+// regularly controlled interval.
 void DownloadFileManager::StartUpdateTimer() {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   if (!update_timer_.IsRunning()) {
     update_timer_.Start(base::TimeDelta::FromMilliseconds(kUpdatePeriodMs),
                         this, &DownloadFileManager::UpdateInProgressDownloads);
@@ -118,22 +143,21 @@
 }
 
 void DownloadFileManager::StopUpdateTimer() {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   update_timer_.Stop();
 }
 
+// Our periodic timer has fired so send the UI thread updates on all in progress
+// downloads.
 void DownloadFileManager::UpdateInProgressDownloads() {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
-  for (DownloadFileMap::iterator i = downloads_.begin();
-       i != downloads_.end(); ++i) {
-    int id = i->first;
-    DownloadFile* download_file = i->second;
-    DownloadManager* manager = download_file->GetDownloadManager();
-    if (manager) {
-      ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
-          NewRunnableMethod(manager, &DownloadManager::UpdateDownload,
-                            id, download_file->bytes_so_far()));
-    }
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  AutoLock lock(progress_lock_);
+  ProgressMap::iterator it = ui_progress_.begin();
+  for (; it != ui_progress_.end(); ++it) {
+    const int id = it->first;
+    DownloadManager* manager = GetDownloadManager(id);
+    if (manager)
+      manager->UpdateDownload(id, it->second);
   }
 }
 
@@ -144,13 +168,21 @@
   return next_id_++;
 }
 
+// Notifications sent from the IO thread and run on the download thread:
+
+// The IO thread created 'info', but the download thread (this method) uses it
+// to create a DownloadFile, then passes 'info' to the UI thread where it is
+// finally consumed and deleted.
 void DownloadFileManager::StartDownload(DownloadCreateInfo* info) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
   DCHECK(info);
 
-  DownloadManager* manager = DownloadManagerForRenderViewHost(
-      info->child_id, info->render_view_id);
-  if (!manager) {
+  DownloadFile* download = new DownloadFile(info);
+  if (!download->Initialize()) {
+    // Couldn't open, cancel the operation. The UI thread does not yet know
+    // about this download so we have to clean up 'info'. We need to get back
+    // to the IO thread to cancel the network request and CancelDownloadRequest
+    // on the UI thread is the safe way to do that.
     ChromeThread::PostTask(
         ChromeThread::IO, FROM_HERE,
         NewRunnableFunction(&download_util::CancelDownloadRequest,
@@ -158,12 +190,22 @@
                             info->child_id,
                             info->request_id));
     delete info;
+    delete download;
     return;
   }
 
-  ChromeThread::PostTask(ChromeThread::FILE, FROM_HERE,
-      NewRunnableMethod(this, &DownloadFileManager::CreateDownloadFile,
-                        info, manager));
+  DCHECK(GetDownloadFile(info->download_id) == NULL);
+  downloads_[info->download_id] = download;
+  // TODO(phajdan.jr): fix the duplication of path info below.
+  info->path = info->save_info.file_path;
+  {
+    AutoLock lock(progress_lock_);
+    ui_progress_[info->download_id] = info->received_bytes;
+  }
+
+  ChromeThread::PostTask(
+      ChromeThread::UI, FROM_HERE,
+      NewRunnableMethod(this, &DownloadFileManager::OnStartDownload, info));
 }
 
 // We don't forward an update to the UI thread here, since we want to throttle
@@ -179,14 +221,25 @@
     contents.swap(buffer->contents);
   }
 
+  // Keep track of how many bytes we have successfully saved to update
+  // our progress status in the UI.
+  int64 progress_bytes = 0;
+
   DownloadFile* download = GetDownloadFile(id);
   for (size_t i = 0; i < contents.size(); ++i) {
     net::IOBuffer* data = contents[i].first;
     const int data_len = contents[i].second;
-    if (download)
-      download->AppendDataToFile(data->data(), data_len);
+    if (download) {
+      if (download->AppendDataToFile(data->data(), data_len))
+        progress_bytes += data_len;
+    }
     data->Release();
   }
+
+  if (download) {
+    AutoLock lock(progress_lock_);
+    ui_progress_[download->id()] += progress_bytes;
+  }
 }
 
 void DownloadFileManager::DownloadFinished(int id, DownloadBuffer* buffer) {
@@ -197,15 +250,18 @@
     DownloadFile* download = it->second;
     download->Finish();
 
-    DownloadManager* download_manager = download->GetDownloadManager();
-    if (download_manager) {
-      ChromeThread::PostTask(
-          ChromeThread::UI, FROM_HERE,
-          NewRunnableMethod(
-              download_manager, &DownloadManager::DownloadFinished,
-              id, download->bytes_so_far()));
+    int64 download_size = -1;
+    {
+      AutoLock lock(progress_lock_);
+      download_size = ui_progress_[download->id()];
     }
 
+    ChromeThread::PostTask(
+        ChromeThread::UI, FROM_HERE,
+        NewRunnableMethod(
+            this, &DownloadFileManager::OnDownloadFinished,
+            id, download_size));
+
     // We need to keep the download around until the UI thread has finalized
     // the name.
     if (download->path_renamed()) {
@@ -215,7 +271,9 @@
   }
 
   if (downloads_.empty())
-    StopUpdateTimer();
+    ChromeThread::PostTask(
+      ChromeThread::UI, FROM_HERE,
+      NewRunnableMethod(this, &DownloadFileManager::StopUpdateTimer));
 }
 
 // This method will be sent via a user action, or shutdown on the UI thread, and
@@ -228,26 +286,94 @@
     DownloadFile* download = it->second;
     download->Cancel();
 
+    ChromeThread::PostTask(
+        ChromeThread::UI, FROM_HERE,
+        NewRunnableMethod(
+            this, &DownloadFileManager::RemoveDownloadFromUIProgress,
+            download->id()));
+
     if (download->path_renamed()) {
       downloads_.erase(it);
       delete download;
     }
   }
 
-  if (downloads_.empty())
-    StopUpdateTimer();
+  if (downloads_.empty()) {
+    ChromeThread::PostTask(
+      ChromeThread::UI, FROM_HERE,
+      NewRunnableMethod(this, &DownloadFileManager::StopUpdateTimer));
+  }
 }
 
-void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
-  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
+// Relate a download ID to its owning DownloadManager.
+DownloadManager* DownloadFileManager::GetDownloadManager(int download_id) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  DownloadManagerMap::iterator it = managers_.find(download_id);
+  if (it != managers_.end())
+    return it->second;
+  return NULL;
+}
+
+// Utility function for look up table maintenance, called on the UI thread.
+// A manager may have multiple downloads in progress, so we just look up the
+// one download (id) and remove it from the set, and remove the set if it
+// becomes empty.
+void DownloadFileManager::RemoveDownload(int id, DownloadManager* manager) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
+  if (manager) {
+    RequestMap::iterator it = requests_.find(manager);
+    if (it != requests_.end()) {
+      DownloadRequests& downloads = it->second;
+      DownloadRequests::iterator rit = downloads.find(id);
+      if (rit != downloads.end())
+        downloads.erase(rit);
+      if (downloads.empty())
+        requests_.erase(it);
+    }
+  }
+
+  // A download can only have one manager, so remove it if it exists.
+  DownloadManagerMap::iterator dit = managers_.find(id);
+  if (dit != managers_.end())
+    managers_.erase(dit);
+}
+
+// Utility function for converting request IDs to a TabContents. Must be called
+// only on the UI thread since Profile operations may create UI objects, such as
+// the first call to profile->GetDownloadManager().
+// static
+DownloadManager* DownloadFileManager::DownloadManagerFromRenderIds(
+    int render_process_id, int render_view_id) {
+  TabContents* contents = tab_util::GetTabContentsByID(render_process_id,
+                                                       render_view_id);
+  if (contents) {
+    Profile* profile = contents->profile();
+    if (profile)
+      return profile->GetDownloadManager();
+  }
+
+  return NULL;
+}
+
+// Called by DownloadManagers in their destructor, and only on the UI thread.
+void DownloadFileManager::RemoveDownloadManager(DownloadManager* manager) {
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));
   DCHECK(manager);
+  RequestMap::iterator it = requests_.find(manager);
+  if (it == requests_.end())
+    return;
 
-  for (DownloadFileMap::iterator i = downloads_.begin();
-       i != downloads_.end(); ++i) {
-    DownloadFile* download_file = i->second;
-    if (download_file->GetDownloadManager() == manager)
-      download_file->OnDownloadManagerShutdown();
+  const DownloadRequests& requests = it->second;
+  DownloadRequests::const_iterator i = requests.begin();
+  for (; i != requests.end(); ++i) {
+    DownloadManagerMap::iterator dit = managers_.find(*i);
+    if (dit != managers_.end()) {
+      DCHECK(dit->second == manager);
+      managers_.erase(dit);
+    }
   }
+
+  requests_.erase(it);
 }
 
 // Actions from the UI thread and run on the download thread
@@ -346,8 +472,11 @@
     delete download;
   }
 
-  if (downloads_.empty())
-    StopUpdateTimer();
+  if (downloads_.empty()) {
+    ChromeThread::PostTask(
+      ChromeThread::UI, FROM_HERE,
+      NewRunnableMethod(this, &DownloadFileManager::StopUpdateTimer));
+  }
 }
 
 // Called only from OnFinalDownloadName or OnIntermediateDownloadName
@@ -359,14 +488,13 @@
   if (!download)
     return;
 
-  DownloadManager* download_manager = download->GetDownloadManager();
-  if (!download_manager) {
+  DownloadManagerMap::iterator dmit = managers_.find(download->id());
+  if (dmit != managers_.end()) {
+    DownloadManager* dlm = dmit->second;
+    ChromeThread::PostTask(
+        ChromeThread::UI, FROM_HERE,
+        NewRunnableMethod(dlm, &DownloadManager::DownloadCancelled, id));
+  } else {
     download->CancelDownloadRequest(resource_dispatcher_host_);
-    return;
   }
-
-  ChromeThread::PostTask(
-      ChromeThread::UI, FROM_HERE,
-      NewRunnableMethod(download_manager,
-                        &DownloadManager::DownloadCancelled, id));
 }