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

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson 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 176348: Patch
https://bugs.webkit.org/attachment.cgi?id=176348&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=176348&action=review


> Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:62
> +    PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin,
false);
> +    pluginProcess->getPluginProcessConnection(reply);
> +}
> +
> +void PluginProcessManager::getSnapshotPluginProcessConnection(const
PluginInfoStore& pluginInfoStore, const String& pluginPath,
PassRefPtr<Messages::WebProcessProxy::GetPluginProcessConnection::DelayedReply>
reply)
> +{
> +    ASSERT(!pluginPath.isNull());
> +    
> +    PluginModuleInfo plugin =
pluginInfoStore.infoForPluginWithPath(pluginPath);
> +    PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin,
true);

As Jon previously requested, could you please used named enums here instead of
bool flags?

I had to lookup what this bool meant to see what the code was doing.  If you'd
named enums I wouldn't have had to look it up... and that's one reason why we
used them!  :)

> Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:76
> +    PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin,
false);

Same here.

> Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:82
> +    PluginProcessProxy* pluginProcess = getOrCreatePluginProcess(plugin,
false);

Same here.

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

Same here.

> Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:97
> +PluginProcessProxy* PluginProcessManager::getOrCreatePluginProcess(const
PluginModuleInfo& plugin, bool isSnapshotProcess)

Same here.

> Source/WebKit2/UIProcess/Plugins/PluginProcessManager.cpp:102
> +    RefPtr<PluginProcessProxy> pluginProcess =
PluginProcessProxy::create(this, plugin, isSnapshotProcess);

Same here.

> Source/WebKit2/UIProcess/Plugins/PluginProcessManager.h:68
> +    PluginProcessProxy* getOrCreatePluginProcess(const PluginModuleInfo&,
bool isSnapshotProcess);
> +    PluginProcessProxy* pluginProcessWithPath(const String& pluginPath, bool
isSnapshotProcess);

Same here.

> Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:55
> +    return adoptRef(new PluginProcessProxy(PluginProcessManager, pluginInfo,
isSnapshotProcess));

Same here.

> Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:58
> +PluginProcessProxy::PluginProcessProxy(PluginProcessManager*
PluginProcessManager, const PluginModuleInfo& pluginInfo, bool
isSnapshotProcess)

Same here.

> Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:67
> +    , m_isSnapshotProcess(isSnapshotProcess)

The member variable itself could be a bool, except then...

> Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:177
> +	  
contexts[i]->sendToAllProcesses(Messages::WebProcess::PluginProcessCrashed(m_pl
uginInfo.path, m_isSnapshotProcess));

You'd have to convert it back to an enum here.

> Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:208
> +    parameters.shouldMuteAudio = m_isSnapshotProcess;

The parameter member could be an enum also, and we have encodeEnum and
decodeEnum in CoreIPC for that reason.

> Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.h:65
> +    static PassRefPtr<PluginProcessProxy> create(PluginProcessManager*,
const PluginModuleInfo&, bool isSnapshotProcess);

At this point I'll stop commenting on how it should be an enum.