Allow chrome://media-app to open "non-native" files.

r772108 prevented these non-native files from being offered to web apps
that use the file handling API due to lack of support. r778183 added
the support for known use-cases used by the chrome://media-app "System"
Web App.

This CL tweaks the logic added by r772108 so that chrome://media-app
(specifically) is allowed to act as a file handler for these non-native
files.

Bug: 1079065, b/157532509
Change-Id: I8f198357b9b5ecd487edb37255a7a55e09295232
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2249402Reviewed-by: Chase Phillips <cmp@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#780036}

chrome/browser/chromeos/file_manager/file_manager_test_util.cc

@@ -6,6 +6,7 @@
 
 #include "base/files/file_util.h"
 #include "base/path_service.h"
+#include "base/test/bind_test_util.h"
 #include "chrome/browser/chromeos/file_manager/app_id.h"
 #include "chrome/browser/chromeos/file_manager/path_util.h"
 #include "chrome/browser/chromeos/file_manager/volume_manager_observer.h"
@@ -16,8 +17,10 @@
 #include "chrome/common/chrome_paths.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/browser/entry_info.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/notification_types.h"
+#include "net/base/mime_util.h"
 
 namespace file_manager {
 namespace test {
@@ -127,5 +130,36 @@ base::WeakPtr<file_manager::Volume> InstallFileSystemProviderChromeApp(
   return volume;
 }
 
+std::vector<file_tasks::FullTaskDescriptor> GetTasksForFile(
+    Profile* profile,
+    const base::FilePath& file) {
+  std::string mime_type;
+  net::GetMimeTypeFromFile(file, &mime_type);
+  CHECK(!mime_type.empty());
+
+  std::vector<extensions::EntryInfo> entries;
+  entries.emplace_back(file, mime_type, false);
+
+  // Use empty URLs in this helper (i.e. only support files backed by volumes).
+  // FindExtensionAndAppTasks() does not pass them to FindWebTasks() or
+  // FindFileHandlerTasks() in any case.
+  std::vector<GURL> file_urls(entries.size(), GURL());
+
+  std::vector<file_tasks::FullTaskDescriptor> result;
+  bool invoked_synchronously = false;
+  auto callback = base::BindLambdaForTesting(
+      [&](std::unique_ptr<std::vector<file_tasks::FullTaskDescriptor>> tasks) {
+        result = *tasks;
+        invoked_synchronously = true;
+      });
+
+  FindAllTypesOfTasks(profile, entries, file_urls, callback);
+
+  // MIME sniffing requires a run loop, but the mime type must be explicitly
+  // available, and is provided in this helper.
+  CHECK(invoked_synchronously);
+  return result;
+}
+
 }  // namespace test
 }  // namespace file_manager

chrome/browser/chromeos/file_manager/file_manager_test_util.h

@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/files/file_path.h"
+#include "chrome/browser/chromeos/file_manager/file_tasks.h"
 #include "chrome/browser/chromeos/file_manager/volume_manager.h"
 
 class Profile;
@@ -52,6 +53,13 @@ scoped_refptr<const extensions::Extension> InstallTestingChromeApp(
 base::WeakPtr<file_manager::Volume> InstallFileSystemProviderChromeApp(
     Profile* profile);
 
+// Gets the list of available tasks for the provided `file`. Note only the path
+// string is used for this helper, so it must have a well-known MIME type
+// according to net::GetMimeTypeFromFile().
+std::vector<file_tasks::FullTaskDescriptor> GetTasksForFile(
+    Profile* profile,
+    const base::FilePath& file);
+
 }  // namespace test
 }  // namespace file_manager
 

chrome/browser/chromeos/file_manager/file_tasks_browsertest.cc

