Port WTF::FileSystem::listDirectory to std::filesystem.
Created attachment 428238 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 428241 [details] Patch
Comment on attachment 428241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428241&action=review > Source/WTF/ChangeLog:16 > + Using a regular expression has the benefit of having an cross-platform implementation available > + in the STL. We also have a regular expression library in JavaScriptCore. Do we really want to start using std::regex, even for this? > Source/WTF/wtf/FileSystem.h:141 > +WTF_EXPORT_PRIVATE Vector<String> listDirectoryUsingRegex(const String& path, const String& regex); I’m not convinced we need a version of this that takes const String&. I would think we could just take const char* for the regular expression. Regular expression seems overkill for this. Do we really need this function of filtering by regular expression built into the function? Can’t callers just skip things they want to ignore? Most clients are just checking for a prefix or suffix, although a few are doing something fancier. > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:94 > - return makeString(constructedPathPrefix(legacyFilename), '*'); > + return makeString(constructedPathPrefix(legacyFilename), ".*"); Concatenating two constant strings can be done at compile time, and getting the length of a constant string can also be done at compile time. There is no need to involve WTF::String in this code. It’s not safe to just take a string from a function result and use it as a regular expression. This is safe only because constructedPathPrefix doesn’t return an arbitrary string, but rather just one of two fixed string constants that are guaranteed not to have any special characters. > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:505 > + Vector<WTF::String> fullPaths = listDirectoryUsingRegex(storePath, constructedPathFilter(false)); > + Vector<WTF::String> legacyFullPaths = listDirectoryUsingRegex(storePath, constructedPathFilter(true)); auto > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:507 > identifiers.reserveInitialCapacity(fullPaths.size() + legacyFullPaths.size()); This path code below is using reverseFind('/') but std::filesystem has functions for doing this kind of operation without that.
(In reply to Darin Adler from comment #4) > Comment on attachment 428241 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428241&action=review > > > Source/WTF/ChangeLog:16 > > + Using a regular expression has the benefit of having an cross-platform implementation available > > + in the STL. > > We also have a regular expression library in JavaScriptCore. Do we really > want to start using std::regex, even for this? I was looking for the closest thing to what we supported that was available in WTF or STL. I do not think I can use JSC’s regex from WTF. I agree that regex is a little overkill here but it is pretty close to what we used to support. I do find it convenient to be able to filter results quickly and call sites were leveraging this. > > Source/WTF/wtf/FileSystem.h:141 > > +WTF_EXPORT_PRIVATE Vector<String> listDirectoryUsingRegex(const String& path, const String& regex); > > I’m not convinced we need a version of this that takes const String&. I > would think we could just take const char* for the regular expression. Makes sense, will update. I wanted to use ASCIILiteral but several call sites prevented me from doing so. > Regular expression seems overkill for this. Do we really need this function > of filtering by regular expression built into the function? Can’t callers > just skip things they want to ignore? Most clients are just checking for a > prefix or suffix, although a few are doing something fancier. > > > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:94 > > - return makeString(constructedPathPrefix(legacyFilename), '*'); > > + return makeString(constructedPathPrefix(legacyFilename), ".*"); > > Concatenating two constant strings can be done at compile time, and getting > the length of a constant string can also be done at compile time. There is > no need to involve WTF::String in this code. I am not sure how to do string concatenation at compile time. I will try and find out but if you have time, could you please point me in the right direction? > > It’s not safe to just take a string from a function result and use it as a > regular expression. This is safe only because constructedPathPrefix doesn’t > return an arbitrary string, but rather just one of two fixed string > constants that are guaranteed not to have any special characters. Yes, I have verified that the only cases where we are passing actual Strings as regex, they are predefined and safe to use as regex. > > > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:505 > > + Vector<WTF::String> fullPaths = listDirectoryUsingRegex(storePath, constructedPathFilter(false)); > > + Vector<WTF::String> legacyFullPaths = listDirectoryUsingRegex(storePath, constructedPathFilter(true)); > > auto Will update. > > > Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:507 > > identifiers.reserveInitialCapacity(fullPaths.size() + legacyFullPaths.size()); > > This path code below is using reverseFind('/') but std::filesystem has > functions for doing this kind of operation without that. Will update.
Comment on attachment 428241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428241&action=review >>> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:94 >>> + return makeString(constructedPathPrefix(legacyFilename), ".*"); >> >> Concatenating two constant strings can be done at compile time, and getting the length of a constant string can also be done at compile time. There is no need to involve WTF::String in this code. >> >> It’s not safe to just take a string from a function result and use it as a regular expression. This is safe only because constructedPathPrefix doesn’t return an arbitrary string, but rather just one of two fixed string constants that are guaranteed not to have any special characters. > > I am not sure how to do string concatenation at compile time. I will try and find out but if you have time, could you please point me in the right direction? Not sure how to do it without macros, but with macros it can be like this, using string token pasting: #define PATH_PREFIX "ContentRuleList-" #define LEGACY_PATH_PREFIX "ContentRuleList-" #define MAKE_FILTER_REGEX(prefix) prefix ".*" ... code ... auto fullPaths = listDirectoryUsingRegex(storePath, MAKE_FILTER_REGEX(PATH_PREFIX)); .. more code ... #undef PATH_PREFIX #undef LEGACY_PATH_PREFIX #undef MAKE_FILTER But also, we don’t have to convert something into a regular expression to do a prefix check. We can just call listDirectory and then filter the strings using compactMap and prefix checking. I feel like this code gets more complicated by trying to make a regular expression, despite the fact that it might seem handy.
(In reply to Chris Dumez from comment #5) > I do find it convenient to be able to filter results quickly and > call sites were leveraging this. I think we can add filtering that’s not based on regex unless we really need the power of regular expressions. Maybe the only one is the one that uses a "template" and that’s not in platform-independent code. and we can just make functions that filter a Vector, no need to build the filtering into the listDirectory function.
(In reply to Darin Adler from comment #6) > Comment on attachment 428241 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428241&action=review > > >>> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:94 > >>> + return makeString(constructedPathPrefix(legacyFilename), ".*"); > >> > >> Concatenating two constant strings can be done at compile time, and getting the length of a constant string can also be done at compile time. There is no need to involve WTF::String in this code. > >> > >> It’s not safe to just take a string from a function result and use it as a regular expression. This is safe only because constructedPathPrefix doesn’t return an arbitrary string, but rather just one of two fixed string constants that are guaranteed not to have any special characters. > > > > I am not sure how to do string concatenation at compile time. I will try and find out but if you have time, could you please point me in the right direction? > > Not sure how to do it without macros, but with macros it can be like this, > using string token pasting: > > #define PATH_PREFIX "ContentRuleList-" > #define LEGACY_PATH_PREFIX "ContentRuleList-" > #define MAKE_FILTER_REGEX(prefix) prefix ".*" > > ... code ... > > auto fullPaths = listDirectoryUsingRegex(storePath, > MAKE_FILTER_REGEX(PATH_PREFIX)); > > .. more code ... > > #undef PATH_PREFIX > #undef LEGACY_PATH_PREFIX > #undef MAKE_FILTER > > But also, we don’t have to convert something into a regular expression to do > a prefix check. We can just call listDirectory and then filter the strings > using compactMap and prefix checking. I feel like this code gets more > complicated by trying to make a regular expression, despite the fact that it > might seem handy. Yes, I am familiar with concatenation using macros but it is a bit ugly I was hoping we had something better.
(In reply to Darin Adler from comment #7) > (In reply to Chris Dumez from comment #5) > > I do find it convenient to be able to filter results quickly and > > call sites were leveraging this. > > I think we can add filtering that’s not based on regex unless we really need > the power of regular expressions. Maybe the only one is the one that uses a > "template" and that’s not in platform-independent code. and we can just make > functions that filter a Vector, no need to build the filtering into the > listDirectory function. There are a few call sites that currently rely on regex-like things, not simply prefixes or suffixes. If listDirectory() does not do the filtering then I am moving the complexity from listDirectory to the call sites and my patch does not look as much as a progression anymore. I understand regex is a bit overkill for this but is is not that different from what we used to support and it is not like regex support is adding much code complexity (just 2 lines thanks to STL).
(In reply to Darin Adler from comment #6) > Comment on attachment 428241 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428241&action=review > > >>> Source/WebKit/UIProcess/API/APIContentRuleListStore.cpp:94 > >>> + return makeString(constructedPathPrefix(legacyFilename), ".*"); > >> > >> Concatenating two constant strings can be done at compile time, and getting the length of a constant string can also be done at compile time. There is no need to involve WTF::String in this code. > >> > >> It’s not safe to just take a string from a function result and use it as a regular expression. This is safe only because constructedPathPrefix doesn’t return an arbitrary string, but rather just one of two fixed string constants that are guaranteed not to have any special characters. > > > > I am not sure how to do string concatenation at compile time. I will try and find out but if you have time, could you please point me in the right direction? > > Not sure how to do it without macros, but with macros it can be like this, > using string token pasting: > > #define PATH_PREFIX "ContentRuleList-" > #define LEGACY_PATH_PREFIX "ContentRuleList-" > #define MAKE_FILTER_REGEX(prefix) prefix ".*" > > ... code ... > > auto fullPaths = listDirectoryUsingRegex(storePath, > MAKE_FILTER_REGEX(PATH_PREFIX)); > > .. more code ... > > #undef PATH_PREFIX > #undef LEGACY_PATH_PREFIX > #undef MAKE_FILTER > > But also, we don’t have to convert something into a regular expression to do > a prefix check. We can just call listDirectory and then filter the strings > using compactMap and prefix checking. I feel like this code gets more > complicated by trying to make a regular expression, despite the fact that it > might seem handy. It seems though that the complexity is added by trying to pass a const char* instead or a String, not by the fact that the listDirectory() supports regex filtering. It is up to the caller to pass a valid regex and the caller was passing a valid regex as a String.
I don’t think our file system layer should add a regex facility right into the file system calls. You positioned WTF::FileSystem as a thin layer over std::filesystem that adds WTF::String compatibility. I would like to remove this filtering later; we can make it quite convenient without building it into the file system layer. However, I will stop debating it with you right now. I am concerned that std::regex may have undesirable code size or performance characteristics. This is the first time we started using it in production WebKit code.
(In reply to Darin Adler from comment #11) > I don’t think our file system layer should add a regex facility right into > the file system calls. You positioned WTF::FileSystem as a thin layer over > std::filesystem that adds WTF::String compatibility. You and Sam have expressed this wish. For now I have been focusing on getting rid of platform-specific code, adding tests while maintaining functionality so that I don’t have to modify the call sites much and hopefully reduce the risk of regression. > I would like to remove this filtering later; we can make it quite convenient > without building it into the file system layer. However, I will stop > debating it with you right now. The reason it is convenient to do the filtering at the file system layer is because the filtering is based on file names while the function returns a vector of full paths. Call sites would have to split the path again in many cases. > I am concerned that std::regex may have undesirable code size or performance > characteristics. This is the first time we started using it in production > WebKit code. Ok. What would you think about passing a struct to listDirectory() that would allow the caller to specify a prefix/suffix? For example: struct ListDirectoryFilter { Optional<String> prefix; Optional<String> suffix: }; Alternatively listDirectory could take in a filter WTF::Function that would take in a String file name. Both of these mean moving some complexity to the call sites but hopefully to a reasonable amount and it is a decent compromise?
(In reply to Chris Dumez from comment #12) > (In reply to Darin Adler from comment #11) > > I don’t think our file system layer should add a regex facility right into > > the file system calls. You positioned WTF::FileSystem as a thin layer over > > std::filesystem that adds WTF::String compatibility. > > You and Sam have expressed this wish. For now I have been focusing on > getting rid of platform-specific code, adding tests while maintaining > functionality so that I don’t have to modify the call sites much and > hopefully reduce the risk of regression. > > > I would like to remove this filtering later; we can make it quite convenient > > without building it into the file system layer. However, I will stop > > debating it with you right now. > > The reason it is convenient to do the filtering at the file system layer is > because the filtering is based on file names while the function returns a > vector of full paths. Call sites would have to split the path again in many > cases. > > > I am concerned that std::regex may have undesirable code size or performance > > characteristics. This is the first time we started using it in production > > WebKit code. > Ok. > > What would you think about passing a struct to listDirectory() that would > allow the caller to specify a prefix/suffix? For example: > struct ListDirectoryFilter { > Optional<String> prefix; > Optional<String> suffix: > }; > > Alternatively listDirectory could take in a filter WTF::Function that would > take in a String file name. > > Both of these mean moving some complexity to the call sites but hopefully to > a reasonable amount and it is a decent compromise? Or another alternative is to change listDirectory() to return filenames instead of full paths so that the call sites can more easily do the filtering. They can then call pathByAppendingComponent() if they need a full path.
(In reply to Chris Dumez from comment #13) > (In reply to Chris Dumez from comment #12) > > (In reply to Darin Adler from comment #11) > > > I don’t think our file system layer should add a regex facility right into > > > the file system calls. You positioned WTF::FileSystem as a thin layer over > > > std::filesystem that adds WTF::String compatibility. > > > > You and Sam have expressed this wish. For now I have been focusing on > > getting rid of platform-specific code, adding tests while maintaining > > functionality so that I don’t have to modify the call sites much and > > hopefully reduce the risk of regression. > > > > > I would like to remove this filtering later; we can make it quite convenient > > > without building it into the file system layer. However, I will stop > > > debating it with you right now. > > > > The reason it is convenient to do the filtering at the file system layer is > > because the filtering is based on file names while the function returns a > > vector of full paths. Call sites would have to split the path again in many > > cases. > > > > > I am concerned that std::regex may have undesirable code size or performance > > > characteristics. This is the first time we started using it in production > > > WebKit code. > > Ok. > > > > What would you think about passing a struct to listDirectory() that would > > allow the caller to specify a prefix/suffix? For example: > > struct ListDirectoryFilter { > > Optional<String> prefix; > > Optional<String> suffix: > > }; > > > > Alternatively listDirectory could take in a filter WTF::Function that would > > take in a String file name. > > > > Both of these mean moving some complexity to the call sites but hopefully to > > a reasonable amount and it is a decent compromise? > > Or another alternative is to change listDirectory() to return filenames > instead of full paths so that the call sites can more easily do the > filtering. They can then call pathByAppendingComponent() if they need a full > path. I am experimenting locally with having listDirectory() return file names instead of full paths and having call sites doing the filtering. I think it'll end up looking OK.
Created attachment 428282 [details] Patch
Created attachment 428285 [details] Patch
Created attachment 428288 [details] Patch
Comment on attachment 428285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428285&action=review I like this. Thanks for taking my suggestion to make this less sophisticated. Maybe some day we can change these callers to use iteration instead of allocating a vector. Some of the prefix/suffix checks might be better if they were ASCII case-insensitive. > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:-1185 > - Vector<String> directoriesWithSameHash = FileSystem::listDirectory(newOriginDirectory, fileNameHash + "*"); Amazing, unused directory list! > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1290 > + auto fileSize = SQLiteFileSystem::getDatabaseFileSize(FileSystem::pathByAppendingComponent(dbDirectoryPath, fileName)); > diskUsage += fileSize; Probably don’t need the local variable. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2214 > + if (!fileName.startsWith(templateDirectory)) Oops! Should be "startsWith(templatePrefix)". > Source/WebCore/platform/text/hyphen/HyphenationLibHyphen.cpp:61 > static const int prefixLength = 5; > static const int suffixLength = 4; > - return fileName.substring(prefixLength, fileName.length() - prefixLength - suffixLength); > + return fileName.substring(prefixLength, fileName.length() - prefixLength - suffixLength).convertToASCIILowercase(); Not great to have the "startsWith/endsWith" checks at the caller, and the substring here, since the two have to match and one creates the precondition that makes the other work. I suggest moving the startsWith/endsWith into this function, and checking for null string (or maybe you prefer empty string) in the scanDirectoryForDictionaries function. Could also change to constexpr since we are tweaking this file. Seems like an example of how pulling the filtering out of the listDirectory function helps, since it allows us to make the code more obviously correct by putting the precondition check next to the code that relies on it. > Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp:107 > + for (const auto& path : FileSystem::listDirectory(localStorageDirectory())) { The const here doesn’t do anything, and I’d leave it out. > Source/WebKitLegacy/win/Plugins/PluginDatabase.cpp:403 > + Vector<String> pluginNames = FileSystem::listDirectory(pluginDirectory); auto Could even put this inside the for loop. I would have written: for (auto& name : FileSystem::listDirectory(pluginDirectory)) {
Comment on attachment 428288 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428288&action=review > Source/WTF/wtf/FileSystem.h:140 > +WTF_EXPORT_PRIVATE Vector<String> listDirectory(const String& path); // Returns file names, not full paths. Interesting that this has always returned an empty vector if it fails. I wonder if that’s safe at all call sites?
Created attachment 428291 [details] Patch
(In reply to Darin Adler from comment #18) > Comment on attachment 428285 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428285&action=review > > I like this. Thanks for taking my suggestion to make this less sophisticated. > > Maybe some day we can change these callers to use iteration instead of > allocating a vector. Since I am already modifying all the call sites, I think I might as well do this change now too. It's not that much work.
Created attachment 428294 [details] Patch
Created attachment 428300 [details] Patch
This latest iteration is no longer allocating a Vector and the callers are instead iterating over the entries. I am asking for review again because of substantial changes.
Comment on attachment 428300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428300&action=review Looks great, but not sure if I should say review+ > Source/WTF/wtf/FileSystem.cpp:762 > + std::error_code ec; > + for (auto& entry : std::filesystem::directory_iterator(toStdFileSystemPath(path), ec)) { > + auto fileName = fromStdFileSystemPath(entry.path().filename()); > + if (!fileName.isNull()) > + apply(fileName); > + } I worry that some clients might be deleting the files as we go, and I am not sure what the behavior of directory_iterator is for such cases. To state the obvious, the old code made a copy of the filenames up front before processing any, so might have worked because of that. > Source/WTF/wtf/FileSystem.h:140 > +WTF_EXPORT_PRIVATE void listDirectory(const String& path, const Function<void(const String& fileName)>& apply); Not sure "listDirectory" is the right name any more. "forEachInDirectory" maybe? Rename can be done later, though. I don’t think the function parameter needs the name "apply" for clarity. > Source/WebCore/platform/text/hyphen/HyphenationLibHyphen.cpp:63 > + static constexpr int prefixLength = 5; > + static constexpr int suffixLength = 4; Don’t need static with constexpr.
(In reply to Darin Adler from comment #25) > Comment on attachment 428300 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428300&action=review > > Looks great, but not sure if I should say review+ > > > Source/WTF/wtf/FileSystem.cpp:762 > > + std::error_code ec; > > + for (auto& entry : std::filesystem::directory_iterator(toStdFileSystemPath(path), ec)) { > > + auto fileName = fromStdFileSystemPath(entry.path().filename()); > > + if (!fileName.isNull()) > > + apply(fileName); > > + } > > I worry that some clients might be deleting the files as we go, and I am not > sure what the behavior of directory_iterator is for such cases. """ If a file or a directory is deleted or added to the directory tree after the directory iterator has been created, it is unspecified whether the change would be observed through the iterator. """ That's not very reassuring.. :/
(In reply to Chris Dumez from comment #26) > (In reply to Darin Adler from comment #25) > > Comment on attachment 428300 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=428300&action=review > > > > Looks great, but not sure if I should say review+ > > > > > Source/WTF/wtf/FileSystem.cpp:762 > > > + std::error_code ec; > > > + for (auto& entry : std::filesystem::directory_iterator(toStdFileSystemPath(path), ec)) { > > > + auto fileName = fromStdFileSystemPath(entry.path().filename()); > > > + if (!fileName.isNull()) > > > + apply(fileName); > > > + } > > > > I worry that some clients might be deleting the files as we go, and I am not > > sure what the behavior of directory_iterator is for such cases. > > """ > If a file or a directory is deleted or added to the directory tree after the > directory iterator has been created, it is unspecified whether the change > would be observed through the iterator. > """ > > That's not very reassuring.. :/ To be safe, I'll go back to using a Vector.
Created attachment 428333 [details] Patch
Created attachment 428336 [details] Patch
Created attachment 428337 [details] Patch
Committed r277357 (237615@main): <https://commits.webkit.org/237615@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428337 [details].
<rdar://problem/77882664>