Skip to content

Commit

Permalink
[Files SWA]: Fix file system URL regex mismatch.
Browse files Browse the repository at this point in the history 
When we switch to Files System Web App the file system URLs change from

a method that returns the correct prefix, based on the state of the
FilesSWA flag. It adds a tests for it. It factors out RegEx pattern
generation to a single function that uses the newly introduced helper
function. Then it uses this single function in places where RegEx
patterns were generated.

filesystem: chrome-extension://.* to filesystem:chrome://.*. This CL adds
Bug: b/225222557
Change-Id: I771eac42e3609dffb1b4980e5d6c7f906e141d66
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3538163
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Reviewed-by: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Commit-Queue: Bo Majewski <majewski@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984248}
  • Loading branch information
Bo Majewski authored and Chromium LUCI CQ committed Mar 23, 2022
1 parent 46fb188 commit 4a55312
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 16 deletions.
34 changes: 22 additions & 12 deletions chrome/browser/apps/app_service/intent_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "base/containers/extend.h"
#include "base/containers/flat_map.h"
#include "base/notreached.h"
#include "base/strings/strcat.h"
#include "base/strings/string_util.h"
#include "base/strings/stringprintf.h"
#include "build/build_config.h"
Expand Down Expand Up @@ -86,6 +87,23 @@ apps::mojom::IntentFilterPtr CreateFileURLFilter(

return intent_filter;
}

// Takes a URL pattern that represents a path like *.pdf and returns a string
// representing a pattern matching a file system URL spec. If |legacy| flag is
// set to true the function returns a pattern that matches URLs generated by the
// legacy Files app (e.g., "filesystem:chrome-extension://.*/.*\.pdf"). If
// |legacy| flag is set to false, the pattern matches URLs generated by the
// Files System Web App (e.g., "filesystem:chrome://file-manager/.*\.pdf).
const std::string URLPatternToFileSystemPattern(const URLPattern& pattern,
bool legacy) {
const char* scheme =
legacy ? "chrome-extension://*" : "chrome://file-manager";
std::string path =
base::StrCat({url::kFileSystemScheme, ":", scheme, pattern.path()});
base::ReplaceChars(path, ".", R"(\.)", &path);
base::ReplaceChars(path, "*", ".*", &path);
return path;
}
#endif

