[webkit-changes] [276906] trunk

Mon, 03 May 2021 10:24:17 -0700

Title: [276906] trunk
Revision
276906
Author
[email protected]
Date
2021-05-03 10:23:25 -0700 (Mon, 03 May 2021)

Log Message

Restore pre-r276879 behavior for FileSystem::deleteFile() and FileSystem::deleteEmptyDirectory()
https://bugs.webkit.org/show_bug.cgi?id=225289

Reviewed by Darin Adler.

Source/_javascript_Core:

Fix build.

* Configurations/_javascript_Core.xcconfig:

Source/WTF:

Restore pre-r276879 behavior for FileSystem::deleteFile() and FileSystem::deleteEmptyDirectory() to
reduce the risk of regressions. In particular, calling FileSystem::deleteFile() on an empty directory
now fails and calling FileSystem::deleteEmptyDirectory() on a non-directory now fails.

I have also gotten rid of the macOS-specific implementation of deleteEmptyDirectory() and merged the
needed behavior (Dealing with .DS_Store files) to the generic function.

* wtf/FileSystem.cpp:
(WTF::FileSystemImpl::deleteFile):
(WTF::FileSystemImpl::deleteEmptyDirectory):
* wtf/mac/FileSystemMac.mm:
(WTF::FileSystem::deleteEmptyDirectory): Deleted.

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WTF/FileSystem.cpp:
(TestWebKitAPI::TEST_F):

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (276905 => 276906)


--- trunk/Source/_javascript_Core/ChangeLog	2021-05-03 17:13:14 UTC (rev 276905)
+++ trunk/Source/_javascript_Core/ChangeLog	2021-05-03 17:23:25 UTC (rev 276906)
@@ -1,3 +1,14 @@
+2021-05-03  Chris Dumez  <[email protected]>
+
+        Restore pre-r276879 behavior for FileSystem::deleteFile() and FileSystem::deleteEmptyDirectory()
+        https://bugs.webkit.org/show_bug.cgi?id=225289
+
+        Reviewed by Darin Adler.
+
+        Fix build.
+
+        * Configurations/_javascript_Core.xcconfig:
+
 2021-05-03  Mark Lam  <[email protected]>
 
         Add some missing exception checks before some jsCasts.

Modified: trunk/Source/_javascript_Core/Configurations/_javascript_Core.xcconfig (276905 => 276906)


--- trunk/Source/_javascript_Core/Configurations/_javascript_Core.xcconfig	2021-05-03 17:13:14 UTC (rev 276905)
+++ trunk/Source/_javascript_Core/Configurations/_javascript_Core.xcconfig	2021-05-03 17:23:25 UTC (rev 276906)
@@ -27,7 +27,7 @@
 MODULEMAP_FILE = $(SRCROOT)/_javascript_Core.modulemap;
 
 // Prevent C++ standard library operator new, delete and their related exception types from being exported as weak symbols.
