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..594cff539ee4cd9e2b991540c400d6b69f793b2f 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
 
 // Browser's download manager: manages all downloads and destination view.
 class DownloadManager
@@ -112,6 +117,20 @@ 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 until
+  // your new DownloadItem is stored in the history.

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

nit: It's not really the incremented counter that's being persisted.  I'd be
tempted to phrase this as "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."  But your call.

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

Done.

+  DownloadId GetNextId();
+
+  // Instead of passing a DownloadManager* between threads and hoping users only
+  // call GetNextId(), you can pass this thunk around instead.  Use it like "id
+  // = next_id_thunk.Run();" Pass the thunk around by const ref and store it by

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

nit: I may be targeting a less-smart class of developers than we actually have,
but could you make this a bit clearer?  Specifically, I'd suggest saying:

---
Use it like:
    const GetNextIdThunkType& next_id_thunk =
download_manager->GetNextIdThunk();
    // next_id_thunk may be safely copied, including between threads.
    id = next_id_thunk.Run();
----

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

I'm a bit worried by this.  My understanding was that when passed between
threads, refs were silently copied (to handle lifetime management
issues--whatever created the object the ref was to might destroy it on the
original thread without warning).  But that obviously won't work if you're
holding on to a reference to a base class, which is what this sounds like. 
Enlighten me as to why this is ok?

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

Done.

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

I'm not sure exactly what you're asking.
If you're asking about destroying DownloadManager, it's ref-counted, so you
don't destroy DM, you just decref it and trust that it will be destroyed when
everybody has decref'd it.
If you're asking about destroying the thunk, that's why it should be stored by
copy instead of by ref. A copy of this thunk is stored in ProfileIOData, which
is created on the UI thread and used on the IO thread. Another copy is stored in
DownloadResourceHandler.
Were you asking about something else?

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

I think I wasn't understanding how base::Callback and base::Bind were
implemented.  I was worried that when you created an object of
base::Calback<DownloadId(void)> you were actually creating an object of a
(hidden) superclass of that type.  If that was the case, passing the object
around by copy would "sheer off" everything except for the base class contents. 
I no longer think that, so I'm no longer worried about this issue :-}.

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

Done.

+  // copy per the base::Callback interface.  If you change GetNextIdThunkType
+  // from base::Callback, then you should think about how you're changing the
+  // ref-count of DownloadManager.
+  typedef base::Callback<DownloadId(void)> GetNextIdThunkType;
+  GetNextIdThunkType GetNextIdThunk();

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

If you want to pass it around by const ref, is there a reason it doesn't return
a const ref?  

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

Error: returning reference to temporary.

+
   // Returns true if initialized properly.
   bool Init(Profile* profile);
 
@@ -350,6 +369,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);
+
   // |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 +425,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_;