Port Filesystem::getFileModificationTime() to std::filesystem.
Created attachment 427686 [details] Patch
Created attachment 427687 [details] Patch
Comment on attachment 427687 [details] Patch Looks like the new test is failing for the GTK port. Will try and figure it out.
Created attachment 427690 [details] Patch
Created attachment 427691 [details] Patch
Test is still failing in GTK: ../../Tools/TestWebKitAPI/Tests/WTF/FileSystem.cpp:560 Expected: (newModificationTime->secondsSinceEpoch().value()) > (timeBeforeModification.secondsSinceEpoch().value()), actual: -4.81751e+09 vs 1.62016e+09 Look like I am getting a negative modification time :/
Created attachment 427694 [details] Patch
Created attachment 427712 [details] Patch
Comment on attachment 427694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427694&action=review > Source/WTF/wtf/FileSystem.cpp:633 > + auto modificationTimeAsTimeT = std::chrono::system_clock::to_time_t(std::chrono::time_point_cast<std::chrono::system_clock::duration>(modificationTime - decltype(modificationTime)::clock::now() + std::chrono::system_clock::now())); There must be a way that doesn’t depend on two subsequent calls to now! I looked here https://stackoverflow.com/questions/61030383/how-to-convert-stdfilesystemfile-time-type-to-time-t Didn’t find something better.
(In reply to Darin Adler from comment #9) > Comment on attachment 427694 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427694&action=review > > > Source/WTF/wtf/FileSystem.cpp:633 > > + auto modificationTimeAsTimeT = std::chrono::system_clock::to_time_t(std::chrono::time_point_cast<std::chrono::system_clock::duration>(modificationTime - decltype(modificationTime)::clock::now() + std::chrono::system_clock::now())); > > There must be a way that doesn’t depend on two subsequent calls to now! > > I looked here > https://stackoverflow.com/questions/61030383/how-to-convert- > stdfilesystemfile-time-type-to-time-t > > Didn’t find something better. I did some research and apparently there is no good way with C++17. This is the best I could find so I did this and added a FIXME comment for when we switch to C++20 (which has a better way).
Comment on attachment 427712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427712&action=review > Source/WTF/wtf/FileSystem.h:120 > -WTF_EXPORT_PRIVATE Optional<FileMetadata> fileMetadata(const String& path); > -WTF_EXPORT_PRIVATE Optional<FileMetadata> fileMetadataFollowingSymlinks(const String& path); > +WTF_EXPORT_PRIVATE Optional<FileMetadata> fileMetadata(const String& path, ShouldFollowSymbolicLinks = ShouldFollowSymbolicLinks::No); Is this an improvement?
(In reply to Darin Adler from comment #11) > Comment on attachment 427712 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427712&action=review > > > Source/WTF/wtf/FileSystem.h:120 > > -WTF_EXPORT_PRIVATE Optional<FileMetadata> fileMetadata(const String& path); > > -WTF_EXPORT_PRIVATE Optional<FileMetadata> fileMetadataFollowingSymlinks(const String& path); > > +WTF_EXPORT_PRIVATE Optional<FileMetadata> fileMetadata(const String& path, ShouldFollowSymbolicLinks = ShouldFollowSymbolicLinks::No); > > Is this an improvement? I think so. We already have the ShouldFollowSymbolicLinks enum and use it for other FileSystem functions. I don't see why fileMetadata() needs a separate function for this. I hesitated making the parameter mandatory though. You seem to disagree?
(In reply to Chris Dumez from comment #12) > You seem to disagree? I generally prefer named functions to parameters that set policy for a function, except in cases where the combinatorial explosion makes them impractical, or when it’s more common to pass an argument through from one level to the next, rather than pass a constant. Just seems less wordy and more human. I think I inherited that preference from Cocoa naming style.
(In reply to Darin Adler from comment #13) > (In reply to Chris Dumez from comment #12) > > You seem to disagree? > > I generally prefer named functions to parameters that set policy for a > function, except in cases where the combinatorial explosion makes them > impractical, or when it’s more common to pass an argument through from one > level to the next, rather than pass a constant. > > Just seems less wordy and more human. > > I think I inherited that preference from Cocoa naming style. Ok. I will revert this part of my change since it seems a little controversial. I do think that we should make our FileSystem API consistent eventually though, in one direction or another (I have a slight preference for the parameter but I won't fight you on this).
(In reply to Chris Dumez from comment #14) > I do think that we should make our FileSystem API consistent > eventually though, in one direction or another (I have a slight preference > for the parameter but I won't fight you on this). I agree. And my preference is also slight!
Another strength of having a separate named function is that it’s easier to find out if it’s used or not.
Created attachment 427722 [details] Patch
Created attachment 427729 [details] Patch
Created attachment 427771 [details] Patch
(In reply to Chris Dumez from comment #10) > (In reply to Darin Adler from comment #9) > > Comment on attachment 427694 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=427694&action=review > > > > > Source/WTF/wtf/FileSystem.cpp:633 > > > + auto modificationTimeAsTimeT = std::chrono::system_clock::to_time_t(std::chrono::time_point_cast<std::chrono::system_clock::duration>(modificationTime - decltype(modificationTime)::clock::now() + std::chrono::system_clock::now())); > > > > There must be a way that doesn’t depend on two subsequent calls to now! > > > > I looked here > > https://stackoverflow.com/questions/61030383/how-to-convert- > > stdfilesystemfile-time-type-to-time-t > > > > Didn’t find something better. > > I did some research and apparently there is no good way with C++17. This is > the best I could find so I did this and added a FIXME comment for when we > switch to C++20 (which has a better way). In this latest iteration, I use SFINAE to use std::filesystem::file_time_type::clock::to_time_t() whenever available (It is available on my machine but according to [1] is not available in MSVC and GCC 9). [1] https://en.cppreference.com/w/cpp/filesystem/file_time_type
(In reply to Chris Dumez from comment #20) > (In reply to Chris Dumez from comment #10) > > (In reply to Darin Adler from comment #9) > > > Comment on attachment 427694 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=427694&action=review > > > > > > > Source/WTF/wtf/FileSystem.cpp:633 > > > > + auto modificationTimeAsTimeT = std::chrono::system_clock::to_time_t(std::chrono::time_point_cast<std::chrono::system_clock::duration>(modificationTime - decltype(modificationTime)::clock::now() + std::chrono::system_clock::now())); > > > > > > There must be a way that doesn’t depend on two subsequent calls to now! > > > > > > I looked here > > > https://stackoverflow.com/questions/61030383/how-to-convert- > > > stdfilesystemfile-time-type-to-time-t > > > > > > Didn’t find something better. > > > > I did some research and apparently there is no good way with C++17. This is > > the best I could find so I did this and added a FIXME comment for when we > > switch to C++20 (which has a better way). > > In this latest iteration, I use SFINAE to use > std::filesystem::file_time_type::clock::to_time_t() whenever available (It > is available on my machine but according to [1] is not available in MSVC and > GCC 9). > > [1] https://en.cppreference.com/w/cpp/filesystem/file_time_type But of course, it doesn't build on GTK :face-palm:
Created attachment 427773 [details] Patch
Created attachment 427774 [details] Patch
Comment on attachment 427774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427774&action=review > Source/WTF/wtf/FileSystem.cpp:671 > + if (shouldFollowSymbolicLinks == ShouldFollowSymbolicLinks::Yes && std::filesystem::is_symlink(fsPath, ec)) { > + fsPath = std::filesystem::read_symlink(fsPath, ec); > + if (ec) > + return WTF::nullopt; > + } This follows one level of symbolic link. To "follow" a symbolic ink I think we may need to do this in a loop, likely with a limit of some sort to deal with cycles.
(In reply to Darin Adler from comment #24) > Comment on attachment 427774 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427774&action=review > > > Source/WTF/wtf/FileSystem.cpp:671 > > + if (shouldFollowSymbolicLinks == ShouldFollowSymbolicLinks::Yes && std::filesystem::is_symlink(fsPath, ec)) { > > + fsPath = std::filesystem::read_symlink(fsPath, ec); > > + if (ec) > > + return WTF::nullopt; > > + } > > This follows one level of symbolic link. To "follow" a symbolic ink I think > we may need to do this in a loop, likely with a limit of some sort to deal > with cycles. Interesting. I did not realize this was a thing. I'll write a test and tweak the code accordingly.
Created attachment 427781 [details] Patch
Anybody willing to review this? I have addressed all the feedback so far.
Comment on attachment 427781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427781&action=review > Source/WTF/wtf/FileSystem.cpp:130 > + std::time_t timeT = toTimeT(fileTime); > + return WallTime::fromRawSeconds(timeT); Don’t think we need the local variable. > Source/WTF/wtf/FileSystem.cpp:133 > +static std::filesystem::path resolveSymlinks(std::filesystem::path path, std::error_code ec) I think we intend that ec is an in/out argument, but here it’s only being passed in. I would expect tests to fail because of this, since failure would not be propagated to the caller. > Source/WTF/wtf/FileSystem.cpp:136 > + static const unsigned maxSymlinkDepth = MAXSYMLINKS; constexpr > Source/WTF/wtf/FileSystem.cpp:138 > + static const unsigned maxSymlinkDepth = 40; constexpr
Started reviewing yesterday and spotted one significant mistake that makes me worry about the thoroughness of our testing. Don’t have time to finish reviewing right now, but sent you my comment in progress that include that mistake. I applaud the goal of making this more elegant and less platform-specific, but I am worried about making a lot of changes here with room for error, without extensive testing, and without obvious compelling, concrete benefit that would cause us to take the risk. That’s why I am not super-fast to review, since I feel we need to be cautious.
(In reply to Darin Adler from comment #29) > Started reviewing yesterday and spotted one significant mistake that makes > me worry about the thoroughness of our testing. Don’t have time to finish > reviewing right now, but sent you my comment in progress that include that > mistake. > > I applaud the goal of making this more elegant and less platform-specific, > but I am worried about making a lot of changes here with room for error, > without extensive testing, and without obvious compelling, concrete benefit > that would cause us to take the risk. That’s why I am not super-fast to > review, since I feel we need to be cautious. I understand. I would just like to point out that we had almost no testing for our FileSystem API and my patches do bring in API tests. Clearly my tests were not enough since you've spotted mistakes but I have added tests for every mistake you spotted.
(In reply to Darin Adler from comment #28) > Comment on attachment 427781 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427781&action=review > > > Source/WTF/wtf/FileSystem.cpp:130 > > + std::time_t timeT = toTimeT(fileTime); > > + return WallTime::fromRawSeconds(timeT); > > Don’t think we need the local variable. Will fix. > > > Source/WTF/wtf/FileSystem.cpp:133 > > +static std::filesystem::path resolveSymlinks(std::filesystem::path path, std::error_code ec) > > I think we intend that ec is an in/out argument, but here it’s only being > passed in. I would expect tests to fail because of this, since failure would > not be propagated to the caller. The function also returns a default-constructed path when failing so the caller (even though it didn't have the right error_code) would still fail later on. > > > Source/WTF/wtf/FileSystem.cpp:136 > > + static const unsigned maxSymlinkDepth = MAXSYMLINKS; > > constexpr > > > Source/WTF/wtf/FileSystem.cpp:138 > > + static const unsigned maxSymlinkDepth = 40; > > constexpr Will fix.
Yes, really happy about the new tests you are adding! If someone else doesn’t review, I will eventually get to it.
Created attachment 427916 [details] Patch
Comment on attachment 427916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427916&action=review > LayoutTests/ChangeLog:8 > + Stop testing the size of a directory in the data transfer API as the result is implementation-specific. I understand that this test wasn’t a good way to test the value of size. But is it right to just stop testing it at all? How can we best make sure it keeps working properly?
(In reply to Darin Adler from comment #34) > Comment on attachment 427916 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427916&action=review > > > LayoutTests/ChangeLog:8 > > + Stop testing the size of a directory in the data transfer API as the result is implementation-specific. > > I understand that this test wasn’t a good way to test the value of size. But > is it right to just stop testing it at all? How can we best make sure it > keeps working properly? Maybe I can just add 0 to the list of allowed values then.
Created attachment 427958 [details] Patch
Found 1 new test failure: http/tests/media/clearkey/collect-webkit-media-session.html
Committed r277158 (237444@main): <https://commits.webkit.org/237444@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427958 [details].
<rdar://problem/77640765>