Register chrome://media-app as handling RAW image file types.

More accurately, this just hides the codepath that would redirect files
sniffed as "image/tiff" back to the old gallery app if the file had an
extension matching a known RAW image format. (chrome://media-app was
already a handler for "image/*", so logic was needed to exclude RAW).

However, it also adds explicit extension-match file handlers to the
web app manifest to handle cases where sniffing is not done. Initially
tried sharing the logic that drives `IsRawFile()` in the files app's
file_tasks but that's not really compatible with the web app manifest,
and `IsRawFile` can just be deleted soon. Encapsulate IsRawFile() to
make that removal easier: nothing outside file_tasks.cc uses it since
r777758.

The logic is put behind a base::Feature, but not a chrome://flag: usage
of this is low and the change is low-risk. And flipping
chrome://flags#media-app also works to use the old gallery for these
file types.

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

chrome/browser/chromeos/file_manager/file_tasks.cc

@@ -47,6 +47,7 @@
 #include "chrome/common/extensions/api/file_manager_private.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/pref_names.h"
+#include "chromeos/constants/chromeos_features.h"
 #include "chromeos/constants/chromeos_switches.h"
 #include "components/drive/drive_api_util.h"
 #include "components/prefs/pref_service.h"
@@ -170,6 +171,20 @@ void RemoveFileManagerInternalActions(const std::set<std::string>& actions,
   tasks->swap(filtered);
 }
 
+// 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) {
+  constexpr const char* kRawExtensions[] = {".arw", ".cr2", ".dng", ".nef",
+                                            ".nrw", ".orf", ".raf", ".rw2"};
+  for (const char* extension : kRawExtensions) {
+    if (path.MatchesExtension(extension))
+      return true;
+  }
+  return false;
+}
+
 // 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
