[webkit-reviews] review granted: [Bug 116127] Add a preference that can disable the fake SYSV SHM shim : [Attachment 201764] 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 116127: Add a preference that can disable the fake SYSV SHM shim
https://bugs.webkit.org/show_bug.cgi?id=116127

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

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


Looks good, please fix style nits and upload a new patch.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:161
> +    static int cachedPrefValue = 0;

Please change cachedPrefValue to cachedPreferenceValue.

This is not one of the idioms we use in WebKit. I'm OK with this code as is,
but for reference, we'd do one of these:

1.
static bool preferenceValue = getShimDisabledPreference();
return preferenceValue;

2. dispatch_once(...)

#2 is the preferred option these days.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:171
> +	   if (keyExistsAndHasValidFormat && prefValue) {
> +	       cachedPrefValue = 1;
> +	   } else {
> +	       cachedPrefValue = -1;
> +	   }

Please remove braces around single line blocks.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:181
> +    if (shim_disabled()) {
> +	   return shmdt(sharedAddress);
> +    }

Please remove braces around single line blocks.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:202
> +    if (shim_disabled()) {
> +	   return shmat(sharedMemoryIdentifier, requestedSharedAddress,
shmflg);
> +    }

Ditto.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:231
> +    if (shim_disabled()) {
> +	   return shmget(key, requestedSizeOfSharedMemory, sharedMemoryFlags);
> +    }

Ditto.

> Source/WebKit2/PluginProcess/mac/PluginProcessShim.mm:267
> +    if (shim_disabled()) {
> +	   return shmctl(sharedMemoryIdentifier, cmd, outputDescriptor);
> +    }

Ditto.