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

chrome/browser/download/download_manager.cc

Index: chrome/browser/download/download_manager.cc
diff --git a/chrome/browser/download/download_manager.cc b/chrome/browser/download/download_manager.cc
index 54c21b40bd71f8a92641015466203f6d15c664cd..465f3913e1f57fe51530c53d3a6068e8ceda5003 100644
--- a/chrome/browser/download/download_manager.cc
+++ b/chrome/browser/download/download_manager.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/download/download_manager.h"
 
+#include "base/bind.h"
 #include "base/callback.h"
 #include "base/file_util.h"
 #include "base/i18n/case_conversion.h"
@@ -12,6 +13,7 @@
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/stringprintf.h"
+#include "base/synchronization/lock.h"
 #include "base/sys_string_conversions.h"
 #include "base/task.h"
 #include "base/utf_string_conversions.h"
@@ -52,6 +54,7 @@
 
 DownloadManager::DownloadManager(DownloadStatusUpdater* status_updater)
     : shutdown_needed_(false),
+      next_id_(-1),
       profile_(NULL),
       file_manager_(NULL),
       status_updater_(status_updater->AsWeakPtr()) {
@@ -222,6 +225,22 @@ void DownloadManager::SearchDownloads(const string16& query,
     original_profile->GetDownloadManager()->SearchDownloads(query, result);
 }
 
+void DownloadManager::OnHistoryGetNextId(int next_id) {
+  DVLOG(1) << __FUNCTION__ << " " << next_id;
+  next_id_ = next_id;
+}
+
+DownloadId DownloadManager::GetNextId() {
+  // May be called on any thread via the GetNextIdThunk.
+  DCHECK(next_id_ >= 0) << "Uh oh, OnHistoryGetNextId hasn't happened yet!";

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

I see nothing architectural preventing this DCHECK() from tripping, and I think
there has to be (i.e. we have to have some guarantee that downloads won't be
initiated for this DM until the id is set).  I think this means we have to delay
completion of profile initialization until this value has been set.  

One way to do that would be to:
* Hold off profile initialization until this round trip (see the ProfileImpl
constructor and OnPrefsLoaded() in that file for the current async profile
initialization; you'd have to hook into that).
* Construct the download manager on profile construction (possibly just passing
in the id?)
* Call init on first use.

But there may be other ways to do it--that's just one possibility.  Still, this
is a key problem.  

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

I think we decided to do this in the next CL.

+  base::AutoLock _(next_id_lock_);
+  return DownloadId(this, next_id_++);
+}
+
+base::Callback<DownloadId(void)> DownloadManager::GetNextIdThunk() {
+  return base::Bind(&DownloadManager::GetNextId, this);
+}
+
 // Query the history service for information about all persisted downloads.
 bool DownloadManager::Init(Profile* profile) {
   DCHECK(profile);
@@ -230,6 +249,8 @@ bool DownloadManager::Init(Profile* profile) {
 
   profile_ = profile;
   download_history_.reset(new DownloadHistory(profile));
+  download_history_->GetNextId(NewCallback(
+      this, &DownloadManager::OnHistoryGetNextId));
   download_history_->Load(
       NewCallback(this, &DownloadManager::OnQueryDownloadEntriesComplete));
 
@@ -609,7 +630,7 @@ void DownloadManager::ContinueDownloadWithPath(DownloadItem* download,
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(
           file_manager_, &DownloadFileManager::RenameInProgressDownloadFile,
-          download->id(), download_path));
+          download->gid(), download_path));
 
   download->Rename(download_path);
 
@@ -759,7 +780,7 @@ bool DownloadManager::IsDownloadReadyForCompletion(DownloadItem* download) {
 void DownloadManager::MaybeCompleteDownload(DownloadItem* download) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   VLOG(20) << __FUNCTION__ << "()" << " download = "
-           << download->DebugString(false);
+           << download->DebugString(true);

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

Why change the logging?  I don't particularly care, but I do believe that in
most cases logging should be kept minimal in the main tree and cranked up in
developers checkouts unless there's some particular reason to believe the
specific piece of logging added will be useful in the general case.

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

Done.

 
   if (!IsDownloadReadyForCompletion(download))
     return;
@@ -777,7 +798,7 @@ void DownloadManager::MaybeCompleteDownload(DownloadItem* download) {
   DCHECK_EQ(1u, history_downloads_.count(download->db_handle()));
 
   VLOG(20) << __FUNCTION__ << "()" << " executing: download = "
-           << download->DebugString(false);
+           << download->DebugString(true);
 
   // Remove the id from in_progress
   in_progress_.erase(download->id());
@@ -816,7 +837,7 @@ void DownloadManager::OnDownloadRenamedToFinalName(int download_id,
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(
-          file_manager_, &DownloadFileManager::CompleteDownload, download_id));
+          file_manager_, &DownloadFileManager::CompleteDownload, item->gid()));
 
   if (uniquifier)
     item->set_path_uniquifier(uniquifier);
@@ -852,10 +873,9 @@ void DownloadManager::DownloadCancelledInternal(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   request_handle.CancelRequest();
 
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager_, &DownloadFileManager::CancelDownload, download_id));
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, NewRunnableMethod(
+      file_manager_, &DownloadFileManager::CancelDownload,
+      DownloadId(this, download_id)));
 }
 
 void DownloadManager::OnDownloadError(int32 download_id,
@@ -888,10 +908,8 @@ void DownloadManager::OnDownloadError(int32 download_id,
     download_history_->UpdateEntry(download);
   }
 
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      NewRunnableMethod(
-          file_manager_, &DownloadFileManager::CancelDownload, download_id));
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, NewRunnableMethod(
+      file_manager_, &DownloadFileManager::CancelDownload, download->gid()));
 }
 
 void DownloadManager::UpdateAppIcon() {