[webkit-reviews] review granted: [Bug 184991] We should cache the compiled sandbox profile in a data vault : [Attachment 345392] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Ben Richards
<benton_richards at apple.com>'s request for review:
Bug 184991: We should cache the compiled sandbox profile in a data vault
https://bugs.webkit.org/show_bug.cgi?id=184991

Attachment 345392: Patch

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




--- Comment #128 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 345392
  --> https://bugs.webkit.org/attachment.cgi?id=345392
Patch

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

> Source/WebCore/platform/FileSystem.h:104
> +WEBCORE_EXPORT bool deleteNonEmptyDirectory(const String&);

We should probably place this under #if PLATFORM(COCOA) since there's only
implementation under COCOA.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:256
> +    auto writeUserCacheDirectoryIntoBuffer = [&] () {

Why does this need to be a lambda? It seems cleaner to have a static function
which takes Vector<char>&.
If we did that, we should probably rename this to getUserCacheDirectory (get
prefix since it has an out parameter).

> Source/WebKit/Shared/mac/ChildProcessMac.mm:550
> +    CString header;

I don't think we need a separate local variable from the one returned by
setAndSerializeSandboxParameters.
Just call setAndSerializeSandboxParameters outside the if like:
auto header = setAndSerializeSandboxParameters(~);
if (!header) {
    ...
    CRASH();
}
filePath = sandboxFilePath(directoryPath, *header);

> Source/WebKit/Shared/mac/ChildProcessMac.mm:572
> +    if (info.profilePath.isEmpty()) {

Why don't we check this condition before constructing SandboxInfo.
In fact, can't we check this condition before calling
setAndSerializeSandboxParameters, no?