Hide the Gallery and prefer Media App when enabled.

We're migrating away from the old Gallery chrome app. When MediaApp is
enabled, r759170 ensures we pick chrome://media-app over Gallery.
E.g., for the task request that comes from the Camera app to open the
camera roll.

This CL updates the set of tasks shown in the files app to match that
logic, And implements the product request to:
 - Hide the Gallery in any case where the Media App can handle a file,
   - (ignoring any explicit preference for Gallery in this case), and
 - Ensure Media App is the "default" handler if there is no preference
   for a different app (even if that app would normally be default
   merely through having been installed).

IsFallbackFileHandler becomes simpler because MediaApp now "can't" be
a fallback handler to apply that logic, which is now encapsulated in
AdjustTasksForMediaApp().

Existing expectations FileTasksBrowserTest don't change, but there are
now some extra checks:
 - Verify that for every test case, MediaApp and Gallery are never both
   presented as options.
 - New tests are added for the logic that ensures Media App is chosen
   over installed apps, unless that app is an explicit user preference.

Bug: 1030935, b/153387960, 1071289
Change-Id: I942dd47712817c0bc8767de7ff28d858f5e11a17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2150528
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760029}

chrome/browser/chromeos/extensions/file_manager/private_api_tasks.cc

@@ -70,16 +70,6 @@ std::set<std::string> GetUniqueMimeTypes(
   return mime_types;
 }
 
-// Returns whether |url| is a RAW image file according to its extension. Note
-// that since none of the extensions of interest are "known" mime types (per
-// net/mime_util.cc), it's enough to simply check the extension rather than
-// using MimeTypeCollector. TODO(crbug/1030935): Remove this.
-bool IsRawImage(const FileSystemURL& url) {
-  constexpr const char* kRawExtensions[] = {".arw", ".cr2", ".dng", ".nef",
-                                            ".nrw", ".orf", ".raf", ".rw2"};
-  return base::Contains(kRawExtensions, url.path().Extension());
-}
-
 // Intercepts usage of executeTask(..) that wants to invoke the old "Gallery"
 // chrome app. If the media app is enabled, substitute it.
 // TODO(crbug/1030935): Remove this when the gallery app is properly removed and
