Bug 225255 – Start leveraging std::filesystem in WTF::FileSystem

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Chris Dumez]

Start leveraging std::filesystem in WTF::FileSystem to reduce the amount of platform-specific code.

[Chris Dumez]

Created attachment 427460 [details]
Patch

[Chris Dumez]

Created attachment 427461 [details]
Patch

[Chris Dumez]

Created attachment 427466 [details]
Patch

[Chris Dumez]

Created attachment 427467 [details]
Patch

[Chris Dumez]

Created attachment 427468 [details]
Patch

[Chris Dumez]

Created attachment 427474 [details]
Patch

[Chris Dumez]

Created attachment 427476 [details]
Patch

[Sam Weinig]

Awesome!

[Chris Dumez]

API-gtk ews keeps saying I broke WTF.StringOperators API test. I am not quite sure how that’s possible. This test does not seem to rely on the file system api in any way…

[Sam Weinig]

Comment on attachment 427476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427476&action=review

> Source/WTF/ChangeLog:9
> +        Start leveraging std::filesystem in WTF::FileSystem to reduce the amount of
> +        platform-specific code.

I think doing this in WTF::FileSystem at first is a good idea, but I would also like to eventually see WTF::FileSystem removed in favor of direct usage of std::filesystem if we can. Time will tell.

> Source/WTF/wtf/FileSystem.cpp:52
> +namespace fs = std::filesystem;

Not a huge fan of this. I think having longer names is probably ok, but I also think this is acceptable in a cpp file.

> Source/WTF/wtf/FileSystem.cpp:491
> +    return fs::exists(fileSystemRepresentation(path).data(), ec);

Would be good to explain why the error code is not being checked, since it seems odd at first glance.

> Source/WTF/wtf/FileSystem.cpp:501
> +#if !PLATFORM(MAC)

Would be good to have a comment explaining why macOS has a non-default implementation.

[Sam Weinig]

I can't remember if we have officially dropped 10.14 yet, but if we have, we can also remove the copy of std::filesystem we have in WTF called StdFilesystem.h/cpp.

[Chris Dumez]

