Skip to content

Commit

Permalink
[FilesBulkPinning] Unpin files that are deleted on the cloud
Browse files Browse the repository at this point in the history
The default option when files are deleted from the cloud is to move the
file to the trash. On ChromeOS the drive trash isn't surfaced so they
sit in the cache and remained pinned. To avoid this (and allow Cello to
prune the cache) unpin the files when a kDelete event is received.

Bug: b:266005690
Test: browser_tests --gtest_filter=*driveCloudDeleteUnpinsItem*
Change-Id: I8af673a86115ad8be4705fb0bab935bdf0723720
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4199118
Reviewed-by: Austin Tankiang <austinct@chromium.org>
Commit-Queue: Ben Reich <benreich@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1098492}
  • Loading branch information
ben-reich authored and Chromium LUCI CQ committed Jan 30, 2023
1 parent 59412b9 commit e109ff3
Show file tree
Hide file tree
Showing 7 changed files with 115 additions and 17 deletions.
3 changes: 2 additions & 1 deletion chrome/browser/ash/file_manager/file_manager_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1466,7 +1466,8 @@ WRAPPED_INSTANTIATE_TEST_SUITE_P(
TestCase("driveDeleteDialogDoesntMentionPermanentDelete"),
TestCase("driveInlineSyncStatusSingleFile").EnableInlineStatusSync(),
TestCase("driveInlineSyncStatusParentFolder").EnableInlineStatusSync(),
TestCase("driveLocalDeleteUnpinsItem").EnableBulkPinning()
TestCase("driveLocalDeleteUnpinsItem").EnableBulkPinning(),
TestCase("driveCloudDeleteUnpinsItem").EnableBulkPinning()
// TODO(b/189173190): Enable
// TestCase("driveEnableDocsOfflineDialog"),
// TODO(b/189173190): Enable
Expand Down
22 changes: 22 additions & 0 deletions chrome/browser/ash/file_manager/file_manager_browsertest_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1321,6 +1321,21 @@ class DriveFsTestVolume : public TestVolume {
drivefs_delegate.FlushForTesting();
}

void SendCloudDeleteEvent(const std::string& path) {
const base::FilePath file_path(path);
absl::optional<drivefs::FakeDriveFs::FileMetadata> metadata =
fake_drivefs_helper_->fake_drivefs().GetItemMetadata(file_path);
ASSERT_TRUE(metadata.has_value()) << "No file metadata with path: " << path;

std::vector<drivefs::mojom::FileChangePtr> file_changes;
file_changes.emplace_back(absl::in_place, file_path,
drivefs::mojom::FileChange::Type::kDelete,
metadata.value().stable_id);
auto& drivefs_delegate = fake_drivefs_helper_->fake_drivefs().delegate();
drivefs_delegate->OnFilesChanged(std::move(file_changes));
drivefs_delegate.FlushForTesting();
}

absl::optional<drivefs::mojom::DialogResult> last_dialog_result() {
return last_dialog_result_;
}
Expand Down Expand Up @@ -3266,6 +3281,13 @@ void FileManagerBrowserTestBase::OnCommand(const std::string& name,
return;
}

if (name == "sendDriveCloudDeleteEvent") {
const std::string* path = value.FindString("path");
ASSERT_TRUE(path) << "No supplied path to sendDriveFilesChangedEvent";
drive_volume_->SendCloudDeleteEvent(*path);
return;
}

if (HandleGuestOsCommands(name, value, output)) {
return;
}
Expand Down
16 changes: 15 additions & 1 deletion chromeos/ash/components/drivefs/drivefs_pin_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -801,7 +801,21 @@ void PinManager::OnFilesChanged(const std::vector<mojom::FileChange>& changes) {

if (progress_.stage != Stage::kSyncing) {
for (const mojom::FileChange& change : changes) {
VLOG(1) << "Ignored FileChange " << Quote(change);
switch (change.type) {
case mojom::FileChange::Type::kDelete:
VLOG(1) << "Got FileChange " << Quote(change);
drivefs_->SetPinnedByStableId(
change.stable_id, /*pinned=*/false,
base::BindOnce([](drive::FileError status) {
LOG_IF(ERROR, status != drive::FILE_ERROR_OK)
<< "Failed to unpin file: "
<< drive::FileErrorToString(status);
}));
break;
default:
VLOG(1) << "Ignored FileChange " << Quote(change);
break;
}
}
return;
}
Expand Down
30 changes: 16 additions & 14 deletions chromeos/ash/components/drivefs/fake_drivefs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -90,20 +90,6 @@ FakeDriveFsBootstrapListener::bootstrap() {
return std::move(bootstrap_);
}

struct FakeDriveFs::FileMetadata {
std::string mime_type;
bool pinned = false;
bool hosted = false;
bool shared = false;
bool available_offline = false;
std::string original_name;
mojom::Capabilities capabilities;
mojom::FolderFeature folder_feature;
std::string doc_id;
int64_t stable_id = 0;
std::string alternate_url;
};

class FakeDriveFs::SearchQuery : public mojom::SearchQuery {
public:
SearchQuery(base::WeakPtr<FakeDriveFs> drive_fs,
Expand Down Expand Up @@ -258,6 +244,13 @@ class FakeDriveFs::SearchQuery : public mojom::SearchQuery {
base::WeakPtrFactory<SearchQuery> weak_ptr_factory_{this};
};

FakeDriveFs::FileMetadata::FileMetadata() = default;
FakeDriveFs::FileMetadata::FileMetadata(const FakeDriveFs::FileMetadata&) =
default;
FakeDriveFs::FileMetadata& FakeDriveFs::FileMetadata::operator=(
const FakeDriveFs::FileMetadata&) = default;
FakeDriveFs::FileMetadata::~FileMetadata() = default;

FakeDriveFs::FakeDriveFs(const base::FilePath& mount_path)
: mount_path_(mount_path) {
CHECK(mount_path.IsAbsolute());
Expand Down Expand Up @@ -331,6 +324,15 @@ absl::optional<bool> FakeDriveFs::IsItemPinned(const std::string& path) {
return absl::nullopt;
}

absl::optional<FakeDriveFs::FileMetadata> FakeDriveFs::GetItemMetadata(
const base::FilePath& path) {
const auto& metadata = metadata_.find(path);
if (metadata == metadata_.end()) {
return absl::nullopt;
}
return metadata->second;
}

void FakeDriveFs::Init(
drivefs::mojom::DriveFsConfigurationPtr config,
mojo::PendingReceiver<drivefs::mojom::DriveFs> receiver,
Expand Down
23 changes: 22 additions & 1 deletion chromeos/ash/components/drivefs/fake_drivefs.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,29 @@ class FakeDriveFs : public drivefs::mojom::DriveFs,

absl::optional<bool> IsItemPinned(const std::string& path);

struct FileMetadata {
FileMetadata();
FileMetadata(const FileMetadata&);
FileMetadata& operator=(const FileMetadata&);
~FileMetadata();

std::string mime_type;
bool pinned = false;
bool hosted = false;
bool shared = false;
bool available_offline = false;
std::string original_name;
mojom::Capabilities capabilities;
mojom::FolderFeature folder_feature;
std::string doc_id;
int64_t stable_id = 0;
std::string alternate_url;
};

absl::optional<FakeDriveFs::FileMetadata> GetItemMetadata(
const base::FilePath& path);

private:
struct FileMetadata;
class SearchQuery;

// drivefs::mojom::DriveFsBootstrap:
Expand Down
24 changes: 24 additions & 0 deletions ui/file_manager/integration_tests/file_manager/drive_specific.js
Original file line number Diff line number Diff line change
Expand Up @@ -1298,3 +1298,27 @@ testcase.driveLocalDeleteUnpinsItem = async () => {
// Ensure the file was unpinned prior to deleting.
await remoteCall.expectDriveItemPinnedStatus(appId, '/root/test.txt', false);
};

/**
* Test that when files get deleted in the cloud, they get unpinned after being
* deleted.
*/
testcase.driveCloudDeleteUnpinsItem = async () => {
const appId = await setupAndWaitUntilReady(RootPath.DRIVE);

// Select test.txt which is already pinned.
await remoteCall.waitAndClickElement(
appId, '#file-list [file-name="test.txt"]');
await remoteCall.waitForElement(
appId, '[file-name="test.txt"][selected] xf-icon[type=offline]');

// Ensure the metadata for the file is set to pinned.
await remoteCall.expectDriveItemPinnedStatus(appId, '/root/test.txt', true);

await remoteCall.sendDriveCloudDeleteEvent(appId, '/root/test.txt');
await remoteCall.waitForElementLost(
appId, '#file-list [file-name="test.txt"]');

// Ensure the file was unpinned prior to deleting.
await remoteCall.expectDriveItemPinnedStatus(appId, '/root/test.txt', false);
};
14 changes: 14 additions & 0 deletions ui/file_manager/integration_tests/remote_call.js
Original file line number Diff line number Diff line change
Expand Up @@ -998,4 +998,18 @@ export class RemoteCallFilesApp extends RemoteCall {
}),
String(status));
}

/**
* Send a delete event via the `OnFilesChanged` drivefs delegate method.
* @param {string} appId app window ID.
* @param {string} path Path from the drive mount point, e.g. /root/test.txt
*/
async sendDriveCloudDeleteEvent(appId, path) {
await this.waitFor('isFileManagerLoaded', appId, true);
await sendTestMessage({
appId,
name: 'sendDriveCloudDeleteEvent',
path,
});
}
}

0 comments on commit e109ff3

Please sign in to comment.