Summary: | Add FileSystem function to read a file at a path | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Don Olmstead <don.olmstead> | ||||||||||||||||||||||
Component: | Web Template Framework | Assignee: | Don Olmstead <don.olmstead> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, benjamin, cdumez, changseok, cmarcelo, esprehn+autocc, ews-watchlist, galpeter, glenn, hi, joepeck, keith_miller, kondapallykalyan, mark.lam, msaboff, pangle, pdr, saam, sihui_liu, tzagallo, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
Don Olmstead
2021-12-09 14:41:27 PST
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]. |