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

chrome/browser/download/download_manager.cc

 // 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.
 
 #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"
 #include "base/logging.h"
 #include "base/path_service.h"
 #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"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_create_info.h"
 #include "chrome/browser/download/download_extensions.h"
 #include "chrome/browser/download/download_file_manager.h"
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_item.h"
@@ skipping 20 matching lines @@
 #include "googleurl/src/gurl.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
 #include "net/base/mime_util.h"
 #include "net/base/net_util.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 
 DownloadManager::DownloadManager(DownloadStatusUpdater* status_updater)
     : shutdown_needed_(false),
+      next_id_(-1),
       profile_(NULL),
       file_manager_(NULL),
       status_updater_(status_updater->AsWeakPtr()) {
   if (status_updater_)
     status_updater_->AddDelegate(this);
 }
 
 DownloadManager::~DownloadManager() {
   DCHECK(!shutdown_needed_);
   if (status_updater_)
@@ skipping 150 matching lines @@
     if (download_item->MatchesQuery(query_lower))
       result->push_back(download_item);
   }
 
   // If we have a parent profile, let it add its downloads to the results.
   Profile* original_profile = profile_->GetOriginalProfile();
   if (original_profile != profile_)
     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);
   DCHECK(!shutdown_needed_)  << "DownloadManager already initialized.";
   shutdown_needed_ = true;
 
   profile_ = profile;
   download_history_.reset(new DownloadHistory(profile));
+  download_history_->GetNextId(NewCallback(
+      this, &DownloadManager::OnHistoryGetNextId));
   download_history_->Load(
       NewCallback(this, &DownloadManager::OnQueryDownloadEntriesComplete));
 
   download_prefs_.reset(new DownloadPrefs(profile_->GetPrefs()));
 
   // In test mode, there may be no ResourceDispatcherHost.  In this case it's
   // safe to avoid setting |file_manager_| because we only call a small set of
   // functions, none of which need it.
   ResourceDispatcherHost* rdh = g_browser_process->resource_dispatcher_host();
   if (rdh) {
@@ skipping 359 matching lines @@
     // name after user confirmation will be set from
     // DownloadItem::OnDownloadCompleting.
     download_path =
         download_util::GetCrDownloadPath(download->full_path());
   }
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(
           file_manager_, &DownloadFileManager::RenameInProgressDownloadFile,
-          download->id(), download_path));
+          download->gid(), download_path));
 
   download->Rename(download_path);
 
   download_history_->AddEntry(download,
       NewCallback(this, &DownloadManager::OnCreateDownloadEntryComplete));
 }
 
 void DownloadManager::UpdateDownload(int32 download_id, int64 size) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadMap::iterator it = active_downloads_.find(download_id);
@@ skipping 129 matching lines @@
   // file rename, and UI display) then it's not ready for completion.
   if (download->db_handle() == DownloadHistory::kUninitializedHandle)
     return false;
 
   return true;
 }
 
 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;
 
   // TODO(rdsmith): DCHECK that we only pass through this point
   // once per download.  The natural way to do this is by a state
   // transition on the DownloadItem.
 
   // Confirm we're in the proper set of states to be here;
   // in in_progress_, have all data, have a history handle, (validated or safe).
   DCHECK_NE(DownloadItem::DANGEROUS, download->safety_state());
   DCHECK_EQ(1u, in_progress_.count(download->id()));
   DCHECK(download->all_data_saved());
   DCHECK(download->db_handle() != DownloadHistory::kUninitializedHandle);
   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());
   UpdateAppIcon();  // Reflect removal from in_progress_.
 
   download_history_->UpdateEntry(download);
 
   // Finish the download.
   download->OnDownloadCompleting(file_manager_);
 }
@@ skipping 18 matching lines @@
   if (!item)
     return;
 
   if (item->safety_state() == DownloadItem::SAFE) {
     DCHECK_EQ(0, uniquifier) << "We should not uniquify SAFE downloads twice";
   }
 
   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);
 
   item->OnDownloadRenamedToFinalName(full_path);
   download_history_->UpdateDownloadPath(item, full_path);
 }
 
 void DownloadManager::DownloadCancelled(int32 download_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ skipping 15 matching lines @@
   }
 
   DownloadCancelledInternal(download_id, download->request_handle());
 }
 
 void DownloadManager::DownloadCancelledInternal(
     int download_id, const DownloadRequestHandle& request_handle) {
   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,
                                       int64 size,
                                       int os_error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadMap::iterator it = active_downloads_.find(download_id);
   // A cancel at the right time could remove the download from the
   // |active_downloads_| map before we get here.
   if (it == active_downloads_.end())
@@ skipping 12 matching lines @@
   //
   // Clean up will happen when the history system create callback runs if we
   // don't have a valid db_handle yet.
   if (download->db_handle() != DownloadHistory::kUninitializedHandle) {
     in_progress_.erase(download_id);
     active_downloads_.erase(download_id);
     UpdateAppIcon();  // Reflect removal from in_progress_.
     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() {
   if (status_updater_)
     status_updater_->Update();
 }
 
 void DownloadManager::RemoveDownload(int64 download_handle) {
   DownloadMap::iterator it = history_downloads_.find(download_handle);
   if (it == history_downloads_.end())
@@ skipping 441 matching lines @@
     observed_download_manager_->RemoveObserver(this);
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ModelChanged() {
   observing_download_manager_->NotifyModelChanged();
 }
 
 void DownloadManager::OtherDownloadManagerObserver::ManagerGoingDown() {
   observed_download_manager_ = NULL;
 }