[webkit-reviews] review denied: [Bug 100997] Add application occlusion criterion to enable/disable process suppression on Mac : [Attachment 172188] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Kiran Muppala
<cmuppala at apple.com>'s request for review:
Bug 100997: Add application occlusion criterion to enable/disable process
suppression on Mac
https://bugs.webkit.org/show_bug.cgi?id=100997

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

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


> Source/WebKit2/ChangeLog:15
> +	   * Platform/mac/ProcessUtilities.h: Added.
> +	   (WebKit): Added helper methods to take and release assertion for
process suppression.

A somewhat more common model is to have a shared header, and per-port
implementations. In cases when most functions are different, we sometimes use
separate headers with the same name, but I don't expect this to be the case for
process utilities.

> Source/WebKit2/Platform/mac/ProcessUtilities.mm:34
> +    // Note: AutoTerminationSupport is currently not enabled for
ChildProcesses.

I don't think that "Note" is useful in comment text. All comments are notes.
Also, this should be stronger: "The below call assumes that
AutoTerminationSupport is not enabled for ChildProcesses". Otherwise, this
factual statement sounds like it's suggesting that auto termination should be
enabled.

> Source/WebKit2/UIProcess/API/C/mac/WKContextPrivateMac.h:35
> +WK_EXPORT void WKContextSetProcessSuppressionSupportEnabled(bool);

Shouldn't WKContext functions be per-context?

This really looks like a preference to me, in fact. Can you use WKPreferences
for it?

> Source/WebKit2/UIProcess/Network/NetworkProcessManager.h:61
> +template<typename U> inline void NetworkProcessManager::sendToProcess(const
U& message)

I'm not sure why this is needed. We already have per-process messages such as
InitializeNetworkProcess, how is this different?

> Source/WebKit2/UIProcess/Plugins/PluginProcessManager.h:70
> +template<typename U> inline void
PluginProcessManager::sendToAllProcesses(const U& message)

We should really think about a better name for these. A "sendToAllProcesses"
function is too tempting to use everywhere, while in reality, there are many
potential flavors - drop or queue messages to invalid processes, wake suspended
processes to message them or not...

> Source/WebKit2/UIProcess/SharedWorkers/SharedWorkerProcessProxy.cpp:171
> +#if PLATFORM(MAC)
> +    if (WebContext::applicationIsOccluded())
> +	  
m_connection->send(Messages::SharedWorkerProcess::SetApplicationIsOccluded(true
), 0);
> +#endif

All this copy/pasted code is making me sad.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:63
> +bool WebContext::s_processSuppressionSupportEnabled = false;

This too should be in preferences, not a standalone boolean.