Merge 8401001 r107836 into branch 912: Fix history importing by delaying DownloadManager creation. Replace GetNextIdThunkType with DownloadIdFactory (RefCountedThreadSafe, created by DownloadService).

DownloadService uses the same DownloadIdFactory for an OTR profile as its original profile.

DownloadService passes the DownloadIdFactory into the DownloadManager so that the DownloadManager can allocate new valid ids for items loaded from the history or downloads started on the ui thread.

Since the DownloadService precedes and outlives its DownloadManager, DownloadManager does not have a scoped_refptr<DownloadIdFactory>. Objects that do have a scoped_refptr<DownloadIdFactory>: DownloadService, ProfileIOData, ShellBrowserContext, ShellResourceContext.

The DownloadIdFactory must be RefCountedThreadSafe because ProfileIOData outlives Profile and because it's used in both the OTR and original profiles.

Longer term, the import process should strictly precede profile initialization, and the next_download_id counter should be loaded from the History db strictly before DownloadService is created and creates a DownloadIdFactory.

BUG=98966
Review URL: http://codereview.chromium.org/8372073

git-svn-id: svn://svn.chromium.org/chrome/branches/912/src@108346 0039d316-1c4b-4281-b951-d872f2087c98

33 files changed, 224 insertions, 162 deletions

diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc
index 4c06ccc..ca5718b 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -63,9 +63,6 @@ void ChromeDownloadManagerDelegate::SetDownloadManager(DownloadManager* dm) {
   download_history_->Load(
       base::Bind(&DownloadManager::OnPersistentStoreQueryComplete,
                  base::Unretained(dm)));
-  download_history_->GetNextId(
-      base::Bind(&DownloadManager::OnPersistentStoreGetNextId,
-                 base::Unretained(dm)));
 }
 
 void ChromeDownloadManagerDelegate::Shutdown() {
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index fbdd369..fefd33c 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -26,6 +26,7 @@
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file.h"
 #include "content/browser/download/download_file_manager.h"
+#include "content/browser/download/download_id_factory.h"
 #include "content/browser/download/download_item.h"
 #include "content/browser/download/download_manager.h"
 #include "content/browser/download/download_status_updater.h"
@@ -40,6 +41,8 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/text/bytes_formatting.h"
 
+DownloadId::Domain kValidIdDomain = "valid DownloadId::Domain";
+
 class DownloadManagerTest : public testing::Test {
  public:
   static const char* kTestData;
@@ -49,8 +52,11 @@ class DownloadManagerTest : public testing::Test {
       : profile_(new TestingProfile()),
         download_manager_delegate_(new ChromeDownloadManagerDelegate(
             profile_.get())),
+        id_factory_(new DownloadIdFactory(kValidIdDomain)),
         download_manager_(new MockDownloadManager(
-            download_manager_delegate_, &download_status_updater_)),
+            download_manager_delegate_,
+            id_factory_.get(),
+            &download_status_updater_)),
         ui_thread_(BrowserThread::UI, &message_loop_),
         file_thread_(BrowserThread::FILE, &message_loop_) {
     download_manager_->Init(profile_.get());
@@ -68,7 +74,7 @@ class DownloadManagerTest : public testing::Test {
   }
 
   void AddDownloadToFileManager(int id, DownloadFile* download_file) {
-    file_manager()->downloads_[DownloadId(download_manager_.get(), id)] =
+    file_manager()->downloads_[DownloadId(kValidIdDomain, id)] =
       download_file;
   }
 
@@ -102,7 +108,7 @@ class DownloadManagerTest : public testing::Test {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         base::Bind(&DownloadFileManager::UpdateDownload, file_manager_.get(),
-                   DownloadId(download_manager_.get(), id), &download_buffer_));
+                   DownloadId(kValidIdDomain, id), &download_buffer_));
 
     message_loop_.RunAllPending();
   }
