Bug 233598 – SharedBuffer should use WTF::FileSystem when creating from a file

• Component: Platform
• Status: RESOLVED FIXED

[Don Olmstead]

All the implementations are roughly the same and could just be abstracted using that instead.

[Don Olmstead]

Created attachment 445350 [details]
Patch

[Alex Christensen]

Comment on attachment 445350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445350&action=review

> Source/WebCore/platform/SharedBuffer.cpp:174
> +#if !PLATFORM(COCOA)

Why not cocoa?

> Source/WebCore/platform/SharedBuffer.cpp:196
> +    while ((bytesRead = FileSystem::readFromFile(fileHandle, buffer.data() + totalBytesRead, bytesToRead.value() - totalBytesRead)) > 0)

This is most like the existing posix implementation.  The glib and Windows (and Cocoa) implementations use system functions that do the reading for you.

[Don Olmstead]

Comment on attachment 445350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445350&action=review

>> Source/WebCore/platform/SharedBuffer.cpp:174
>> +#if !PLATFORM(COCOA)
> 
> Why not cocoa?

The Cocoa code just had an ObjC function to read an entire file into NSData so its code was more straightforward.

I could remove that if its preferable.

>> Source/WebCore/platform/SharedBuffer.cpp:196
>> +    while ((bytesRead = FileSystem::readFromFile(fileHandle, buffer.data() + totalBytesRead, bytesToRead.value() - totalBytesRead)) > 0)
> 
> This is most like the existing posix implementation.  The glib and Windows (and Cocoa) implementations use system functions that do the reading for you.

The glib variant does call a different function, g_file_get_contents instead of g_input_stream_read, but the Windows still calls ReadFile under the covers and POSIX still calls read.

[Alex Christensen]

Comment on attachment 445350 [details]
Patch

Before this change, Cocoa Windows and Glib all do one function call to read something with known size and Posix calls read inside a loop.  After this change, everything except Cocoa calls read inside a loop.  I don't think that's an improvement.

[Radar WebKit Bug Importer]

<rdar://problem/86123732>

[Don Olmstead]

Created attachment 446911 [details]
Keep Cocoa and Glib specific function

[Don Olmstead]

Created attachment 446912 [details]
No platform specific implementation

[Don Olmstead]

Please let me know which option you prefer Alex and I'll turn that into the proper patch for it now that FileSystem::readEntireFile is implemented.

[Chris Dumez]

Comment on attachment 446912 [details]
No platform specific implementation

I personally like this approach better. We should try and avoid platform-specific code unless there is a clear reason otherwise. I looked at the Cocoa implementation and it is not clear to me that it is any better. I think we should give this a try.

Note that there is a tiny risk that the Cocoa implementation is more performant for some reason (hard to say). If we notice it does regress something though, we can always add it back (like we did for some lower-level FileSystem functions).

[Don Olmstead]

Created attachment 446915 [details]
Patch

[Chris Dumez]

Comment on attachment 446915 [details]
Patch

r=me assuming the bots are happy

[EWS]

Committed r286922 (245148@main): <https://commits.webkit.org/245148@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446915 [details].