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()); }