Skip to content

Commit

Permalink
[Files]: Adds a query field to the getRecentFiles parameters.
Browse files Browse the repository at this point in the history
This CL is one of many that aims at eliminating the searchFiles private
API. The goal is to make getRecentFiles as capable as searchFiles. Then
use the getRecentFiles as the source provider of search results. The
reason why getRecentFiles is made more capable is that offers a better
architecture with recent sources, and caching.

In this CL we add the query parameter and push it to all recent sources.
Unit tests are augmented to test this new functionality.

Bug: b:307455066
Change-Id: I48a2d5667fefdf0eb5d3c39bc654b2e61e6510a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4969261
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Bo Majewski <majewski@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1216776}
  • Loading branch information
Bo Majewski authored and Chromium LUCI CQ committed Oct 30, 2023
1 parent 0229c0d commit d519089
Show file tree
Hide file tree
Showing 21 changed files with 175 additions and 63 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1017,7 +1017,7 @@ FileManagerPrivateInternalGetRecentFilesFunction::Run() {
model->SetScanTimeout(base::Milliseconds(3000));
}
model->GetRecentFiles(
file_system_context.get(), source_url(), file_type,
file_system_context.get(), source_url(), params->query, file_type,
params->invalidate_cache,
base::BindOnce(
&FileManagerPrivateInternalGetRecentFilesFunction::OnGetRecentFiles,
Expand Down
10 changes: 8 additions & 2 deletions chrome/browser/ash/fileapi/recent_arc_media_source.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "base/memory/raw_ptr.h"
#include "base/metrics/histogram_macros.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/ash/arc/arc_util.h"
#include "chrome/browser/ash/arc/fileapi/arc_documents_provider_root.h"
#include "chrome/browser/ash/arc/fileapi/arc_documents_provider_root_map.h"
Expand Down Expand Up @@ -187,6 +188,7 @@ void RecentArcMediaSource::MediaRoot::OnGetRecentDocuments(
DCHECK_EQ(0, num_inflight_readdirs_);
DCHECK(document_id_to_file_.empty());

const std::u16string q16 = base::UTF8ToUTF16(params_->query());
// Initialize |document_id_to_file_| with recent document IDs returned.
if (maybe_documents.has_value()) {
for (const auto& document : maybe_documents.value()) {
Expand All @@ -197,6 +199,9 @@ void RecentArcMediaSource::MediaRoot::OnGetRecentDocuments(
document->android_file_system_path.value())) {
continue;
}
if (!FileNameMatches(base::UTF8ToUTF16(document->display_name), q16)) {
continue;
}
document_id_to_file_.emplace(document->document_id, absl::nullopt);
}
}
Expand Down Expand Up @@ -377,8 +382,9 @@ void RecentArcMediaSource::GetRecentFiles(Params params) {
for (auto& root : roots_) {
root->GetRecentFiles(
Params(params_.value().file_system_context(), params_.value().origin(),
params_.value().max_files(), params_.value().cutoff_time(),
params_.value().end_time(), params_.value().file_type(),
params_.value().max_files(), params_.value().query(),
params_.value().cutoff_time(), params_.value().end_time(),
params_.value().file_type(),
base::BindOnce(&RecentArcMediaSource::OnGetRecentFilesForRoot,
weak_ptr_factory_.GetWeakPtr())));
}
Expand Down
47 changes: 37 additions & 10 deletions chrome/browser/ash/fileapi/recent_arc_media_source_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -168,14 +168,16 @@ class RecentArcMediaSourceTest : public testing::Test {
}

std::vector<RecentFile> GetRecentFiles(
const std::string query,
RecentSource::FileType file_type = RecentSource::FileType::kAll) {
std::vector<RecentFile> files;

base::RunLoop run_loop;

source_->GetRecentFiles(RecentSource::Params(
nullptr /* file_system_context */, GURL() /* origin */,
1 /* max_files: ignored */, base::Time() /* cutoff_time: ignored */,
1 /* max_files: ignored */, query,
base::Time() /* cutoff_time: ignored */,
base::TimeTicks::Max() /* end_time: ignored */,
file_type /* file_type */,
base::BindOnce(
Expand Down Expand Up @@ -209,7 +211,7 @@ class RecentArcMediaSourceTest : public testing::Test {
TEST_F(RecentArcMediaSourceTest, Normal) {
EnableFakeFileSystemInstance();

std::vector<RecentFile> files = GetRecentFiles();
std::vector<RecentFile> files = GetRecentFiles("");

ASSERT_EQ(6u, files.size());
EXPECT_EQ(
Expand Down Expand Up @@ -254,28 +256,40 @@ TEST_F(RecentArcMediaSourceTest, Normal) {
files[5].url().path());
EXPECT_EQ(base::Time::FromMillisecondsSinceUnixEpoch(9),
files[5].last_modified());

files = GetRecentFiles("text");
ASSERT_EQ(1u, files.size());
EXPECT_EQ(
arc::GetDocumentsProviderMountPath(arc::kMediaDocumentsProviderAuthority,
arc::kDocumentsRootDocumentId)
.Append("text.txt"),
files[0].url().path());
}

TEST_F(RecentArcMediaSourceTest, ArcNotAvailable) {
std::vector<RecentFile> files = GetRecentFiles();
std::vector<RecentFile> files = GetRecentFiles("");
EXPECT_EQ(0u, files.size());

files = GetRecentFiles("hot");
EXPECT_EQ(0u, files.size());
}

TEST_F(RecentArcMediaSourceTest, Deferred) {
EnableFakeFileSystemInstance();
EnableDefer();

std::vector<RecentFile> files = GetRecentFiles();
std::vector<RecentFile> files = GetRecentFiles("");
EXPECT_EQ(0u, files.size());

files = GetRecentFiles("word");
EXPECT_EQ(0u, files.size());
}

TEST_F(RecentArcMediaSourceTest, GetAudioFiles) {
EnableFakeFileSystemInstance();

std::vector<RecentFile> files =
GetRecentFiles(RecentSource::FileType::kAudio);
GetRecentFiles("", RecentSource::FileType::kAudio);
// Query for recently-modified audio files should be ignored, since
// MediaDocumentsProvider doesn't support queryRecentDocuments for audio.
ASSERT_EQ(0u, files.size());
Expand All @@ -285,7 +299,7 @@ TEST_F(RecentArcMediaSourceTest, GetImageFiles) {
EnableFakeFileSystemInstance();

std::vector<RecentFile> files =
GetRecentFiles(RecentSource::FileType::kImage);
GetRecentFiles("", RecentSource::FileType::kImage);

ASSERT_EQ(2u, files.size());
EXPECT_EQ(
Expand All @@ -308,7 +322,7 @@ TEST_F(RecentArcMediaSourceTest, GetVideoFiles) {
EnableFakeFileSystemInstance();

std::vector<RecentFile> files =
GetRecentFiles(RecentSource::FileType::kVideo);
GetRecentFiles("", RecentSource::FileType::kVideo);

ASSERT_EQ(2u, files.size());
EXPECT_EQ(
Expand All @@ -331,7 +345,7 @@ TEST_F(RecentArcMediaSourceTest, GetDocumentFiles) {
EnableFakeFileSystemInstance();

std::vector<RecentFile> files =
GetRecentFiles(RecentSource::FileType::kDocument);
GetRecentFiles("", RecentSource::FileType::kDocument);

ASSERT_EQ(2u, files.size());
EXPECT_EQ(
Expand All @@ -348,14 +362,27 @@ TEST_F(RecentArcMediaSourceTest, GetDocumentFiles) {
files[1].url().path());
EXPECT_EQ(base::Time::FromMillisecondsSinceUnixEpoch(9),
files[1].last_modified());

files = GetRecentFiles("word", RecentSource::FileType::kDocument);
ASSERT_EQ(1u, files.size());
EXPECT_EQ(
arc::GetDocumentsProviderMountPath(arc::kMediaDocumentsProviderAuthority,
arc::kDocumentsRootDocumentId)
.Append("word.doc"),
files[0].url().path());
EXPECT_EQ(base::Time::FromMillisecondsSinceUnixEpoch(9),
files[0].last_modified());

files = GetRecentFiles("no-match", RecentSource::FileType::kDocument);
ASSERT_EQ(0u, files.size());
}

TEST_F(RecentArcMediaSourceTest, UmaStats) {
EnableFakeFileSystemInstance();

base::HistogramTester histogram_tester;

GetRecentFiles();
GetRecentFiles("");

histogram_tester.ExpectTotalCount(RecentArcMediaSource::kLoadHistogramName,
1);
Expand All @@ -367,7 +394,7 @@ TEST_F(RecentArcMediaSourceTest, UmaStats_Deferred) {

base::HistogramTester histogram_tester;

GetRecentFiles();
GetRecentFiles("");

histogram_tester.ExpectTotalCount(RecentArcMediaSource::kLoadHistogramName,
0);
Expand Down
5 changes: 5 additions & 0 deletions chrome/browser/ash/fileapi/recent_disk_source.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "base/functional/bind.h"
#include "base/metrics/histogram_functions.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "base/time/time.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
Expand Down Expand Up @@ -144,6 +145,7 @@ void RecentDiskSource::OnReadDirectory(
bool has_more) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DCHECK(params_.has_value());
const std::u16string q16 = base::UTF8ToUTF16(params_->query());

for (const auto& entry : entries) {
// Ignore directories and files that start with dot.
Expand All @@ -163,6 +165,9 @@ void RecentDiskSource::OnReadDirectory(
if (!MatchesFileType(entry.name, params_.value().file_type())) {
continue;
}
if (!FileNameMatches(base::UTF8ToUTF16(entry.name.value()), q16)) {
continue;
}
storage::FileSystemURL url = BuildDiskURL(subpath);
++inflight_stats_;
content::GetIOThreadTaskRunner({})->PostTask(
Expand Down
27 changes: 21 additions & 6 deletions chrome/browser/ash/fileapi/recent_disk_source_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,14 @@ class RecentDiskSourceTest : public testing::Test {
std::vector<RecentFile> GetRecentFiles(
size_t max_files,
const base::Time& cutoff_time,
const std::string& query = "",
RecentSource::FileType file_type = RecentSource::FileType::kAll) {
std::vector<RecentFile> files;

base::RunLoop run_loop;

source_->GetRecentFiles(RecentSource::Params(
file_system_context_.get(), origin_, max_files, cutoff_time,
file_system_context_.get(), origin_, max_files, query, cutoff_time,
base::TimeTicks::Max(), file_type,
base::BindOnce(
[](base::RunLoop* run_loop, std::vector<RecentFile>* out_files,
Expand Down Expand Up @@ -121,7 +122,7 @@ TEST_F(RecentDiskSourceTest, GetRecentFiles) {
CreateEmptyFile("4.jpg", base::Time::FromSecondsSinceUnixEpoch(4)));
// Newest

std::vector<RecentFile> files = GetRecentFiles(3, base::Time());
std::vector<RecentFile> files = GetRecentFiles(3, base::Time(), "");

std::sort(files.begin(), files.end(), RecentFileComparator());

Expand All @@ -132,6 +133,13 @@ TEST_F(RecentDiskSourceTest, GetRecentFiles) {
EXPECT_EQ(base::Time::FromSecondsSinceUnixEpoch(3), files[1].last_modified());
EXPECT_EQ("2.jpg", files[2].url().path().BaseName().value());
EXPECT_EQ(base::Time::FromSecondsSinceUnixEpoch(2), files[2].last_modified());

files = GetRecentFiles(3, base::Time(), "4");
ASSERT_EQ(1u, files.size());
EXPECT_EQ("4.jpg", files[0].url().path().BaseName().value());

files = GetRecentFiles(3, base::Time(), "foo");
ASSERT_EQ(0u, files.size());
}

TEST_F(RecentDiskSourceTest, GetRecentFiles_CutoffTime) {
Expand Down Expand Up @@ -254,7 +262,7 @@ TEST_F(RecentDiskSourceTest, GetAudioFiles) {
// Newest

std::vector<RecentFile> files =
GetRecentFiles(7, base::Time(), RecentSource::FileType::kAudio);
GetRecentFiles(7, base::Time(), "", RecentSource::FileType::kAudio);

std::sort(files.begin(), files.end(), RecentFileComparator());

Expand All @@ -263,6 +271,13 @@ TEST_F(RecentDiskSourceTest, GetAudioFiles) {
EXPECT_EQ(base::Time::FromSecondsSinceUnixEpoch(7), files[0].last_modified());
EXPECT_EQ("4.mp3", files[1].url().path().BaseName().value());
EXPECT_EQ(base::Time::FromSecondsSinceUnixEpoch(4), files[1].last_modified());

files = GetRecentFiles(7, base::Time(), "7", RecentSource::FileType::kAudio);
ASSERT_EQ(1u, files.size());
EXPECT_EQ("7.amr", files[0].url().path().BaseName().value());

files = GetRecentFiles(7, base::Time(), "6", RecentSource::FileType::kAudio);
ASSERT_EQ(0u, files.size());
}

TEST_F(RecentDiskSourceTest, GetImageFiles) {
Expand All @@ -287,7 +302,7 @@ TEST_F(RecentDiskSourceTest, GetImageFiles) {
// Newest

std::vector<RecentFile> files =
GetRecentFiles(8, base::Time(), RecentSource::FileType::kImage);
GetRecentFiles(8, base::Time(), "", RecentSource::FileType::kImage);

std::sort(files.begin(), files.end(), RecentFileComparator());

Expand Down Expand Up @@ -328,7 +343,7 @@ TEST_F(RecentDiskSourceTest, GetVideoFiles) {
// Newest

std::vector<RecentFile> files =
GetRecentFiles(9, base::Time(), RecentSource::FileType::kVideo);
GetRecentFiles(9, base::Time(), "", RecentSource::FileType::kVideo);

std::sort(files.begin(), files.end(), RecentFileComparator());

Expand Down Expand Up @@ -364,7 +379,7 @@ TEST_F(RecentDiskSourceTest, GetDocumentFiles) {
// Newest

std::vector<RecentFile> files =
GetRecentFiles(8, base::Time(), RecentSource::FileType::kDocument);
GetRecentFiles(8, base::Time(), "", RecentSource::FileType::kDocument);

std::sort(files.begin(), files.end(), RecentFileComparator());

Expand Down
1 change: 1 addition & 0 deletions chrome/browser/ash/fileapi/recent_drive_source.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ void RecentDriveSource::GetRecentFiles(Params params) {
std::vector<std::string> type_filters =
RecentDriveSource::CreateTypeFilters(params_->file_type());
query_params->modified_time = params_->cutoff_time();
query_params->title = params_->query();
query_params->modified_time_operator =
drivefs::mojom::QueryParameters::DateComparisonOperator::kGreaterThan;
if (type_filters.size() == 1) {
Expand Down
23 changes: 14 additions & 9 deletions chrome/browser/ash/fileapi/recent_model.cc
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ RecentModel::~RecentModel() {
void RecentModel::GetRecentFiles(
storage::FileSystemContext* file_system_context,
const GURL& origin,
const std::string& query,
FileType file_type,
bool invalidate_cache,
GetRecentFilesCallback callback) {
Expand All @@ -125,7 +126,8 @@ void RecentModel::GetRecentFiles(
* Otherwise clear cache if it has values.
*/
if (cached_files_.has_value()) {
if (!invalidate_cache && cached_files_type_ == file_type) {
if (!invalidate_cache && cached_files_type_ == file_type &&
cached_query_ == query) {
std::move(callback).Run(cached_files_.value());
return;
}
Expand All @@ -149,7 +151,7 @@ void RecentModel::GetRecentFiles(

num_inflight_sources_ = sources_.size();
if (sources_.empty()) {
OnGetRecentFilesCompleted(file_type);
OnGetRecentFilesCompleted(query, file_type);
return;
}

Expand All @@ -168,17 +170,17 @@ void RecentModel::GetRecentFiles(

for (const auto& source : sources_) {
source->GetRecentFiles(RecentSource::Params(
file_system_context, origin, max_files_, cutoff_time, end_time,
file_system_context, origin, max_files_, query, cutoff_time, end_time,
file_type,
base::BindOnce(&RecentModel::OnGetRecentFiles,
weak_ptr_factory_.GetWeakPtr(), run_on_sequence_id,
max_files_, cutoff_time, file_type)));
max_files_, cutoff_time, query, file_type)));
}
if (scan_timeout_duration_) {
deadline_timer_.Start(
FROM_HERE, base::TimeTicks::Now() + *scan_timeout_duration_,
base::BindOnce(&RecentModel::OnScanTimeout,
weak_ptr_factory_.GetWeakPtr(), file_type));
weak_ptr_factory_.GetWeakPtr(), query, file_type));
}
}

Expand All @@ -190,10 +192,10 @@ void RecentModel::ClearScanTimeout() {
scan_timeout_duration_.reset();
}

void RecentModel::OnScanTimeout(FileType file_type) {
void RecentModel::OnScanTimeout(const std::string& query, FileType file_type) {
if (num_inflight_sources_ > 0) {
num_inflight_sources_ = 0;
OnGetRecentFilesCompleted(file_type);
OnGetRecentFilesCompleted(query, file_type);
}
}

Expand All @@ -208,6 +210,7 @@ void RecentModel::Shutdown() {
void RecentModel::OnGetRecentFiles(uint32_t run_on_sequence_id,
size_t max_files,
const base::Time& cutoff_time,
const std::string& query,
FileType file_type,
std::vector<RecentFile> files) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
Expand All @@ -231,11 +234,12 @@ void RecentModel::OnGetRecentFiles(uint32_t run_on_sequence_id,

--num_inflight_sources_;
if (num_inflight_sources_ == 0) {
OnGetRecentFilesCompleted(file_type);
OnGetRecentFilesCompleted(query, file_type);
}
}

void RecentModel::OnGetRecentFilesCompleted(FileType file_type) {
void RecentModel::OnGetRecentFilesCompleted(const std::string& query,
FileType file_type) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);

DCHECK_EQ(0, num_inflight_sources_);
Expand All @@ -253,6 +257,7 @@ void RecentModel::OnGetRecentFilesCompleted(FileType file_type) {
std::reverse(files.begin(), files.end());
cached_files_ = std::move(files);
cached_files_type_ = file_type;
cached_query_ = query;

DCHECK(cached_files_.has_value());
DCHECK(intermediate_files_.empty());
Expand Down

0 comments on commit d519089

Please sign in to comment.