sql::MetaTable.next_download_id, DownloadManager::GetNextId()

chrome/browser/download/download_file_manager.cc

Index: chrome/browser/download/download_file_manager.cc
diff --git a/chrome/browser/download/download_file_manager.cc b/chrome/browser/download/download_file_manager.cc
index ac0c55e77280f179710eebf67d64d808f3d7c601..4b4b17d8cb1fe97990247559d09103c068ebcc56 100644
--- a/chrome/browser/download/download_file_manager.cc
+++ b/chrome/browser/download/download_file_manager.cc
@@ -37,8 +37,7 @@ const int kUpdatePeriodMs = 500;
 }  // namespace
 
 DownloadFileManager::DownloadFileManager(ResourceDispatcherHost* rdh)
-    : next_id_(0),
-      resource_dispatcher_host_(rdh) {
+    : resource_dispatcher_host_(rdh) {
 }
 
 DownloadFileManager::~DownloadFileManager() {
@@ -75,7 +74,7 @@ void DownloadFileManager::CreateDownloadFile(DownloadCreateInfo* info,
     return;
   }
 
-  int32 id = info->download_id;
+  DownloadId id(download_manager, info->download_id);
   DCHECK(GetDownloadFile(id) == NULL);
   downloads_[id] = download_file.release();
 
@@ -87,10 +86,10 @@ void DownloadFileManager::CreateDownloadFile(DownloadCreateInfo* info,
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(download_manager,
-                        &DownloadManager::StartDownload, id));
+                        &DownloadManager::StartDownload, id.id()));

[Randy Smith (Not in Mondays), 2011/07/25 20:20:12]

nit: I'd feel somewhat better if we distinguished in variable names between
global and per-profile ids, at least for the cases where it's not really obvious
which one we're using within a function.  Either global_id, local_id, or
global_id, id, or id, local_id would all be fine.  But id.id() is one of those
confusing idioms I think it's worth avoiding.

(Throughout CL.)

[benjhayden, 2011/07/27 19:40:54]

PTAL

 }
 
-DownloadFile* DownloadFileManager::GetDownloadFile(int id) {
+DownloadFile* DownloadFileManager::GetDownloadFile(DownloadId id) {
   DownloadFileMap::iterator it = downloads_.find(id);
   return it == downloads_.end() ? NULL : it->second;
 }
@@ -112,24 +111,17 @@ void DownloadFileManager::UpdateInProgressDownloads() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   for (DownloadFileMap::iterator i = downloads_.begin();
        i != downloads_.end(); ++i) {
-    int id = i->first;
+    DownloadId id = i->first;
     DownloadFile* download_file = i->second;
     DownloadManager* manager = download_file->GetDownloadManager();
     if (manager) {
       BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
           NewRunnableMethod(manager, &DownloadManager::UpdateDownload,
-                            id, download_file->bytes_so_far()));
+                            id.id(), download_file->bytes_so_far()));
     }
   }
 }
 
