[webkit-reviews] review granted: [Bug 114233] Don't create another plugin process for restarted plugins : [Attachment 196984] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Dean Jackson <dino at apple.com>'s
request for review:
Bug 114233: Don't create another plugin process for restarted plugins
https://bugs.webkit.org/show_bug.cgi?id=114233

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

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=196984&action=review


r=me

This is a clear improvement.

But I wonder why we have two processes at all: Why can't snapshotting take
place in the singleton plug-in process?

> Source/WebKit2/WebProcess/Plugins/PluginProxy.h:62
> -    static PassRefPtr<PluginProxy> create(const String& pluginPath,
PluginProcess::Type);
> +    static PassRefPtr<PluginProxy> create(const String& pluginPath,
PluginProcess::Type, bool);
>      ~PluginProxy();
>  
>      uint64_t pluginInstanceID() const { return m_pluginInstanceID; }

This function should name its bool parameter since the meaning is not obvious.

> Source/WebKit2/WebProcess/Plugins/PluginProxy.h:71
> +    explicit PluginProxy(const String& pluginPath, PluginProcess::Type,
bool);

Ditto.

> Source/WebKit2/WebProcess/Plugins/PluginProxy.h:221
> +    bool m_restartedProcess;

I prefer the "is" prefix for bools, so they read more clearly. "Restarted
process" might sound like "I restarted a process", which is not what we mean.
Also, it's confusing to name this bool about a process, since we have a
separate variable for process type. How about: "bool m_isRestartedPlugin"?