Commit 950748a2 authored by Min Qin's avatar Min Qin Committed by Commit Bot

move InProgressCache and some methods into InProgressDownloadManager

Currently the InProgressCache is owned by ChromeDownloadManagerDelegate.
In order for download to work as a service,  the InProgressDownloadManager
should be able to manage InProgressCache. It should read from the cache
and recreate the download before resuming it.
This CL moves the InProgressCache from ChromeDownloadManagerDelegate into
InProgressDownloadmanager. And it also moves StartDownload() method as
this is needed when resuming the download without DownloadManagerImpl.
DownloadManagerImpl now becomes a delegate of InProgressDownloadManager.
It will provide InProgressDownloadManager the information to create a new
download. When resuming a download after launching download service,
DownloadManagerImpl is not needed.

BUG=803135

Change-Id: Ie7a22213e935638dac10dd82d2e079326e8083f2
Reviewed-on: https://chromium-review.googlesource.com/1024723
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: default avatarJohn Abd-El-Malek <jam@chromium.org>
Reviewed-by: default avatarDavid Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553359}
parent a6860a42
......@@ -272,11 +272,6 @@ ChromeDownloadManagerDelegate::ChromeDownloadManagerDelegate(Profile* profile)
{base::MayBlock(), base::TaskPriority::BACKGROUND,
base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN})),
weak_ptr_factory_(this) {
DCHECK(!profile_->GetPath().empty());
base::FilePath metadata_cache_file =
profile_->GetPath().Append(chrome::kDownloadMetadataStoreFilename);
download_metadata_cache_.reset(new download::InProgressCacheImpl(
metadata_cache_file, disk_access_task_runner_));
#if defined(OS_ANDROID)
location_dialog_bridge_.reset(new DownloadLocationDialogBridgeImpl);
#endif
......@@ -572,11 +567,6 @@ void ChromeDownloadManagerDelegate::ChooseSavePath(
callback);
}
download::InProgressCache* ChromeDownloadManagerDelegate::GetInProgressCache() {
DCHECK(download_metadata_cache_ != nullptr);
return download_metadata_cache_.get();
}
void ChromeDownloadManagerDelegate::SanitizeSavePackageResourceName(
base::FilePath* filename) {
safe_browsing::FileTypePolicies* file_type_policies =
......
......@@ -97,7 +97,6 @@ class ChromeDownloadManagerDelegate
const base::FilePath::StringType& default_extension,
bool can_save_as_complete,
const content::SavePackagePathPickedCallback& callback) override;
download::InProgressCache* GetInProgressCache() override;
void SanitizeSavePackageResourceName(base::FilePath* filename) override;
void OpenDownload(download::DownloadItem* download) override;
bool IsMostRecentDownloadItemAtFilePath(
......@@ -215,8 +214,6 @@ class ChromeDownloadManagerDelegate
Profile* profile_;
std::unique_ptr<download::InProgressCache> download_metadata_cache_;
#if defined(OS_ANDROID)
std::unique_ptr<DownloadLocationDialogBridge> location_dialog_bridge_;
#endif
......@@ -231,9 +228,9 @@ class ChromeDownloadManagerDelegate
IdCallbackVector id_callbacks_;
std::unique_ptr<DownloadPrefs> download_prefs_;
// SequencedTaskRunner to check for file existence and read/write metadata
// cache. A sequence is used so that a large download history doesn't cause a
// large number of concurrent disk operations.
// SequencedTaskRunner to check for file existence. A sequence is used so
// that a large download history doesn't cause a large number of concurrent
// disk operations.
const scoped_refptr<base::SequencedTaskRunner> disk_access_task_runner_;
#if BUILDFLAG(ENABLE_EXTENSIONS)
......
......@@ -139,8 +139,6 @@ const base::FilePath::CharType kCRLSetFilename[] =
FPL("Certificate Revocation Lists");
const base::FilePath::CharType kCustomDictionaryFileName[] =
FPL("Custom Dictionary.txt");
const base::FilePath::CharType kDownloadMetadataStoreFilename[] =
FPL("in_progress_download_metadata_store");
const base::FilePath::CharType kDownloadServiceStorageDirname[] =
FPL("Download Service");
const base::FilePath::CharType kExtensionActivityLogFilename[] =
......
......@@ -47,7 +47,6 @@ extern const base::FilePath::CharType kChannelIDFilename[];
extern const base::FilePath::CharType kCookieFilename[];
extern const base::FilePath::CharType kCRLSetFilename[];
extern const base::FilePath::CharType kCustomDictionaryFileName[];
extern const base::FilePath::CharType kDownloadMetadataStoreFilename[];
extern const base::FilePath::CharType kDownloadServiceStorageDirname[];
extern const base::FilePath::CharType kExtensionActivityLogFilename[];
extern const base::FilePath::CharType kExtensionsCookieFilename[];
......
......@@ -7,6 +7,7 @@
#include <string>
#include "components/download/public/common/download_item.h"
#include "components/download/public/common/download_source.h"
#include "components/download/public/common/download_url_parameters.h"
#include "services/metrics/public/cpp/ukm_source_id.h"
......
......@@ -12,6 +12,8 @@
namespace download {
extern const base::FilePath::CharType kDownloadMetadataStoreFilename[];
// InProgressCache provides a write-through cache that persists
// information related to an in-progress download such as request origin, retry
// count, resumption parameters etc to the disk. The entries are written to disk
......
......@@ -14,6 +14,9 @@
namespace download {
const base::FilePath::CharType kDownloadMetadataStoreFilename[] =
FILE_PATH_LITERAL("in_progress_download_metadata_store");
namespace {
// Helper functions for |entries_| related operations.
......
......@@ -5,6 +5,7 @@
#include "components/download/public/common/download_utils.h"
#include "base/format_macros.h"
#include "base/rand_util.h"
#include "base/strings/stringprintf.h"
#include "components/download/public/common/download_create_info.h"
#include "components/download/public/common/download_interrupt_reasons_utils.h"
......@@ -347,4 +348,24 @@ std::unique_ptr<net::HttpRequestHeaders> GetAdditionalRequestHeaders(
return headers;
}
DownloadEntry CreateDownloadEntryFromItem(
const DownloadItem& item,
const std::string& request_origin,
DownloadSource download_source,
bool fetch_error_body,
const DownloadUrlParameters::RequestHeadersType& request_headers) {
return DownloadEntry(item.GetGuid(), request_origin, download_source,
fetch_error_body, request_headers,
GetUniqueDownloadId());
}
uint64_t GetUniqueDownloadId() {
// Get a new UKM download_id that is not 0.
uint64_t download_id = 0;
do {
download_id = base::RandUint64();
} while (download_id == 0);
return download_id;
}
} // namespace download
......@@ -5,8 +5,11 @@
#ifndef COMPONENTS_DOWNLOAD_PUBLIC_COMMON_DOWNLOAD_UTILS_H_
#define COMPONENTS_DOWNLOAD_PUBLIC_COMMON_DOWNLOAD_UTILS_H_
#include "components/download/downloader/in_progress/download_entry.h"
#include "components/download/public/common/download_export.h"
#include "components/download/public/common/download_interrupt_reasons.h"
#include "components/download/public/common/download_item.h"
#include "components/download/public/common/download_source.h"
#include "net/base/net_errors.h"
#include "net/cert/cert_status_flags.h"
#include "net/http/http_response_headers.h"
......@@ -57,6 +60,16 @@ COMPONENTS_DOWNLOAD_EXPORT int GetLoadFlags(DownloadUrlParameters* params,
COMPONENTS_DOWNLOAD_EXPORT std::unique_ptr<net::HttpRequestHeaders>
GetAdditionalRequestHeaders(DownloadUrlParameters* params);
// Helper functions for DownloadItem -> DownloadEntry for InProgressCache.
COMPONENTS_DOWNLOAD_EXPORT DownloadEntry CreateDownloadEntryFromItem(
const DownloadItem& item,
const std::string& request_origin,
DownloadSource download_source,
bool fetch_error_body,
const DownloadUrlParameters::RequestHeadersType& request_headers);
COMPONENTS_DOWNLOAD_EXPORT uint64_t GetUniqueDownloadId();
} // namespace download
#endif // COMPONENTS_DOWNLOAD_PUBLIC_COMMON_DOWNLOAD_UTILS_H_
......@@ -12,10 +12,15 @@
#include "base/memory/weak_ptr.h"
#include "base/optional.h"
#include "components/download/public/common/download_export.h"
#include "components/download/public/common/download_file_factory.h"
#include "components/download/public/common/download_item_impl_delegate.h"
#include "components/download/public/common/url_download_handler.h"
#include "url/gurl.h"
namespace net {
class URLRequestContextGetter;
}
namespace network {
struct ResourceResponse;
}
......@@ -24,17 +29,53 @@ namespace download {
class DownloadURLLoaderFactoryGetter;
class DownloadUrlParameters;
class InProgressCache;
// Manager for handling all active downloads.
class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
: public UrlDownloadHandler::Delegate,
public DownloadItemImplDelegate {
public:
InProgressDownloadManager(UrlDownloadHandler::Delegate* delegate);
using DownloadIdCallback = base::RepeatingCallback<void(uint32_t)>;
// Class to be notified when download starts/stops.
class COMPONENTS_DOWNLOAD_EXPORT Delegate {
public:
// Intercepts the download to another system if applicable. Returns true if
// the download was intercepted.
virtual bool InterceptDownload(
const DownloadCreateInfo& download_create_info) = 0;
// Called to get an ID for a new download. |callback| may be called
// synchronously.
virtual void GetNextId(const DownloadIdCallback& callback) = 0;
// Gets the default download directory.
virtual base::FilePath GetDefaultDownloadDirectory() = 0;
// Gets the download item for the given id.
// TODO(qinmin): remove this method and let InProgressDownloadManager
// create the DownloadItem from in-progress cache.
virtual DownloadItemImpl* GetDownloadItem(
uint32_t id,
bool new_download,
const DownloadCreateInfo& download_create_info) = 0;
// Gets the URLRequestContextGetter for sending requests.
// TODO(qinmin): remove this once network service is fully enabled.
virtual net::URLRequestContextGetter* GetURLRequestContextGetter(
const DownloadCreateInfo& download_create_info) = 0;
// Called when a new download is started.
virtual void OnNewDownloadStarted(DownloadItem* download) = 0;
};
using IsOriginSecureCallback = base::RepeatingCallback<bool(const GURL&)>;
InProgressDownloadManager(Delegate* delegate,
const IsOriginSecureCallback& is_origin_secure_cb);
~InProgressDownloadManager() override;
// Called to start a download.
void StartDownload(
void BeginDownload(
std::unique_ptr<DownloadUrlParameters> params,
scoped_refptr<DownloadURLLoaderFactoryGetter> url_loader_factory_getter,
uint32_t download_id,
......@@ -57,9 +98,32 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
network::mojom::URLLoaderClientEndpointsPtr url_loader_client_endpoints,
scoped_refptr<DownloadURLLoaderFactoryGetter> url_loader_factory_getter);
// TODO(qinmin): change the |callback| to be an OnceClosure.
void Initialize(const base::FilePath& metadata_cache_dir,
const base::RepeatingClosure& callback);
void StartDownload(
std::unique_ptr<DownloadCreateInfo> info,
std::unique_ptr<InputStream> stream,
scoped_refptr<DownloadURLLoaderFactoryGetter> url_loader_factory_getter,
const DownloadUrlParameters::OnStartedCallback& on_started);
// Shutting down the manager and stop all downloads.
void ShutDown();
// DownloadItemImplDelegate implementations.
void ResumeInterruptedDownload(std::unique_ptr<DownloadUrlParameters> params,
uint32_t id,
const GURL& site_url) override;
base::Optional<DownloadEntry> GetInProgressEntry(
DownloadItemImpl* download) override;
void ReportBytesWasted(DownloadItemImpl* download) override;
void set_file_factory(std::unique_ptr<DownloadFileFactory> file_factory) {
file_factory_ = std::move(file_factory);
}
DownloadFileFactory* file_factory() { return file_factory_.get(); }
private:
// UrlDownloadHandler::Delegate implementations.
void OnUrlDownloadStarted(
......@@ -71,20 +135,33 @@ class COMPONENTS_DOWNLOAD_EXPORT InProgressDownloadManager
void OnUrlDownloadHandlerCreated(
UrlDownloadHandler::UniqueUrlDownloadHandlerPtr downloader) override;
// Overridden from DownloadItemImplDelegate.
void ResumeInterruptedDownload(std::unique_ptr<DownloadUrlParameters> params,
uint32_t id,
const GURL& site_url) override;
// Start a download with given ID.
void StartDownloadWithId(
std::unique_ptr<DownloadCreateInfo> info,
std::unique_ptr<InputStream> stream,
scoped_refptr<DownloadURLLoaderFactoryGetter> url_loader_factory_getter,
const DownloadUrlParameters::OnStartedCallback& on_started,
bool new_download,
uint32_t id);
// Active download handlers.
std::vector<UrlDownloadHandler::UniqueUrlDownloadHandlerPtr>
url_download_handlers_;
// Delegate to call when download starts. Only OnUrlDownloadStarted() is being
// passed through for now as we need content layer to provide default download
// dir.
// TODO(qinmin): remove this once this class drives all active download.
UrlDownloadHandler::Delegate* delegate_;
// Delegate to provide information to create a new download. Can be null.
Delegate* delegate_;
// Factory for the creation of download files.
std::unique_ptr<DownloadFileFactory> file_factory_;
// Cache for storing metadata about in progress downloads.
std::unique_ptr<InProgressCache> download_metadata_cache_;
// listens to information about in-progress download items.
std::unique_ptr<DownloadItem::Observer> in_progress_download_observer_;
// callback to check if an origin is secure.
IsOriginSecureCallback is_origin_secure_cb_;
base::WeakPtrFactory<InProgressDownloadManager> weak_factory_;
......
......@@ -22,6 +22,7 @@
#include "base/synchronization/lock.h"
#include "components/download/public/common/download_item_impl_delegate.h"
#include "components/download/public/common/download_url_parameters.h"
#include "components/download/public/common/in_progress_download_manager.h"
#include "components/download/public/common/url_download_handler.h"
#include "content/browser/loader/navigation_url_loader.h"
#include "content/common/content_export.h"
......@@ -37,7 +38,6 @@ class DownloadFileFactory;
class DownloadItemFactory;
class DownloadItemImpl;
class DownloadRequestHandleInterface;
class InProgressDownloadManager;
}
namespace content {
......@@ -47,6 +47,7 @@ class StoragePartitionImpl;
class CONTENT_EXPORT DownloadManagerImpl
: public DownloadManager,
public download::UrlDownloadHandler::Delegate,
public download::InProgressDownloadManager::Delegate,
private download::DownloadItemImplDelegate {
public:
using DownloadItemImplCreated =
......@@ -181,15 +182,6 @@ class CONTENT_EXPORT DownloadManagerImpl
friend class DownloadManagerTest;
friend class DownloadTest;
void StartDownloadWithId(
std::unique_ptr<download::DownloadCreateInfo> info,
std::unique_ptr<download::InputStream> stream,
scoped_refptr<download::DownloadURLLoaderFactoryGetter>
url_loader_factory_getter,
const download::DownloadUrlParameters::OnStartedCallback& on_started,
bool new_download,
uint32_t id);
void CreateSavePackageDownloadItemWithId(
const base::FilePath& main_file_path,
const GURL& page_url,
......@@ -200,9 +192,17 @@ class CONTENT_EXPORT DownloadManagerImpl
const DownloadItemImplCreated& on_started,
uint32_t id);
// Intercepts the download to another system if applicable. Returns true if
// the download was intercepted.
bool InterceptDownload(const download::DownloadCreateInfo& info);
// InProgressDownloadManager::Delegate implementations.
bool InterceptDownload(const download::DownloadCreateInfo& info) override;
base::FilePath GetDefaultDownloadDirectory() override;
download::DownloadItemImpl* GetDownloadItem(
uint32_t id,
bool new_download,
const download::DownloadCreateInfo& info) override;
net::URLRequestContextGetter* GetURLRequestContextGetter(
const download::DownloadCreateInfo& info) override;
void OnNewDownloadStarted(download::DownloadItem* download) override;
void GetNextId(const DownloadIdCallback& callback) override;
// Create a new active item based on the info. Separate from
// StartDownload() for testing.
......@@ -210,9 +210,6 @@ class CONTENT_EXPORT DownloadManagerImpl
uint32_t id,
const download::DownloadCreateInfo& info);
// Get next download id. |callback| is called on the UI thread and may
// be called synchronously.
void GetNextId(const DownloadIdCallback& callback);
// Called with the result of DownloadManagerDelegate::CheckForFileExistence.
// Updates the state of the file and then notifies this update to the file's
......@@ -264,9 +261,6 @@ class CONTENT_EXPORT DownloadManagerImpl
// Factory for creation of downloads items.
std::unique_ptr<download::DownloadItemFactory> item_factory_;
// Factory for the creation of download files.
std::unique_ptr<download::DownloadFileFactory> file_factory_;
// |downloads_| is the owning set for all downloads known to the
// DownloadManager. This includes downloads started by the user in
// this session, downloads initialized from the history system, and
......
......@@ -161,14 +161,20 @@ class MockDownloadItemFactory
std::unique_ptr<download::DownloadRequestHandleInterface> request_handle)
override;
void set_is_download_started(bool is_download_started) {
is_download_started_ = is_download_started;
}
private:
std::map<uint32_t, download::MockDownloadItemImpl*> items_;
download::DownloadItemImplDelegate item_delegate_;
bool is_download_started_;
DISALLOW_COPY_AND_ASSIGN(MockDownloadItemFactory);
};
MockDownloadItemFactory::MockDownloadItemFactory() {}
MockDownloadItemFactory::MockDownloadItemFactory()
: is_download_started_(false) {}
MockDownloadItemFactory::~MockDownloadItemFactory() {}
......@@ -243,6 +249,10 @@ download::DownloadItemImpl* MockDownloadItemFactory::CreateActiveItem(
.WillRepeatedly(Return(download_id));
EXPECT_CALL(*result, GetGuid())
.WillRepeatedly(ReturnRefOfCopy(base::GenerateGUID()));
if (is_download_started_) {
EXPECT_CALL(*result, RemoveObserver(_));
EXPECT_CALL(*result, AddObserver(_));
}
items_[download_id] = result;
// Active items are created and then immediately are called to start
......@@ -444,6 +454,7 @@ class DownloadManagerTest : public testing::Test {
// Key test variable; we'll keep it available to sub-classes.
std::unique_ptr<DownloadManagerImpl> download_manager_;
base::WeakPtr<MockDownloadFileFactory> mock_download_file_factory_;
base::WeakPtr<MockDownloadItemFactory> mock_download_item_factory_;
// Target detetermined callback.
bool callback_called_;
......@@ -457,7 +468,6 @@ class DownloadManagerTest : public testing::Test {
private:
TestBrowserThreadBundle thread_bundle_;
base::WeakPtr<MockDownloadItemFactory> mock_download_item_factory_;
std::unique_ptr<MockDownloadManagerDelegate> mock_download_manager_delegate_;
std::unique_ptr<MockDownloadManagerObserver> observer_;
std::unique_ptr<TestBrowserContext> browser_context_;
......@@ -495,6 +505,7 @@ TEST_F(DownloadManagerTest, StartDownload) {
MockCreateFile(Ref(*info->save_info.get()), input_stream.get()))
.WillOnce(Return(mock_file));
mock_download_item_factory_->set_is_download_started(true);
download_manager_->StartDownload(
std::move(info), std::move(input_stream), nullptr,
download::DownloadUrlParameters::OnStartedCallback());
......
......@@ -6,7 +6,6 @@
#include "base/format_macros.h"
#include "base/process/process_handle.h"
#include "base/rand_util.h"
#include "base/strings/stringprintf.h"
#include "base/task_scheduler/post_task.h"
#include "components/download/downloader/in_progress/download_entry.h"
......@@ -99,14 +98,4 @@ std::unique_ptr<net::URLRequest> CreateURLRequestOnIOThread(
return request;
}
// Gets the unique download id for ukm reporting.
uint64_t GetUniqueDownloadId() {
// Get a new UKM download_id that is not 0.
uint64_t download_id = 0;
do {
download_id = base::RandUint64();
} while (download_id == 0);
return download_id;
}
} // namespace content
......@@ -33,8 +33,6 @@ CreateURLRequestOnIOThread(download::DownloadUrlParameters* params);
storage::BlobStorageContext* BlobStorageContextGetter(
ResourceContext* resource_context);
uint64_t GetUniqueDownloadId();
} // namespace content
#endif // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_UTILS_H_
......@@ -30,11 +30,11 @@
#include "components/download/public/common/download_stats.h"
#include "components/download/public/common/download_task_runner.h"
#include "components/download/public/common/download_ukm_helper.h"
#include "components/download/public/common/download_utils.h"
#include "components/filename_generation/filename_generation.h"
#include "components/url_formatter/url_formatter.h"
#include "content/browser/bad_message.h"
#include "content/browser/download/download_manager_impl.h"
#include "content/browser/download/download_utils.h"
#include "content/browser/download/save_file.h"
#include "content/browser/download/save_file_manager.h"
#include "content/browser/download/save_item.h"
......@@ -259,7 +259,7 @@ void SavePackage::InternalInit() {
download::RecordSavePackageEvent(download::SAVE_PACKAGE_STARTED);
ukm_source_id_ = ukm::UkmRecorder::GetNewSourceID();
ukm_download_id_ = GetUniqueDownloadId();
ukm_download_id_ = download::GetUniqueDownloadId();
download::DownloadUkmHelper::RecordDownloadStarted(
ukm_download_id_, ukm_source_id_, download::DownloadContent::TEXT,
download::DownloadSource::UNKNOWN);
......
......@@ -53,10 +53,6 @@ bool DownloadManagerDelegate::GenerateFileHash() {
return false;
}
download::InProgressCache* DownloadManagerDelegate::GetInProgressCache() {
return nullptr;
}
std::string
DownloadManagerDelegate::ApplicationClientIdForFileScanning() const {
return std::string();
......
......@@ -17,9 +17,6 @@
#include "content/public/browser/resource_request_info.h"
#include "content/public/browser/save_page_type.h"
namespace download {
class InProgressCache;
} // namespace download
namespace content {
......@@ -138,9 +135,6 @@ class CONTENT_EXPORT DownloadManagerDelegate {
base::FilePath* download_save_dir,
bool* skip_dir_check) {}
// Returns the metadata cache for in-progress downloads.
virtual download::InProgressCache* GetInProgressCache();
// Asks the user for the path to save a page. The delegate calls the callback
// to give the answer.
virtual void ChooseSavePath(
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment