There's lots of code that wants to load a file into a buffer.
Created attachment 446610 [details] WIP Patch
Created attachment 446636 [details] Patch
Created attachment 446638 [details] Patch
Created attachment 446639 [details] Patch
Comment on attachment 446639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446639&action=review > Source/WTF/wtf/FileSystem.cpp:543 > + auto handle = FileSystem::openFile(path, FileSystem::FileOpenMode::Read); if (FileSystem::isHandleValid(handle)) return std::nullopt;
Comment on attachment 446639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446639&action=review >> Source/WTF/wtf/FileSystem.cpp:543 >> + auto handle = FileSystem::openFile(path, FileSystem::FileOpenMode::Read); > > if (FileSystem::isHandleValid(handle)) return std::nullopt; The first line of `readEntireFile` is that check so I didn't want to do that check twice.
ok
Created attachment 446660 [details] Patch Adding another usage
Comment on attachment 446660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446660&action=review > Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocket.cpp:268 > + return contents.has_value() ? String::adopt(WTFMove(contents.value())) : emptyString(); You may remove has_value(), std::optional supports operator bool; and you already use "content ? ... : ... " below. > Source/WTF/wtf/FileSystem.cpp:538 > +std::optional<Vector<uint8_t>> readEntireFileAt(const String& path) We don't use "At" for functions that takes path string parameter in FileSystem.h/.cpp, so it seems better to just use "readEntireFile". > Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:118 > + if (!buffer.has_value()) { has_value() not needed. > Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:158 > + if (!buffer.has_value()) { Ditto. > Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:-227 > -bool CurlCacheEntry::loadFileToBuffer(const String& filepath, Vector<uint8_t>& buffer) Do you need to remove its definition in header file? > Source/WebCore/platform/network/curl/CurlCacheManager.cpp:106 > + if (!buffer.has_value()) { has_value() not needed. > Tools/TestWebKitAPI/Tests/WTF/FileSystem.cpp:830 > + EXPECT_FALSE(FileSystem::readEntireFile(FileSystem::invalidPlatformFileHandle).has_value()); > + EXPECT_FALSE(FileSystem::readEntireFileAt(emptyString()).has_value()); > + EXPECT_FALSE(FileSystem::readEntireFileAt(FileSystem::pathByAppendingComponent(tempEmptyFolderPath(), "does-not-exist")).has_value()); > + EXPECT_FALSE(FileSystem::readEntireFileAt(tempEmptyFilePath()).has_value()); > + > + auto buffer = FileSystem::readEntireFileAt(tempFilePath()); > + EXPECT_TRUE(buffer.has_value()); Ditto
Comment on attachment 446660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446660&action=review > Source/WTF/wtf/FileSystem.cpp:540 > + if (path.isEmpty()) Also, I'm not sure if this does anything. Won't openFile return an invalid file handle if given an empty path?
Created attachment 446766 [details] Patch
Created attachment 446768 [details] Patch Addressing both reviewer feedback
(In reply to Alex Christensen from comment #10) > Comment on attachment 446660 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=446660&action=review > > > Source/WTF/wtf/FileSystem.cpp:540 > > + if (path.isEmpty()) > > Also, I'm not sure if this does anything. Won't openFile return an invalid > file handle if given an empty path? The tests say yes so removed.
Comment on attachment 446768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446768&action=review > Source/WTF/wtf/FileSystem.cpp:541 > + auto contents = readEntireFile(handle); readEntireFile calls readFromFile once. readFromFile can return a positive number of bytes indicating that the file was partially read successfully and you need to call it again to read the rest. I think it would be worth fixing that while making more functionality rely on it just working. Otherwise we will increase the number of times a file read is reported to fail, especially with larger files. It would probably also be worth returning failure if the large allocation fails instead of crashing.
Created attachment 446807 [details] Patch Read in a loop until all the contents are read.
Created attachment 446808 [details] Patch
Comment on attachment 446808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446808&action=review > Source/WTF/wtf/FileSystem.cpp:531 > + unsigned totalBytesRead = 0; nit: this might limit the capability to 4GB. size_t would increase that to the size of the virtual address space. > Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:123 > + if (auto bufferSize = buffer.value().size()) nit: buffer->size() would also work > Source/WebCore/platform/network/curl/CurlCacheEntry.cpp:124 > + job->getInternal()->client()->didReceiveBuffer(job, SharedBuffer::create(WTFMove(buffer.value())), bufferSize); nit: WTFMove(*buffer) would also work. I don't actually care about this though. > Source/WebCore/rendering/RenderThemeWin.cpp:1028 > - return String(); > + return emptyString(); Why was this change made? I guess it makes it more consistent with the !USE(CF) path, but why not change that to be a null string?
Created attachment 446825 [details] Patch
Committed r286883 (245113@main): <https://commits.webkit.org/245113@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 446825 [details].
<rdar://problem/86345889>