-// Called on the IO thread once the ResourceDispatcherHost has decided that a
-// request is a download.
-int DownloadFileManager::GetNextId() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  return next_id_++;
-}
-
 void DownloadFileManager::StartDownload(DownloadCreateInfo* info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(info);
@@ -164,7 +156,8 @@ void DownloadFileManager::StartDownload(DownloadCreateInfo* info) {
 // download (in the UI thread), we may receive a few more updates before the IO
 // thread gets the cancel message: we just delete the data since the
 // DownloadFile has been deleted.
-void DownloadFileManager::UpdateDownload(int id, DownloadBuffer* buffer) {
+void DownloadFileManager::UpdateDownload(
+    DownloadId id, DownloadBuffer* buffer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   std::vector<DownloadBuffer::Contents> contents;
   {
@@ -183,11 +176,11 @@ void DownloadFileManager::UpdateDownload(int id, DownloadBuffer* buffer) {
 }
 
 void DownloadFileManager::OnResponseCompleted(
-    int id,
+    DownloadId id,
     DownloadBuffer* buffer,
     int os_error,
     const std::string& security_info) {
-  VLOG(20) << __FUNCTION__ << "()" << " id = " << id
+  VLOG(20) << __FUNCTION__ << "()" << " id = " << id.id()
            << " os_error = " << os_error
            << " security_info = \"" << security_info << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ -212,7 +205,7 @@ void DownloadFileManager::OnResponseCompleted(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(
         download_manager, &DownloadManager::OnResponseCompleted,
-        id, download_file->bytes_so_far(), os_error, hash));
+        id.id(), download_file->bytes_so_far(), os_error, hash));
   // We need to keep the download around until the UI thread has finalized
   // the name.
 }
@@ -220,8 +213,8 @@ void DownloadFileManager::OnResponseCompleted(
 // This method will be sent via a user action, or shutdown on the UI thread, and
 // run on the download thread. Since this message has been sent from the UI
 // thread, the download may have already completed and won't exist in our map.
-void DownloadFileManager::CancelDownload(int id) {
-  VLOG(20) << __FUNCTION__ << "()" << " id = " << id;
+void DownloadFileManager::CancelDownload(DownloadId id) {
+  VLOG(20) << __FUNCTION__ << "()" << " id = " << id.id();

[Randy Smith (Not in Mondays), 2011/07/25 20:20:12]

nit: Reducing the download id down to a local id for logging loses information. 
  I'd replace with both local id and download manager pointer.  (Thoughout file)

[benjhayden, 2011/07/27 19:40:54]

PTAL.

   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   DownloadFileMap::iterator it = downloads_.find(id);
   if (it == downloads_.end())
@@ -235,7 +228,7 @@ void DownloadFileManager::CancelDownload(int id) {
   EraseDownload(id);
 }
 
-void DownloadFileManager::CompleteDownload(int id) {
+void DownloadFileManager::CompleteDownload(DownloadId id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   if (!ContainsKey(downloads_, id))
@@ -244,7 +237,7 @@ void DownloadFileManager::CompleteDownload(int id) {
   DownloadFile* download_file = downloads_[id];
 
   VLOG(20) << " " << __FUNCTION__ << "()"
-           << " id = " << id
+           << " id = " << id.id()
            << " download_file = " << download_file->DebugString();
 
   download_file->Detach();
@@ -269,7 +262,7 @@ void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
 
   for (std::set<DownloadFile*>::iterator i = to_remove.begin();
        i != to_remove.end(); ++i) {
-    downloads_.erase((*i)->id());
+    downloads_.erase(DownloadId((*i)->GetDownloadManager(), (*i)->id()));
     delete *i;
   }
 }
@@ -283,8 +276,8 @@ void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
 // 1. tmp -> foo.crdownload (not final, safe)
 // 2. tmp-> Unconfirmed.xxx.crdownload (not final, dangerous)
 void DownloadFileManager::RenameInProgressDownloadFile(
-    int id, const FilePath& full_path) {
-  VLOG(20) << __FUNCTION__ << "()" << " id = " << id
+    DownloadId id, const FilePath& full_path) {
+  VLOG(20) << __FUNCTION__ << "()" << " id = " << id.id()
            << " full_path = \"" << full_path.value() << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
@@ -310,8 +303,8 @@ void DownloadFileManager::RenameInProgressDownloadFile(
 // 1. foo.crdownload -> foo (final, safe)
 // 2. Unconfirmed.xxx.crdownload -> xxx (final, validated)
 void DownloadFileManager::RenameCompletingDownloadFile(
-    int id, const FilePath& full_path, bool overwrite_existing_file) {
-  VLOG(20) << __FUNCTION__ << "()" << " id = " << id
+    DownloadId id, const FilePath& full_path, bool overwrite_existing_file) {
+  VLOG(20) << __FUNCTION__ << "()" << " id = " << id.id()
            << " overwrite_existing_file = " << overwrite_existing_file
            << " full_path = \"" << full_path.value() << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ -356,16 +349,14 @@ void DownloadFileManager::RenameCompletingDownloadFile(
   download_file->AnnotateWithSourceInformation();
 #endif
 
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      NewRunnableMethod(
-          download_manager, &DownloadManager::OnDownloadRenamedToFinalName, id,
-          new_path, uniquifier));
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, NewRunnableMethod(
+      download_manager, &DownloadManager::OnDownloadRenamedToFinalName,
+      id.id(), new_path, uniquifier));
 }
 
 // Called only from RenameInProgressDownloadFile and
 // RenameCompletingDownloadFile on the FILE thread.
-void DownloadFileManager::CancelDownloadOnRename(int id) {
+void DownloadFileManager::CancelDownloadOnRename(DownloadId id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   DownloadFile* download_file = GetDownloadFile(id);
@@ -381,13 +372,11 @@ void DownloadFileManager::CancelDownloadOnRename(int id) {
     return;
   }
 
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      NewRunnableMethod(download_manager,
-                        &DownloadManager::DownloadCancelled, id));
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, NewRunnableMethod(
+      download_manager, &DownloadManager::DownloadCancelled, id.id()));
 }
 
-void DownloadFileManager::EraseDownload(int id) {
+void DownloadFileManager::EraseDownload(DownloadId id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   if (!ContainsKey(downloads_, id))
@@ -396,7 +385,7 @@ void DownloadFileManager::EraseDownload(int id) {
   DownloadFile* download_file = downloads_[id];
 
   VLOG(20) << " " << __FUNCTION__ << "()"
-           << " id = " << id
+           << " id = " << id.id()
            << " download_file = " << download_file->DebugString();
 
   downloads_.erase(id);