@@ -192,13 +207,15 @@ void AdjustTasksForMediaApp(const std::vector<extensions::EntryInfo>& entries,
   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.
+  // TODO(crbug/1030935): Delete the IsRawImage function and early exit when
+  // kMediaAppHandlesRaw is removed. The any_non_image check can be removed once
+  // video player functionality of the Media App is fully polished
+  // (b/171154148).
   bool any_non_image = false;
   for (const auto& entry : entries) {
-    if (IsRawImage(entry.path)) {
+    if (!base::FeatureList::IsEnabled(
+            chromeos::features::kMediaAppHandlesRaw) &&
+        IsRawImage(entry.path)) {
       tasks->erase(media_app_task);
       return;  // Let Gallery handle it.
     }
@@ -914,16 +931,6 @@ void ChooseAndSetDefaultTask(const PrefService& pref_service,
   }
 }
 
-bool IsRawImage(const base::FilePath& path) {
-  constexpr const char* kRawExtensions[] = {".arw", ".cr2", ".dng", ".nef",
-                                            ".nrw", ".orf", ".raf", ".rw2"};
-  for (const char* extension : kRawExtensions) {
-    if (path.MatchesExtension(extension))
-      return true;
-  }
-  return false;
-}
-
 bool IsHtmlFile(const base::FilePath& path) {
   constexpr const char* kHtmlExtensions[] = {".htm", ".html", ".mhtml",
                                              ".xht", ".xhtm", ".xhtml"};

chrome/browser/chromeos/file_manager/file_tasks.h

@@ -340,12 +340,6 @@ 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);
-
 // Returns whether |path| is an HTML file according to its extension.
 bool IsHtmlFile(const base::FilePath& path);
 

chrome/browser/chromeos/file_manager/file_tasks_browsertest.cc

@@ -126,13 +126,14 @@ class FileTasksBrowserTestBase
         base::FilePath path = prefix.AddExtension(extension);
         std::string mime_type;
         net::GetMimeTypeFromFile(path, &mime_type);
-        if (test.mime_type) {
+        if (test.mime_type != nullptr) {
           // Sniffing isn't used when GetMimeTypeFromFile() succeeds, so there
           // shouldn't be a hard-coded mime type configured.
           EXPECT_TRUE(mime_type.empty());
           mime_type = test.mime_type;
+        } else {
+          EXPECT_FALSE(mime_type.empty()) << "No mime type for " << path;
         }
-        EXPECT_FALSE(mime_type.empty()) << "No mime type for " << path;
         entries.push_back({path, mime_type, false});
       }
       std::vector<GURL> file_urls{entries.size(), GURL()};
@@ -160,8 +161,24 @@ class FileTasksBrowserTest : public FileTasksBrowserTestBase {
 class FileTasksBrowserTestWithMediaApp : public FileTasksBrowserTestBase {
  public:
   FileTasksBrowserTestWithMediaApp() {
-    // Enable Media App.
-    scoped_feature_list_.InitWithFeatures({chromeos::features::kMediaApp}, {});
+    // Enable Media App with Raw support.
+    scoped_feature_list_.InitWithFeatures(
+        {chromeos::features::kMediaApp,
+         chromeos::features::kMediaAppHandlesRaw},
+        {});
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+class FileTasksBrowserTestWithMediaAppNoRaw : public FileTasksBrowserTestBase {
+ public:
+  FileTasksBrowserTestWithMediaAppNoRaw() {
+    // Enable Media App. Disable Raw support.
+    scoped_feature_list_.InitWithFeatures(
+        {chromeos::features::kMediaApp},
+        {chromeos::features::kMediaAppHandlesRaw});
   }
 
  private:
@@ -179,17 +196,6 @@ class FileTasksBrowserTestWithMediaApp : public FileTasksBrowserTestBase {
 // not operate on real files. We hard code MIME types that file sniffing
 // obtained experimentally from sample files.
 constexpr Expectation kUnchangedExpectations[] = {
-    // Raw.
-    {"arw", kGalleryAppId, "image/tiff"},
-    {"cr2", kGalleryAppId, "image/tiff"},
-    {"dng", kGalleryAppId, "image/tiff"},
-    {"nef", kGalleryAppId, "image/tiff"},
-    {"nrw", kGalleryAppId, "image/tiff"},
-    {"orf", kGalleryAppId, "image/tiff"},
-    {"raf", kGalleryAppId, "image/tiff"},
-    {"rw2", kGalleryAppId, "image/tiff"},
-    {"NRW", kGalleryAppId, "image/tiff"},  // Uppercase extension.
-
     // Video.
     {"3gp", kVideoPlayerAppId, "application/octet-stream"},
     {"avi", kVideoPlayerAppId, "application/octet-stream"},
@@ -293,9 +299,24 @@ IN_PROC_BROWSER_TEST_P(FileTasksBrowserTest, DefaultHandlerChangeDetector) {
   //  With the Media App disabled, all images should be handled by Gallery.
   std::vector<Expectation> expectations = {
       // Images.
-      {"bmp", kGalleryAppId},  {"gif", kGalleryAppId},  {"ico", kGalleryAppId},
-      {"jpg", kGalleryAppId},  {"jpeg", kGalleryAppId}, {"png", kGalleryAppId},
+      {"bmp", kGalleryAppId},
+      {"gif", kGalleryAppId},
+      {"ico", kGalleryAppId},
+      {"jpg", kGalleryAppId},
+      {"jpeg", kGalleryAppId},
+      {"png", kGalleryAppId},
       {"webp", kGalleryAppId},
+      // Raw.
+      {"arw", kGalleryAppId, "image/tiff"},
+      {"cr2", kGalleryAppId, "image/tiff"},
+      {"dng", kGalleryAppId, "image/tiff"},
+      {"nef", kGalleryAppId, "image/tiff"},
+      {"nrw", kGalleryAppId, "image/tiff"},
+      {"orf", kGalleryAppId, "image/tiff"},
+      {"raf", kGalleryAppId, "image/tiff"},
+      {"rw2", kGalleryAppId, "image/tiff"},
+      {"NRW", kGalleryAppId, "image/tiff"},  // Uppercase extension.
+      {"arw", kGalleryAppId, ""},  // Missing MIME type (unable to sniff).
   };
   expectations.insert(expectations.end(), std::begin(kUnchangedExpectations),
                       std::end(kUnchangedExpectations));
@@ -321,9 +342,57 @@ IN_PROC_BROWSER_TEST_P(FileTasksBrowserTestWithMediaApp,
   // video, which it also handles should be unchanged).
   std::vector<Expectation> expectations = {
       // Images.
-      {"bmp", kMediaAppId},  {"gif", kMediaAppId},  {"ico", kMediaAppId},
-      {"jpg", kMediaAppId},  {"jpeg", kMediaAppId}, {"png", kMediaAppId},
+      {"bmp", kMediaAppId},
+      {"gif", kMediaAppId},
+      {"ico", kMediaAppId},
+      {"jpg", kMediaAppId},
+      {"jpeg", kMediaAppId},
+      {"png", kMediaAppId},
+      {"webp", kMediaAppId},
+      // Raw (handled by MediaApp).
+      {"arw", kMediaAppId, "image/tiff"},
+      {"cr2", kMediaAppId, "image/tiff"},
+      {"dng", kMediaAppId, "image/tiff"},
+      {"nef", kMediaAppId, "image/tiff"},
+      {"nrw", kMediaAppId, "image/tiff"},
+      {"orf", kMediaAppId, "image/tiff"},
+      {"raf", kMediaAppId, "image/tiff"},
+      {"rw2", kMediaAppId, "image/tiff"},
+      {"NRW", kMediaAppId, "image/tiff"},  // Uppercase extension.
+      {"arw", kMediaAppId, ""},  // Missing MIME type (unable to sniff).
+  };
+  expectations.insert(expectations.end(), std::begin(kUnchangedExpectations),
+                      std::end(kUnchangedExpectations));
+
+  TestExpectationsAgainstDefaultTasks(expectations);
+}
+
+// Tests the default handlers with the Media App installed, but RAW support
+// disabled.
+IN_PROC_BROWSER_TEST_P(FileTasksBrowserTestWithMediaAppNoRaw,
+                       DefaultHandlerChangeDetector) {
+  // With the Media App enabled, images should be handled by it by default (but
+  // video, which it also handles should be unchanged).
+  std::vector<Expectation> expectations = {
+      // Images.
+      {"bmp", kMediaAppId},
+      {"gif", kMediaAppId},
+      {"ico", kMediaAppId},
+      {"jpg", kMediaAppId},
+      {"jpeg", kMediaAppId},
+      {"png", kMediaAppId},
       {"webp", kMediaAppId},
+      // Raw (still handled by gallery).
+      {"arw", kGalleryAppId, "image/tiff"},
+      {"cr2", kGalleryAppId, "image/tiff"},
+      {"dng", kGalleryAppId, "image/tiff"},
+      {"nef", kGalleryAppId, "image/tiff"},
+      {"nrw", kGalleryAppId, "image/tiff"},
+      {"orf", kGalleryAppId, "image/tiff"},
+      {"raf", kGalleryAppId, "image/tiff"},
+      {"rw2", kGalleryAppId, "image/tiff"},
+      {"NRW", kGalleryAppId, "image/tiff"},  // Uppercase extension.
+      {"arw", kGalleryAppId, ""},  // Missing MIME type (unable to sniff).
   };
   expectations.insert(expectations.end(), std::begin(kUnchangedExpectations),
                       std::end(kUnchangedExpectations));
@@ -460,5 +529,12 @@ INSTANTIATE_TEST_SUITE_P(All,
                                            TestProfileType::kGuest),
                          TestProfileTypeToString);
 
+INSTANTIATE_TEST_SUITE_P(All,
+                         FileTasksBrowserTestWithMediaAppNoRaw,
+                         ::testing::Values(TestProfileType::kRegular,
+                                           TestProfileType::kIncognito,
+                                           TestProfileType::kGuest),
+                         TestProfileTypeToString);
+
 }  // namespace file_tasks
 }  // namespace file_manager

chrome/browser/chromeos/web_applications/media_web_app_info.cc

@@ -7,6 +7,7 @@
 #include <memory>
 
 #include "base/strings/string16.h"
+#include "base/strings/string_split.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chromeos/web_applications/system_web_app_install_utils.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
@@ -17,6 +18,38 @@
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/l10n/l10n_util.h"
 
+namespace {
+
+// FileHandler configuration.
+// See https://github.com/WICG/file-handling/blob/master/explainer.md.
+constexpr std::tuple<const char*, const char*> kFileHandlers[] = {
+    {"image/*", ""},
+    {"video/*", ""},
+
+    // Raw images. Note the MIME type doesn't really matter here. MIME sniffing
+    // logic in the files app tends to detect image/tiff, but sniffing is only
+    // done for "local" files, so the extension list is needed in addition to
+    // the "image/*" wildcard above. The MIME type is never sent to the web app.
+    {"image/tiff", ".arw,.cr2,.dng,.nef,.nrw,.orf,.raf,.rw2"},
+};
+
+using AcceptMap = decltype(blink::Manifest::FileHandler::accept);
+
+// Converts the kFileHandlers constexpr into the std::map needed to populate the
+// web app manifest's `accept` property.
+AcceptMap MakeHandlerAccept() {
+  AcceptMap result;
+  const base::string16 separator = base::ASCIIToUTF16(",");
+  for (const auto& handler : kFileHandlers) {
+    result[base::ASCIIToUTF16(std::get<0>(handler))] =
+        base::SplitString(base::ASCIIToUTF16(std::get<1>(handler)), separator,
+                          base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
+  }
+  return result;
+}
+
+}  // namespace
+
 std::unique_ptr<WebApplicationInfo> CreateWebAppInfoForMediaWebApp() {
   std::unique_ptr<WebApplicationInfo> info =
       std::make_unique<WebApplicationInfo>();
@@ -45,8 +78,7 @@ std::unique_ptr<WebApplicationInfo> CreateWebAppInfoForMediaWebApp() {
   blink::Manifest::FileHandler file_handler;
   file_handler.action = GURL(chromeos::kChromeUIMediaAppURL);
   file_handler.name = base::UTF8ToUTF16("Media File");
-  file_handler.accept[base::UTF8ToUTF16("image/*")] = {};
-  file_handler.accept[base::UTF8ToUTF16("video/*")] = {};
-  info->file_handlers.push_back(file_handler);
+  file_handler.accept = MakeHandlerAccept();
+  info->file_handlers.push_back(std::move(file_handler));
   return info;
 }

chromeos/constants/chromeos_features.cc

@@ -435,6 +435,11 @@ const base::Feature kMinimumChromeVersion{"MinimumChromeVersion",
 // ChromeOS Media App. https://crbug.com/996088.
 const base::Feature kMediaApp{"MediaApp", base::FEATURE_ENABLED_BY_DEFAULT};
 
+// Whether known extensions for RAW image formats are handled by the ChromeOS
+// Media App.
+const base::Feature kMediaAppHandlesRaw{"MediaAppHandlesRaw",
+                                        base::FEATURE_ENABLED_BY_DEFAULT};
+
 // Enables a unique URL for each path in CrOS settings.
 // This allows deep linking to individual settings, i.e. in settings search.
 const base::Feature kOsSettingsDeepLinking{"OsSettingsDeepLinking",

chromeos/constants/chromeos_features.h

@@ -196,6 +196,8 @@ COMPONENT_EXPORT(CHROMEOS_CONSTANTS)
 extern const base::Feature kLoginDisplayPasswordButton;
 COMPONENT_EXPORT(CHROMEOS_CONSTANTS) extern const base::Feature kMediaApp;
 COMPONENT_EXPORT(CHROMEOS_CONSTANTS)
+extern const base::Feature kMediaAppHandlesRaw;
+COMPONENT_EXPORT(CHROMEOS_CONSTANTS)
 extern const base::Feature kMinimumChromeVersion;
 COMPONENT_EXPORT(CHROMEOS_CONSTANTS)
 extern const base::Feature kOsSettingsDeepLinking;