Skip to content

Commit

Permalink
Refactor FileSystemContext::OpenFileSystem() for custom bucket locators.
Browse files Browse the repository at this point in the history
As part of this change, OpenFileSystem() now expects a storage bucket to be provided (crbug.com/1317334) and returns a FileSystemURL object (possibly containing the custom bucket) instead of a GURL instance (crbug.com/1270739). For design details, see http://shortn/_8KlcyfHjA2.

Callers to these functions have been updated to expect a FileSystemURL object. First-party callers convert the value to a GURL wherever it is the cleanest (see crbug.com/1317395).

Note that FileSystemURL::ToGURL() doesn't know what to do with some storage types (e.g. kFileSystemTypeSyncable). In these cases, I'm using existing utility libraries to construct root GURLs based on the returned FileSystemURL's origin.

Fixed: 1317334
Fixed: 1270739
Fixed: 1317395
Change-Id: I6c27898b4bf4a0fc815491af563ef34f1096881e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3589849
Reviewed-by: Kyra Seevers <kyraseevers@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Nathan Eliason <eliason@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Derek Schuff <dschuff@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Reviewed-by: Christopher Lam <calamity@chromium.org>
Reviewed-by: Tim Sergeant <tsergeant@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1002659}
  • Loading branch information
Nathan Eliason authored and Chromium LUCI CQ committed May 12, 2022
1 parent eea1004 commit bca1174
Show file tree
Hide file tree
Showing 39 changed files with 212 additions and 112 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "chrome/browser/sync_file_system/sync_file_status.h"
#include "chrome/browser/sync_file_system/sync_file_system_service.h"
#include "chrome/browser/sync_file_system/sync_file_system_service_factory.h"
#include "chrome/browser/sync_file_system/syncable_file_system_util.h"
#include "chrome/common/apps/platform_apps/api/sync_file_system.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_task_traits.h"
Expand Down Expand Up @@ -155,12 +156,13 @@ SyncFileSystemRequestFileSystemFunction::Run() {
// Initializes sync context for this extension and continue to open
// a new file system.
content::GetIOThreadTaskRunner({})->PostTask(
FROM_HERE, BindOnce(&storage::FileSystemContext::OpenFileSystem,
GetFileSystemContext(),
blink::StorageKey(url::Origin::Create(source_url())),
storage::kFileSystemTypeSyncable,
storage::OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
base::BindOnce(&self::DidOpenFileSystem, this)));
FROM_HERE,
BindOnce(&storage::FileSystemContext::OpenFileSystem,
GetFileSystemContext(),
blink::StorageKey(url::Origin::Create(source_url())),
/*bucket=*/absl::nullopt, storage::kFileSystemTypeSyncable,
storage::OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
base::BindOnce(&self::DidOpenFileSystem, this)));
return RespondLater();
}

Expand All @@ -173,7 +175,7 @@ SyncFileSystemRequestFileSystemFunction::GetFileSystemContext() {
}