@@ -372,11 +372,9 @@ IN_PROC_BROWSER_TEST_F(FileTasksBrowserTestWithMediaApp,
 IN_PROC_BROWSER_TEST_F(FileTasksBrowserTestWithMediaApp,
                        ProvidedFileSystemFileSource) {
   // The current test expectation: a GIF file in the provided file system called
-  // "readwrite.gif" should open with Gallery.
-  // TODO(crbug/1079065): Open with MediaApp when the NativeFileSystem API has
-  // good support for ChromeOS special filesystems.
+  // "readwrite.gif" should open with the MediaApp.
   const char kTestFile[] = "readwrite.gif";
-  Expectation test = {"gif", kGalleryAppId};
+  Expectation test = {"gif", kMediaAppId};
   int remaining_expectations = 1;
 
   Profile* profile = browser()->profile();

chrome/browser/chromeos/file_manager/web_file_tasks.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/apps/app_service/launch_utils.h"
 #include "chrome/browser/chromeos/file_manager/file_tasks.h"
 #include "chrome/browser/chromeos/file_manager/filesystem_api_util.h"
+#include "chrome/browser/chromeos/web_applications/default_web_app_ids.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/file_handler_manager.h"
@@ -38,10 +39,17 @@ void FindWebTasks(Profile* profile,
   DCHECK(!entries.empty());
   DCHECK(result_list);
 
-  // WebApps only support files backed by inodes. See https://crbug.com/1079065.
+  // WebApps only have full support files backed by inodes, so tasks provided by
+  // most Web Apps will be skipped if any non-native files are present. "System"
+  // Web Apps are an exception: we have more control over what they can do, so
+  // tasks provided by System Web Apps are the only ones permitted at present.
+  // See https://crbug.com/1079065.
+  bool has_special_file = false;
   for (const auto& entry : entries) {
-    if (util::IsUnderNonNativeLocalPath(profile, entry.path))
-      return;
+    if (util::IsUnderNonNativeLocalPath(profile, entry.path)) {
+      has_special_file = true;
+      break;
+    }
   }
 
   web_app::WebAppProviderBase* provider =
@@ -50,8 +58,11 @@ void FindWebTasks(Profile* profile,
   web_app::FileHandlerManager& file_handler_manager =
       provider->file_handler_manager();
 
-  auto app_ids = registrar.GetAppIds();
+  std::vector<web_app::AppId> app_ids = registrar.GetAppIds();
   for (const auto& app_id : app_ids) {
+    if (has_special_file && app_id != chromeos::default_web_apps::kMediaAppId)
+      continue;
+
     if (!file_handler_manager.IsFileHandlingAPIAvailable(app_id))
       continue;
 

chrome/browser/ui/web_applications/web_app_file_handling_browsertest.cc

@@ -33,6 +33,10 @@
 #include "third_party/blink/public/common/features.h"
 #include "third_party/blink/public/mojom/web_launch/file_handling_expiry.mojom-test-utils.h"
 
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/chromeos/file_manager/file_manager_test_util.h"
+#endif
+
 // A fake file handling expiry service. This service allows us to mock having an
 // origin trial which expires having a certain time, without needing to manage
 // actual origin trial tokens.
@@ -492,8 +496,21 @@ class WebAppFileHandlingOriginTrialTest
                                     kOriginTrialPublicKeyForTesting);
   }
 
+  void TearDownOnMainThread() override { interceptor_.reset(); }
+
  protected:
-  web_app::AppId InstallFileHandlingWebApp(const GURL& app_url) {
+  web_app::AppId InstallFileHandlingWebApp(GURL* app_url_out = nullptr) {
+    std::string origin = "https://file-handling-pwa";
+
+    // We need to use URLLoaderInterceptor (rather than a EmbeddedTestServer),
+    // because origin trial token is associated with a fixed origin, whereas
+    // EmbeddedTestServer serves content on a random port.
+    interceptor_ =
+        content::URLLoaderInterceptor::ServeFilesFromDirectoryAtOrigin(
+            kBaseDataDir, GURL(origin));
+
+    GURL app_url = GURL(origin + "/index.html");
+
     auto web_app_info = std::make_unique<WebApplicationInfo>();
     web_app_info->app_url = app_url;
     web_app_info->scope = app_url.GetWithoutFilename();
@@ -516,23 +533,19 @@ class WebAppFileHandlingOriginTrialTest
         LaunchApplication(profile(), app_id, app_url);
     web_content->Close();
 
+    if (app_url_out)
+      *app_url_out = app_url;
     return app_id;
   }
+
+ private:
+  std::unique_ptr<content::URLLoaderInterceptor> interceptor_;
 };
 
 IN_PROC_BROWSER_TEST_P(WebAppFileHandlingOriginTrialTest,
                        LaunchParamsArePassedCorrectly) {
-  std::string origin = "https://file-handling-pwa";
-
-  // We need to use URLLoaderInterceptor (rather than a EmbeddedTestServer),
-  // because origin trial token is associated with a fixed origin, whereas
-  // EmbeddedTestServer serves content on a random port.
-  auto interceptor =
-      content::URLLoaderInterceptor::ServeFilesFromDirectoryAtOrigin(
-          kBaseDataDir, GURL(origin));
-
-  const GURL app_url = GURL(origin + "/index.html");
-  const web_app::AppId app_id = InstallFileHandlingWebApp(app_url);
+  GURL app_url;
+  const web_app::AppId app_id = InstallFileHandlingWebApp(&app_url);
   base::FilePath test_file_path = NewTestFilePath(FILE_PATH_LITERAL("txt"));
   content::WebContents* web_content = LaunchApplication(
       profile(), app_id, app_url,
@@ -544,6 +557,49 @@ IN_PROC_BROWSER_TEST_P(WebAppFileHandlingOriginTrialTest,
             content::EvalJs(web_content, "window.launchParams.files[0].name"));
 }
 
+#if defined(OS_CHROMEOS)
+
+// End-to-end test to ensure the file handler is registered on ChromeOS when the
+// extension system is initialized. Gives more coverage than the unit tests for
+// web_file_tasks.cc.
+IN_PROC_BROWSER_TEST_P(WebAppFileHandlingOriginTrialTest,
+                       IsFileHandlerOnChromeOS) {
+  const web_app::AppId app_id = InstallFileHandlingWebApp();
+  base::FilePath test_file_path = NewTestFilePath(FILE_PATH_LITERAL("txt"));
+  std::vector<file_manager::file_tasks::FullTaskDescriptor> tasks =
+      file_manager::test::GetTasksForFile(profile(), test_file_path);
+
+  // Note that there are normally multiple tasks due to default-installed
+  // handlers (e.g. add to zip file). But those handlers are not installed by
+  // default in browser tests.
+  EXPECT_EQ(1u, tasks.size());
+  EXPECT_EQ(tasks[0].task_descriptor().app_id, app_id);
+}
+
+// Ensures correct behavior for files on "special volumes", such as file systems
+// provided by extensions. These do not have local files (i.e. backed by
+// inodes).
+IN_PROC_BROWSER_TEST_P(WebAppFileHandlingOriginTrialTest,
+                       NotHandlerForNonNativeFiles) {
+  const web_app::AppId app_id = InstallFileHandlingWebApp();
+  base::WeakPtr<file_manager::Volume> fsp_volume =
+      file_manager::test::InstallFileSystemProviderChromeApp(profile());
+
+  // File in chrome/test/data/extensions/api_test/file_browser/image_provider/.
+  base::FilePath test_file_path =
+      fsp_volume->mount_path().AppendASCII("readonly.txt");
+  std::vector<file_manager::file_tasks::FullTaskDescriptor> tasks =
+      file_manager::test::GetTasksForFile(profile(), test_file_path);
+
+  // Current expectation is for the task not to be found while the native
+  // filesystem API is still being built up. See https://crbug.com/1079065.
+  // When the "special file" check in file_manager::file_tasks::FindWebTasks()
+  // is removed, this test should work the same as IsFileHandlerOnChromeOS.
+  EXPECT_EQ(0u, tasks.size());
+}
+
+#endif  // OS_CHROMEOS
+
 INSTANTIATE_TEST_SUITE_P(All,
                          WebAppFileHandlingBrowserTest,
                          ::testing::Values(web_app::ProviderType::kBookmarkApps,

chrome/test/data/extensions/api_test/file_browser/image_provider/background.js

@@ -24,6 +24,8 @@ const PNG_DATA = new Uint8Array([
 ]);
 const PNG_FILE = new File([PNG_DATA], 'readonly.png', {type: 'image/png'});
 
+const TXT_FILE = new File(['txt_data'], 'readonly.txt', {type: 'text/plain'});
+
 const GIF_ENTRY = Object.freeze({
   isDirectory: false,
   name: GIF_FILE.name,
@@ -42,6 +44,15 @@ const PNG_ENTRY = Object.freeze({
   file: PNG_FILE,
   writable: false
 });
+const TXT_ENTRY = Object.freeze({
+  isDirectory: false,
+  name: TXT_FILE.name,
+  size: TXT_FILE.size,
+  modificationTime: new Date(),
+  mimeType: TXT_FILE.type,
+  file: TXT_FILE,
+  writable: false
+});
 const ROOT_ENTRY = Object.freeze({
   isDirectory: true,
   name: '',
@@ -54,6 +65,7 @@ const ENTRY_PATHS = {
   ['/']: ROOT_ENTRY,
   [`/${GIF_ENTRY.name}`]: GIF_ENTRY,
   [`/${PNG_ENTRY.name}`]: PNG_ENTRY,
+  [`/${TXT_ENTRY.name}`]: TXT_ENTRY,
 };
 
 // A mapping from |requestId| to file entry. Used to respond to subsequent file