[webkit-reviews] review granted: [Bug 118920] 10.7: Java applets do not work due to sandbox violation/exception : [Attachment 207224] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Simon Cooper
<scooper at apple.com>'s request for review:
Bug 118920: 10.7: Java applets do not work due to sandbox violation/exception
https://bugs.webkit.org/show_bug.cgi?id=118920

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

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


>
Source/WebKit2/Resources/PlugInSandboxProfiles/com.apple.WebKit.plugin-common.s
b:325
> +(if (equal? os-version "10.7")
> +    (allow ipc-posix-shm)
> +    (begin
> +	   (if (equal? os-version "10.8")

This nested code is very hard to read. Preprocessor would make it so much
easier, because it has an easy to use wide set of operations, and doesn't force
deep nesting:

#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1080 
...
#else
...
#endif

Also, preprocessor would have had zero cost at runtime.

> Source/WebKit2/Shared/SandboxInitializationParameters.h:49
> +    void addParameter(const char* name, NSString *value);

You probably did this for consistency with existing addPathParameter function,
however it stands out to me as unnecessary to round-trip via an Objective C
value in this case.

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:173
> +    if(osVersionParts.size() < 2) {

There should be a space between "if" and "(".

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:180
> +    sandboxParameters.addParameter("_OS_VERSION", osVersion);

Why does this parameter name start with an underscore? It's inconsistent with
other names we pass, and I don't see how it is different.