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

chrome/browser/download/download_manager.h

 // 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.
 //
 // The DownloadManager object manages the process of downloading, including
 // updates to the history system and providing the information for displaying
 // the downloads view in the Destinations tab. There is one DownloadManager per
 // active profile in Chrome.
 //
 // Download observers:
@@ skipping 16 matching lines @@
 #ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_H_
 #define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_H_
 #pragma once
 
 #include <map>
 #include <set>
 #include <string>
 #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"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #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"
 
 class DownloadFileManager;
 class DownloadHistory;
 class DownloadPrefs;
 class DownloadStatusUpdater;
 class GURL;
 class Profile;
 class ResourceDispatcherHost;
 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
     : public base::RefCountedThreadSafe<DownloadManager,
                                         BrowserThread::DeleteOnUIThread>,
       public DownloadStatusUpdaterDelegate,
       public SelectFileDialog::Listener {
  public:
   explicit DownloadManager(DownloadStatusUpdater* status_updater);
 
@@ skipping 33 matching lines @@
   // Return all non-temporary downloads in the specified directory that are
   // in-progress (including dangerous downloads waiting for user confirmation).
   void GetCurrentDownloads(const FilePath& dir_path,
                            std::vector<DownloadItem*>* result);
 
   // Returns all non-temporary downloads matching |query|. Empty query matches
   // everything.
   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);
 
   // Notifications sent from the download thread to the UI thread
   void StartDownload(int32 id);
   void UpdateDownload(int32 download_id, int64 size);
   // |hash| is sha256 hash for the downloaded file. It is empty when the hash
   // is not available.
   void OnResponseCompleted(int32 download_id, int64 size, int os_error,
                            const std::string& hash);
@@ skipping 218 matching lines @@
   // yet in the history map.
   DownloadItem* GetActiveDownloadItem(int id);
 
   // Debugging routine to confirm relationship between below
   // containers; no-op if NDEBUG.
   void AssertContainersConsistent() const;
 
   // 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
   // "save page as" downloads.  All other DownloadItem containers in
   // the DownloadManager are maps; they do not own the DownloadItems.
   // Note that this is the only place (with any functional implications;
   // see save_page_as_downloads_ below) that "save page as" downloads are
   // kept, as the DownloadManager's only job is to hold onto those
   // until destruction.
   //
@@ skipping 33 matching lines @@
 #if !defined(NDEBUG)
   DownloadSet save_page_as_downloads_;
 #endif
 
   // True if the download manager has been initialized and requires a shutdown.
   bool shutdown_needed_;
 
   // 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_;
 
   scoped_ptr<DownloadHistory> download_history_;
 
   scoped_ptr<DownloadPrefs> download_prefs_;
 
   // Non-owning pointer for handling file writing on the download_thread_.
   DownloadFileManager* file_manager_;
 
   // Non-owning pointer for updating the download status.
   base::WeakPtr<DownloadStatusUpdater> status_updater_;
 
   // The user's last choice for download directory. This is only used when the
   // user wants us to prompt for a save location for each download.
   FilePath last_download_path_;
 
   // The "Save As" dialog box used to ask the user where a file should be
   // saved.
   scoped_refptr<SelectFileDialog> select_file_dialog_;
 
   scoped_ptr<OtherDownloadManagerObserver> other_download_manager_observer_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadManager);
 };
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_H_