[webkit-reviews] review denied: [Bug 134000] Expose the location of website data : [Attachment 233264] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org <mitz at webkit.org> has denied Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 134000: Expose the location of website data
https://bugs.webkit.org/show_bug.cgi?id=134000

Attachment 233264: Patch
https://bugs.webkit.org/attachment.cgi?id=233264&action=review

------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=233264&action=review


> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:50
> +static bool processIsAppSandboxed(pid_t pid)

Given what this does, I think the name is wrong. There is a difference between
being in a container and being app-sandboxed (which implies the former), but
this checks just for the container. So I think the name should be something
like processHasContainer().

> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:57
> +    if (path[0] == '\0')

!path[0]

> Source/WebKit2/Shared/mac/SandboxUtilities.cpp:63
> +bool processIsAppSandboxed()

Ditto.

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessPool.mm:108
> ++ (NSURL *)_websiteDataURLForContainerWithURL:(NSURL *)containerURL

The fact that the containerURL parameter isn’t used is telling. This won’t work
if called from a process that doesn’t have a container and wants to find the
website data location for a process with the given container.