Backgroun download: Update database for download progress.

On other platforms, the bytes downloaded is persisted in history db,
and write to the background download proto db after download completion.

On iOS, since we don't use history db, update progress should be
directly written to background download proto db.

Also a throttle is used to limit the frequency of db update.

When download is finished, we read the file size and also update the db
with the actual file size.

Bug: 1215315
Change-Id: I19d9a479a67ef21ef9896d4b3c4fed8b56f3b067
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3043448
Reviewed-by: Min Qin <qinmin@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#904497}

components/download/internal/background_service/ios/background_download_service_impl.cc

@@ -8,6 +8,7 @@
 
 #include "base/logging.h"
 #include "base/notreached.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/download/internal/background_service/client_set.h"
 #include "components/download/internal/background_service/config.h"
@@ -21,6 +22,9 @@
 namespace download {
 namespace {
 
+// Interval to throttle the download update that results in a database update.
+const base::TimeDelta kUpdateInterval = base::TimeDelta::FromSeconds(5);
+
 void InvokeStartCallback(const std::string& guid,
                          DownloadParams::StartResult result,
                          DownloadParams::StartCallback callback) {
@@ -35,12 +39,14 @@ void InvokeStartCallback(const std::string& guid,
 BackgroundDownloadServiceImpl::BackgroundDownloadServiceImpl(
     std::unique_ptr<ClientSet> clients,
     std::unique_ptr<Model> model,
-    std::unique_ptr<BackgroundDownloadTaskHelper> download_helper)
+    std::unique_ptr<BackgroundDownloadTaskHelper> download_helper,
+    base::Clock* clock)
     : config_(std::make_unique<Configuration>()),
       service_config_(config_.get()),
       clients_(std::move(clients)),
       model_(std::move(model)),
-      download_helper_(std::move(download_helper)) {
+      download_helper_(std::move(download_helper)),
+      clock_(clock) {
   model_->Initialize(this);
 }
 
@@ -175,7 +181,8 @@ void BackgroundDownloadServiceImpl::OnDownloadFinished(
     DownloadClient download_client,
     const std::string& guid,
     bool success,
-    const base::FilePath& file_path) {
+    const base::FilePath& file_path,
+    int64_t file_size) {
   download::Client* client = clients_->GetClient(download_client);
   if (!client)
     return;
@@ -189,6 +196,14 @@ void BackgroundDownloadServiceImpl::OnDownloadFinished(
 
     return;
   }
+
+  Entry* entry = model_->Get(guid);
+  if (!entry)
+    return;
+
+  entry->bytes_downloaded = base::saturated_cast<uint64_t>(file_size);
+  model_->Update(*entry);
+
   CompletionInfo completion_info;
   completion_info.path = file_path;
   client->OnDownloadSucceeded(guid, completion_info);
@@ -198,12 +213,28 @@ void BackgroundDownloadServiceImpl::OnDownloadUpdated(
     DownloadClient download_client,
     const std::string& guid,
     int64_t bytes_downloaded) {
+  uint64_t bytes_count = base::saturated_cast<uint64_t>(bytes_downloaded);
+  MaybeUpdateProgress(guid, bytes_count);
+
   download::Client* client = clients_->GetClient(download_client);
   if (!client)
     return;
 
-  client->OnDownloadUpdated(guid, /*bytes_uploaded*/ 0u,
-                            static_cast<uint64_t>(bytes_downloaded));
+  client->OnDownloadUpdated(guid, /*bytes_uploaded*/ 0u, bytes_count);
+}
+
+void BackgroundDownloadServiceImpl::MaybeUpdateProgress(
+    const std::string& guid,
+    uint64_t bytes_downloaded) {
+  // Throttle the model update frequency.
+  if (clock_->Now() - update_time_ < kUpdateInterval)
+    return;
+
+  update_time_ = clock_->Now();
+  Entry* entry = model_->Get(guid);
+  DCHECK_GE(bytes_downloaded, 0u);
+  entry->bytes_downloaded = bytes_downloaded;
+  model_->Update(*entry);
 }
 
 }  // namespace download

components/download/internal/background_service/ios/background_download_service_impl.h

@@ -10,6 +10,7 @@
 #include <string>
 
 #include "base/memory/weak_ptr.h"
+#include "base/time/clock.h"
 #include "components/download/internal/background_service/model_impl.h"
 #include "components/download/internal/background_service/service_config_impl.h"
 #include "components/download/public/background_service/background_download_service.h"
@@ -31,7 +32,8 @@ class BackgroundDownloadServiceImpl : public BackgroundDownloadService,
   BackgroundDownloadServiceImpl(
       std::unique_ptr<ClientSet> clients,
       std::unique_ptr<Model> model,
-      std::unique_ptr<BackgroundDownloadTaskHelper> download_helper);
+      std::unique_ptr<BackgroundDownloadTaskHelper> download_helper,
+      base::Clock* clock);
   ~BackgroundDownloadServiceImpl() override;
 
  private:
@@ -65,11 +67,13 @@ class BackgroundDownloadServiceImpl : public BackgroundDownloadService,
   void OnDownloadFinished(DownloadClient download_client,
                           const std::string& guid,
                           bool success,
-                          const base::FilePath& file_path);
+                          const base::FilePath& file_path,
+                          int64_t file_size);
 
   void OnDownloadUpdated(DownloadClient download_client,
                          const std::string& guid,
                          int64_t bytes_downloaded);
+  void MaybeUpdateProgress(const std::string& guid, uint64_t bytes_downloaded);
 
   std::unique_ptr<Configuration> config_;
   ServiceConfigImpl service_config_;
@@ -78,6 +82,8 @@ class BackgroundDownloadServiceImpl : public BackgroundDownloadService,
   std::unique_ptr<BackgroundDownloadTaskHelper> download_helper_;
   absl::optional<bool> init_success_;
   std::map<std::string, DownloadParams::StartCallback> start_callbacks_;
+  base::Time update_time_;
+  base::Clock* clock_;
 
   base::WeakPtrFactory<BackgroundDownloadServiceImpl> weak_ptr_factory_{this};
 };

components/download/internal/background_service/ios/background_download_service_impl_unittest.cc

@@ -10,6 +10,7 @@
 
 #include "base/test/gmock_callback_support.h"
 #include "base/test/mock_callback.h"
+#include "base/test/simple_test_clock.h"
 #include "base/test/task_environment.h"
 #include "components/download/internal/background_service/client_set.h"
 #include "components/download/internal/background_service/ios/background_download_task_helper.h"
@@ -68,7 +69,8 @@ class BackgroundDownloadServiceImplTest : public PlatformTest {
     auto download_helper = std::make_unique<MockBackgroundDownloadTaskHelper>();
     download_helper_ = download_helper.get();
     service_ = std::make_unique<BackgroundDownloadServiceImpl>(
-        std::move(client_set), std::move(model), std::move(download_helper));
+        std::move(client_set), std::move(model), std::move(download_helper),
+        &clock_);
   }
 
   BackgroundDownloadService* service() { return service_.get(); }
