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

chrome/browser/download/download_manager.h

Index: chrome/browser/download/download_manager.h
diff --git a/chrome/browser/download/download_manager.h b/chrome/browser/download/download_manager.h
index 3c7284fe48d10ba03aefea938e73b30024ac9eb0..69925ad466a1522c8c9a3084ec76ea6c74ef0d57 100644
--- a/chrome/browser/download/download_manager.h
+++ b/chrome/browser/download/download_manager.h
@@ -34,6 +34,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/hash_tables.h"
@@ -44,6 +45,7 @@
 #include "base/time.h"
 #include "chrome/browser/download/download_item.h"
 #include "chrome/browser/download/download_request_handle.h"
+#include "chrome/browser/download/download_state_info.h"
 #include "chrome/browser/download/download_status_updater_delegate.h"
 #include "chrome/browser/ui/shell_dialogs.h"
 #include "content/browser/browser_thread.h"
@@ -59,6 +61,9 @@ class TabContents;
 struct DownloadCreateInfo;
 struct DownloadHistoryInfo;
 struct DownloadSaveInfo;
+namespace base {
+class Lock;
+}  // namespace base

[Randy Smith (Not in Mondays), 2011/07/28 21:03:16]

I'm having a "I don't think this can work" reaction here.  An incomplete type
delcaration is ok for pointers or references, but if you declare an actual
variable, you need a complete type declaration.  And next_id_lock_ is declared
inline, below.  So I presume that this is compiling only because some other
include file is including base/lock.h, and that this file should also include
base/lock.h.

[benjhayden, 2011/08/03 17:44:46]

Done.

 
 // Browser's download manager: manages all downloads and destination view.
 class DownloadManager
@@ -112,6 +117,24 @@ class DownloadManager
   void SearchDownloads(const string16& query,
                        std::vector<DownloadItem*>* 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(Profile* profile);
 
@@ -350,6 +373,9 @@ class DownloadManager
   // Add a DownloadItem to history_downloads_.
   void AddDownloadItemToHistory(DownloadItem* item, int64 db_handle);
 
+  // The DownloadHistory grabbed the next_id counter from the sql MetaTable.
+  void OnHistoryGetNextId(int next_id);

[Randy Smith (Not in Mondays), 2011/07/28 21:03:16]

For the next CL: Consider making the next_id field an argument to the
DownloadManager constructor.  That'll fit well with the "pulling history out"
stuff that jam's aiming to do, and should work well with the delaying of the
profile initialization until we have that information (because we won't need the
download manager until then anyway).

[benjhayden, 2011/08/03 17:44:46]

So, Profile will use its HistoryService directly and skip DownloadHistory?

[Randy Smith (Not in Mondays), 2011/08/03 21:10:29]

Yep.  Or if there's some reason to, it can construct DownloadHistory, use it,
and pass it in to DownloadManager.  My guess is that leaving this in
DownloadManager will mean there are some difficulties in the profile
synchronization implementation (basically, figuring out when DownloadManager has
gotten the next_id), and that those problems will be avoided if the profile impl
does it itself.  But it's just a suggestion based on my guess as to what'll
produce the most clean code; I wanted it in your head, but use your judgement
when you actually do it.

[benjhayden, 2011/08/04 17:15:00]

Done.

+
   // |downloads_| is the owning set for all downloads known to the
   // DownloadManager.  This includes downloads started by the user in
   // this session, downloads initialized from the history system, and
@@ -403,6 +429,9 @@ class DownloadManager
   // Observers that want to be notified of changes to the set of downloads.
   ObserverList<Observer> observers_;
 
+  base::Lock next_id_lock_;
+  int next_id_;
+
   // The current active profile.
   Profile* profile_;