Bug 234103 – Add FileSystem function to read a file at a path

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Don Olmstead]

There's lots of code that wants to load a file into a buffer.

[Don Olmstead]

Created attachment 446610 [details]
WIP Patch

[Don Olmstead]

Created attachment 446636 [details]
Patch

[Don Olmstead]

Created attachment 446638 [details]
Patch

[Don Olmstead]

Created attachment 446639 [details]
Patch

[Alex Christensen]

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;

[Don Olmstead]

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.

[Alex Christensen]

ok

[Don Olmstead]

Created attachment 446660 [details]
Patch

Adding another usage

[Sihui Liu]

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

[Alex Christensen]

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?

[Don Olmstead]

Created attachment 446766 [details]
Patch

[Don Olmstead]

Created attachment 446768 [details]
Patch

Addressing both reviewer feedback

[Don Olmstead]

(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.

[Alex Christensen]

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.

[Don Olmstead]

Created attachment 446807 [details]
Patch

Read in a loop until all the contents are read.

[Don Olmstead]

Created attachment 446808 [details]
Patch

[Alex Christensen]

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?

[Don Olmstead]

Created attachment 446825 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/86345889>