Remove DownloadDBForNewDownloads finch flag

This is defaulted to true for several releases now. Remove
this flag and fixing some tests that still relying on DownloadHistory.

BUG=1001629

Change-Id: Ica9ede823d49b3fa2680342616ca8842d56071a6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1789924
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Xi Han <hanxi@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695432}

chrome/browser/apps/guest_view/web_view_browsertest.cc

@@ -22,6 +22,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
@@ -52,7 +53,7 @@
 #include "chrome/common/webui_url_constants.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
-#include "components/download/public/common/download_features.h"
+#include "components/download/public/common/download_task_runner.h"
 #include "components/guest_view/browser/guest_view_manager.h"
 #include "components/guest_view/browser/guest_view_manager_delegate.h"
 #include "components/guest_view/browser/guest_view_manager_factory.h"
@@ -2745,57 +2746,34 @@ std::unique_ptr<net::test_server::HttpResponse> HandleDownloadRequestWithCookie(
   return std::move(response);
 }
 
-class DownloadHistoryWaiter : public DownloadHistory::Observer {
+// Class for waiting for download manager to be initiailized.
+class DownloadManagerWaiter : public content::DownloadManager::Observer {
  public:
-  explicit DownloadHistoryWaiter(content::BrowserContext* browser_context) {
-    DownloadCoreService* service =
-        DownloadCoreServiceFactory::GetForBrowserContext(browser_context);
-    download_history_ = service->GetDownloadHistory();
-    download_history_->AddObserver(this);
+  explicit DownloadManagerWaiter(content::DownloadManager* download_manager)
+      : initialized_(false), download_manager_(download_manager) {
+    download_manager_->AddObserver(this);
   }
 
-  ~DownloadHistoryWaiter() override { download_history_->RemoveObserver(this); }
+  ~DownloadManagerWaiter() override { download_manager_->RemoveObserver(this); }
 
-  void WaitForStored(size_t download_count) {
-    if (stored_downloads_.size() >= download_count)
+  void WaitForInitialized() {
+    if (initialized_)
       return;
-    stored_download_target_ = download_count;
-    Wait();
-  }
-
-  void WaitForHistoryLoad() {
-    if (history_query_complete_)
-      return;
-    Wait();
-  }
-
- private:
-  void Wait() {
     base::RunLoop run_loop;
     quit_closure_ = run_loop.QuitClosure();
     run_loop.Run();
   }
 
-  void OnDownloadStored(download::DownloadItem* item,
-                        const history::DownloadRow& info) override {
-    stored_downloads_.insert(item);
-    if (!quit_closure_.is_null() &&
-        stored_downloads_.size() >= stored_download_target_) {
+  void OnManagerInitialized() override {
+    initialized_ = true;
+    if (quit_closure_)
       std::move(quit_closure_).Run();
-    }
   }
 
-  void OnHistoryQueryComplete() override {
-    history_query_complete_ = true;
-    if (!quit_closure_.is_null())
-      std::move(quit_closure_).Run();
-  }
-
-  std::unordered_set<download::DownloadItem*> stored_downloads_;
-  size_t stored_download_target_ = 0;
-  bool history_query_complete_ = false;
+ private:
   base::Closure quit_closure_;
-  DownloadHistory* download_history_ = nullptr;
+  bool initialized_;
+  content::DownloadManager* download_manager_;
 };
 
 }  // namespace
@@ -2894,15 +2872,14 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, MAYBE_DownloadCookieIsolation) {
 }
 
 IN_PROC_BROWSER_TEST_F(WebViewTest, PRE_DownloadCookieIsolation_CrossSession) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndDisableFeature(
-      download::features::kDownloadDBForNewDownloads);
-
   embedded_test_server()->RegisterRequestHandler(
       base::BindRepeating(&HandleDownloadRequestWithCookie));
   ASSERT_TRUE(StartEmbeddedTestServer());  // For serving guest pages.
   LoadAndLaunchPlatformApp("web_view/download_cookie_isolation",
                            "created-webviews");
+  scoped_refptr<base::TestMockTimeTaskRunner> task_runner(
+      new base::TestMockTimeTaskRunner);
+  download::SetDownloadDBTaskRunnerForTesting(task_runner);
 
   content::WebContents* web_contents = GetFirstAppWindowWebContents();
   ASSERT_TRUE(web_contents);
@@ -2910,9 +2887,6 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, PRE_DownloadCookieIsolation_CrossSession) {
   content::DownloadManager* download_manager =
       content::BrowserContext::GetDownloadManager(
           web_contents->GetBrowserContext());
-
-  DownloadHistoryWaiter history_waiter(web_contents->GetBrowserContext());
-
   std::unique_ptr<content::DownloadTestObserver> interrupted_observer(
       new content::DownloadTestObserverInterrupted(
           download_manager, 2,
@@ -2945,7 +2919,7 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, PRE_DownloadCookieIsolation_CrossSession) {
   interrupted_observer->WaitForFinished();
 
   // Wait for both downloads to be stored.
-  history_waiter.WaitForStored(2);
+  task_runner->FastForwardUntilNoTasksRemain();
 
   content::EnsureCookiesFlushed(profile());
 }
@@ -2961,21 +2935,21 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, PRE_DownloadCookieIsolation_CrossSession) {
 
 IN_PROC_BROWSER_TEST_F(WebViewTest,
                        MAYBE_DownloadCookieIsolation_CrossSession) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndDisableFeature(
-      download::features::kDownloadDBForNewDownloads);
-
   embedded_test_server()->RegisterRequestHandler(
       base::BindRepeating(&HandleDownloadRequestWithCookie));
   ASSERT_TRUE(StartEmbeddedTestServer());  // For serving guest pages.
 
+  scoped_refptr<base::TestMockTimeTaskRunner> task_runner(
+      new base::TestMockTimeTaskRunner);
+  download::SetDownloadDBTaskRunnerForTesting(task_runner);
+
   content::BrowserContext* browser_context = profile();
   content::DownloadManager* download_manager =
       content::BrowserContext::GetDownloadManager(browser_context);
 
-  DownloadHistoryWaiter history_waiter(browser_context);
-  history_waiter.WaitForHistoryLoad();
-
+  task_runner->FastForwardUntilNoTasksRemain();
+  DownloadManagerWaiter waiter(download_manager);
+  waiter.WaitForInitialized();
   content::DownloadManager::DownloadVector saved_downloads;
   download_manager->GetAllDownloads(&saved_downloads);
   ASSERT_EQ(2u, saved_downloads.size());
@@ -3018,7 +2992,7 @@ IN_PROC_BROWSER_TEST_F(WebViewTest,
     ASSERT_TRUE(download->GetFullPath().empty());
     EXPECT_EQ(download::DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED,
               download->GetLastReason());
-    download->Resume(false);
+    download->Resume(true);
   }
 
   completion_observer->WaitForFinished();

chrome/browser/download/download_history.cc

@@ -558,10 +558,6 @@ bool DownloadHistory::NeedToUpdateDownloadHistory(
   }
 #endif
 
-  if (!base::FeatureList::IsEnabled(
-          download::features::kDownloadDBForNewDownloads)) {
-    return true;
-  }
   // When download DB is enabled, only downloads that are in terminal state
   // are added to or updated in history DB. Non-transient in-progress and
   // interrupted download will be stored in the in-progress DB.

components/download/internal/common/download_db_cache.cc

@@ -171,18 +171,6 @@ void DownloadDBCache::UpdateDownloadDB() {
 }
 
 void DownloadDBCache::OnDownloadUpdated(DownloadItem* download) {
-  // TODO(crbug.com/778425): Properly handle fail/resume/retry for downloads
-  // that are in the INTERRUPTED state for a long time.
-  if (!base::FeatureList::IsEnabled(features::kDownloadDBForNewDownloads)) {
-    // If history service is still managing in-progress download, we can safely
-    // remove a download from the in-progress DB whenever it is in the terminal
-    // state. Otherwise, we need to propagate the completed download to
-    // history service before we can remove it.
-    if (download->IsDone()) {
-      OnDownloadRemoved(download);
-      return;
-    }
-  }
   DownloadDBEntry entry = CreateDownloadDBEntryFromItem(
       *(static_cast<DownloadItemImpl*>(download)));
   AddOrReplaceEntry(entry);

components/download/internal/common/download_task_runner.cc

@@ -35,6 +35,9 @@ base::LazySequencedTaskRunner g_download_task_runner =
 base::LazyInstance<scoped_refptr<base::SingleThreadTaskRunner>>::
     DestructorAtExit g_io_task_runner = LAZY_INSTANCE_INITIALIZER;
 
+base::LazyInstance<scoped_refptr<base::SequencedTaskRunner>>::DestructorAtExit
+    g_db_task_runner = LAZY_INSTANCE_INITIALIZER;
+
 // Lock to protect |g_io_task_runner|
 base::Lock& GetIOTaskRunnerLock() {
   static base::NoDestructor<base::Lock> instance;
@@ -63,4 +66,14 @@ scoped_refptr<base::SingleThreadTaskRunner> GetIOTaskRunner() {
   return g_io_task_runner.Get();
 }
 
+void SetDownloadDBTaskRunnerForTesting(
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner) {
+  DCHECK(task_runner);
+  g_db_task_runner.Get() = task_runner;
+}
+
+scoped_refptr<base::SequencedTaskRunner> GetDownloadDBTaskRunnerForTesting() {
+  return g_db_task_runner.Get();
+}
+
 }  // namespace download

components/download/internal/common/in_progress_download_manager.cc

@@ -353,6 +353,10 @@ void InProgressDownloadManager::Initialize(
 
   download_db_cache_ =
       std::make_unique<DownloadDBCache>(std::move(download_db));
+  if (GetDownloadDBTaskRunnerForTesting()) {
+    download_db_cache_->SetTimerTaskRunnerForTesting(
+        GetDownloadDBTaskRunnerForTesting());
+  }
   download_db_cache_->Initialize(base::BindOnce(
       &InProgressDownloadManager::OnDBInitialized, weak_factory_.GetWeakPtr()));
 }
@@ -581,36 +585,34 @@ void InProgressDownloadManager::OnDownloadNamesRetrieved(
         CreateDownloadEntryFromDownloadDBEntry(entry);
     if (download_entry)
       download_entries_[download_entry->guid] = download_entry.value();
-    if (base::FeatureList::IsEnabled(features::kDownloadDBForNewDownloads)) {
-      auto item = CreateDownloadItemImpl(this, entry);
-      if (!item)
-        continue;
-      uint32_t download_id = item->GetId();
-      // Remove entries with duplicate ids.
-      if (download_id != DownloadItem::kInvalidId &&
-          base::Contains(download_ids, download_id)) {
+    auto item = CreateDownloadItemImpl(this, entry);
+    if (!item)
+      continue;
+    uint32_t download_id = item->GetId();
+    // Remove entries with duplicate ids.
+    if (download_id != DownloadItem::kInvalidId &&
+        base::Contains(download_ids, download_id)) {
+      RemoveInProgressDownload(item->GetGuid());
+      num_duplicates++;
+      continue;
+    }
+#if defined(OS_ANDROID)
+    const base::FilePath& path = item->GetTargetFilePath();
+    if (path.IsContentUri()) {
+      base::FilePath display_name = GetDownloadDisplayName(path);
+      // If a download doesn't have a display name, remove it.
+      if (display_name.empty()) {
         RemoveInProgressDownload(item->GetGuid());
-        num_duplicates++;
         continue;
+      } else {
+        item->SetDisplayName(display_name);
       }
-#if defined(OS_ANDROID)
-      const base::FilePath& path = item->GetTargetFilePath();
-      if (path.IsContentUri()) {
-        base::FilePath display_name = GetDownloadDisplayName(path);
-        // If a download doesn't have a display name, remove it.
-        if (display_name.empty()) {
-          RemoveInProgressDownload(item->GetGuid());
-          continue;
-        } else {
-          item->SetDisplayName(display_name);
-        }
-      }
-#endif
-      item->AddObserver(download_db_cache_.get());
-      OnNewDownloadCreated(item.get());
-      in_progress_downloads_.emplace_back(std::move(item));
-      download_ids.insert(download_id);
     }
+#endif
+    item->AddObserver(download_db_cache_.get());
+    OnNewDownloadCreated(item.get());
+    in_progress_downloads_.emplace_back(std::move(item));
+    download_ids.insert(download_id);
   }
   if (num_duplicates > 0)
     RecordDuplicateInProgressDownloadIdCount(num_duplicates);

components/download/public/common/download_features.cc

@@ -30,9 +30,6 @@ const base::Feature kParallelDownloading {
 #endif
 };
 
-const base::Feature kDownloadDBForNewDownloads{
-    "DownloadDBForNewDownloads", base::FEATURE_ENABLED_BY_DEFAULT};
-
 #if defined(OS_ANDROID)
 const base::Feature kRefreshExpirationDate{"RefreshExpirationDate",
                                            base::FEATURE_ENABLED_BY_DEFAULT};

components/download/public/common/download_features.h

@@ -23,11 +23,6 @@ COMPONENTS_DOWNLOAD_EXPORT extern const base::Feature
 // Whether a download can be handled by parallel jobs.
 COMPONENTS_DOWNLOAD_EXPORT extern const base::Feature kParallelDownloading;
 
-// Whether metadata for new in-progress downloads will be be stored in download
-// DB, rather than history DB.
-COMPONENTS_DOWNLOAD_EXPORT extern const base::Feature
-    kDownloadDBForNewDownloads;
-
 #if defined(OS_ANDROID)
 // Whether download expiration date will be refreshed on resumption.
 COMPONENTS_DOWNLOAD_EXPORT extern const base::Feature kRefreshExpirationDate;

components/download/public/common/download_task_runner.h

@@ -23,6 +23,14 @@ COMPONENTS_DOWNLOAD_EXPORT void SetIOTaskRunner(
 COMPONENTS_DOWNLOAD_EXPORT scoped_refptr<base::SingleThreadTaskRunner>
 GetIOTaskRunner();
 
+// Sets the task runner for download DB, must be called on UI thread.
+COMPONENTS_DOWNLOAD_EXPORT void SetDownloadDBTaskRunnerForTesting(
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner);
+
+// Gets the task runner for download DB, must be called on UI thread.
+COMPONENTS_DOWNLOAD_EXPORT scoped_refptr<base::SequencedTaskRunner>
+GetDownloadDBTaskRunnerForTesting();
+
 }  // namespace download
 
 #endif  // COMPONENTS_DOWNLOAD_PUBLIC_COMMON_DOWNLOAD_TASK_RUNNER_H_

content/browser/download/download_manager_impl.cc

@@ -175,11 +175,9 @@ class DownloadItemFactoryImpl : public download::DownloadItemFactory {
       bool transient,
       const std::vector<download::DownloadItem::ReceivedSlice>& received_slices)
       override {
-    int auto_resume_count = 0;
-    if (base::FeatureList::IsEnabled(
-            download::features::kDownloadDBForNewDownloads)) {
-      auto_resume_count = download::DownloadItemImpl::kMaxAutoResumeAttempts;
-    }
+    // For history download only as history don't have auto resumption count
+    // saved.
+    int auto_resume_count = download::DownloadItemImpl::kMaxAutoResumeAttempts;
 
     return new download::DownloadItemImpl(
         delegate, guid, download_id, current_path, target_path, url_chain,