[webkit-reviews] review granted: [Bug 106513] Enable process suppression by default on Mac : [Attachment 182246] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kiran Muppala <cmuppala at apple.com>'s
request for review:
Bug 106513: Enable process suppression by default on Mac
https://bugs.webkit.org/show_bug.cgi?id=106513

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=182246&action=review


My only serious concern is apparent duplication of the
notifyGlobalChildProcessesOfApplicationOcclusionState handling between
WebContext::setProcessSuppressionEnabled and
WebContext::updateProcessSuppressionEnabledForAllContexts.

This seems deficient for the case where one window in the app is occluded and
the other window is not, and there is a child process only used in the occluded
window.

> Source/WebKit2/UIProcess/WebContext.h:351
> +    static void updateProcessSuppressionEnabledForAnyContext();
> +    static void updateProcessSuppressionEnabledForAllContexts();

I think it would be slightly nicer factoring here to use three functions. One
that would loop through the processes and compute new values for both
processSuppressionEnabledForAnyContext and
processSuppressionEnabledForAllContexts. And two other that the first function
calls that are just setters that would check to see if the values changed and
then do the necessary work triggered by those changes. But this current
structure is probably OK. I just like mine slightly better.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:298
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090

Would be nicer to define something named at the top of the file instead of
checking the OS version directly at each call site. That name could also help
explain why the code is conditional perhaps.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:299
> +    bool occluded = m_processSuppressionEnabled && s_applicationIsOccluded;

Would be nicer to name this isOccluded.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:312
> +    bool occluded = s_processSuppressionEnabledForAllContexts &&
s_applicationIsOccluded;

Would be nicer to name this isOccluded.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:330
> +    if (s_applicationIsOccluded)
> +	   notifyChildProcessesOfApplicationOcclusionState();

This is non-obvious. Two things are not obvious.

1) Why is a change in process suppression the right time to tell child
processes the application is occluded? Especially, why is it equally important
to do this both when enabling process suppression and also when disabling it?
Really mysterious.

2) Why do we only want to tell child processes if the application is occluded?
What about telling them it is not occluded?

I’m sure there is a good explanation, but it’s not obvious and probably
requires a brief comment.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:366
> +	   if (s_applicationIsOccluded)
> +	       notifyGlobalChildProcessesOfApplicationOcclusionState();

Given this code, I don’t understand why
WebContext::setProcessSuppressionEnabled also has to do this. But I have the
same two questions as above:

1) Why is a change in process suppression the right time to tell child
processes the application is occluded? Especially, why is it equally important
to do this both when enabling process suppression and also when disabling it?
Really mysterious.

2) Why do we only want to tell child processes if the application is occluded?
What about telling them it is not occluded?

I’m sure there is a good explanation, but it’s not obvious and probably
requires a brief comment.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:374
> +    static bool omitProcessSuppression = [[NSUserDefaults
standardUserDefaults] boolForKey:@"WebKit2OmitProcessSuppression"];
> +    if (omitProcessSuppression)
>	   return;

It seems a little oblique to turn off process suppression by just not
registering occlusion notification handlers. In fact, it seems a bit random to
have the check here in this function. It could instead be in the
registerOcclusionNotificationHandlers function, for example. A brief comment
could make it clear why this is function is a good choice.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:425
> +	   WTFLogAlways("Unregistration of \"Applicaton Became Occluded\"
notification handler failed.\n");

Typo: "applicaton"