[M111] FSA: Ship moves of local files

Support moving files that do not live in the OPFS. See the explainer: a-sully/fs#2 I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/ogS8CeyZ3n8 (cherry picked from commit ccf41fa) Bug: 1140805 Change-Id: Idf1765c44b916935135fa55489a1870452a366d4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3627034 Reviewed-by: Daseul Lee <[email protected]> Commit-Queue: Austin Sullivan <[email protected]> Cr-Original-Commit-Position: refs/heads/main@{#1100150} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4220493 Commit-Queue: Daseul Lee <[email protected]> Auto-Submit: Austin Sullivan <[email protected]> Cr-Commit-Position: refs/branch-heads/5563@{chromium#155} Cr-Branched-From: 3ac59a6-refs/heads/main@{#1097615}
chirayudesai · Feb 3, 2023 · 785f7b9 · 785f7b9
1 parent 4e070d6
commit 785f7b9
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 15 deletions.
diff --git a/content/browser/file_system_access/features.cc b/content/browser/file_system_access/features.cc
@@ -25,6 +25,13 @@ BASE_FEATURE(kFileSystemAccessDoNotOverwriteOnMove,
              "FileSystemAccessDoNotOverwriteOnMove",
              base::FEATURE_DISABLED_BY_DEFAULT);
 
+// TODO(crbug.com/1140805): Remove this flag eventually.
+// When enabled, move() supports moving local files (i.e. that do not live in
+// the OPFS).
+BASE_FEATURE(kFileSystemAccessMoveLocalFiles,
+             "FileSystemAccessMoveLocalFiles",
+             base::FEATURE_ENABLED_BY_DEFAULT);
+
 // TODO(crbug.com/1114923): Remove this flag eventually.
 // When enabled, the remove() method is enabled. Otherwise, throws a
 // NotSupportedError DomException.

diff --git a/content/browser/file_system_access/features.h b/content/browser/file_system_access/features.h
@@ -16,6 +16,7 @@ namespace content::features {
 // Alphabetical:
 CONTENT_EXPORT BASE_DECLARE_FEATURE(kFileSystemAccessDragAndDropCheckBlocklist);
 CONTENT_EXPORT BASE_DECLARE_FEATURE(kFileSystemAccessDoNotOverwriteOnMove);
+CONTENT_EXPORT BASE_DECLARE_FEATURE(kFileSystemAccessMoveLocalFiles);
 CONTENT_EXPORT BASE_DECLARE_FEATURE(kFileSystemAccessRemove);
 CONTENT_EXPORT BASE_DECLARE_FEATURE(kFileSystemAccessRemoveEntryExclusiveLock);
 CONTENT_EXPORT BASE_DECLARE_FEATURE(

diff --git a/content/browser/file_system_access/file_system_access_file_handle_move_browsertest.cc b/content/browser/file_system_access/file_system_access_file_handle_move_browsertest.cc
@@ -4,6 +4,8 @@
 
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/test/scoped_feature_list.h"
+#include "content/browser/file_system_access/features.h"
 #include "content/browser/file_system_access/file_system_chooser_test_helpers.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test.h"
@@ -108,4 +110,30 @@ IN_PROC_BROWSER_TEST_F(FileSystemAccessFileHandleMoveBrowserTest,
       << result.error;
 }
 
+class FileSystemAccessFileHandleMoveLocalBrowserTest
+    : public FileSystemAccessFileHandleMoveBrowserTest {
+ public:
+  FileSystemAccessFileHandleMoveLocalBrowserTest() {
+    scoped_feature_list_.InitAndDisableFeature(
+        features::kFileSystemAccessMoveLocalFiles);
+  }
+
+  void SetUp() override { FileSystemAccessFileHandleMoveBrowserTest::SetUp(); }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(FileSystemAccessFileHandleMoveLocalBrowserTest,
+                       RenameLocal) {
+  std::string file_contents = "move me";
+  CreateTestFileInDirectory(temp_dir_.GetPath(), file_contents);
+
+  auto result = EvalJs(shell(),
+                       "(async () => {"
+                       "return await self.localFile.move('renamed.txt'); })()");
+  EXPECT_TRUE(result.error.find("not support") != std::string::npos)
+      << result.error;
+}
+
 }  // namespace content
diff --git a/content/browser/file_system_access/file_system_access_handle_base.cc b/content/browser/file_system_access/file_system_access_handle_base.cc
@@ -7,7 +7,6 @@
 #include <memory>
 #include <vector>
 
-#include "base/command_line.h"
 #include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/functional/bind.h"
@@ -26,7 +25,6 @@
 #include "content/public/browser/file_system_access_permission_context.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_client.h"
-#include "content/public/common/content_switches.h"
 #include "storage/browser/file_system/file_system_operation.h"
 #include "storage/browser/file_system/file_system_url.h"
 #include "storage/common/file_system/file_system_types.h"
@@ -220,12 +218,11 @@ void FileSystemAccessHandleBase::DoMove(
   DCHECK_EQ(GetWritePermissionStatus(),
             blink::mojom::PermissionStatus::GRANTED);
 
-  // TODO(crbug.com/1247850): Allow moves of files outside of the OPFS.
-  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableExperimentalWebPlatformFeatures)) {
+  if (!base::FeatureList::IsEnabled(
+          features::kFileSystemAccessMoveLocalFiles)) {
     if (url().type() != storage::FileSystemType::kFileSystemTypeTemporary) {
       std::move(callback).Run(file_system_access_error::FromStatus(
-          blink::mojom::FileSystemAccessStatus::kOperationAborted));
+          blink::mojom::FileSystemAccessStatus::kNotSupportedError));
       return;
     }
   }