@@ -123,6 +129,7 @@ class DownloadManagerTest : public testing::Test {
   DownloadStatusUpdater download_status_updater_;
   scoped_ptr<TestingProfile> profile_;
   scoped_refptr<ChromeDownloadManagerDelegate> download_manager_delegate_;
+  scoped_refptr<DownloadIdFactory> id_factory_;
   scoped_refptr<DownloadManager> download_manager_;
   scoped_refptr<DownloadFileManager> file_manager_;
   MessageLoopForUI message_loop_;
@@ -384,7 +391,7 @@ TEST_F(DownloadManagerTest, StartDownload) {
     // responsible for deleting it.  In these unit tests, however, we
     // don't call the function that deletes it, so we do so ourselves.
     scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-    info->download_id = static_cast<int>(i);
+    info->download_id = DownloadId(kValidIdDomain, static_cast<int>(i));
     info->prompt_user_for_save_location = kStartDownloadCases[i].save_as;
     info->url_chain.push_back(GURL(kStartDownloadCases[i].url));
     info->mime_type = kStartDownloadCases[i].mime_type;
@@ -392,9 +399,9 @@ TEST_F(DownloadManagerTest, StartDownload) {
 
     DownloadFile* download_file(
         new DownloadFile(info.get(), download_manager_));
-    AddDownloadToFileManager(info->download_id, download_file);
+    AddDownloadToFileManager(info->download_id.local(), download_file);
     download_file->Initialize(false);
-    download_manager_->StartDownload(info->download_id);
+    download_manager_->StartDownload(info->download_id.local());
     message_loop_.RunAllPending();
 
     // SelectFileObserver will have recorded any attempt to open the
@@ -416,14 +423,14 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) {
     // responsible for deleting it.  In these unit tests, however, we
     // don't call the function that deletes it, so we do so ourselves.
     scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-    info->download_id = static_cast<int>(i);
+    info->download_id = DownloadId(kValidIdDomain, static_cast<int>(i));
     info->prompt_user_for_save_location = false;
     info->url_chain.push_back(GURL());
     const FilePath new_path(kDownloadRenameCases[i].suggested_path);
 
     MockDownloadFile* download_file(
         new MockDownloadFile(info.get(), download_manager_));
-    AddDownloadToFileManager(info->download_id, download_file);
+    AddDownloadToFileManager(info->download_id.local(), download_file);
 
     // |download_file| is owned by DownloadFileManager.
     ::testing::Mock::AllowLeak(download_file);
@@ -479,7 +486,7 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
   // responsible for deleting it.  In these unit tests, however, we
   // don't call the function that deletes it, so we do so ourselves.
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  info->download_id = static_cast<int>(0);
+  info->download_id = DownloadId(kValidIdDomain, static_cast<int>(0));
   info->prompt_user_for_save_location = false;
   info->url_chain.push_back(GURL());
   info->total_bytes = static_cast<int64>(kTestDataLen);
@@ -488,7 +495,7 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
 
   MockDownloadFile* download_file(
       new MockDownloadFile(info.get(), download_manager_));
-  AddDownloadToFileManager(info->download_id, download_file);
+  AddDownloadToFileManager(info->download_id.local(), download_file);
 
   // |download_file| is owned by DownloadFileManager.
   ::testing::Mock::AllowLeak(download_file);
@@ -570,7 +577,7 @@ TEST_F(DownloadManagerTest, DownloadFileErrorTest) {
   // don't call the function that deletes it, so we do so ourselves.
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
   int32 id = 0;
-  info->download_id = id;
+  info->download_id = DownloadId(kValidIdDomain, id);
   info->prompt_user_for_save_location = false;
   info->url_chain.push_back(GURL());
   info->total_bytes = static_cast<int64>(kTestDataLen * 3);
@@ -648,7 +655,7 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) {
   // responsible for deleting it.  In these unit tests, however, we
   // don't call the function that deletes it, so we do so ourselves.
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  info->download_id = static_cast<int>(0);
+  info->download_id = DownloadId(kValidIdDomain, static_cast<int>(0));
   info->prompt_user_for_save_location = false;
   info->url_chain.push_back(GURL());
   const FilePath new_path(FILE_PATH_LITERAL("foo.zip"));
@@ -656,7 +663,7 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) {
 
   MockDownloadFile* download_file(
       new MockDownloadFile(info.get(), download_manager_));
-  AddDownloadToFileManager(info->download_id, download_file);
+  AddDownloadToFileManager(info->download_id.local(), download_file);
 
   // |download_file| is owned by DownloadFileManager.
   ::testing::Mock::AllowLeak(download_file);
@@ -732,7 +739,7 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
   // responsible for deleting it.  In these unit tests, however, we
   // don't call the function that deletes it, so we do so ourselves.
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  info->download_id = static_cast<int>(0);
+  info->download_id = DownloadId(kValidIdDomain, static_cast<int>(0));
   info->prompt_user_for_save_location = true;
   info->url_chain.push_back(GURL());
 
@@ -756,7 +763,7 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
   // This creates the .crdownload version of the file.
   download_file->Initialize(false);
   // |download_file| is owned by DownloadFileManager.
-  AddDownloadToFileManager(info->download_id, download_file);
+  AddDownloadToFileManager(info->download_id.local(), download_file);
 
   ContinueDownloadWithPath(download, new_path);
   message_loop_.RunAllPending();
@@ -808,7 +815,7 @@ TEST_F(DownloadManagerTest, DownloadRemoveTest) {
   // responsible for deleting it.  In these unit tests, however, we
   // don't call the function that deletes it, so we do so ourselves.
   scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
-  info->download_id = static_cast<int>(0);
+  info->download_id = DownloadId(kValidIdDomain, static_cast<int>(0));
   info->prompt_user_for_save_location = true;
   info->url_chain.push_back(GURL());
 
@@ -832,7 +839,7 @@ TEST_F(DownloadManagerTest, DownloadRemoveTest) {
   // This creates the .crdownload version of the file.
   download_file->Initialize(false);
   // |download_file| is owned by DownloadFileManager.
-  AddDownloadToFileManager(info->download_id, download_file);
+  AddDownloadToFileManager(info->download_id.local(), download_file);
 
   ContinueDownloadWithPath(download, new_path);
   message_loop_.RunAllPending();
diff --git a/chrome/browser/download/download_service.cc b/chrome/browser/download/download_service.cc
index 659af4f..3e1df85 100644
--- a/chrome/browser/download/download_service.cc
+++ b/chrome/browser/download/download_service.cc
@@ -6,15 +6,28 @@
 
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/chrome_download_manager_delegate.h"
+#include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "content/browser/download/download_id_factory.h"
 #include "content/browser/download/download_manager.h"
 
 DownloadService::DownloadService(Profile* profile)
     : download_manager_created_(false),
-      profile_(profile) {}
+      profile_(profile) {
+  if (profile_->IsOffTheRecord()) {
+    id_factory_ = DownloadServiceFactory::GetForProfile(
+        profile_->GetOriginalProfile())->GetDownloadIdFactory();
+  } else {
+    id_factory_ = new DownloadIdFactory(this);
+  }
+}
 
 DownloadService::~DownloadService() {}
 
+DownloadIdFactory* DownloadService::GetDownloadIdFactory() const {
+  return id_factory_.get();
+}
+
 DownloadManager* DownloadService::GetDownloadManager() {
   if (!download_manager_created_) {
     // In case the delegate has already been set by
@@ -22,7 +35,9 @@ DownloadManager* DownloadService::GetDownloadManager() {
     if (!manager_delegate_.get())
       manager_delegate_ = new ChromeDownloadManagerDelegate(profile_);
     manager_ = new DownloadManager(
-        manager_delegate_.get(), g_browser_process->download_status_updater());
+        manager_delegate_.get(),
+        id_factory_.get(),
+        g_browser_process->download_status_updater());
     manager_->Init(profile_);
     manager_delegate_->SetDownloadManager(manager_);
     download_manager_created_ = true;
diff --git a/chrome/browser/download/download_service.h b/chrome/browser/download/download_service.h
index 4d754b0..d321106 100644
--- a/chrome/browser/download/download_service.h
+++ b/chrome/browser/download/download_service.h
@@ -14,6 +14,7 @@
 class ChromeDownloadManagerDelegate;
 class DownloadManager;
 class Profile;
+class DownloadIdFactory;
 
 // Owning class for DownloadManager (content) and
 // ChromeDownloadManagerDelegate (chrome)
@@ -22,6 +23,8 @@ class DownloadService : public ProfileKeyedService {
   explicit DownloadService(Profile* profile);
   virtual ~DownloadService();
 
+  DownloadIdFactory* GetDownloadIdFactory() const;
+
   // Get the download manager.  Creates the download manager if
   // it does not already exist.
   DownloadManager* GetDownloadManager();
@@ -40,6 +43,8 @@ class DownloadService : public ProfileKeyedService {
   virtual void Shutdown() OVERRIDE;
 
  private:
+  scoped_refptr<DownloadIdFactory> id_factory_;
+
   bool download_manager_created_;
   Profile* profile_;
 
diff --git a/chrome/browser/profiles/profile_io_data.cc b/chrome/browser/profiles/profile_io_data.cc
index 95af1dd..168da6b 100644
--- a/chrome/browser/profiles/profile_io_data.cc
+++ b/chrome/browser/profiles/profile_io_data.cc
@@ -17,6 +17,8 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry.h"
+#include "chrome/browser/download/download_service.h"
+#include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/extensions/extension_info_map.h"
 #include "chrome/browser/extensions/extension_protocols.h"
 #include "chrome/browser/io_thread.h"
@@ -42,6 +44,7 @@
 #include "content/browser/appcache/chrome_appcache_service.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/chrome_blob_storage_context.h"
+#include "content/browser/download/download_id_factory.h"
 #include "content/browser/host_zoom_map.h"
 #include "content/browser/renderer_host/media/media_stream_manager.h"
 #include "content/browser/renderer_host/resource_dispatcher_host.h"
@@ -186,7 +189,8 @@ void ProfileIOData::InitializeOnUIThread(Profile* profile) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   PrefService* pref_service = profile->GetPrefs();
 
-  next_download_id_thunk_ = profile->GetDownloadManager()->GetNextIdThunk();
+  download_id_factory_ = DownloadServiceFactory::GetForProfile(profile)->
+    GetDownloadIdFactory();
 
   scoped_ptr<ProfileParams> params(new ProfileParams);
   params->path = profile->GetPath();
@@ -500,7 +504,7 @@ void ProfileIOData::LazyInitialize() const {
   resource_context_.SetUserData(NULL, const_cast<ProfileIOData*>(this));
   resource_context_.set_media_observer(
       io_thread_globals->media.media_internals.get());
-  resource_context_.set_next_download_id_thunk(next_download_id_thunk_);
+  resource_context_.set_download_id_factory(download_id_factory_);
   resource_context_.set_media_stream_manager(media_stream_manager_.get());
 
   LazyInitializeInternal(profile_params_.get());
diff --git a/chrome/browser/profiles/profile_io_data.h b/chrome/browser/profiles/profile_io_data.h
index 9787647..8b3d595 100644
--- a/chrome/browser/profiles/profile_io_data.h
+++ b/chrome/browser/profiles/profile_io_data.h
@@ -16,7 +16,6 @@
 #include "base/synchronization/lock.h"
 #include "chrome/browser/net/chrome_url_request_context.h"
 #include "chrome/browser/prefs/pref_member.h"
-#include "content/browser/download/download_manager.h"
 #include "content/browser/resource_context.h"
 #include "net/base/cookie_monster.h"
 
@@ -24,6 +23,7 @@ class CommandLine;
 class ChromeAppCacheService;
 class ChromeBlobStorageContext;
 class DesktopNotificationService;
+class DownloadIdFactory;
 class ExtensionInfoMap;
 class HostContentSettingsMap;
 class HostZoomMap;
@@ -294,7 +294,7 @@ class ProfileIOData {
   mutable scoped_refptr<fileapi::FileSystemContext> file_system_context_;
   mutable scoped_refptr<quota::QuotaManager> quota_manager_;
   mutable scoped_refptr<HostZoomMap> host_zoom_map_;
-  mutable DownloadManager::GetNextIdThunkType next_download_id_thunk_;
+  mutable scoped_refptr<DownloadIdFactory> download_id_factory_;
   mutable scoped_ptr<media_stream::MediaStreamManager> media_stream_manager_;
 
   // TODO(willchan): Remove from ResourceContext.
diff --git a/content/browser/download/download_create_info.cc b/content/browser/download/download_create_info.cc
index 2fd52f9..3a183d5 100644
--- a/content/browser/download/download_create_info.cc
+++ b/content/browser/download/download_create_info.cc
@@ -15,7 +15,7 @@ DownloadCreateInfo::DownloadCreateInfo(const FilePath& path,
                                        int64 received_bytes,
                                        int64 total_bytes,
                                        int32 state,
-                                       int32 download_id,
+                                       const DownloadId& download_id,
                                        bool has_user_gesture,
                                        content::PageTransition transition_type)
     : path(path),
@@ -37,7 +37,7 @@ DownloadCreateInfo::DownloadCreateInfo()
       received_bytes(0),
       total_bytes(0),
       state(-1),
-      download_id(-1),
+      download_id(DownloadId::Invalid()),
       has_user_gesture(false),
       transition_type(content::PAGE_TRANSITION_LINK),
       db_handle(0),
@@ -49,7 +49,7 @@ DownloadCreateInfo::~DownloadCreateInfo() {
 
 std::string DownloadCreateInfo::DebugString() const {
   return base::StringPrintf("{"
-                            " download_id = %d"
+                            " download_id = %s"
                             " url = \"%s\""
                             " path = \"%" PRFilePath "\""
                             " received_bytes = %" PRId64
@@ -57,7 +57,7 @@ std::string DownloadCreateInfo::DebugString() const {
                             " request_handle = %s"
                             " prompt_user_for_save_location = %c"
                             " }",
-                            download_id,
+                            download_id.DebugString().c_str(),
                             url().spec().c_str(),
                             path.value().c_str(),
                             received_bytes,
diff --git a/content/browser/download/download_create_info.h b/content/browser/download/download_create_info.h
index 59cab84..a7cdc6f 100644
--- a/content/browser/download/download_create_info.h
+++ b/content/browser/download/download_create_info.h
@@ -13,6 +13,7 @@
 #include "base/file_path.h"
 #include "base/time.h"
 #include "content/browser/download/download_file.h"
+#include "content/browser/download/download_id.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/common/content_export.h"
 #include "content/public/common/page_transition_types.h"
@@ -27,7 +28,7 @@ struct CONTENT_EXPORT DownloadCreateInfo {
                      int64 received_bytes,
                      int64 total_bytes,
                      int32 state,
-                     int32 download_id,
+                     const DownloadId& download_id,
                      bool has_user_gesture,
                      content::PageTransition transition_type);
   DownloadCreateInfo();
@@ -66,7 +67,7 @@ struct CONTENT_EXPORT DownloadCreateInfo {
   int32 state;
 
   // The (per-session) ID of the download.
-  int32 download_id;
+  DownloadId download_id;
 
   // True if the download was initiated by user action.
   bool has_user_gesture;
diff --git a/content/browser/download/download_file.cc b/content/browser/download/download_file.cc
index 5bc5654..6b45b6b 100644
--- a/content/browser/download/download_file.cc
+++ b/content/browser/download/download_file.cc
@@ -56,7 +56,7 @@ std::string DownloadFile::DebugString() const {
                             " request_handle = %s"
                             " Base File = %s"
                             " }",
-                            id_,
+                            id_.local(),
                             request_handle_.DebugString().c_str(),
                             BaseFile::DebugString().c_str());
 }
diff --git a/content/browser/download/download_file.h b/content/browser/download/download_file.h
index f42a1af..3c0a719 100644
--- a/content/browser/download/download_file.h
+++ b/content/browser/download/download_file.h
@@ -11,6 +11,7 @@
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "content/browser/download/base_file.h"
+#include "content/browser/download/download_id.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/download_types.h"
 #include "content/common/content_export.h"
@@ -32,7 +33,8 @@ class CONTENT_EXPORT DownloadFile : public BaseFile {
   // Cancels the download request associated with this file.
   void CancelDownloadRequest();
 
-  int id() const { return id_; }
+  int id() const { return id_.local(); }
+  const DownloadId& global_id() const { return id_; }
   DownloadManager* GetDownloadManager();
 
   virtual std::string DebugString() const;
@@ -60,7 +62,7 @@ class CONTENT_EXPORT DownloadFile : public BaseFile {
  private:
   // The unique identifier for this download, assigned at creation by
   // the DownloadFileManager for its internal record keeping.
-  int id_;
+  DownloadId id_;
 
   // The handle to the request information.  Used for operations outside the
   // download system, specifically canceling a download.
diff --git a/content/browser/download/download_file_manager.cc b/content/browser/download/download_file_manager.cc
index 9cab271..bdcd484 100644
--- a/content/browser/download/download_file_manager.cc
+++ b/content/browser/download/download_file_manager.cc
@@ -66,9 +66,8 @@ void DownloadFileManager::CreateDownloadFile(DownloadCreateInfo* info,
     return;
   }
 
-  DownloadId global_id(download_manager, info->download_id);
-  DCHECK(GetDownloadFile(global_id) == NULL);
-  downloads_[global_id] = download_file.release();
+  DCHECK(GetDownloadFile(info->download_id) == NULL);
+  downloads_[info->download_id] = download_file.release();
 
   // The file is now ready, we can un-pause the request and start saving data.
   info->request_handle.ResumeRequest();
@@ -78,7 +77,8 @@ void DownloadFileManager::CreateDownloadFile(DownloadCreateInfo* info,
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(download_manager,
-                        &DownloadManager::StartDownload, info->download_id));
+                        &DownloadManager::StartDownload,
+                        info->download_id.local()));
 }
 
 DownloadFile* DownloadFileManager::GetDownloadFile(DownloadId global_id) {
@@ -300,7 +300,7 @@ void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
 
   for (std::set<DownloadFile*>::iterator i = to_remove.begin();
        i != to_remove.end(); ++i) {
-    downloads_.erase(DownloadId((*i)->GetDownloadManager(), (*i)->id()));
+    downloads_.erase((*i)->global_id());
     delete *i;
   }
 }
diff --git a/content/browser/download/download_file_unittest.cc b/content/browser/download/download_file_unittest.cc
index 190c2792..29294de 100644
--- a/content/browser/download/download_file_unittest.cc
+++ b/content/browser/download/download_file_unittest.cc
@@ -8,6 +8,8 @@
 #include "content/browser/browser_thread.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file.h"
+#include "content/browser/download/download_id.h"
+#include "content/browser/download/download_id_factory.h"
 #include "content/browser/download/download_manager.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/download_status_updater.h"
@@ -17,6 +19,8 @@
 #include "net/base/net_errors.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+DownloadId::Domain kValidIdDomain = "valid DownloadId::Domain";
+
 class DownloadFileTest : public testing::Test {
  public:
 
@@ -33,6 +37,7 @@ class DownloadFileTest : public testing::Test {
   // calling Release() on |download_manager_| won't ever result in its
   // destructor being called and we get a leak.
   DownloadFileTest() :
+      id_factory_(new DownloadIdFactory(kValidIdDomain)),
       ui_thread_(BrowserThread::UI, &loop_),
       file_thread_(BrowserThread::FILE, &loop_) {
   }
@@ -43,7 +48,9 @@ class DownloadFileTest : public testing::Test {
   virtual void SetUp() {
     download_manager_delegate_.reset(new MockDownloadManagerDelegate());
     download_manager_ = new MockDownloadManager(
-        download_manager_delegate_.get(), &download_status_updater_);
+        download_manager_delegate_.get(),
+        id_factory_,
+        &download_status_updater_);
   }
 
   virtual void TearDown() {
@@ -58,7 +65,7 @@ class DownloadFileTest : public testing::Test {
 
   virtual void CreateDownloadFile(scoped_ptr<DownloadFile>* file, int offset) {
     DownloadCreateInfo info;
-    info.download_id = kDummyDownloadId + offset;
+    info.download_id = DownloadId(kValidIdDomain, kDummyDownloadId + offset);
     // info.request_handle default constructed to null.
     info.save_info.file_stream = file_stream_;
     file->reset(new DownloadFile(&info, download_manager_));
@@ -103,6 +110,7 @@ class DownloadFileTest : public testing::Test {
 
  private:
   MessageLoop loop_;
+  scoped_refptr<DownloadIdFactory> id_factory_;
   // UI thread.
   BrowserThread ui_thread_;
   // File thread to satisfy debug checks in DownloadFile.
diff --git a/content/browser/download/download_id.cc b/content/browser/download/download_id.cc
index bef4964..ce2b263 100644
--- a/content/browser/download/download_id.cc
+++ b/content/browser/download/download_id.cc
@@ -4,6 +4,14 @@
 
 #include "content/browser/download/download_id.h"
 
+#include <string>
+
+#include "base/stringprintf.h"
+
+std::string DownloadId::DebugString() const {
+  return base::StringPrintf("%p:%d", domain_, local_id_);
+}
+
 std::ostream& operator<<(std::ostream& out, const DownloadId& global_id) {
-  return out << global_id.manager_ << ":" << global_id.local();
+  return out << global_id.DebugString();
 }
diff --git a/content/browser/download/download_id.h b/content/browser/download/download_id.h
index 187a79c..108e219 100644
--- a/content/browser/download/download_id.h
+++ b/content/browser/download/download_id.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ID_H_
-#define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ID_H_
+#ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ID_H_
+#define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ID_H_
 #pragma once
 
 #include <iosfwd>
@@ -11,8 +11,6 @@
 #include "base/hash_tables.h"
 #include "content/common/content_export.h"
 
-class DownloadManager;
-
 // DownloadId combines per-profile Download ids with an indication of which
 // profile in order to be globally unique. DownloadIds are not persistent across
 // sessions, but their local() field is.
@@ -20,8 +18,11 @@ class DownloadId {
  public:
   static DownloadId Invalid() { return DownloadId(NULL, -1); }
 
-  DownloadId(const DownloadManager* manager, int32 local_id)
-    : manager_(manager),
+  // Domain separates spaces of local ids.
+  typedef const void* Domain;
+
+  DownloadId(Domain domain, int32 local_id)
+    : domain_(domain),
       local_id_(local_id) {
   }
 
@@ -30,22 +31,22 @@ class DownloadId {
 
   // Returns true if this DownloadId has been allocated and could possibly refer
   // to a DownloadItem that exists.
-  bool IsValid() const { return ((manager_ != NULL) && (local_id_ >= 0)); }
+  bool IsValid() const { return ((domain_ != NULL) && (local_id_ >= 0)); }
 
   // The following methods (operator==, hash(), copy, and assign) provide
   // support for STL containers such as hash_map.
 
   bool operator==(const DownloadId& that) const {
     return ((that.local_id_ == local_id_) &&
-            (that.manager_ == manager_));
+            (that.domain_ == domain_));
   }
   bool operator<(const DownloadId& that) const {
-    // Even though DownloadManager* < DownloadManager* is not well defined and
-    // GCC does not require it for hash_map, MSVC requires operator< for
-    // hash_map. We don't ifdef it out here because we will probably make a
-    // set<DownloadId> at some point, when GCC will require it.
-    return ((manager_ < that.manager_) ||
-            ((manager_ == that.manager_) && (local_id_ < that.local_id_)));
+    // Even though Domain::operator< is not well defined and GCC does not
+    // require it for hash_map, MSVC requires operator< for hash_map. We don't
+    // ifdef it out here because we will probably make a set<DownloadId> at some
+    // point, when GCC will require it.
+    return ((domain_ < that.domain_) ||
+            ((domain_ == that.domain_) && (local_id_ < that.local_id_)));
   }
 
   size_t hash() const {
@@ -53,20 +54,17 @@ class DownloadId {
     // unlikely to have >64K downloads. If these assumptions are incorrect, then
     // DownloadFileManager's hash_map might have a few collisions, but it will
     // use operator== to safely disambiguate.
-    return reinterpret_cast<size_t>(manager_) +
+    return reinterpret_cast<size_t>(domain_) +
            (static_cast<size_t>(local_id_) << (4 * sizeof(size_t)));
   }
 
+  std::string DebugString() const;
+
  private:
-  // DownloadId is used mostly off the UI thread, so manager's methods can't be
-  // called, but the pointer can be compared.
-  const DownloadManager* manager_;
+  Domain domain_;
 
   int32 local_id_;
 
-  friend CONTENT_EXPORT std::ostream& operator<<(std::ostream& out,
-                                                 const DownloadId& global_id);
-
   // Allow copy and assign.
 };
 
@@ -88,4 +86,4 @@ inline size_t hash_value(const DownloadId& id) {
 }
 #endif  // COMPILER
 }
-#endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ID_H_
+#endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ID_H_
diff --git a/content/browser/download/download_id_factory.cc b/content/browser/download/download_id_factory.cc
new file mode 100644
index 0000000..6b0797f
--- /dev/null
+++ b/content/browser/download/download_id_factory.cc
@@ -0,0 +1,17 @@
+// 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 "content/browser/download/download_id_factory.h"
+
+#include "content/browser/download/download_id.h"
+
+DownloadIdFactory::DownloadIdFactory(DownloadId::Domain domain)
+  : domain_(domain),
+    next_id_(0) {
+}
+
+DownloadId DownloadIdFactory::GetNextId() {
+  base::AutoLock lock(lock_);
+  return DownloadId(domain_, next_id_++);
+}
diff --git a/content/browser/download/download_id_factory.h b/content/browser/download/download_id_factory.h
new file mode 100644
index 0000000..6882ea0
--- /dev/null
+++ b/content/browser/download/download_id_factory.h
@@ -0,0 +1,33 @@
+// 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 CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ID_FACTORY_H_
+#define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ID_FACTORY_H_
+#pragma once
+
+#include "base/memory/ref_counted.h"
+#include "base/synchronization/lock.h"
+#include "content/browser/download/download_id.h"
+#include "content/browser/browser_thread.h"
+
+class DownloadManager;
+
+class CONTENT_EXPORT DownloadIdFactory
+    : public base::RefCountedThreadSafe<DownloadIdFactory> {
+ public:
+  // TODO(benjhayden): Instantiate with an explicit next id counter read from
+  // persistent storage.
+  DownloadIdFactory(DownloadId::Domain domain);
+
+  DownloadId GetNextId();
+
+ private:
+  DownloadId::Domain domain_;
+  int next_id_;
+  base::Lock lock_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadIdFactory);
+};
+
+#endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ID_FACTORY_H_
diff --git a/content/browser/download/download_id_unittest.cc b/content/browser/download/download_id_unittest.cc
index 39d9f3b..702ae35 100644
--- a/content/browser/download/download_id_unittest.cc
+++ b/content/browser/download/download_id_unittest.cc
@@ -25,7 +25,7 @@ class DownloadIdTest : public testing::Test {
     // Create the download managers.
     for (i = 0; i < num_managers_; ++i) {
       managers[i] =
-          new MockDownloadManager(download_manager_delegate_.get(), NULL);
+          new MockDownloadManager(download_manager_delegate_.get(), NULL, NULL);
     }
     // Sort by pointer value.
     std::sort(managers.begin(), managers.end());
diff --git a/content/browser/download/download_item.cc b/content/browser/download/download_item.cc
index 4ffe6c7..0d54911 100644
--- a/content/browser/download/download_item.cc
+++ b/content/browser/download/download_item.cc
@@ -121,7 +121,7 @@ const int DownloadItem::kUninitializedHandle = 0;
 // Constructor for reading from the history service.
 DownloadItem::DownloadItem(DownloadManager* download_manager,
                            const DownloadPersistentStoreInfo& info)
-    : download_id_(-1),
+    : download_id_(download_manager->GetNextId()),
       full_path_(info.path),
       url_chain_(1, info.url),
       referrer_url_(info.referrer_url),
@@ -197,7 +197,7 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
                            const GURL& url,
                            bool is_otr,
                            DownloadId download_id)
-    : download_id_(download_id.local()),
+    : download_id_(download_id),
       full_path_(path),
       url_chain_(1, url),
       referrer_url_(GURL()),
@@ -231,10 +231,6 @@ DownloadItem::~DownloadItem() {
   download_manager_->AssertQueueStateConsistent(this);
 }
 
-DownloadId DownloadItem::global_id() const {
-  return DownloadId(download_manager_, id());
-}
-
 void DownloadItem::AddObserver(Observer* observer) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -773,7 +769,7 @@ std::string DownloadItem::DebugString(bool verbose) const {
   std::string description =
       base::StringPrintf("{ id = %d"
                          " state = %s",
-                         download_id_,
+                         download_id_.local(),
                          DebugDownloadStateString(state()));
 
   // Construct a string of the URL chain.
diff --git a/content/browser/download/download_item.h b/content/browser/download/download_item.h
index baf3ca1..909edb0 100644
--- a/content/browser/download/download_item.h
+++ b/content/browser/download/download_item.h
@@ -25,6 +25,7 @@
 #include "base/observer_list.h"
 #include "base/time.h"
 #include "base/timer.h"
+#include "content/browser/download/download_id.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/download_state_info.h"
 #include "content/browser/download/interrupt_reasons.h"
@@ -276,8 +277,8 @@ class CONTENT_EXPORT DownloadItem {
     total_bytes_ = total_bytes;
   }
   int64 received_bytes() const { return received_bytes_; }
-  int32 id() const { return download_id_; }
-  DownloadId global_id() const;
+  int32 id() const { return download_id_.local(); }
+  DownloadId global_id() const { return download_id_; }
   base::Time start_time() const { return start_time_; }
   base::Time end_time() const { return end_time_; }
   void set_db_handle(int64 handle) { db_handle_ = handle; }
@@ -378,7 +379,7 @@ class CONTENT_EXPORT DownloadItem {
   DownloadRequestHandle request_handle_;
 
   // Download ID assigned by DownloadResourceHandler.
-  int32 download_id_;
+  DownloadId download_id_;
 
   // Full path to the downloaded or downloading file.
   FilePath full_path_;
diff --git a/content/browser/download/download_manager.cc b/content/browser/download/download_manager.cc
index eb91ef0..1c5f0d5 100644
--- a/content/browser/download/download_manager.cc
+++ b/content/browser/download/download_manager.cc
@@ -22,6 +22,7 @@
 #include "content/browser/content_browser_client.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file_manager.h"
+#include "content/browser/download/download_id_factory.h"
 #include "content/browser/download/download_item.h"
 #include "content/browser/download/download_manager_delegate.h"
 #include "content/browser/download/download_persistent_store_info.h"
@@ -60,15 +61,16 @@ void BeginDownload(
 }  // namespace
 
 DownloadManager::DownloadManager(DownloadManagerDelegate* delegate,
+                                 DownloadIdFactory* id_factory,
                                  DownloadStatusUpdater* status_updater)
     : shutdown_needed_(false),
       browser_context_(NULL),
-      next_id_(0),
       file_manager_(NULL),
       status_updater_((status_updater != NULL)
                       ? status_updater->AsWeakPtr()
                       : base::WeakPtr<DownloadStatusUpdater>()),
       delegate_(delegate),
+      id_factory_(id_factory),
       largest_db_handle_in_history_(DownloadItem::kUninitializedHandle) {
   // NOTE(benjhayden): status_updater may be NULL when using
   // TestingBrowserProcess.
@@ -82,6 +84,10 @@ DownloadManager::~DownloadManager() {
     status_updater_->RemoveDelegate(this);
 }
 
+DownloadId DownloadManager::GetNextId() {
+  return id_factory_->GetNextId();
+}
+
 void DownloadManager::Shutdown() {
   VLOG(20) << __FUNCTION__ << "()"
            << " shutdown_needed_ = " << shutdown_needed_;
@@ -196,30 +202,6 @@ void DownloadManager::SearchDownloads(const string16& query,
   }
 }
 
-void DownloadManager::OnPersistentStoreGetNextId(int next_id) {
-  DVLOG(1) << __FUNCTION__ << " " << next_id;
-  base::AutoLock lock(next_id_lock_);
-  // TODO(benjhayden) Delay Profile initialization until here, and set next_id_
-  // = next_id. The '+=' works for now because these ids are not yet persisted
-  // to the database. GetNextId() can allocate zero or more ids starting from 0,
-  // then this callback can increment next_id_, and the items with lower ids
-  // won't clash with any other items even though there may be items loaded from
-  // the history because items from the history don't have valid ids.
-  next_id_ += next_id;
-}
-
-DownloadId DownloadManager::GetNextId() {
-  // May be called on any thread via the GetNextIdThunk.
-  // TODO(benjhayden) If otr, forward to parent DM.
-  base::AutoLock lock(next_id_lock_);
-  return DownloadId(this, next_id_++);
-}
-
-DownloadManager::GetNextIdThunkType DownloadManager::GetNextIdThunk() {
-  // TODO(benjhayden) If otr, forward to parent DM.
-  return base::Bind(&DownloadManager::GetNextId, this);
-}
-
 // Query the history service for information about all persisted downloads.
 bool DownloadManager::Init(content::BrowserContext* browser_context) {
   DCHECK(browser_context);
@@ -330,7 +312,7 @@ void DownloadManager::CreateDownloadItem(DownloadCreateInfo* info) {
 
   DownloadItem* download = new DownloadItem(this, *info,
                                             browser_context_->IsOffTheRecord());
-  int32 download_id = info->download_id;
+  int32 download_id = info->download_id.local();
   DCHECK(!ContainsKey(in_progress_, download_id));
 
   // TODO(rdsmith): Remove after http://crbug.com/85408 resolved.
diff --git a/content/browser/download/download_manager.h b/content/browser/download/download_manager.h
index 8d1a161..7a19640 100644
--- a/content/browser/download/download_manager.h
+++ b/content/browser/download/download_manager.h
@@ -45,6 +45,7 @@
 #include "base/synchronization/lock.h"
 #include "base/time.h"
 #include "content/browser/browser_thread.h"
+#include "content/browser/download/download_id.h"
 #include "content/browser/download/download_item.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/download_status_updater_delegate.h"
@@ -53,6 +54,7 @@
 #include "net/base/net_errors.h"
 
 class DownloadFileManager;
+class DownloadIdFactory;
 class DownloadManagerDelegate;
 class DownloadStatusUpdater;
 class GURL;
@@ -72,6 +74,7 @@ class CONTENT_EXPORT DownloadManager
       public DownloadStatusUpdaterDelegate {
  public:
   DownloadManager(DownloadManagerDelegate* delegate,
+                  DownloadIdFactory* id_factory,
                   DownloadStatusUpdater* status_updater);
 
   // Shutdown the download manager. Must be called before destruction.
@@ -111,24 +114,8 @@ class CONTENT_EXPORT DownloadManager
   // everything.
   void SearchDownloads(const string16& query, DownloadVector* result);
 
-  // Returns the next download id in a DownloadId and increments the counter.
-  // May be called on any thread. The incremented counter is not persisted, but
-  // the base counter for this accessor is initialized from the largest id
-  // actually saved to the download history database.
   DownloadId GetNextId();
 
-  // Instead of passing a DownloadManager* between threads and hoping users only
-  // call GetNextId(), you can pass this thunk around instead. Pass the thunk
-  // around by const ref and store it by copy per the base::Callback interface.
-  // The thunk may be copied, including between threads.  If you change
-  // GetNextIdThunkType from base::Callback, then you should think about how
-  // you're changing the ref-count of DownloadManager. Use it like:
-  //   const DownloadManager::GetNextIdThunkType& next_id_thunk =
-  //       download_manager->GetNextIdThunk();
-  //   id = next_id_thunk.Run();
-  typedef base::Callback<DownloadId(void)> GetNextIdThunkType;
-  GetNextIdThunkType GetNextIdThunk();
-
   // Returns true if initialized properly.
   bool Init(content::BrowserContext* browser_context);
 
@@ -219,11 +206,6 @@ class CONTENT_EXPORT DownloadManager
   // Remove a download observer from ourself.
   void RemoveObserver(Observer* observer);
 
-  // Called by the embedder after creating the download manager to inform it of
-  // the next available download id.
-  // TODO(benjhayden): Separate this functionality out into a separate object.
-  void OnPersistentStoreGetNextId(int next_id);
-
   // Called by the embedder, after creating the download manager, to let it know
   // about downloads from previous runs of the browser.
   void OnPersistentStoreQueryComplete(
@@ -417,9 +399,6 @@ class CONTENT_EXPORT DownloadManager
   // The current active browser context.
   content::BrowserContext* browser_context_;
 
-  base::Lock next_id_lock_;
-  int next_id_;
-
   // Non-owning pointer for handling file writing on the download_thread_.
   DownloadFileManager* file_manager_;
 
@@ -433,6 +412,8 @@ class CONTENT_EXPORT DownloadManager
   // Allows an embedder to control behavior. Guaranteed to outlive this object.
   DownloadManagerDelegate* delegate_;
 
+  DownloadIdFactory* id_factory_;
+
   // TODO(rdsmith): Remove when http://crbug.com/85408 is fixed.
   // For debugging only.
   int64 largest_db_handle_in_history_;
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index e726bfb..e2b2e58 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -86,7 +86,7 @@ bool DownloadResourceHandler::OnResponseStarted(int request_id,
   // Deleted in DownloadManager.
   DownloadCreateInfo* info = new DownloadCreateInfo(FilePath(), GURL(),
       base::Time::Now(), 0, content_length_, DownloadItem::IN_PROGRESS,
-      download_id_.local(), request_info->has_user_gesture(),
+      download_id_, request_info->has_user_gesture(),
       request_info->transition_type());
   info->url_chain = request_->url_chain();
   info->referrer_url = GURL(request_->referrer());
@@ -94,7 +94,7 @@ bool DownloadResourceHandler::OnResponseStarted(int request_id,
   info->received_bytes = 0;
   info->total_bytes = content_length_;
   info->state = DownloadItem::IN_PROGRESS;
-  info->download_id = download_id_.local();
+  info->download_id = download_id_;
   info->has_user_gesture = request_info->has_user_gesture();
   info->request_handle = DownloadRequestHandle(rdh_,
                                                global_id_.child_id,
diff --git a/content/browser/download/mock_download_manager.h b/content/browser/download/mock_download_manager.h
index 48689b5..94a123c 100644
--- a/content/browser/download/mock_download_manager.h
+++ b/content/browser/download/mock_download_manager.h
@@ -14,8 +14,9 @@ class DownloadItem;
 class MockDownloadManager : public DownloadManager {
  public:
   explicit MockDownloadManager(DownloadManagerDelegate* delegate,
+                               DownloadIdFactory* id_factory,
                                DownloadStatusUpdater* updater)
-      : DownloadManager(delegate, updater) {
+      : DownloadManager(delegate, id_factory, updater) {
   }
 
   // Override some functions.
diff --git a/content/browser/renderer_host/buffered_resource_handler.cc b/content/browser/renderer_host/buffered_resource_handler.cc
index 9002bfc..f4e8943 100644
--- a/content/browser/renderer_host/buffered_resource_handler.cc
+++ b/content/browser/renderer_host/buffered_resource_handler.cc
@@ -12,6 +12,7 @@
 #include "base/string_util.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/content_browser_client.h"
+#include "content/browser/download/download_id_factory.h"
 #include "content/browser/download/download_resource_handler.h"
 #include "content/browser/plugin_service.h"
 #include "content/browser/renderer_host/resource_dispatcher_host.h"
@@ -317,7 +318,7 @@ bool BufferedResourceHandler::CompleteResponseStarted(int request_id,
 
     info->set_is_download(true);
 
-    DownloadId dl_id = info->context()->next_download_id_thunk().Run();
+    DownloadId dl_id = info->context()->download_id_factory()->GetNextId();
 
     scoped_refptr<ResourceHandler> handler(
       new DownloadResourceHandler(host_,
diff --git a/content/browser/renderer_host/resource_dispatcher_host.cc b/content/browser/renderer_host/resource_dispatcher_host.cc
index 184a54a..0657094 100644
--- a/content/browser/renderer_host/resource_dispatcher_host.cc
+++ b/content/browser/renderer_host/resource_dispatcher_host.cc
@@ -27,6 +27,7 @@
 #include "content/browser/content_browser_client.h"
 #include "content/browser/cross_site_request_manager.h"
 #include "content/browser/download/download_file_manager.h"
+#include "content/browser/download/download_id_factory.h"
 #include "content/browser/download/download_manager.h"
 #include "content/browser/download/download_resource_handler.h"
 #include "content/browser/download/save_file_manager.h"
@@ -830,7 +831,7 @@ void ResourceDispatcherHost::BeginDownload(
 
   request_id_--;
 
-  DownloadId dl_id = context.next_download_id_thunk().Run();
+  DownloadId dl_id = context.download_id_factory()->GetNextId();
 
   scoped_refptr<ResourceHandler> handler(
       new DownloadResourceHandler(this,
diff --git a/content/browser/renderer_host/resource_dispatcher_host_unittest.cc b/content/browser/renderer_host/resource_dispatcher_host_unittest.cc
index 1b57703..4845554 100644
--- a/content/browser/renderer_host/resource_dispatcher_host_unittest.cc
+++ b/content/browser/renderer_host/resource_dispatcher_host_unittest.cc
@@ -13,6 +13,7 @@
 #include "content/browser/browser_thread.h"
 #include "content/browser/child_process_security_policy.h"
 #include "content/browser/download/download_id.h"
+#include "content/browser/download/download_id_factory.h"
 #include "content/browser/mock_resource_context.h"
 #include "content/browser/renderer_host/dummy_resource_handler.h"
 #include "content/browser/renderer_host/global_request_id.h"
@@ -1135,12 +1136,6 @@ TEST_F(ResourceDispatcherHostTest, ForbiddenDownload) {
   EXPECT_EQ(net::ERR_FILE_NOT_FOUND, status.error());
 }
 
-namespace {
-DownloadId MockNextDownloadId() {
-  return DownloadId(reinterpret_cast<DownloadManager*>(0xFFFFFFFF), 0);
-}
-}
-
 // Test for http://crbug.com/76202 .  We don't want to destroy a
 // download request prematurely when processing a cancellation from
 // the renderer.
@@ -1161,8 +1156,10 @@ TEST_F(ResourceDispatcherHostTest, IgnoreCancelForDownloads) {
   HandleScheme("http");
 
   MakeTestRequest(render_view_id, request_id, GURL("http://example.com/blah"));
-  content::MockResourceContext::GetInstance()->set_next_download_id_thunk(
-      base::Bind(&MockNextDownloadId));
+  scoped_refptr<DownloadIdFactory> id_factory(
+      new DownloadIdFactory("valid DownloadId::Domain"));
+  content::MockResourceContext::GetInstance()->set_download_id_factory(
+      id_factory);
   // Return some data so that the request is identified as a download
   // and the proper resource handlers are created.
   EXPECT_TRUE(net::URLRequestTestJob::ProcessOnePendingMessage());
@@ -1198,8 +1195,10 @@ TEST_F(ResourceDispatcherHostTest, CancelRequestsForContext) {
   HandleScheme("http");
 
   MakeTestRequest(render_view_id, request_id, GURL("http://example.com/blah"));
-  content::MockResourceContext::GetInstance()->set_next_download_id_thunk(
-      base::Bind(&MockNextDownloadId));
+  scoped_refptr<DownloadIdFactory> id_factory(
+      new DownloadIdFactory("valid DownloadId::Domain"));
+  content::MockResourceContext::GetInstance()->set_download_id_factory(
+      id_factory);
   // Return some data so that the request is identified as a download
   // and the proper resource handlers are created.
   EXPECT_TRUE(net::URLRequestTestJob::ProcessOnePendingMessage());
diff --git a/content/browser/resource_context.cc b/content/browser/resource_context.cc
index 552ed1e..c9da0fb 100644
--- a/content/browser/resource_context.cc
+++ b/content/browser/resource_context.cc
@@ -153,16 +153,15 @@ void ResourceContext::set_media_observer(MediaObserver* media_observer) {
   media_observer_ = media_observer;
 }
 
-const DownloadManager::GetNextIdThunkType&
-ResourceContext::next_download_id_thunk() const {
+DownloadIdFactory* ResourceContext::download_id_factory() const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   EnsureInitialized();
-  return next_download_id_thunk_;
+  return download_id_factory_;
 }
-void ResourceContext::set_next_download_id_thunk(
-    const DownloadManager::GetNextIdThunkType& thunk) {
+void ResourceContext::set_download_id_factory(
+    DownloadIdFactory* download_id_factory) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  next_download_id_thunk_ = thunk;
+  download_id_factory_ = download_id_factory;
 }
 
 media_stream::MediaStreamManager*
diff --git a/content/browser/resource_context.h b/content/browser/resource_context.h
index 0e04d07..3879390 100644
--- a/content/browser/resource_context.h
+++ b/content/browser/resource_context.h
@@ -11,10 +11,10 @@
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "content/common/content_export.h"
-#include "content/browser/download/download_manager.h"
 
 class ChromeAppCacheService;
 class ChromeBlobStorageContext;
+class DownloadIdFactory;
 class ExtensionInfoMap;
 class HostZoomMap;
 class MediaObserver;
@@ -77,10 +77,8 @@ class CONTENT_EXPORT ResourceContext {
   MediaObserver* media_observer() const;
   void set_media_observer(MediaObserver* media_observer);
 
-  // TODO(benjhayden): Promote GetNextIdThunkType to a separate object.
-  const DownloadManager::GetNextIdThunkType& next_download_id_thunk() const;
-  void set_next_download_id_thunk(
-      const DownloadManager::GetNextIdThunkType& thunk);
+  DownloadIdFactory* download_id_factory() const;
+  void set_download_id_factory(DownloadIdFactory* download_id_factory);
 
   media_stream::MediaStreamManager* media_stream_manager() const;
   void set_media_stream_manager(
@@ -101,7 +99,7 @@ class CONTENT_EXPORT ResourceContext {
   quota::QuotaManager* quota_manager_;
   HostZoomMap* host_zoom_map_;
   MediaObserver* media_observer_;
-  DownloadManager::GetNextIdThunkType next_download_id_thunk_;
+  DownloadIdFactory* download_id_factory_;
   media_stream::MediaStreamManager* media_stream_manager_;
 
   // Externally-defined data accessible by key.
diff --git a/content/content_browser.gypi b/content/content_browser.gypi
index 5977a39..7ab16fe 100644
--- a/content/content_browser.gypi
+++ b/content/content_browser.gypi
@@ -116,6 +116,8 @@
     'browser/download/download_file_manager.h',
     'browser/download/download_id.cc',
     'browser/download/download_id.h',
+    'browser/download/download_id_factory.cc',
+    'browser/download/download_id_factory.h',
     'browser/download/download_item.cc',
     'browser/download/download_item.h',
     'browser/download/download_manager.cc',
diff --git a/content/shell/shell_browser_context.cc b/content/shell/shell_browser_context.cc
index 561382e..246c9f3 100644
--- a/content/shell/shell_browser_context.cc
+++ b/content/shell/shell_browser_context.cc
@@ -10,14 +10,15 @@
 #include "content/browser/appcache/chrome_appcache_service.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/chrome_blob_storage_context.h"
+#include "content/browser/download/download_id_factory.h"
 #include "content/browser/download/download_manager.h"
 #include "content/browser/download/download_status_updater.h"
 #include "content/browser/file_system/browser_file_system_helper.h"
 #include "content/browser/geolocation/geolocation_permission_context.h"
 #include "content/browser/host_zoom_map.h"
 #include "content/browser/in_process_webkit/webkit_context.h"
-#include "content/browser/ssl/ssl_host_state.h"
 #include "content/browser/speech/speech_input_preferences.h"
+#include "content/browser/ssl/ssl_host_state.h"
 #include "content/shell/shell_browser_main.h"
 #include "content/shell/shell_download_manager_delegate.h"
 #include "content/shell/shell_resource_context.h"
@@ -80,7 +81,8 @@ namespace content {
 
 ShellBrowserContext::ShellBrowserContext(
     ShellBrowserMainParts* shell_main_parts)
-    : shell_main_parts_(shell_main_parts) {
+    : download_id_factory_(new DownloadIdFactory(this)),
+      shell_main_parts_(shell_main_parts) {
 }
 
 ShellBrowserContext::~ShellBrowserContext() {
@@ -123,6 +125,7 @@ DownloadManager* ShellBrowserContext::GetDownloadManager()  {
 
     download_manager_delegate_ = new ShellDownloadManagerDelegate();
     download_manager_ = new DownloadManager(download_manager_delegate_,
+                                            download_id_factory_,
                                             download_status_updater_.get());
     download_manager_delegate_->SetDownloadManager(download_manager_.get());
     download_manager_->Init(this);
@@ -156,7 +159,7 @@ const ResourceContext& ShellBrowserContext::GetResourceContext()  {
     resource_context_.reset(new ShellResourceContext(
         static_cast<ShellURLRequestContextGetter*>(GetRequestContext()),
         GetBlobStorageContext(),
-        GetDownloadManager()->GetNextIdThunk()));
+        download_id_factory_));
   }
   return *resource_context_.get();
 }
diff --git a/content/shell/shell_browser_context.h b/content/shell/shell_browser_context.h
index 595392e..c937553 100644
--- a/content/shell/shell_browser_context.h
+++ b/content/shell/shell_browser_context.h
@@ -12,6 +12,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "content/browser/browser_context.h"
 
+class DownloadIdFactory;
 class DownloadManager;
 class DownloadManagerDelegate;
 class DownloadStatusUpdater;
@@ -60,6 +61,7 @@ class ShellBrowserContext : public BrowserContext {
   scoped_ptr<SSLHostState> ssl_host_state_;
   scoped_ptr<DownloadStatusUpdater> download_status_updater_;
   scoped_refptr<ShellDownloadManagerDelegate> download_manager_delegate_;
+  scoped_refptr<DownloadIdFactory> download_id_factory_;
   scoped_refptr<DownloadManager> download_manager_;
   scoped_refptr<net::URLRequestContextGetter> url_request_getter_;
   scoped_refptr<HostZoomMap> host_zoom_map_;
diff --git a/content/shell/shell_resource_context.cc b/content/shell/shell_resource_context.cc
index 5058992..21a696f 100644
--- a/content/shell/shell_resource_context.cc
+++ b/content/shell/shell_resource_context.cc
@@ -5,6 +5,7 @@
 #include "content/shell/shell_resource_context.h"
 
 #include "content/browser/chrome_blob_storage_context.h"
+#include "content/browser/download/download_id_factory.h"
 #include "content/shell/shell_url_request_context_getter.h"
 
 namespace content {
@@ -12,10 +13,10 @@ namespace content {
 ShellResourceContext::ShellResourceContext(
     ShellURLRequestContextGetter* getter,
     ChromeBlobStorageContext* blob_storage_context,
-    DownloadManager::GetNextIdThunkType next_download_id_thunk)
+    DownloadIdFactory* download_id_factory)
     : getter_(getter),
       blob_storage_context_(blob_storage_context),
-      next_download_id_thunk_(next_download_id_thunk) {
+      download_id_factory_(download_id_factory) {
 }
 
 ShellResourceContext::~ShellResourceContext() {
@@ -29,7 +30,7 @@ void ShellResourceContext::InitializeInternal() {
   set_request_context(getter_->GetURLRequestContext());
   set_host_resolver(getter_->host_resolver());
   set_blob_storage_context(blob_storage_context_);
-  set_next_download_id_thunk(next_download_id_thunk_);
+  set_download_id_factory(download_id_factory_);
 }
 
 }  // namespace content
diff --git a/content/shell/shell_resource_context.h b/content/shell/shell_resource_context.h
index bafb05b..710778e 100644
--- a/content/shell/shell_resource_context.h
+++ b/content/shell/shell_resource_context.h
@@ -8,10 +8,10 @@
 
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
-#include "content/browser/download/download_manager.h"
 #include "content/browser/resource_context.h"
 
 class ChromeBlobStorageContext;
+class DownloadIdFactory;
 
 namespace content {
 
@@ -22,7 +22,7 @@ class ShellResourceContext : public content::ResourceContext {
   ShellResourceContext(
       ShellURLRequestContextGetter* getter,
       ChromeBlobStorageContext* blob_storage_context,
-      DownloadManager::GetNextIdThunkType next_download_id_thunk);
+      DownloadIdFactory* download_id_factory);
   virtual ~ShellResourceContext();
 
  private:
@@ -32,7 +32,7 @@ class ShellResourceContext : public content::ResourceContext {
 
   scoped_refptr<ShellURLRequestContextGetter> getter_;
   scoped_refptr<ChromeBlobStorageContext> blob_storage_context_;
-  DownloadManager::GetNextIdThunkType next_download_id_thunk_;
+  scoped_refptr<DownloadIdFactory> download_id_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ShellResourceContext);
 };