Make DownloadManagerImpl to generate download IDs for in-progress DB

Chrome assigns a download Id to each new download.
This ID was retrieved from the history DB on start up, and then
keep on incrementing.
For in-progress DB work, download will only be stored to history DB
once they finish.
As a result, the history DB don't have all the Ids on startup.
This CL lets the DownloadManagerImpl to get all Ids from both
history DB and In-progress downloads.
And use the largest Id to issueing out new Ids for new downloads.
This allows in-progress downloads to have their own IDs without
reporting them to history DB.

BUG=842245

Change-Id: Ia5130f02b0e32e09ba4395a6bfa10dfd5adafc82
Reviewed-on: https://chromium-review.googlesource.com/1144311
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579101}

components/download/database/in_progress/in_progress_cache_impl.cc

@@ -160,9 +160,10 @@ void InProgressCacheImpl::OnInitialized(base::OnceClosure callback,
   if (!entries.empty()) {
     if (!entries_.ParseFromArray(entries.data(), entries.size())) {
       // TODO(crbug.com/778425): Get UMA for errors.
+      // If the data cannot be parsed, just call the callback and the cache
+      // will be overwritten by the next write.
       LOG(ERROR) << "Could not read download entries from file "
                  << "because there was a parse failure.";
-      return;
     }
   }
 

content/browser/download/download_manager_impl.cc

@@ -299,6 +299,8 @@ DownloadManagerImpl::DownloadManagerImpl(BrowserContext* browser_context)
       delegate_(nullptr),
       in_progress_manager_(
           browser_context_->RetriveInProgressDownloadManager()),
+      next_download_id_(download::DownloadItem::kInvalidId),
+      is_history_download_id_retrieved_(false),
       weak_factory_(this) {
   DCHECK(browser_context);
   download::SetIOTaskRunner(
@@ -344,14 +346,43 @@ download::DownloadItemImpl* DownloadManagerImpl::CreateActiveItem(
   return download;
 }
 
-void DownloadManagerImpl::GetNextId(const DownloadIdCallback& callback) {
+void DownloadManagerImpl::GetNextId(GetNextIdCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (delegate_) {
-    delegate_->GetNextId(callback);
+  if (IsNextIdInitialized()) {
+    std::move(callback).Run(next_download_id_++);
     return;
   }
-  static uint32_t next_id = download::DownloadItem::kInvalidId + 1;
-  callback.Run(next_id++);
+
+  id_callbacks_.emplace_back(
+      std::make_unique<GetNextIdCallback>(std::move(callback)));
+  // If we are first time here, call the delegate to get the next ID from
+  // history db.
+  if (!is_history_download_id_retrieved_ && id_callbacks_.size() == 1u) {
+    if (delegate_) {
+      delegate_->GetNextId(
+          base::BindRepeating(&DownloadManagerImpl::OnHistoryNextIdRetrived,
+                              weak_factory_.GetWeakPtr()));
+    } else {
+      OnHistoryNextIdRetrived(download::DownloadItem::kInvalidId + 1);
+    }
+  }
+}
+
+void DownloadManagerImpl::SetNextId(uint32_t next_id) {
+  if (next_id > next_download_id_)
+    next_download_id_ = next_id;
+  if (!IsNextIdInitialized())
+    return;
+
+  for (auto& callback : id_callbacks_)
+    std::move(*callback).Run(next_download_id_++);
+  id_callbacks_.clear();
+}
+
+void DownloadManagerImpl::OnHistoryNextIdRetrived(uint32_t next_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  is_history_download_id_retrieved_ = true;
+  SetNextId(next_id);
 }
 
 void DownloadManagerImpl::DetermineDownloadTarget(
@@ -507,6 +538,7 @@ base::FilePath DownloadManagerImpl::GetDefaultDownloadDirectory() {
 void DownloadManagerImpl::OnInProgressDownloadManagerInitialized() {
   std::vector<std::unique_ptr<download::DownloadItemImpl>>
       in_progress_downloads = in_progress_manager_->TakeInProgressDownloads();
+  uint32_t max_id = download::DownloadItem::kInvalidId;
   for (auto& download : in_progress_downloads) {
     DCHECK(!base::ContainsKey(downloads_by_guid_, download->GetGuid()));
     DCHECK(!base::ContainsKey(downloads_, download->GetId()));
@@ -514,12 +546,16 @@ void DownloadManagerImpl::OnInProgressDownloadManagerInitialized() {
     download::DownloadItemImpl* item = download.get();
     item->SetDelegate(this);
     downloads_by_guid_[download->GetGuid()] = item;
-    downloads_[download->GetId()] = std::move(download);
+    uint32_t id = download->GetId();
+    downloads_[id] = std::move(download);
+    if (id > max_id)
+      max_id = id;
     for (auto& observer : observers_)
       observer.OnDownloadCreated(this, item);
     DVLOG(20) << __func__ << "() download = " << item->DebugString(true);
   }
   PostInitialization(DOWNLOAD_INITIALIZATION_DEPENDENCY_IN_PROGRESS_CACHE);
+  SetNextId(max_id + 1);
 }
 
 void DownloadManagerImpl::StartDownloadItem(
@@ -535,10 +571,9 @@ void DownloadManagerImpl::StartDownloadItem(
     std::move(callback).Run(std::move(info), download);
     OnDownloadStarted(download, on_started);
   } else {
-    GetNextId(
-        base::BindRepeating(&DownloadManagerImpl::CreateNewDownloadItemToStart,
-                            weak_factory_.GetWeakPtr(), base::Passed(&info),
-                            on_started, base::Passed(&callback)));
+    GetNextId(base::BindOnce(&DownloadManagerImpl::CreateNewDownloadItemToStart,
+                             weak_factory_.GetWeakPtr(), std::move(info),
+                             on_started, std::move(callback)));
   }
 }
 
@@ -641,10 +676,10 @@ void DownloadManagerImpl::CreateSavePackageDownloadItem(
     const DownloadItemImplCreated& item_created) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   GetNextId(
-      base::Bind(&DownloadManagerImpl::CreateSavePackageDownloadItemWithId,
-                 weak_factory_.GetWeakPtr(), main_file_path, page_url,
-                 mime_type, render_process_id, render_frame_id,
-                 base::Passed(std::move(request_handle)), item_created));
+      base::BindOnce(&DownloadManagerImpl::CreateSavePackageDownloadItemWithId,
+                     weak_factory_.GetWeakPtr(), main_file_path, page_url,
+                     mime_type, render_process_id, render_frame_id,
+                     std::move(request_handle), item_created));
 }
 
 void DownloadManagerImpl::CreateSavePackageDownloadItemWithId(
@@ -1232,4 +1267,8 @@ void DownloadManagerImpl::BeginDownloadInternal(
   }
 }
 
+bool DownloadManagerImpl::IsNextIdInitialized() const {
+  return is_history_download_id_retrieved_ && in_progress_cache_initialized_;
+}
+
 }  // namespace content

content/browser/download/download_manager_impl.h

@@ -214,9 +214,18 @@ class CONTENT_EXPORT DownloadManagerImpl
       download::InProgressDownloadManager::StartDownloadItemCallback callback,
       uint32_t id);
 
+  using GetNextIdCallback = base::OnceCallback<void(uint32_t)>;
   // Called to get an ID for a new download. |callback| may be called
   // synchronously.
-  void GetNextId(const DownloadIdCallback& callback);
+  void GetNextId(GetNextIdCallback callback);
+
+  // Sets the |next_download_id_| if the |next_id| is larger. Runs all the
+  // |id_callbacks_| if both the ID from both history db and in-progress db
+  // are retrieved.
+  void SetNextId(uint32_t next_id);
+
+  // Called when the next ID from history db is retrieved.
+  void OnHistoryNextIdRetrived(uint32_t next_id);
 
   // Create a new active item based on the info.  Separate from
   // StartDownload() for testing.
@@ -279,6 +288,9 @@ class CONTENT_EXPORT DownloadManagerImpl
       const GURL& site_url,
       bool is_download_allowed);
 
+  // Whether |next_download_id_| is initialized.
+  bool IsNextIdInitialized() const;
+
   // Factory for creation of downloads items.
   std::unique_ptr<download::DownloadItemFactory> item_factory_;
 
@@ -329,6 +341,18 @@ class CONTENT_EXPORT DownloadManagerImpl
   // Callback to run to load all history downloads.
   base::OnceClosure load_history_downloads_cb_;
 
+  // The next download id to issue to new downloads. The |next_download_id_| can
+  // only be used when both history and in-progress db have provided their
+  // values.
+  uint32_t next_download_id_;
+
+  // Whether next download ID from history DB is being retrieved.
+  bool is_history_download_id_retrieved_;
+
+  // Callbacks to run once download ID is determined.
+  using IdCallbackVector = std::vector<std::unique_ptr<GetNextIdCallback>>;
+  IdCallbackVector id_callbacks_;
+
   base::WeakPtrFactory<DownloadManagerImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadManagerImpl);

content/browser/download/download_manager_impl_unittest.cc

@@ -522,6 +522,7 @@ TEST_F(DownloadManagerTest, StartDownload) {
   std::unique_ptr<ByteStreamReader> stream(new MockByteStreamReader);
   uint32_t local_id(5);  // Random value
   base::FilePath download_path(FILE_PATH_LITERAL("download/path"));
+  OnInProgressDownloadManagerInitialized();
 
   EXPECT_FALSE(download_manager_->GetDownload(local_id));