@@ -254,12 +251,11 @@ void FileSystemAccessHandleBase::DoRename(
   DCHECK_EQ(GetWritePermissionStatus(),
             blink::mojom::PermissionStatus::GRANTED);
 
-  // TODO(crbug.com/1247850): Allow moves of files outside of the OPFS.
-  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableExperimentalWebPlatformFeatures)) {
+  if (!base::FeatureList::IsEnabled(
+          features::kFileSystemAccessMoveLocalFiles)) {
     if (url().type() != storage::FileSystemType::kFileSystemTypeTemporary) {
       std::move(callback).Run(file_system_access_error::FromStatus(
-          blink::mojom::FileSystemAccessStatus::kOperationAborted));
+          blink::mojom::FileSystemAccessStatus::kNotSupportedError));
       return;
     }
   }
@@ -337,12 +333,11 @@ void FileSystemAccessHandleBase::DidCreateDestinationDirectoryHandle(
     return;
   }
 
-  // TODO(crbug.com/1247850): Allow moves of files outside of the OPFS.
-  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableExperimentalWebPlatformFeatures)) {
+  if (!base::FeatureList::IsEnabled(
+          features::kFileSystemAccessMoveLocalFiles)) {
     if (dest_url.type() != storage::FileSystemType::kFileSystemTypeTemporary) {
       std::move(callback).Run(file_system_access_error::FromStatus(
-          blink::mojom::FileSystemAccessStatus::kOperationAborted));
+          blink::mojom::FileSystemAccessStatus::kNotSupportedError));
       return;
     }
   }

diff --git a/third_party/blink/web_tests/TestExpectations b/third_party/blink/web_tests/TestExpectations
@@ -5550,7 +5550,6 @@ crbug.com/1378476 [ Mac12-arm64 ] virtual/threaded/fast/idleToBlob/OffscreenCanv
 crbug.com/1250457 [ Mac ] storage/websql/open-database-set-empty-version.html [ Timeout ]
 crbug.com/1250457 [ Mac ] virtual/prefer_compositing_to_lcd_text/scrollbars/custom-scrollbar-inactive-pseudo.html [ Timeout ]
 
-# FileSystemHandle::move() is temporarily disabled outside of the Origin Private File System
 # FileSystemHandle::move() is temporarily disabled for directory handles
 crbug.com/1250534 external/wpt/file-system-access/sandboxed_FileSystemDirectoryHandle-move.https.any.html [ Failure ]
 crbug.com/1250534 external/wpt/file-system-access/sandboxed_FileSystemDirectoryHandle-move.https.any.worker.html [ Failure ]