-OTHER_LDFLAGS_HIDE_SYMBOLS = -Wl,-unexported_symbol,__ZTISt9bad_alloc -Wl,-unexported_symbol,__ZTISt9exception -Wl,-unexported_symbol,__ZTSSt9bad_alloc -Wl,-unexported_symbol,__ZTSSt9exception -Wl,-unexported_symbol,__ZdlPvS_ -Wl,-unexported_symbol,__ZnwmPv -Wl,-unexported_symbol,__ZNKSt3__18functionIFvvEEclEv -Wl,-unexported_symbol,__ZNSt3__18functionIFvvEEC1EOS2_ -Wl,-unexported_symbol,__ZNSt3__18functionIFvvEEC2EOS2_ -Wl,-unexported_symbol,__ZNKSt3__18functionIFvRN3JSC17BytecodeGeneratorEPNS1_10RegisterIDEEEclES3_S5_ -Wl,-unexported_symbol,__ZNSt3__18functionIFvRN3JSC17BytecodeGeneratorEPNS1_10RegisterIDEEED1Ev -Wl,-unexported_symbol,__ZNSt3__18functionIFvRN3JSC17BytecodeGeneratorEPNS1_10RegisterIDEEED2Ev -Wl,-unexported_symbol,__ZNSt3__18functionIFvvEED1Ev -Wl,-unexported_symbol,__ZNSt3__18functionIFvvEED2Ev -Wl,-unexported_symbol,__ZTVNSt3__117bad_function_callE -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem24__is_pathable_char_arrayIPKcS4_cLb1EE11__range_endES4
 _ -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem24__is_pathable_char_arrayIPKcS4_cLb1EE13__range_beginES4_ -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem8_PathCVTIcE14__append_rangeIPKcEENS_9enable_ifIXsr27__is_cpp17_forward_iteratorIT_EE5valueEvE4typeERNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEES8_S8_ -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem8_PathCVTIcE15__append_sourceIPKcEEvRNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEERKT_ -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem8_PathCVTIcE14__append_rangeIPKcEENS_9enable_ifIXsr21__is_forward_iteratorIT_EE5valueEvE4typeERNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEES8_S8_;
+OTHER_LDFLAGS_HIDE_SYMBOLS = -Wl,-unexported_symbol,__ZTISt9bad_alloc -Wl,-unexported_symbol,__ZTISt9exception -Wl,-unexported_symbol,__ZTSSt9bad_alloc -Wl,-unexported_symbol,__ZTSSt9exception -Wl,-unexported_symbol,__ZdlPvS_ -Wl,-unexported_symbol,__ZnwmPv -Wl,-unexported_symbol,__ZNKSt3__18functionIFvvEEclEv -Wl,-unexported_symbol,__ZNSt3__18functionIFvvEEC1EOS2_ -Wl,-unexported_symbol,__ZNSt3__18functionIFvvEEC2EOS2_ -Wl,-unexported_symbol,__ZNKSt3__18functionIFvRN3JSC17BytecodeGeneratorEPNS1_10RegisterIDEEEclES3_S5_ -Wl,-unexported_symbol,__ZNSt3__18functionIFvRN3JSC17BytecodeGeneratorEPNS1_10RegisterIDEEED1Ev -Wl,-unexported_symbol,__ZNSt3__18functionIFvRN3JSC17BytecodeGeneratorEPNS1_10RegisterIDEEED2Ev -Wl,-unexported_symbol,__ZNSt3__18functionIFvvEED1Ev -Wl,-unexported_symbol,__ZNSt3__18functionIFvvEED2Ev -Wl,-unexported_symbol,__ZTVNSt3__117bad_function_callE -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem24__is_pathable_char_arrayIPKcS4_cLb1EE11__range_endES4_
  -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem24__is_pathable_char_arrayIPKcS4_cLb1EE13__range_beginES4_ -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem8_PathCVTIcE14__append_rangeIPKcEENS_9enable_ifIXsr27__is_cpp17_forward_iteratorIT_EE5valueEvE4typeERNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEES8_S8_ -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem8_PathCVTIcE15__append_sourceIPKcEEvRNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEERKT_ -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem8_PathCVTIcE14__append_rangeIPKcEENS_9enable_ifIXsr21__is_forward_iteratorIT_EE5valueEvE4typeERNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEES8_S8_ -Wl,-unexported_symbol,__ZNKSt3__14__fs10filesystem18directory_iteratordeEv -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem18directory_iteratorppEv -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem24__is_pathable_char_arrayIA10_cPccLb1EE11__range_endEPKc -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem24
 __is_pathable_char_arrayIA10_cPccLb1EE13__range_beginEPKc -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem4pathdVERKS2_ -Wl,-unexported_symbol,__ZNSt3__14__fs10filesystem8_PathCVTIcE15__append_sourceIA10_cEEvRNS_12basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEERKT_;
 
 OTHER_LDFLAGS_BASE = $(OTHER_LDFLAGS_HIDE_SYMBOLS) -force_load "$(BUILT_PRODUCTS_DIR)/DerivedSources/_javascript_Core/libWTF.a";
 OTHER_LDFLAGS[sdk=embedded*] = $(inherited) $(OTHER_LDFLAGS_BASE);

