[webkit-reviews] review denied: [Bug 101536] Instantiate snapshot plugins in a PluginProcess with muted audio : [Attachment 173433] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has denied Kiran Muppala
<cmuppala at apple.com>'s request for review:
Bug 101536: Instantiate snapshot plugins in a PluginProcess with muted audio
https://bugs.webkit.org/show_bug.cgi?id=101536

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

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=173433&action=review


Looks great overall but needs some minor fixes.

> Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:310
> +    AudioObjectPropertyAddress theAddress =
{kAudioHardwarePropertyProcessIsAudible, kAudioObjectPropertyScopeGlobal,
kAudioObjectPropertyElementMaster};

Please add spaces after the { and before the }, and use a more descriptive name
than "theAddress". How about, propertyAddress.

> Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:311
> +    UInt32 theData = 0;

Same thing here. Just "data" or maybe "processIsAudibleProperty".

> Source/WebKit2/PluginProcess/mac/PluginProcessMac.mm:313
> +	   CRASH();

Do we really want to crash in this case?

> Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:86
> +PluginProcessProxy* PluginProcessManager::pluginProcessWithPath(const
String& pluginPath, bool snapshotProcess)

You should call this isSnapshotProcess since otherwise it sounds like you're
snapshotting the process itself. Same with all other occurrences of "bool
snapshotProcess" in the patch.