(In reply to Sam Weinig from comment #10)
> Comment on attachment 427476 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427476&action=review
> 
> > Source/WTF/ChangeLog:9
> > +        Start leveraging std::filesystem in WTF::FileSystem to reduce the amount of
> > +        platform-specific code.
> 
> I think doing this in WTF::FileSystem at first is a good idea, but I would
> also like to eventually see WTF::FileSystem removed in favor of direct usage
> of std::filesystem if we can. Time will tell.

I see the WTF::FileSystem as a layer over std::filesystem allowing use to use WTF::String, which is convenient. Having to deal with conversion to and from file system representations would be annoying and potentially error-prone.

> > Source/WTF/wtf/FileSystem.cpp:52
> > +namespace fs = std::filesystem;
> 
> Not a huge fan of this. I think having longer names is probably ok, but I
> also think this is acceptable in a cpp file.

Ok. I will revert. I don't feel super strongly.

> > Source/WTF/wtf/FileSystem.cpp:491
> > +    return fs::exists(fileSystemRepresentation(path).data(), ec);
> 
> Would be good to explain why the error code is not being checked, since it
> seems odd at first glance.

Ok. Basically, exists() returns false in case of error so it is not useful to check ec. This function was already returning false in case of error so the behavior is maintained as far as I can tell.

> 
> > Source/WTF/wtf/FileSystem.cpp:501
> > +#if !PLATFORM(MAC)
> 
> Would be good to have a comment explaining why macOS has a non-default
> implementation.

I am planning to improve this in a follow-up. Basically the issue is that on Mac, we need to support removing a folder that has a hidden .DS_Store file in there. I'll add a comment for now but I'll improve things and write a test later on.

[Chris Dumez]

(In reply to Sam Weinig from comment #11)
> I can't remember if we have officially dropped 10.14 yet, but if we have, we
> can also remove the copy of std::filesystem we have in WTF called
> StdFilesystem.h/cpp.

I will try and find out. I hope we have because the copy of std::filesystem in StdFilesystem.h doesn't seem to contain nearly enough API to support this patch.

[Chris Dumez]

Created attachment 427506 [details]
Patch

[Chris Dumez]

Created attachment 427511 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #15)
> Created attachment 427511 [details]
> Patch

Should take care of WTF.StringOperators in gtk-ews. Turns out using operator+ for String in other API tests can cause WTF.StringOperators to fail. We should probably fix that at some point. For now, I am working around it by not using operator+.

[Chris Dumez]

Created attachment 427512 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #13)
> (In reply to Sam Weinig from comment #11)
> > I can't remember if we have officially dropped 10.14 yet, but if we have, we
> > can also remove the copy of std::filesystem we have in WTF called
> > StdFilesystem.h/cpp.
> 
> I will try and find out. I hope we have because the copy of std::filesystem
> in StdFilesystem.h doesn't seem to contain nearly enough API to support this
> patch.

I checked with Ryan and he said it was OK. As a result, I dropped our own copy of std::filesystem in StdFilesystem.h. I kept the StdFilesystem.h header to conditional include the experimental version when needed.

[EWS]

Committed r276879 (237225@main): <https://commits.webkit.org/237225@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427512 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77425312>

[Sam Weinig]

(In reply to Chris Dumez from comment #18)
> (In reply to Chris Dumez from comment #13)
> > (In reply to Sam Weinig from comment #11)
> > > I can't remember if we have officially dropped 10.14 yet, but if we have, we
> > > can also remove the copy of std::filesystem we have in WTF called
> > > StdFilesystem.h/cpp.
> > 
> > I will try and find out. I hope we have because the copy of std::filesystem
> > in StdFilesystem.h doesn't seem to contain nearly enough API to support this
> > patch.
> 
> I checked with Ryan and he said it was OK. As a result, I dropped our own
> copy of std::filesystem in StdFilesystem.h. I kept the StdFilesystem.h
> header to conditional include the experimental version when needed.

Excellent. Thanks for following up!

I added StdFilesystem.h to make work in WebKitTestRunner and DumpRenderTree nicer but am very glad to see it gone.

[Darin Adler]

This patch removes some features from WTF::FileSystem that std::filesystem chose not to include. We need to decide whether they are valuable:

WTF::FileSystem::deleteEmptyDirectory and WTF::FileSystem::deleteNonEmptyDirectory on a file: The std::filesystem functions work on both files and directories, so these have lost their "if it’s a file just leave it alone" semantic, but without changing the function names.

WTF::FileSystem::deleteFile on a directory: The std::filesystem functions work on both files and directories, so it have lost its "if it’s a directory just leave it alone" semantic, but we made that change without changing the function name.

WTF::FileSystem::moveFile: The Cocoa implementation of this function had the power to move files across file systems, by copying and then deleting the original. As did the Windows version and probably the others as well. I don’t think that std::filesystem::rename does this. Not sure if it matters; need to audit callers to see if they need this cross-file-system behavior.

There may be other changes as well. I don’t object to this change, but really we should make WTF::FileSystem a thinner layer on top of std::filesystem and remove the names that imply we have features that we no longer have.

And in the process look very carefully at code calling these functions to see if there were benefits to the old implementation that are lost and make sure we made an informed decision.

[Chris Dumez]

(In reply to Darin Adler from comment #22)
> This patch removes some features from WTF::FileSystem that std::filesystem
> chose not to include. We need to decide whether they are valuable:
> 
> WTF::FileSystem::deleteEmptyDirectory and
> WTF::FileSystem::deleteNonEmptyDirectory on a file: The std::filesystem
> functions work on both files and directories, so these have lost their "if
> it’s a file just leave it alone" semantic, but without changing the function
> names.
> 
> WTF::FileSystem::deleteFile on a directory: The std::filesystem functions
> work on both files and directories, so it have lost its "if it’s a directory
> just leave it alone" semantic, but we made that change without changing the
> function name.
> 
> WTF::FileSystem::moveFile: The Cocoa implementation of this function had the
> power to move files across file systems, by copying and then deleting the
> original. As did the Windows version and probably the others as well. I
> don’t think that std::filesystem::rename does this. Not sure if it matters;
> need to audit callers to see if they need this cross-file-system behavior.
> 
> There may be other changes as well. I don’t object to this change, but
> really we should make WTF::FileSystem a thinner layer on top of
> std::filesystem and remove the names that imply we have features that we no
> longer have.
> 
> And in the process look very carefully at code calling these functions to
> see if there were benefits to the old implementation that are lost and make
> sure we made an informed decision.

Thanks for pointing this out. I was trying to maintain previous behavior to reduce the chance of breakage but it looks like I missed some subtleties. I will follow up.

[Chris Dumez]

(In reply to Darin Adler from comment #22)
> This patch removes some features from WTF::FileSystem that std::filesystem
> chose not to include. We need to decide whether they are valuable:
> 
> WTF::FileSystem::deleteEmptyDirectory and
> WTF::FileSystem::deleteNonEmptyDirectory on a file: The std::filesystem
> functions work on both files and directories, so these have lost their "if
> it’s a file just leave it alone" semantic, but without changing the function
> names.
> 
> WTF::FileSystem::deleteFile on a directory: The std::filesystem functions
> work on both files and directories, so it have lost its "if it’s a directory
> just leave it alone" semantic, but we made that change without changing the
> function name.

Restoring previous behavior via Bug 225289.

> 
> WTF::FileSystem::moveFile: The Cocoa implementation of this function had the
> power to move files across file systems, by copying and then deleting the
> original. As did the Windows version and probably the others as well. I
> don’t think that std::filesystem::rename does this. Not sure if it matters;
> need to audit callers to see if they need this cross-file-system behavior.

There are not many call sites:
1. IDBServer::renameOrigin(): Simple rename within the same directory so the new implementation is fine.
2. SQLiteIDBBackingStore::fullDatabaseDirectoryWithUpgrade(): Simple rename of a directory
3. CDMInstanceFairPlayStreamingAVFObjC::setStorageDirectory(): It is calling FileSystem::createTemporaryDirectory(@"MediaKeys") and then moving that directory to the CDMInstanceFairPlayStreamingAVFObjC storage directory. I think that in theory at least, the temp directory and the actual temp directory could be on different volumes so we may have an issue here. I'll look into it.
4. CurlDownload::curlDidComplete(): Moves files from FileSystem::openTemporaryFile("download", m_downloadFileHandle) to the download destination. In theory, those could be on different Volumes I think.
5. StorageManager::renameOrigin(): Simple rename within a folder

So it looks like there are 2 call sites that may be problematic. I have not checked yet what std::filesystem::rename() does across volumes.

[Chris Dumez]

(In reply to Chris Dumez from comment #24)
> (In reply to Darin Adler from comment #22)
> > This patch removes some features from WTF::FileSystem that std::filesystem
> > chose not to include. We need to decide whether they are valuable:
> > 
> > WTF::FileSystem::deleteEmptyDirectory and
> > WTF::FileSystem::deleteNonEmptyDirectory on a file: The std::filesystem
> > functions work on both files and directories, so these have lost their "if
> > it’s a file just leave it alone" semantic, but without changing the function
> > names.
> > 
> > WTF::FileSystem::deleteFile on a directory: The std::filesystem functions
> > work on both files and directories, so it have lost its "if it’s a directory
> > just leave it alone" semantic, but we made that change without changing the
> > function name.
> 
> Restoring previous behavior via Bug 225289.
> 
> > 
> > WTF::FileSystem::moveFile: The Cocoa implementation of this function had the
> > power to move files across file systems, by copying and then deleting the
> > original. As did the Windows version and probably the others as well. I
> > don’t think that std::filesystem::rename does this. Not sure if it matters;
> > need to audit callers to see if they need this cross-file-system behavior.
> 
> There are not many call sites:
> 1. IDBServer::renameOrigin(): Simple rename within the same directory so the
> new implementation is fine.
> 2. SQLiteIDBBackingStore::fullDatabaseDirectoryWithUpgrade(): Simple rename
> of a directory
> 3. CDMInstanceFairPlayStreamingAVFObjC::setStorageDirectory(): It is calling
> FileSystem::createTemporaryDirectory(@"MediaKeys") and then moving that
> directory to the CDMInstanceFairPlayStreamingAVFObjC storage directory. I
> think that in theory at least, the temp directory and the actual temp
> directory could be on different volumes so we may have an issue here. I'll
> look into it.
> 4. CurlDownload::curlDidComplete(): Moves files from
> FileSystem::openTemporaryFile("download", m_downloadFileHandle) to the
> download destination. In theory, those could be on different Volumes I think.
> 5. StorageManager::renameOrigin(): Simple rename within a folder
> 
> So it looks like there are 2 call sites that may be problematic. I have not
> checked yet what std::filesystem::rename() does across volumes.

Give that call sites seem to need it, I am adding back support for moving across volumes via Bug 225307.