apps::mojom::IntentFilterPtr CreateMimeTypeShareFilter(
Expand Down Expand Up @@ -529,12 +547,8 @@ apps::IntentFilters CreateIntentFiltersForExtension(
for (const std::unique_ptr<FileBrowserHandler>& handler : *handler_list) {
std::vector<std::string> patterns;
for (const URLPattern& pattern : handler->file_url_patterns()) {
// "filesystem:chrome-extension://*/*.txt"
std::string path = "filesystem:chrome-extension://*" + pattern.path();
base::ReplaceChars(path, ".", R"(\.)", &path);
base::ReplaceChars(path, "*", ".*", &path);
// "filesystem:chrome-extension://.*/.*\.txt"
patterns.push_back(path);
patterns.push_back(URLPatternToFileSystemPattern(pattern, true));
patterns.push_back(URLPatternToFileSystemPattern(pattern, false));
}
filters.push_back(apps::ConvertMojomIntentFilterToIntentFilter(
CreateFileURLFilter(patterns, handler->id(), handler->title())));
Expand All @@ -557,12 +571,8 @@ std::vector<apps::mojom::IntentFilterPtr> CreateExtensionIntentFilters(
for (const std::unique_ptr<FileBrowserHandler>& handler : *handler_list) {
std::vector<std::string> patterns;
for (const URLPattern& pattern : handler->file_url_patterns()) {
// "filesystem:chrome-extension://*/*.txt"
std::string path = "filesystem:chrome-extension://*" + pattern.path();
base::ReplaceChars(path, ".", R"(\.)", &path);
base::ReplaceChars(path, "*", ".*", &path);
// "filesystem:chrome-extension://.*/.*\.txt"
patterns.push_back(path);
patterns.push_back(URLPatternToFileSystemPattern(pattern, true));
patterns.push_back(URLPatternToFileSystemPattern(pattern, false));
}
filters.push_back(
CreateFileURLFilter(patterns, handler->id(), handler->title()));
Expand Down
23 changes: 19 additions & 4 deletions chrome/browser/apps/app_service/intent_util_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,10 +655,13 @@ TEST_F(IntentUtilsTest, CreateIntentFiltersForExtension_FileHandlers) {
// "html" filter - glob match
const Condition& file_cond = *mime_filter->conditions[1];
EXPECT_EQ(file_cond.condition_type, ConditionType::kFile);
ASSERT_EQ(file_cond.condition_values.size(), 1u);
ASSERT_EQ(file_cond.condition_values.size(), 2u);
EXPECT_EQ(file_cond.condition_values[0]->match_type, PatternMatchType::kGlob);
EXPECT_EQ(file_cond.condition_values[0]->value,
R"(filesystem:chrome-extension://.*/.*\.html)");
EXPECT_EQ(file_cond.condition_values[1]->match_type, PatternMatchType::kGlob);
EXPECT_EQ(file_cond.condition_values[1]->value,
R"(filesystem:chrome://file-manager/.*\.html)");

// "any" filter - View action
const IntentFilterPtr& mime_filter2 = filters[1];
Expand All @@ -672,11 +675,15 @@ TEST_F(IntentUtilsTest, CreateIntentFiltersForExtension_FileHandlers) {
// "any" filter - glob match
const Condition& file_cond2 = *mime_filter2->conditions[1];
EXPECT_EQ(file_cond2.condition_type, ConditionType::kFile);
ASSERT_EQ(file_cond2.condition_values.size(), 1u);
ASSERT_EQ(file_cond2.condition_values.size(), 2u);
EXPECT_EQ(file_cond2.condition_values[0]->match_type,
PatternMatchType::kGlob);
EXPECT_EQ(file_cond2.condition_values[0]->value,
R"(filesystem:chrome-extension://.*/.*\..*)");
EXPECT_EQ(file_cond2.condition_values[1]->match_type,
PatternMatchType::kGlob);
EXPECT_EQ(file_cond2.condition_values[1]->value,
R"(filesystem:chrome://file-manager/.*\..*)");
}

// TODO(crbug.com/1253250): Remove after migrating to non-mojo AppService.
Expand Down Expand Up @@ -738,11 +745,15 @@ TEST_F(IntentUtilsTest, CreateExtensionIntentFilters_FileHandlers) {
// "html" filter - glob match
const apps::mojom::Condition& file_cond = *mime_filter->conditions[1];
EXPECT_EQ(file_cond.condition_type, apps::mojom::ConditionType::kFile);
ASSERT_EQ(file_cond.condition_values.size(), 1u);
ASSERT_EQ(file_cond.condition_values.size(), 2u);
EXPECT_EQ(file_cond.condition_values[0]->match_type,
apps::mojom::PatternMatchType::kGlob);
EXPECT_EQ(file_cond.condition_values[0]->value,
R"(filesystem:chrome-extension://.*/.*\.html)");
EXPECT_EQ(file_cond.condition_values[1]->match_type,
apps::mojom::PatternMatchType::kGlob);
EXPECT_EQ(file_cond.condition_values[1]->value,
R"(filesystem:chrome://file-manager/.*\.html)");

// "any" filter - View action
const apps::mojom::IntentFilterPtr& mime_filter2 = filters[1];
Expand All @@ -756,11 +767,15 @@ TEST_F(IntentUtilsTest, CreateExtensionIntentFilters_FileHandlers) {
// "any" filter - glob match
const apps::mojom::Condition& file_cond2 = *mime_filter2->conditions[1];
EXPECT_EQ(file_cond2.condition_type, apps::mojom::ConditionType::kFile);
ASSERT_EQ(file_cond2.condition_values.size(), 1u);
ASSERT_EQ(file_cond2.condition_values.size(), 2u);
EXPECT_EQ(file_cond2.condition_values[0]->match_type,
apps::mojom::PatternMatchType::kGlob);
EXPECT_EQ(file_cond2.condition_values[0]->value,
R"(filesystem:chrome-extension://.*/.*\..*)");
EXPECT_EQ(file_cond2.condition_values[1]->match_type,
apps::mojom::PatternMatchType::kGlob);
EXPECT_EQ(file_cond2.condition_values[1]->value,
R"(filesystem:chrome://file-manager/.*\..*)");
}

// Converting an Arc Intent filter for a URL view intent filter should add a
Expand Down
27 changes: 27 additions & 0 deletions components/services/app_service/public/cpp/intent_util_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -965,6 +965,12 @@ GURL ext_test_url(const std::string& file_name) {
return url;
}

GURL system_web_app_test_url(const std::string& file_name) {
GURL url = GURL("filesystem:chrome://file-manager/external/" + file_name);
EXPECT_TRUE(url.is_valid());
return url;
}

std::vector<apps::mojom::IntentFilePtr> vectorise(
const apps::mojom::IntentFilePtr& file) {
std::vector<apps::mojom::IntentFilePtr> vector;
Expand Down Expand Up @@ -1174,6 +1180,27 @@ TEST_F(IntentUtilTest, FileURLMatch) {
EXPECT_FALSE(intent->MatchFilter(ext_wild_filter));
}

TEST_F(IntentUtilTest, FileSystemWebAppURLMatch) {
std::string mp3_url_pattern = R"(filesystem:chrome://.*/.*\.mp3)";

auto url_filter = apps_util::MakeURLFilterForView(mp3_url_pattern, "label");

// Test match with mp3 file extension.
auto intent = std::make_unique<apps::Intent>(
CreateIntentFiles(system_web_app_test_url("abc.mp3"), "", false));
EXPECT_TRUE(intent->MatchFilter(url_filter));

// Test non-match with mp4 file extension.
intent = std::make_unique<apps::Intent>(
CreateIntentFiles(system_web_app_test_url("abc.mp4"), "", false));
EXPECT_FALSE(intent->MatchFilter(url_filter));

// Test non-match with just the end of a file extension.
intent = std::make_unique<apps::Intent>(
CreateIntentFiles(system_web_app_test_url("abc.testmp3"), "", false));
EXPECT_FALSE(intent->MatchFilter(url_filter));
}

// TODO(crbug.com/1253250): Remove after migrating to non-mojo AppService.
TEST_F(IntentUtilTest, FileWithTitleTextMojom) {
const std::string mime_type = "image/jpeg";
Expand Down

0 comments on commit 4a55312

Please sign in to comment.