[webkit-reviews] review granted: [Bug 199759] Bytecode cache should use FileSystem : [Attachment 374164] Updated Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Christopher Reid
<chris.reid at sony.com>'s request for review:
Bug 199759: Bytecode cache should use FileSystem
https://bugs.webkit.org/show_bug.cgi?id=199759

Attachment 374164: Updated Patch

https://bugs.webkit.org/attachment.cgi?id=374164&action=review




--- Comment #12 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 374164
  --> https://bugs.webkit.org/attachment.cgi?id=374164
Updated Patch

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

r=me with nits.

> Source/JavaScriptCore/API/JSScript.mm:164
> +    FileSystem::MappedFileData mappedFile(fd,
FileSystem::MappedFileMode::Private, success);

We should change this interface to something like this instead of passing
boolean `success`.

Optional<MappedFileData> mappedFile = FileSystem::MappedFileData::open(...);

But this is not directly related to this patch. So I'm OK with this. Just
commented for further refactoring.

> Source/JavaScriptCore/runtime/CachePayload.cpp:82
>  #if !OS(WINDOWS)
>	   munmap(m_data, m_size);
>  #else
> -	   RELEASE_ASSERT_NOT_REACHED();
> +	   UnmapViewOfFile(m_data);
>  #endif
>      } else

Can we extract this part in WTF::FileSystem as
`FileSystem::unmapViewOfFile(buffer, size)` (and using it in
MappedFileData::~MappedFileData)?

> Source/WTF/wtf/FileSystem.cpp:299
> +bool MappedFileData::mapFileHandle(PlatformFileHandle handle, MappedFileMode
mode)

Nice cleanup.