@@ -100,8 +90,11 @@ void MaybeAdjustTaskForGalleryAppRemoval(
   // is known to register as a handler. Although from a product perspective, the
   // Gallery should be entirely hidden, we still direct RAW images to Gallery
   // until chrome://media-app supports them.
-  if (std::find_if(urls.begin(), urls.end(), &IsRawImage) != urls.end())
+  if (std::find_if(urls.begin(), urls.end(), [](const auto& url) {
+        return file_manager::file_tasks::IsRawImage(url.path());
+      }) != urls.end()) {
     return;
+  }
 
   auto* provider = web_app::WebAppProvider::Get(profile);
   DCHECK(provider);

chrome/browser/chromeos/file_manager/file_tasks.cc

@@ -156,26 +156,78 @@ void RemoveFileManagerInternalActions(const std::set<std::string>& actions,
   tasks->swap(filtered);
 }
 
-// Returns true if the given task is a handler by built-in apps like the Files
-// app itself or QuickOffice etc. They are used as the initial default app.
-bool IsFallbackFileHandler(const FullTaskDescriptor& task, bool is_all_images) {
-  // TODO(crbug/1030935): Once Media app is the default app for all the types
-  // it accepts (defined in
-  // chromeos/components/media_app_ui/resources/manifest.json) delete this and
-  // merge Media App logic with below.
-  // The task to open with the Media App (kMediaAppId) is only returned as an
-  // option when the Media App is enabled, so there is no need to check feature
-  // flags here. This logic will choose the Media App (when it is enabled) as
-  // the default over Gallery even when they are both installed because of the
-  // order of operations in FindExtensionAndAppTasks, which appends web tasks
-  // (like Media App) before file handler tasks (like Gallery). Relying on this
-  // order of operations is a temporary measure.
-  if (task.task_descriptor().app_id ==
-          chromeos::default_web_apps::kMediaAppId &&
-      is_all_images) {
-    return true;
+// Adjusts |tasks| to reflect the product decision that chrome://media-app
+// should behave more like a user-installed app than a fallback handler.
+// Specifically, only apps set as the default in user prefs should be preferred
+// over chrome://media-app.
+// Until the Gallery app is removed, this function will also hide the Gallery
+// task in cases where choosing it would instead launch chrome://media-app due
+// to interception done in executeTask to support the "camera roll" function,
+// when the MediaApp feature is enabled. If the feature is not enabled, there
+// will be no MediaApp task and this function does nothing.
+void AdjustTasksForMediaApp(const std::vector<extensions::EntryInfo>& entries,
+                            std::vector<FullTaskDescriptor>* tasks) {
+  const auto task_for_app = [&](const std::string& app_id) {
+    return std::find_if(tasks->begin(), tasks->end(), [&](const auto& task) {
+      return task.task_descriptor().app_id == app_id;
+    });
+  };
+
+  const auto media_app_task =
+      task_for_app(chromeos::default_web_apps::kMediaAppId);
+  if (media_app_task == tasks->end())
+    return;
+
+  // TODO(crbug/1030935): Once Media app supports RAW files, delete the
+  // IsRawImage early exit. This is necessary while Gallery is still the
+  // better option for RAW files. The any_non_image check can be removed once
+  // video player functionality of the Media App is fully polished.
+  bool any_non_image = false;
+  for (const auto& entry : entries) {
+    if (IsRawImage(entry.path))
+      return;  // Let Gallery handle it.
+
+    any_non_image =
+        any_non_image || !net::MatchesMimeType("image/*", entry.mime_type);
   }
 
+  const auto gallery_task = task_for_app(kGalleryAppId);
+  if (any_non_image) {
+    // Remove the gallery app (if it was found), but don't re-order prefs.
+    // Picking it would launch Media App due to executeTask interception, but we
+    // should still prefer the Video Player app over Media App.
+    if (gallery_task != tasks->end())
+      tasks->erase(gallery_task);  // Note: invalidates iterators.
+    return;
+  }
+
+  // Due to https://crbug.com/1071289, configuring extension matches in SWA
+  // manifests has no effect on is_file_extension_match(), which means a non-
+  // "fallback" web app (i.e. a built-in app) can never be an automatic default.
+  // Fallback handlers are never preferred over extension-matched handlers, so
+  // we must instead pretend that the media app has an extension match.
+  // First DCHECK to see if the hack can be removed.
+  DCHECK(!media_app_task->is_file_extension_match());
+  media_app_task->set_is_file_extension_match(true);
+
+  // The logic in ChooseAndSetDefaultTask() also requires the following to hold.
+  // This should only fail if the media app is configured for "*" (e.g. like
+  // Zip Archiver). "image/*" does not count as "generic".
+  DCHECK(!media_app_task->is_generic_file_handler());
+
+  // Otherwise, build a new list with Media App at the front, and no Gallery.
+  std::vector<FullTaskDescriptor> new_tasks;
+  new_tasks.push_back(*media_app_task);
+  for (auto it = tasks->begin(); it != tasks->end(); ++it) {
+    if (it != media_app_task && it != gallery_task)
+      new_tasks.push_back(std::move(*it));
+  }
+  std::swap(*tasks, new_tasks);
+}
+
+// Returns true if the given task is a handler by built-in apps like the Files
+// app itself or QuickOffice etc. They are used as the initial default app.
+bool IsFallbackFileHandler(const FullTaskDescriptor& task) {
   if ((task.task_descriptor().task_type !=
            file_tasks::TASK_TYPE_FILE_BROWSER_HANDLER &&
        task.task_descriptor().task_type !=
@@ -184,6 +236,11 @@ bool IsFallbackFileHandler(const FullTaskDescriptor& task, bool is_all_images) {
     return false;
   }
 
+  // Note that chromeos::default_web_apps::kMediaAppId does not appear in the
+  // list of built-in apps below. Doing so would mean the presence of any other
+  // handler of image files (e.g. Keep, Photos) would take precedence. But we
+  // want that only to occur if the user has explicitly set the preference for
+  // an app other than kMediaAppId to be the default (b/153387960).
   constexpr const char* kBuiltInApps[] = {
       kFileManagerAppId,
       kVideoPlayerAppId,
@@ -243,6 +300,8 @@ void PostProcessFoundTasks(
   if (!disabled_actions.empty())
     RemoveFileManagerInternalActions(disabled_actions, result_list.get());
 
+  AdjustTasksForMediaApp(entries, result_list.get());
+
   ChooseAndSetDefaultTask(*profile->GetPrefs(), entries, result_list.get());
   std::move(callback).Run(std::move(result_list));
 }
@@ -769,14 +828,10 @@ void ChooseAndSetDefaultTask(const PrefService& pref_service,
   // No default task, check for an explicit file extension match (without
   // MIME match) in the extension manifest and pick that over the fallback
   // handlers below (see crbug.com/803930)
-  const bool all_images =
-      std::all_of(entries.begin(), entries.end(), [](const auto& e) {
-        return net::MatchesMimeType("image/*", e.mime_type);
-      });
   for (size_t i = 0; i < tasks->size(); ++i) {
     FullTaskDescriptor& task = (*tasks)[i];
     if (task.is_file_extension_match() && !task.is_generic_file_handler() &&
-        !IsFallbackFileHandler(task, all_images)) {
+        !IsFallbackFileHandler(task)) {
       task.set_is_default(true);
       return;
     }
@@ -787,12 +842,18 @@ void ChooseAndSetDefaultTask(const PrefService& pref_service,
   for (size_t i = 0; i < tasks->size(); ++i) {
     FullTaskDescriptor& task = (*tasks)[i];
     DCHECK(!task.is_default());
-    if (IsFallbackFileHandler(task, all_images)) {
+    if (IsFallbackFileHandler(task)) {
       task.set_is_default(true);
       return;
     }
   }
 }
 
+bool IsRawImage(const base::FilePath& path) {
+  constexpr const char* kRawExtensions[] = {".arw", ".cr2", ".dng", ".nef",
+                                            ".nrw", ".orf", ".raf", ".rw2"};
+  return base::Contains(kRawExtensions, path.Extension());
+}
+
 }  // namespace file_tasks
 }  // namespace file_manager

chrome/browser/chromeos/file_manager/file_tasks.h

@@ -335,6 +335,12 @@ void ChooseAndSetDefaultTask(const PrefService& pref_service,
                              const std::vector<extensions::EntryInfo>& entries,
                              std::vector<FullTaskDescriptor>* tasks);
 
+// Returns whether |path| is a RAW image file according to its extension. Note
+// that since none of the extensions of interest are "known" mime types (per
+// net/mime_util.cc), it's enough to simply check the extension rather than
+// using MimeTypeCollector. TODO(crbug/1030935): Remove this.
+bool IsRawImage(const base::FilePath& path);
+
 }  // namespace file_tasks
 }  // namespace file_manager
 

chrome/browser/chromeos/file_manager/file_tasks_browsertest.cc

@@ -3,16 +3,23 @@
 // found in the LICENSE file.
 
 #include "base/bind.h"
+#include "base/path_service.h"
 #include "chrome/browser/chromeos/extensions/default_web_app_ids.h"
 #include "chrome/browser/chromeos/file_manager/app_id.h"
 #include "chrome/browser/chromeos/file_manager/file_manager_test_util.h"
 #include "chrome/browser/chromeos/file_manager/file_tasks.h"
+#include "chrome/browser/extensions/chrome_test_extension_loader.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/web_applications/system_web_app_manager.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chromeos/constants/chromeos_features.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/test/test_utils.h"
 #include "extensions/browser/entry_info.h"
+#include "extensions/browser/notification_types.h"
 #include "net/base/mime_util.h"
 #include "third_party/blink/public/common/features.h"
 
@@ -40,6 +47,15 @@ void VerifyTasks(int* remaining,
                  std::unique_ptr<std::vector<FullTaskDescriptor>> result) {
   ASSERT_TRUE(result) << expectation.file_extensions;
 
+  bool has_media_app = false;
+  bool has_gallery = false;
+  for (const auto& t : *result) {
+    has_media_app = has_media_app || t.task_descriptor().app_id == kMediaAppId;
+    has_gallery = has_gallery || t.task_descriptor().app_id == kGalleryAppId;
+  }
+  // Gallery and Media App should never both appear as task options.
+  EXPECT_TRUE(!(has_media_app && has_gallery)) << expectation.file_extensions;
+
   auto default_task =
       std::find_if(result->begin(), result->end(),
                    [](const auto& task) { return task.is_default(); });
@@ -58,6 +74,25 @@ void VerifyTasks(int* remaining,
   --*remaining;
 }
 
+// Installs a chrome app that handles .tiff.
+scoped_refptr<const extensions::Extension> InstallTiffHandlerChromeApp(
+    Profile* profile) {
+  base::ScopedAllowBlockingForTesting allow_io;
+  content::WindowedNotificationObserver handler_ready(
+      extensions::NOTIFICATION_EXTENSION_BACKGROUND_PAGE_READY,
+      content::NotificationService::AllSources());
+  extensions::ChromeTestExtensionLoader loader(profile);
+
+  base::FilePath path;
+  EXPECT_TRUE(base::PathService::Get(chrome::DIR_TEST_DATA, &path));
+  path = path.AppendASCII("extensions/api_test/file_browser/app_file_handler");
+
+  auto extension = loader.LoadExtension(path);
+  EXPECT_TRUE(extension);
+  handler_ready.Wait();
+  return extension;
+}
+
 class FileTasksBrowserTest : public InProcessBrowserTest {
  public:
   void SetUpOnMainThread() override {
@@ -280,5 +315,25 @@ IN_PROC_BROWSER_TEST_F(FileTasksBrowserTestWithMediaApp,
   TestExpectationsAgainstDefaultTasks(expectations);
 }
 
+// Sanity check: the tiff-specific file handler is preferred when MediaApp is
+// not enabled.
+IN_PROC_BROWSER_TEST_F(FileTasksBrowserTest, InstalledAppsAreImplicitDefaults) {
+  auto extension = InstallTiffHandlerChromeApp(browser()->profile());
+  TestExpectationsAgainstDefaultTasks({{"tiff", extension->id().c_str()}});
+}
+
+// If the media app is enabled, it will be preferred over a chrome app with a
+// specific extension, unless that app is set default via prefs.
+IN_PROC_BROWSER_TEST_F(FileTasksBrowserTestWithMediaApp,
+                       MediaAppPreferredOverChromeApps) {
+  Profile* profile = browser()->profile();
+  auto extension = InstallTiffHandlerChromeApp(profile);
+  TestExpectationsAgainstDefaultTasks({{"tiff", kMediaAppId}});
+
+  UpdateDefaultTask(profile->GetPrefs(), extension->id() + "|app|tiffAction",
+                    {"tiff"}, {"image/tiff"});
+  TestExpectationsAgainstDefaultTasks({{"tiff", extension->id().c_str()}});
+}
+
 }  // namespace file_tasks
 }  // namespace file_manager