void SyncFileSystemRequestFileSystemFunction::DidOpenFileSystem(
const GURL& root_url,
const storage::FileSystemURL& root_url,
const std::string& file_system_name,
base::File::Error error) {
// Repost to switch from IO thread to UI thread for SendResponse().
Expand All @@ -195,7 +197,9 @@ void SyncFileSystemRequestFileSystemFunction::DidOpenFileSystem(

std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
dict->SetStringKey("name", file_system_name);
dict->SetStringKey("root", root_url.spec());
dict->SetStringKey("root", ::sync_file_system::GetSyncableFileSystemRootURI(
root_url.origin().GetURL())
.spec());
Respond(OneArgument(base::Value::FromUniquePtrValue(std::move(dict))));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ class SyncFileSystemRequestFileSystemFunction : public ExtensionFunction {
// Returns the file system context for this extension.
storage::FileSystemContext* GetFileSystemContext();

void DidOpenFileSystem(const GURL& root_url,
void DidOpenFileSystem(const storage::FileSystemURL& root_url,
const std::string& file_system_name,
base::File::Error error);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@
#include "base/run_loop.h"
#include "content/public/test/browser_task_environment.h"
#include "storage/browser/file_system/file_system_context.h"
#include "storage/browser/file_system/file_system_url.h"
#include "storage/browser/quota/quota_manager_proxy.h"
#include "storage/browser/test/async_file_test_helper.h"
#include "storage/browser/test/test_file_system_context.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "third_party/blink/public/common/storage_key/storage_key.h"
#include "url/gurl.h"

namespace arc {

Expand Down Expand Up @@ -48,10 +48,10 @@ class FileStreamForwarderTest : public testing::Test {

context_->OpenFileSystem(
blink::StorageKey::CreateFromStringForTesting(kURLOrigin),
storage::kFileSystemTypeTemporary,
/*bucket=*/absl::nullopt, storage::kFileSystemTypeTemporary,
storage::OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
base::BindOnce([](const GURL& root_url, const std::string& name,
base::File::Error result) {
base::BindOnce([](const storage::FileSystemURL& root_url,
const std::string& name, base::File::Error result) {
EXPECT_EQ(base::File::FILE_OK, result);
}));
base::RunLoop().RunUntilIdle();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@
#include "mojo/public/cpp/system/data_pipe.h"
#include "mojo/public/cpp/system/simple_watcher.h"
#include "storage/browser/file_system/file_system_context.h"
#include "storage/browser/file_system/file_system_url.h"
#include "storage/browser/quota/quota_manager_proxy.h"
#include "storage/browser/test/async_file_test_helper.h"
#include "storage/browser/test/test_file_system_context.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/storage_key/storage_key.h"
#include "url/gurl.h"

namespace arc {

Expand Down Expand Up @@ -62,10 +62,10 @@ class ShareInfoFileStreamAdapterTest : public testing::Test {

file_system_context_->OpenFileSystem(
blink::StorageKey::CreateFromStringForTesting(kURLOrigin),
storage::kFileSystemTypeTemporary,
/*bucket=*/absl::nullopt, storage::kFileSystemTypeTemporary,
storage::OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
base::BindOnce([](const GURL& root_url, const std::string& name,
base::File::Error result) {
base::BindOnce([](const storage::FileSystemURL& root_url,
const std::string& name, base::File::Error result) {
ASSERT_EQ(base::File::FILE_OK, result);
}));
base::RunLoop().RunUntilIdle();
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/chromeos/fileapi/file_system_backend.cc
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ void FileSystemBackend::Initialize(storage::FileSystemContext* context) {}

void FileSystemBackend::ResolveURL(const storage::FileSystemURL& url,
storage::OpenFileSystemMode mode,
OpenFileSystemCallback callback) {
ResolveURLCallback callback) {
std::string id;
storage::FileSystemType type;
std::string cracked_id;
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/chromeos/fileapi/file_system_backend.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ constexpr char kSystemMountNameRemovable[] = "removable";
//
class FileSystemBackend : public storage::ExternalFileSystemBackend {
public:
using storage::FileSystemBackend::OpenFileSystemCallback;
using storage::FileSystemBackend::ResolveURLCallback;

// |system_mount_points| should outlive FileSystemBackend instance.
FileSystemBackend(
Expand Down Expand Up @@ -105,7 +105,7 @@ class FileSystemBackend : public storage::ExternalFileSystemBackend {
void Initialize(storage::FileSystemContext* context) override;
void ResolveURL(const storage::FileSystemURL& url,
storage::OpenFileSystemMode mode,
OpenFileSystemCallback callback) override;
ResolveURLCallback callback) override;
storage::AsyncFileUtil* GetAsyncFileUtil(
storage::FileSystemType type) override;
storage::WatcherManager* GetWatcherManager(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ void MediaFileSystemBackend::Initialize(storage::FileSystemContext* context) {

void MediaFileSystemBackend::ResolveURL(const FileSystemURL& url,
storage::OpenFileSystemMode mode,
OpenFileSystemCallback callback) {
ResolveURLCallback callback) {
// We never allow opening a new FileSystem via usual ResolveURL.
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(callback), GURL(), std::string(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class MediaFileSystemBackend : public storage::FileSystemBackend {
void Initialize(storage::FileSystemContext* context) override;
void ResolveURL(const storage::FileSystemURL& url,
storage::OpenFileSystemMode mode,
OpenFileSystemCallback callback) override;
ResolveURLCallback callback) override;
storage::AsyncFileUtil* GetAsyncFileUtil(
storage::FileSystemType type) override;
storage::WatcherManager* GetWatcherManager(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -526,8 +526,9 @@ void CannedSyncableFileSystem::DoOpenFileSystem(
EXPECT_TRUE(io_task_runner_->RunsTasksInCurrentSequence());
EXPECT_FALSE(is_filesystem_opened_);
file_system_context_->OpenFileSystem(
blink::StorageKey(url::Origin::Create(origin_)), type_,
storage::OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT, std::move(callback));
blink::StorageKey(url::Origin::Create(origin_)), /*bucket=*/absl::nullopt,
type_, storage::OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
std::move(callback));
}

void CannedSyncableFileSystem::DoCreateDirectory(const FileSystemURL& url,
Expand Down Expand Up @@ -685,7 +686,7 @@ void CannedSyncableFileSystem::DoGetUsageAndQuota(
void CannedSyncableFileSystem::DidOpenFileSystem(
base::SingleThreadTaskRunner* original_task_runner,
base::OnceClosure quit_closure,
const GURL& root,
const storage::FileSystemURL& root,
const std::string& name,
File::Error result) {
if (io_task_runner_->RunsTasksInCurrentSequence()) {
Expand All @@ -702,7 +703,7 @@ void CannedSyncableFileSystem::DidOpenFileSystem(
return;
}
result_ = result;
root_url_ = root;
root_url_ = GetSyncableFileSystemRootURI(root.origin().GetURL());
std::move(quit_closure).Run();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ class SyncFileSystemBackend;
class CannedSyncableFileSystem
: public LocalFileSyncStatus::Observer {
public:
typedef base::OnceCallback<
void(const GURL& root, const std::string& name, base::File::Error result)>
typedef base::OnceCallback<void(const storage::FileSystemURL& root,
const std::string& name,
base::File::Error result)>
OpenFileSystemCallback;
typedef base::OnceCallback<void(base::File::Error)> StatusCallback;
typedef base::RepeatingCallback<void(int64_t)> WriteCallback;
Expand Down Expand Up @@ -214,7 +215,7 @@ class CannedSyncableFileSystem
// Callbacks.
void DidOpenFileSystem(base::SingleThreadTaskRunner* original_task_runner,
base::OnceClosure quit_closure,
const GURL& root,
const storage::FileSystemURL& root,
const std::string& name,
base::File::Error result);
void DidInitializeFileSystemContext(base::OnceClosure quit_closure,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "base/bind.h"
#include "base/callback_helpers.h"
#include "base/containers/contains.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/location.h"
#include "base/observer_list.h"
Expand All @@ -29,6 +30,8 @@
#include "storage/browser/file_system/file_system_file_util.h"
#include "storage/browser/file_system/file_system_operation_context.h"
#include "storage/browser/file_system/file_system_operation_runner.h"
#include "storage/browser/file_system/file_system_url.h"
#include "storage/common/file_system/file_system_types.h"
#include "storage/common/file_system/file_system_util.h"
#include "third_party/blink/public/common/storage_key/storage_key.h"
#include "url/origin.h"
Expand Down Expand Up @@ -90,7 +93,7 @@ void LocalFileSyncContext::MaybeInitializeFileSystemContext(
// for writable way (even when MaybeInitializeFileSystemContext is called
// from read-only OpenFileSystem), so open the filesystem with
// CREATE_IF_NONEXISTENT here.
storage::FileSystemBackend::OpenFileSystemCallback open_filesystem_callback =
storage::FileSystemBackend::ResolveURLCallback open_filesystem_callback =
base::BindOnce(
&LocalFileSyncContext::InitializeFileSystemContextOnIOThread, this,
source_url, base::RetainedRef(file_system_context));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ void SyncFileSystemBackend::Initialize(storage::FileSystemContext* context) {

void SyncFileSystemBackend::ResolveURL(const storage::FileSystemURL& url,
storage::OpenFileSystemMode mode,
OpenFileSystemCallback callback) {
ResolveURLCallback callback) {
DCHECK(CanHandleType(url.type()));

if (skip_initialize_syncfs_service_for_testing_) {
Expand Down Expand Up @@ -266,7 +266,7 @@ void SyncFileSystemBackend::DidInitializeSyncFileSystemService(
const GURL& origin_url,
storage::FileSystemType type,
storage::OpenFileSystemMode mode,
OpenFileSystemCallback callback,
ResolveURLCallback callback,
SyncStatusCode status) {
// Repost to switch from UI thread to IO thread.
if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class SyncFileSystemBackend : public storage::FileSystemBackend {
void Initialize(storage::FileSystemContext* context) override;
void ResolveURL(const storage::FileSystemURL& url,
storage::OpenFileSystemMode mode,
OpenFileSystemCallback callback) override;
ResolveURLCallback callback) override;
storage::AsyncFileUtil* GetAsyncFileUtil(
storage::FileSystemType type) override;
storage::WatcherManager* GetWatcherManager(
Expand Down Expand Up @@ -106,7 +106,7 @@ class SyncFileSystemBackend : public storage::FileSystemBackend {
const GURL& origin_url,
storage::FileSystemType type,
storage::OpenFileSystemMode mode,
OpenFileSystemCallback callback,
ResolveURLCallback callback,
SyncStatusCode status);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@
#include "mojo/public/cpp/bindings/self_owned_receiver.h"
#include "mojo/public/cpp/system/data_pipe.h"
#include "mojo/public/cpp/system/simple_watcher.h"
#include "storage/browser/file_system/file_system_url.h"
#include "storage/browser/file_system/open_file_system_mode.h"
#include "storage/browser/quota/quota_manager_proxy.h"
#include "storage/browser/test/async_file_test_helper.h"
#include "storage/browser/test/test_file_system_context.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/storage_key/storage_key.h"
#include "url/gurl.h"
#include "url/origin.h"

namespace {
Expand Down Expand Up @@ -90,14 +90,14 @@ class FileStreamDataPipeGetterTest : public testing::Test {
base::RunLoop run_loop;
file_system_context_->OpenFileSystem(
blink::StorageKey::CreateFromStringForTesting(kURLOrigin),
storage::kFileSystemTypeTemporary,
/*bucket=*/absl::nullopt, storage::kFileSystemTypeTemporary,
storage::OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
base::BindLambdaForTesting([&run_loop](const GURL& root_url,
const std::string& name,
base::File::Error result) {
ASSERT_EQ(base::File::FILE_OK, result);
run_loop.Quit();
}));
base::BindLambdaForTesting(
[&run_loop](const storage::FileSystemURL& root_url,
const std::string& name, base::File::Error result) {
ASSERT_EQ(base::File::FILE_OK, result);
run_loop.Quit();
}));
run_loop.Run();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ class FileSystemHelperTest : public testing::Test {
// Callback that should be executed in response to
// storage::FileSystemContext::OpenFileSystem.
void OpenFileSystemCallback(base::RunLoop* run_loop,
const GURL& root,
const storage::FileSystemURL& root,
const std::string& name,
base::File::Error error) {
open_file_system_result_ = error;
Expand All @@ -93,7 +93,8 @@ class FileSystemHelperTest : public testing::Test {
browser_context_.GetDefaultStoragePartition()
->GetFileSystemContext()
->OpenFileSystem(
blink::StorageKey(origin), type, open_mode,
blink::StorageKey(origin), /*bucket=*/absl::nullopt, type,
open_mode,
base::BindOnce(&FileSystemHelperTest::OpenFileSystemCallback,
base::Unretained(this), &run_loop));
BlockUntilQuit(&run_loop);
Expand Down
9 changes: 5 additions & 4 deletions content/browser/blob_storage/blob_url_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ class BlobURLTest : public testing::Test {
base::RunLoop run_loop;
file_system_context_->OpenFileSystem(
blink::StorageKey::CreateFromStringForTesting(kFileSystemURLOrigin),
kFileSystemType, storage::OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
/*bucket=*/absl::nullopt, kFileSystemType,
storage::OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
base::BindOnce(&BlobURLTest::OnValidateFileSystem,
base::Unretained(this), run_loop.QuitClosure()));
run_loop.Run();
Expand All @@ -147,7 +148,7 @@ class BlobURLTest : public testing::Test {
}

GURL GetFileSystemURL(const std::string& filename) {
return GURL(file_system_root_url_.spec() + filename);
return GURL(file_system_root_url_.ToGURL().spec() + filename);
}

void WriteFileSystemFile(const std::string& filename,
Expand All @@ -172,7 +173,7 @@ class BlobURLTest : public testing::Test {
}

void OnValidateFileSystem(base::OnceClosure quit_closure,
const GURL& root,
const storage::FileSystemURL& root,
const std::string& name,
base::File::Error result) {
ASSERT_EQ(base::File::FILE_OK, result);
Expand Down Expand Up @@ -329,7 +330,7 @@ class BlobURLTest : public testing::Test {
base::FilePath temp_file2_;
base::Time temp_file_modification_time1_;
base::Time temp_file_modification_time2_;
GURL file_system_root_url_;
storage::FileSystemURL file_system_root_url_;
GURL temp_file_system_file1_;
GURL temp_file_system_file2_;
base::Time temp_file_system_file_modification_time1_;
Expand Down
7 changes: 4 additions & 3 deletions content/browser/file_system/file_system_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,8 @@ void FileSystemManagerImpl::ContinueOpen(
RecordAction(base::UserMetricsAction("OpenFileSystemPersistent"));
}
context_->OpenFileSystem(
storage_key, ToStorageFileSystemType(file_system_type),
storage_key, /*bucket=*/absl::nullopt,
ToStorageFileSystemType(file_system_type),
storage::OPEN_FILE_SYSTEM_CREATE_IF_NONEXISTENT,
base::BindOnce(&FileSystemManagerImpl::DidOpenFileSystem, GetWeakPtr(),
std::move(callback)));
Expand Down Expand Up @@ -1136,12 +1137,12 @@ void FileSystemManagerImpl::DidWriteSync(WriteSyncCallbackEntry* entry,

void FileSystemManagerImpl::DidOpenFileSystem(
OpenCallback callback,
const GURL& root,
const FileSystemURL& root,
const std::string& filesystem_name,
base::File::Error result) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
DCHECK(root.is_valid() || result != base::File::FILE_OK);
std::move(callback).Run(filesystem_name, root, result);
std::move(callback).Run(filesystem_name, root.ToGURL(), result);
// For OpenFileSystem we do not create a new operation, so no unregister here.
}

Expand Down
2 changes: 1 addition & 1 deletion content/browser/file_system/file_system_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ class CONTENT_EXPORT FileSystemManagerImpl
int64_t bytes,
bool complete);
void DidOpenFileSystem(OpenCallback callback,
const GURL& root,
const storage::FileSystemURL& root,
const std::string& filesystem_name,
base::File::Error result);
void DidResolveURL(ResolveURLCallback callback,
Expand Down

0 comments on commit bca1174

Please sign in to comment.