From cec7cc2e7229d1e662211a40a9efec9f13534c0e Mon Sep 17 00:00:00 2001
From: Xing Liu <xingliu@google.com>
Date: Thu, 22 Jul 2021 21:55:18 +0000
Subject: [PATCH] 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}
---
.../ios/background_download_service_impl.cc | 41 ++++++++++++++++---
.../ios/background_download_service_impl.h | 10 ++++-
...ckground_download_service_impl_unittest.cc | 13 ++++--
.../ios/background_download_task_helper.h | 6 +--
.../ios/background_download_task_helper.mm | 34 +++++++++++----
...ackground_download_task_helper_unittest.mm | 14 ++++---
.../background_service/ios/entry_utils.cc | 3 ++
.../background_download_service_factory.cc | 4 +-
8 files changed, 98 insertions(+), 27 deletions(-)
diff --git a/components/download/internal/background_service/ios/background_download_service_impl.cc b/components/download/internal/background_service/ios/background_download_service_impl.cc
index 1a5297e648a36..6bc59811aec0c 100644
--- a/components/download/internal/background_service/ios/background_download_service_impl.cc
+++ b/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
diff --git a/components/download/internal/background_service/ios/background_download_service_impl.h b/components/download/internal/background_service/ios/background_download_service_impl.h
index 7b593325de27f..483d3110c882a 100644
--- a/components/download/internal/background_service/ios/background_download_service_impl.h
+++ b/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};
};
diff --git a/components/download/internal/background_service/ios/background_download_service_impl_unittest.cc b/components/download/internal/background_service/ios/background_download_service_impl_unittest.cc
index fce552cf94fb5..9e088039061ed 100644
--- a/components/download/internal/background_service/ios/background_download_service_impl_unittest.cc
+++ b/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();
}
diff --git a/components/download/internal/background_service/ios/background_download_task_helper.h b/components/download/internal/background_service/ios/background_download_task_helper.h
index 46802a655f86e..4a6d8066b1530 100644
--- a/components/download/internal/background_service/ios/background_download_task_helper.h
+++ b/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(
diff --git a/components/download/internal/background_service/ios/background_download_task_helper.mm b/components/download/internal/background_service/ios/background_download_task_helper.mm
index d562cc16d135a..f2cbc7ca1c726 100644
--- a/components/download/internal/background_service/ios/background_download_task_helper.mm
+++ b/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
diff --git a/components/download/internal/background_service/ios/background_download_task_helper_unittest.mm b/components/download/internal/background_service/ios/background_download_task_helper_unittest.mm
index dda06a4078881..c2b4f38351396 100644
--- a/components/download/internal/background_service/ios/background_download_task_helper_unittest.mm
+++ b/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_);
diff --git a/components/download/internal/background_service/ios/entry_utils.cc b/components/download/internal/background_service/ios/entry_utils.cc
index 5c190db0fa498..4e586e0dceb3b 100644
--- a/components/download/internal/background_service/ios/entry_utils.cc
+++ b/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
diff --git a/ios/chrome/browser/download/background_service/background_download_service_factory.cc b/ios/chrome/browser/download/background_service/background_download_service_factory.cc
index b79c256206395..1f9abc73eaec6 100644
--- a/ios/chrome/browser/download/background_service/background_download_service_factory.cc
+++ b/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());
}