Commit 6b349f67 authored by tby's avatar tby Committed by Commit Bot

[Suggested files] Several misc fixes.

 - Increases the max response size to 1MB. 10KB was too small, and
   valid responses were being ignored.

 - Removes the `type_detail_fields` in the request. I may re-add this
   later once we have a good idea of what fields we want to filter.

 - Adds TODOs for error metrics in drive_zero_state_provider.

 - Fixes how the FilePathOrErrorPtr is used in OnFilePathsLocated.
   Calling get_error() isn't valid unless is_error().

 - Reparents result file paths into the Drive mount. It turns out the
   LocateFilesByItemIds only returns paths relative to the root of
   the user's drive fs mount, so we need to prepend the location of the
   mount itself. This is copied from drive_quick_access_provider.

 - If suggested files is enabled, create chip results.

Bug: 1034842
Change-Id: I1aa8aad4b313a9e15af7beb1a6346bb00e1ba98d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2434596
Commit-Queue: Tony Yeoman <tby@chromium.org>
Reviewed-by: default avatarRachel Wong <wrong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811591}
parent 3d05a0fe
...@@ -29,8 +29,17 @@ namespace { ...@@ -29,8 +29,17 @@ namespace {
constexpr char kSchema[] = "drive_zero_state://"; constexpr char kSchema[] = "drive_zero_state://";
// Given an absolute path representing a file in the user's Drive, returns a
// reparented version of the path within the user's drive fs mount.
base::FilePath ReparentToDriveMount(
const base::FilePath& path,
const drive::DriveIntegrationService* drive_service) {
DCHECK(path.IsAbsolute());
return drive_service->GetMountPointPath().Append(path.value());
} }
} // namespace
DriveZeroStateProvider::DriveZeroStateProvider( DriveZeroStateProvider::DriveZeroStateProvider(
Profile* profile, Profile* profile,
SearchController* search_controller, SearchController* search_controller,
...@@ -82,6 +91,7 @@ void DriveZeroStateProvider::Start(const base::string16& query) { ...@@ -82,6 +91,7 @@ void DriveZeroStateProvider::Start(const base::string16& query) {
// - the |file_tasks_notifier_| is unavailable, as we stat files using it. // - the |file_tasks_notifier_| is unavailable, as we stat files using it.
const bool drive_fs_mounted = drive_service_ && drive_service_->IsMounted(); const bool drive_fs_mounted = drive_service_ && drive_service_->IsMounted();
if (!query.empty() || !drive_fs_mounted || !file_tasks_notifier_) { if (!query.empty() || !drive_fs_mounted || !file_tasks_notifier_) {
// TODO(crbug.com/1034842): Log error metrics.
return; return;
} }
...@@ -90,8 +100,10 @@ void DriveZeroStateProvider::Start(const base::string16& query) { ...@@ -90,8 +100,10 @@ void DriveZeroStateProvider::Start(const base::string16& query) {
// Get the most recent results from the cache. // Get the most recent results from the cache.
cache_results_ = item_suggest_cache_.GetResults(); cache_results_ = item_suggest_cache_.GetResults();
if (!cache_results_) if (!cache_results_) {
// TODO(crbug.com/1034842): Log error metrics.
return; return;
}
std::vector<std::string> item_ids; std::vector<std::string> item_ids;
for (const auto& result : cache_results_->results) { for (const auto& result : cache_results_->results) {
...@@ -105,8 +117,10 @@ void DriveZeroStateProvider::Start(const base::string16& query) { ...@@ -105,8 +117,10 @@ void DriveZeroStateProvider::Start(const base::string16& query) {
void DriveZeroStateProvider::OnFilePathsLocated( void DriveZeroStateProvider::OnFilePathsLocated(
base::Optional<std::vector<drivefs::mojom::FilePathOrErrorPtr>> paths) { base::Optional<std::vector<drivefs::mojom::FilePathOrErrorPtr>> paths) {
if (!paths) if (!paths) {
// TODO(crbug.com/1034842): Log error metrics.
return; return;
}
DCHECK(cache_results_); DCHECK(cache_results_);
DCHECK_EQ(cache_results_->results.size(), paths->size()); DCHECK_EQ(cache_results_->results.size(), paths->size());
...@@ -117,16 +131,24 @@ void DriveZeroStateProvider::OnFilePathsLocated( ...@@ -117,16 +131,24 @@ void DriveZeroStateProvider::OnFilePathsLocated(
int item_index = 0; int item_index = 0;
SearchProvider::Results provider_results; SearchProvider::Results provider_results;
for (int i = 0; i < static_cast<int>(paths->size()); ++i) { for (int i = 0; i < static_cast<int>(paths->size()); ++i) {
if ((*paths)[i]->get_error() != drive::FILE_ERROR_OK) const auto& path_or_error = paths.value()[i];
if (path_or_error->is_error()) {
// TODO(crbug.com/1034842): Log error metrics.
continue; continue;
}
const double score = 1.0 - (item_index / total_items); const double score = 1.0 - (item_index / total_items);
++item_index; ++item_index;
// TODO(crbug.com/1034842): Use |cache_results_| to attach the session id to // TODO(crbug.com/1034842): Use |cache_results_| to attach the session id to
// the result. // the result.
provider_results.emplace_back( provider_results.emplace_back(
MakeResult((*paths)[i]->get_path(), score, /*is_chip=*/false)); MakeResult(path_or_error->get_path(), score, /*is_chip=*/false));
if (suggested_files_enabled_) {
provider_results.emplace_back(
MakeResult(path_or_error->get_path(), score, /*is_chip=*/true));
}
} }
cache_results_.reset(); cache_results_.reset();
...@@ -138,7 +160,8 @@ std::unique_ptr<FileResult> DriveZeroStateProvider::MakeResult( ...@@ -138,7 +160,8 @@ std::unique_ptr<FileResult> DriveZeroStateProvider::MakeResult(
const float relevance, const float relevance,
const bool is_chip) { const bool is_chip) {
return std::make_unique<FileResult>( return std::make_unique<FileResult>(
kSchema, filepath, ash::AppListSearchResultType::kDriveQuickAccessChip, kSchema, ReparentToDriveMount(filepath, drive_service_),
ash::AppListSearchResultType::kDriveQuickAccessChip,
is_chip ? ash::SearchResultDisplayType::kChip is_chip ? ash::SearchResultDisplayType::kChip
: ash::SearchResultDisplayType::kList, : ash::SearchResultDisplayType::kList,
relevance, profile_); relevance, profile_);
......
...@@ -30,8 +30,8 @@ ...@@ -30,8 +30,8 @@
namespace app_list { namespace app_list {
namespace { namespace {
// Maximum accepted size of an ItemSuggest response. 10 KB. // Maximum accepted size of an ItemSuggest response. 1MB.
constexpr int kMaxResponseSize = 10 * 1024; constexpr int kMaxResponseSize = 1024 * 1024;
// TODO(crbug.com/1034842): Investigate: // TODO(crbug.com/1034842): Investigate:
// - enterprise policies that should limit this traffic. // - enterprise policies that should limit this traffic.
...@@ -69,8 +69,7 @@ constexpr char kRequestBody[] = R"({ ...@@ -69,8 +69,7 @@ constexpr char kRequestBody[] = R"({
'platform_type': 'CHROME_OS', 'platform_type': 'CHROME_OS',
'scenario_type': 'CHROME_OS_ZSS_FILES' 'scenario_type': 'CHROME_OS_ZSS_FILES'
}, },
'max_suggestions': 10, 'max_suggestions': 10
'type_detail_fields': []
})"; })";
bool IsDisabledByPolicy(const Profile* profile) { bool IsDisabledByPolicy(const Profile* profile) {
......
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