@@ -85,6 +87,7 @@ class BackgroundDownloadServiceImplTest : public PlatformTest {
   }
 
   base::test::TaskEnvironment task_environment_;
+  base::SimpleTestClock clock_;
   MockBackgroundDownloadTaskHelper* download_helper_;
   test::TestStore* store_;
   test::MockClient* client_;
@@ -122,7 +125,7 @@ TEST_F(BackgroundDownloadServiceImplTest, StartDownloadHelperFailure) {
   store_->TriggerInit(/*success=*/true, empty_entries());
   EXPECT_CALL(start_callback_, Run(kGuid, StartResult::ACCEPTED));
   EXPECT_CALL(*download_helper_, StartDownload(_, _, _, _, _))
-      .WillOnce(RunOnceCallback<3>(/*success=*/false, base::FilePath()));
+      .WillOnce(RunOnceCallback<3>(/*success=*/false, base::FilePath(), 0));
   EXPECT_CALL(*client_,
               OnDownloadFailed(kGuid, CompletionInfoIs(base::FilePath()),
                                download::Client::FailureReason::UNKNOWN));
@@ -138,7 +141,7 @@ TEST_F(BackgroundDownloadServiceImplTest, StartDownloadSuccess) {
   EXPECT_CALL(start_callback_, Run(kGuid, StartResult::ACCEPTED));
   EXPECT_CALL(*download_helper_, StartDownload(_, _, _, _, _))
       .WillOnce(
-          RunOnceCallback<3>(/*success=*/true, base::FilePath(kFilePath)));
+          RunOnceCallback<3>(/*success=*/true, base::FilePath(kFilePath), 0));
   EXPECT_CALL(
       *client_,
       OnDownloadSucceeded(kGuid, CompletionInfoIs(base::FilePath(kFilePath))));
@@ -158,8 +161,12 @@ TEST_F(BackgroundDownloadServiceImplTest, OnDownloadUpdated) {
   EXPECT_CALL(*client_, OnDownloadUpdated(kGuid, 0u, 10u));
   auto download_params = CreateDownloadParams(kURL);
   service()->StartDownload(std::move(download_params));
+
+  // Advance the time to make sure the update is not throttled.
+  clock_.Advance(base::TimeDelta::FromSeconds(11));
   store_->TriggerUpdate(/*success=*/true);
   EXPECT_EQ(kGuid, store_->LastUpdatedEntry()->guid);
+  EXPECT_EQ(10u, store_->LastUpdatedEntry()->bytes_downloaded);
   task_environment_.RunUntilIdle();
 }
 

components/download/internal/background_service/ios/background_download_task_helper.h

@@ -25,10 +25,10 @@ struct SchedulingParams;
 // app will not be waked up to resume the download.
 class BackgroundDownloadTaskHelper {
  public:
-  // Callback with whether download is succeeded and the file path of the
-  // succeeded download.
+  // Callback with whether download is succeeded and the file path and the file
+  // size of the succeeded download.
   using CompletionCallback =
-      base::OnceCallback<void(bool, const base::FilePath&)>;
+      base::OnceCallback<void(bool, const base::FilePath&, int64_t)>;
   // Callback with number of bytes downloaded.
   using UpdateCallback = base::RepeatingCallback<void(int64_t)>;
   static std::unique_ptr<BackgroundDownloadTaskHelper> Create(

components/download/internal/background_service/ios/background_download_task_helper.mm

@@ -50,9 +50,10 @@ using UpdateCallback = download::BackgroundDownloadTaskHelper::UpdateCallback;
 }
 
 - (void)invokeCompletionHandler:(bool)success
-                       filePath:(base::FilePath)filePath {
+                       filePath:(base::FilePath)filePath
+                       fileSize:(int64_t)fileSize {
   if (_completionCallback)
-    std::move(_completionCallback).Run(success, filePath);
+    std::move(_completionCallback).Run(success, filePath, fileSize);
 }
 
 #pragma mark - NSURLSessionDownloadDelegate
@@ -83,29 +84,48 @@ using UpdateCallback = download::BackgroundDownloadTaskHelper::UpdateCallback;
     didFinishDownloadingToURL:(NSURL*)location {
   DVLOG(1) << __func__;
   if (!location) {
-    [self invokeCompletionHandler:/*success=*/false filePath:base::FilePath()];
+    [self invokeCompletionHandler:/*success=*/false
+                         filePath:base::FilePath()
+                         fileSize:0];
     return;
   }
 
   // Make sure the target directory exists.
   if (!base::CreateDirectory(_downloadDir)) {
     LOG(ERROR) << "Failed to create dir:" << _downloadDir;
-    [self invokeCompletionHandler:/*success=*/false filePath:base::FilePath()];
+    [self invokeCompletionHandler:/*success=*/false
+                         filePath:base::FilePath()
+                         fileSize:0];
     return;
   }
 
   // Move the downloaded file from platform temporary directory to download
-  // service's target directory.
+  // service's target directory. This must happen immediately on the current
+  // thread or iOS may delete the file.
   const base::FilePath tempPath =
       base::mac::NSStringToFilePath([location path]);
   base::FilePath newFile = _downloadDir.AppendASCII(_guid);
   if (!base::Move(tempPath, newFile)) {
     LOG(ERROR) << "Failed to move file from:" << tempPath
                << ", to:" << _downloadDir;
-    [self invokeCompletionHandler:/*success=*/false filePath:base::FilePath()];
+    [self invokeCompletionHandler:/*success=*/false
+                         filePath:base::FilePath()
+                         fileSize:0];
     return;
   }
-  [self invokeCompletionHandler:/*success=*/true filePath:newFile];
+
+  // Get the file size on current thread.
+  int64_t fileSize = 0;
+  if (!base::GetFileSize(newFile, &fileSize)) {
+    LOG(ERROR) << "Failed to get file size from:" << newFile;
+    [self invokeCompletionHandler:/*success=*/false
+                         filePath:base::FilePath()
+                         fileSize:0];
+    return;
+  }
+  [self invokeCompletionHandler:/*success=*/true
+                       filePath:newFile
+                       fileSize:fileSize];
 }
 
 #pragma mark - NSURLSessionDelegate

components/download/internal/background_service/ios/background_download_task_helper_unittest.mm

@@ -56,12 +56,14 @@ class BackgroundDownloadTaskHelperTest : public PlatformTest {
     base::RunLoop loop;
     helper_->StartDownload(
         kGuid, params.request_params, params.scheduling_params,
-        base::BindLambdaForTesting([&](bool, const base::FilePath& file_path) {
-          std::string content;
-          ASSERT_TRUE(base::ReadFileToString(file_path, &content));
-          EXPECT_EQ(kDefaultResponseContent, content);
-          loop.Quit();
-        }),
+        base::BindLambdaForTesting(
+            [&](bool, const base::FilePath& file_path, int64_t file_size) {
+              std::string content;
+              ASSERT_TRUE(base::ReadFileToString(file_path, &content));
+              EXPECT_EQ(kDefaultResponseContent, content);
+              EXPECT_EQ(file_size, static_cast<int64_t>(content.size()));
+              loop.Quit();
+            }),
         base::DoNothing());
     loop.Run();
     DCHECK(request_sent_);

components/download/internal/background_service/ios/entry_utils.cc

@@ -24,6 +24,9 @@ MapEntriesToMetadataForClients(const std::set<DownloadClient>& clients,
     meta_data.guid = entry->guid;
     // iOS currently doesn't support pause.
     meta_data.paused = false;
+    // Unlike other platforms that uses history db through download driver, the
+    // current size on iOS is always based on background download proto db
+    // record.
     meta_data.current_size = entry->bytes_downloaded;
     if (entry->state == Entry::State::COMPLETE) {
       // TODO(xingliu): Implement the response headers and url chain with

ios/chrome/browser/download/background_service/background_download_service_factory.cc

@@ -8,6 +8,7 @@
 
 #include "base/task/task_traits.h"
 #include "base/task/thread_pool.h"
+#include "base/time/default_clock.h"
 #include "components/download/internal/background_service/client_set.h"
 #include "components/download/internal/background_service/download_store.h"
 #include "components/download/internal/background_service/ios/background_download_service_impl.h"
@@ -72,5 +73,6 @@ BackgroundDownloadServiceFactory::BuildServiceInstanceFor(
   return std::make_unique<download::BackgroundDownloadServiceImpl>(
       std::move(client_set), std::move(model),
       download::BackgroundDownloadTaskHelper::Create(
-          storage_dir.Append(kFilesStorageDir)));
+          storage_dir.Append(kFilesStorageDir)),
+      base::DefaultClock::GetInstance());
 }