Modified: trunk/Source/WTF/ChangeLog (276905 => 276906)


--- trunk/Source/WTF/ChangeLog	2021-05-03 17:13:14 UTC (rev 276905)
+++ trunk/Source/WTF/ChangeLog	2021-05-03 17:23:25 UTC (rev 276906)
@@ -1,3 +1,23 @@
+2021-05-03  Chris Dumez  <[email protected]>
+
+        Restore pre-r276879 behavior for FileSystem::deleteFile() and FileSystem::deleteEmptyDirectory()
+        https://bugs.webkit.org/show_bug.cgi?id=225289
+
+        Reviewed by Darin Adler.
+
+        Restore pre-r276879 behavior for FileSystem::deleteFile() and FileSystem::deleteEmptyDirectory() to
+        reduce the risk of regressions. In particular, calling FileSystem::deleteFile() on an empty directory
+        now fails and calling FileSystem::deleteEmptyDirectory() on a non-directory now fails.
+
+        I have also gotten rid of the macOS-specific implementation of deleteEmptyDirectory() and merged the
+        needed behavior (Dealing with .DS_Store files) to the generic function.
+
+        * wtf/FileSystem.cpp:
+        (WTF::FileSystemImpl::deleteFile):
+        (WTF::FileSystemImpl::deleteEmptyDirectory):
+        * wtf/mac/FileSystemMac.mm:
+        (WTF::FileSystem::deleteEmptyDirectory): Deleted.
+
 2021-05-02  Sam Weinig  <[email protected]>
 
         Add support for DestinationColorSpace::DisplayP3 in preparation for DisplayP3 canvas

Modified: trunk/Source/WTF/wtf/FileSystem.cpp (276905 => 276906)


--- trunk/Source/WTF/wtf/FileSystem.cpp	2021-05-03 17:13:14 UTC (rev 276905)
+++ trunk/Source/WTF/wtf/FileSystem.cpp	2021-05-03 17:23:25 UTC (rev 276906)
@@ -493,19 +493,42 @@
 bool deleteFile(const String& path)
 {
     std::error_code ec;
+    std::filesystem::path fsPath = fileSystemRepresentation(path).data();
+
+    auto fileStatus = std::filesystem::symlink_status(fsPath, ec);
+    if (ec || fileStatus.type() == std::filesystem::file_type::directory)
+        return false;
+
     // remove() returns false on error so no need to check ec.
-    return std::filesystem::remove(fileSystemRepresentation(path).data(), ec);
+    return std::filesystem::remove(fsPath, ec);
 }
 
-// We currently use a different implementation on macOS to deal with .DS_Store files that may be present in otherwise empty directories.
-#if !PLATFORM(MAC)
 bool deleteEmptyDirectory(const String& path)
 {
     std::error_code ec;
+    std::filesystem::path fsPath = fileSystemRepresentation(path).data();
+
+    auto fileStatus = std::filesystem::symlink_status(fsPath, ec);
+    if (ec || fileStatus.type() != std::filesystem::file_type::directory)
+        return false;
+
+#if PLATFORM(MAC)
+    bool containsSingleDSStoreFile = false;
+    for (auto& entry : std::filesystem::directory_iterator(fsPath)) {
+        if (entry.path().filename() == ".DS_Store")
+            containsSingleDSStoreFile = true;
+        else {
+            containsSingleDSStoreFile = false;
+            break;
+        }
+    }
+    if (containsSingleDSStoreFile)
+        std::filesystem::remove(fsPath / ".DS_Store", ec);
+#endif
+
     // remove() returns false on error so no need to check ec.
-    return std::filesystem::remove(fileSystemRepresentation(path).data(), ec);
+    return std::filesystem::remove(fsPath, ec);
 }
-#endif
 
 bool moveFile(const String& oldPath, const String& newPath)
 {

Modified: trunk/Source/WTF/wtf/mac/FileSystemMac.mm (276905 => 276906)


--- trunk/Source/WTF/wtf/mac/FileSystemMac.mm	2021-05-03 17:13:14 UTC (rev 276905)
+++ trunk/Source/WTF/wtf/mac/FileSystemMac.mm	2021-05-03 17:23:25 UTC (rev 276906)
@@ -34,20 +34,6 @@
 
 namespace WTF {
 
-bool FileSystem::deleteEmptyDirectory(const String& path)
-{
-    auto fileManager = adoptNS([[NSFileManager alloc] init]);
-
-    if (NSArray *directoryContents = [fileManager contentsOfDirectoryAtPath:path error:nullptr]) {
-        // Explicitly look for and delete .DS_Store files.
-        if (directoryContents.count == 1 && [directoryContents.firstObject isEqualToString:@".DS_Store"])
-            [fileManager removeItemAtPath:[path stringByAppendingPathComponent:directoryContents.firstObject] error:nullptr];
-    }
-
-    // rmdir(...) returns 0 on successful deletion of the path and non-zero in any other case (including invalid permissions or non-existent file)
-    return !rmdir(FileSystem::fileSystemRepresentation(path).data());
-}
-
 void FileSystem::setMetadataURL(const String& path, const String& metadataURLString, const String& referrer)
 {
     String urlString;

Modified: trunk/Tools/ChangeLog (276905 => 276906)


--- trunk/Tools/ChangeLog	2021-05-03 17:13:14 UTC (rev 276905)
+++ trunk/Tools/ChangeLog	2021-05-03 17:23:25 UTC (rev 276906)
@@ -1,3 +1,15 @@
+2021-05-03  Chris Dumez  <[email protected]>
+
+        Restore pre-r276879 behavior for FileSystem::deleteFile() and FileSystem::deleteEmptyDirectory()
+        https://bugs.webkit.org/show_bug.cgi?id=225289
+
+        Reviewed by Darin Adler.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WTF/FileSystem.cpp:
+        (TestWebKitAPI::TEST_F):
+
 2021-05-03  Sam Weinig  <[email protected]>
 
         Remove default parameter values for color space and pixel format from ImageBuffer::create to make it clear everwhere we are explicitly requesting SRGB/BGRA8 buffers

Modified: trunk/Tools/TestWebKitAPI/Tests/WTF/FileSystem.cpp (276905 => 276906)


--- trunk/Tools/TestWebKitAPI/Tests/WTF/FileSystem.cpp	2021-05-03 17:13:14 UTC (rev 276905)
+++ trunk/Tools/TestWebKitAPI/Tests/WTF/FileSystem.cpp	2021-05-03 17:23:25 UTC (rev 276906)
@@ -223,6 +223,13 @@
     EXPECT_FALSE(FileSystem::deleteFile(tempFilePath()));
 }
 
+TEST_F(FileSystemTest, deleteFileOnEmptyDirectory)
+{
+    EXPECT_TRUE(FileSystem::fileExists(tempEmptyFolderPath()));
+    EXPECT_FALSE(FileSystem::deleteFile(tempEmptyFolderPath()));
+    EXPECT_TRUE(FileSystem::fileExists(tempEmptyFolderPath()));
+}
+
 TEST_F(FileSystemTest, deleteEmptyDirectory)
 {
     EXPECT_TRUE(FileSystem::fileExists(tempEmptyFolderPath()));
@@ -231,6 +238,64 @@
     EXPECT_FALSE(FileSystem::deleteEmptyDirectory(tempEmptyFolderPath()));
 }
 
+#if PLATFORM(MAC)
+TEST_F(FileSystemTest, deleteEmptyDirectoryContainingDSStoreFile)
+{
+    EXPECT_TRUE(FileSystem::fileExists(tempEmptyFolderPath()));
+
+    // Create .DSStore file.
+    auto dsStorePath = FileSystem::pathByAppendingComponent(tempEmptyFolderPath(), ".DS_Store");
+    auto dsStoreHandle = FileSystem::openFile(dsStorePath, FileSystem::FileOpenMode::Write);
+    FileSystem::writeToFile(dsStoreHandle, FileSystemTestData, strlen(FileSystemTestData));
+    FileSystem::closeFile(dsStoreHandle);
+    EXPECT_TRUE(FileSystem::fileExists(dsStorePath));
+
+    EXPECT_TRUE(FileSystem::deleteEmptyDirectory(tempEmptyFolderPath()));
+    EXPECT_FALSE(FileSystem::fileExists(tempEmptyFolderPath()));
+}
+#endif
+
+TEST_F(FileSystemTest, deleteEmptyDirectoryOnNonEmptyDirectory)
+{
+    EXPECT_TRUE(FileSystem::fileExists(tempEmptyFolderPath()));
+
+    // Create .DSStore file.
+    auto dsStorePath = FileSystem::pathByAppendingComponent(tempEmptyFolderPath(), ".DS_Store");
+    auto dsStoreHandle = FileSystem::openFile(dsStorePath, FileSystem::FileOpenMode::Write);
+    FileSystem::writeToFile(dsStoreHandle, FileSystemTestData, strlen(FileSystemTestData));
+    FileSystem::closeFile(dsStoreHandle);
+    EXPECT_TRUE(FileSystem::fileExists(dsStorePath));
+
+    // Create a dummy file.
+    auto dummyFilePath = FileSystem::pathByAppendingComponent(tempEmptyFolderPath(), "dummyFile");
+    auto dummyFileHandle = FileSystem::openFile(dummyFilePath, FileSystem::FileOpenMode::Write);
+    FileSystem::writeToFile(dummyFileHandle, FileSystemTestData, strlen(FileSystemTestData));
+    FileSystem::closeFile(dummyFileHandle);
+    EXPECT_TRUE(FileSystem::fileExists(dummyFilePath));
+
+    EXPECT_FALSE(FileSystem::deleteEmptyDirectory(tempEmptyFolderPath()));
+    EXPECT_TRUE(FileSystem::fileExists(tempEmptyFolderPath()));
+    EXPECT_TRUE(FileSystem::fileExists(dsStorePath));
+    EXPECT_TRUE(FileSystem::fileExists(dummyFilePath));
+
+    EXPECT_TRUE(FileSystem::deleteNonEmptyDirectory(tempEmptyFolderPath()));
+    EXPECT_FALSE(FileSystem::fileExists(tempEmptyFolderPath()));
+}
+
+TEST_F(FileSystemTest, deleteEmptyDirectoryOnARegularFile)
+{
+    EXPECT_TRUE(FileSystem::fileExists(tempFilePath()));
+    EXPECT_FALSE(FileSystem::deleteEmptyDirectory(tempFilePath()));
+    EXPECT_TRUE(FileSystem::fileExists(tempFilePath()));
+}
+
+TEST_F(FileSystemTest, deleteEmptyDirectoryDoesNotExist)
+{
+    auto doesNotExistPath = FileSystem::pathByAppendingComponent(tempEmptyFolderPath(), "does-not-exist");
+    EXPECT_FALSE(FileSystem::fileExists(doesNotExistPath));
+    EXPECT_FALSE(FileSystem::deleteEmptyDirectory(doesNotExistPath));
+}
+
 TEST_F(FileSystemTest, moveFile)
 {
     auto destination = FileSystem::pathByAppendingComponent(tempEmptyFolderPath(), "tempFile-moved");

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to