Bug 106513 – Groundwork to enable process suppression by default on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Kiran Muppala]

Process suppression has so far been controlled by a user default and disabled by default.  This needs to be replaced with the policy of "enabled by default" and add a private API to change this value on a per-context basis.

[Kiran Muppala]

Created attachment 182045 [details]
Patch

[Kiran Muppala]

<rdar://problem/12984009>

[Early Warning System Bot]

Comment on attachment 182045 [details]
Patch

Attachment 182045 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15761712

[kov's GTK+ EWS bot]

Comment on attachment 182045 [details]
Patch

Attachment 182045 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/15759786

[Build Bot]

Comment on attachment 182045 [details]
Patch

Attachment 182045 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15762764

[Kiran Muppala]

Created attachment 182051 [details]
Patch

[Kiran Muppala]

(In reply to comment #6)
> Created an attachment (id=182051) [details]
> Patch

Fixed build failure due to Mac specific API being declared in WKContextPrivate.h.

[Build Bot]

Comment on attachment 182051 [details]
Patch

Attachment 182051 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15759802

[Kiran Muppala]

Created attachment 182059 [details]
Patch

[Kiran Muppala]

(In reply to comment #9)
> Created an attachment (id=182059) [details]
> Patch

Fixed build failure due to a method not correctly guarded by __MAC_OS_X_VERSION_MIN_REQUIRED resulting in a unused method error.

[Kiran Muppala]

Created attachment 182152 [details]
Patch

[Kiran Muppala]

(In reply to comment #11)
> Created an attachment (id=182152) [details]
> Patch

Added a internal user default to omit process suppression support globally in (WebContext::processSuppressionSupportChanged).  This is useful for doing comparisons with and without process suppression.  No other changes.

[Kiran Muppala]

Created attachment 182198 [details]
Patch

[Kiran Muppala]

(In reply to comment #13)
> Created an attachment (id=182198) [details]
> Patch

Added missing API getter method to determine the current state of process suppression support of a context.  Update ChangeLog.

[Benjamin Poulain]

Comment on attachment 182198 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182198&action=review

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:373
> +    // Register for occlusion notifications if atleast one context has process suppression support enabled

"atleast".
No period.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:395
> +        }
> +        else {

Coding style.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:5079
>  				E1790891169BAA82006904C7 /* SecItemShimMessages.h in Headers */,
> +				9FB5F395169E6A80002C25BF /* WKContextPrivateMac.h in Headers */,

Keeping the build section sorted helps with the merges.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:6059
>  				E1790890169BAA7F006904C7 /* SecItemShimMessageReceiver.cpp in Sources */,
> +				9FB5F394169E6A80002C25BF /* WKContextPrivateMac.cpp in Sources */,

Ditto.

[Alexey Proskuryakov]

Comment on attachment 182198 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182198&action=review

I'm not sure if it's beneficial for NetworkProcess to ever be suppressed.

It might be helpful to explain the initial state in ChildProcess::platformInitialize(). Perhaps something like below, or maybe more in-depth:

    // Starting with suppression disabled. UI process will set an actual value from didFinishLaunching().

r- for readability refactoring, as discussed in person.

> Source/WebKit2/UIProcess/API/C/mac/WKContextPrivateMac.h:27
> +#ifndef WKContextPrivateMac_h
> +#define WKContextPrivateMac_h

I'm a little uneasy about adding an API header with "Mac" in it, but given the precedent of WKPagePrivateMac.h, it's probably OK.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:368
> +void WebContext::processSuppressionSupportChanged()

I have a very difficult time understanding what this function does, and when it should be called. Kiran and myself spent some time in person discussing this, and it seems like this would really benefit from a re-design. There are multiple things done here:

1. Iterate over contexts, check if any or all of them disable the feature

2. Notify global processes (plugin, shared worker)

3. Register or unregister for notifications

Can you split this into functions that do each of these things, and have descriptive names?

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:374
> +    bool newOcclusionNotificationsEnabled = false;

This should be called either newProcessSuppressionSupportEnabledForAnyContext, or shouldEnableOccusionNotifications.

Perhaps you can split the iteration into separate functions. This is not hot code, so it's OK to iterate contexts twice, or even more.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:395
> +        }
> +        else {

Style nit: there should be no newline here.

[Kiran Muppala]

Created attachment 182246 [details]
Patch

[Kiran Muppala]

(In reply to comment #15)
> (From update of attachment 182198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182198&action=review
> 
> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:373
> > +    // Register for occlusion notifications if atleast one context has process suppression support enabled
> 
> "atleast".
> No period.
> 
> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:395
> > +        }
> > +        else {
> 
> Coding style.
> 

I fixed both of these prior to start of refactoring, although they don't show up in the final product.  Thanks for pointing these out.

> > Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:5079
> >  				E1790891169BAA82006904C7 /* SecItemShimMessages.h in Headers */,
> > +				9FB5F395169E6A80002C25BF /* WKContextPrivateMac.h in Headers */,
> 
> Keeping the build section sorted helps with the merges.
> 
> > Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:6059
> >  				E1790890169BAA7F006904C7 /* SecItemShimMessageReceiver.cpp in Sources */,
> > +				9FB5F394169E6A80002C25BF /* WKContextPrivateMac.cpp in Sources */,
> 
> Ditto.

Fixed.

[Kiran Muppala]

(In reply to comment #16)
> (From update of attachment 182198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182198&action=review
> 
> I'm not sure if it's beneficial for NetworkProcess to ever be suppressed.
> 

Even if the benefits are not clear, I am under the impression based on what Maciej and others have told me is to enable it for as broad a scope as possible.  If we run into issues because of this, we can revisit the decision.

> It might be helpful to explain the initial state in ChildProcess::platformInitialize(). Perhaps something like below, or maybe more in-depth:
> 
>     // Starting with suppression disabled. UI process will set an actual value from didFinishLaunching().
> 

Added a comment.

> r- for readability refactoring, as discussed in person.
> 
> > Source/WebKit2/UIProcess/API/C/mac/WKContextPrivateMac.h:27
> > +#ifndef WKContextPrivateMac_h
> > +#define WKContextPrivateMac_h
> 
> I'm a little uneasy about adding an API header with "Mac" in it, but given the precedent of WKPagePrivateMac.h, it's probably OK.
> 
> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:368
> > +void WebContext::processSuppressionSupportChanged()
> 
> I have a very difficult time understanding what this function does, and when it should be called. Kiran and myself spent some time in person discussing this, and it seems like this would really benefit from a re-design. There are multiple things done here:
> 
> 1. Iterate over contexts, check if any or all of them disable the feature
> 
> 2. Notify global processes (plugin, shared worker)
> 
> 3. Register or unregister for notifications
> 
> Can you split this into functions that do each of these things, and have descriptive names?
> 
> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:374
> > +    bool newOcclusionNotificationsEnabled = false;
> 
> This should be called either newProcessSuppressionSupportEnabledForAnyContext, or shouldEnableOccusionNotifications.
> 
> Perhaps you can split the iteration into separate functions. This is not hot code, so it's OK to iterate contexts twice, or even more.
> 

I created two new functions updateProcessSuppressionEnabledForAnyContext() and updateProcessSuppressionEnabledForAllContexts() that do the iteration separately as you suggested.  I find the new code much easier to read and follow.  Thanks for giving me very helpful pointers for refactoring.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:395
> > +        }
> > +        else {
> 
> Style nit: there should be no newline here.

I fixed this prior to refactoring, although it is not to be found in the final result.

[Darin Adler]

Comment on attachment 182246 [details]
Patch

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"

[Kiran Muppala]

Created attachment 182447 [details]
Patch

[Kiran Muppala]

(In reply to comment #20)
> (From update of attachment 182246 [details])
> 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.
> 

The duplication is a misunderstanding caused by the similarly named notify methods.  The first method only notifies child processes belonging to the context, namely web and network processes, where as the latter notifies them for non context specific (which I have called global) processes, namely plugin and shared worker processes.

> 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.
>

Yes, right now only occluding all windows triggers suppression.  But, a patch in the near future will add support for a single window occlusion as well.

> > 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.
> 

Thank you for the suggestion, I created a single method called updateProcessSuppressionEnabledForAnyAndAllContexts and added setters for each of the two variables.  I find the setters approach better too and eliminated the need for updateOcclusionNotificationRegistration.

> > 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.
> 

I was looking for fixing this but on closer inspection, the version check is only required in register and unregister occlusion notifications, where a comment is probably unnecessary since there are WKSI methods immediately following the check.  And the reason these are unnecessary in the notify methods is that they are only triggered if s_applicationIsOccluded ever becomes true, which can't happen unless the notifications fire.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:299
> > +    bool occluded = m_processSuppressionEnabled && s_applicationIsOccluded;
> 
> Would be nicer to name this isOccluded.
> 

Fixed.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:312
> > +    bool occluded = s_processSuppressionEnabledForAllContexts && s_applicationIsOccluded;
> 
> Would be nicer to name this isOccluded.
> 

Fixed.

> > 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.
> 

You are right, the code wasn't very clear.  I added the following comment in code.

    // If the application is currently occluded, then notify child processes.
    // This will ensure that, when enabling process suppression, the processes
    // set themselves as occluded and become eligible for suppression and when disabling
    // process suppression, they set themselves as unoccluded and become unsuppressed.

> > 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.
> 

Yeah, the register notification functions are the better place to put it.  I created a omitProcessSuppression() getter and added the check there itself.

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

Fixed.

Besides those fixes, I decided to leave process suppression disabled by default (m_processSuppressionEnabled is initialized to false), since a stable OS build is not available where it can be enabled.  Will use a followup patch to enable it.

[Alexey Proskuryakov]

Comment on attachment 182447 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182447&action=review

This looks much clearer to me! Please see more comments below.

> Source/WebKit2/UIProcess/WebContext.cpp:186
> +    updateProcessSuppressionEnabledForAnyAndAllContexts();

I still don't think that this is descriptive enough. Given that there are two actions to take here, perhaps this can be spit into two function calls? Something like

    installOcclusionNotificationHandlerIfNeeded();
    disableOcclusionHandlingForGlobalProcesses(); // FIXME (106804): Remove when m_processSuppressionEnabled is changed to start with true.

Starting from here, I think that you'll need to refactor other functions a bit more.

> Source/WebKit2/UIProcess/WebContext.h:358
> +    static bool omitProcessSuppression();

Maybe processSuppressionIsDisabledViaDebugPreference()?

> Source/WebKit2/UIProcess/WebContext.h:463
> +    static bool s_processSuppressionEnabledForAnyContext;
> +    static bool s_processSuppressionEnabledForAllContexts;

Please consider moving these to implementation file. The reason is that future refactorings will not touch the header then, and recompiles will be faster.

For the same reason, we like to have static functions declared in .cpp files, too, if they don't access member variables through a pointer.

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

isOccluded probably needs a better name now, because it combines "is occluded" with "this context tracks occlusion".

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:307
> +{

I'd put a comment here, explaining that we intend to get rid of global processes eventually.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:331
> +    updateProcessSuppressionEnabledForAnyAndAllContexts();

This needs to be split into more specific function calls, too.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:407
> +    if (omitProcessSuppression())
> +        return;

This function is only called once. It would be nice to make it do exactly what its name says  by moving the check out.

[Kiran Muppala]

Created attachment 182682 [details]
Patch

[Kiran Muppala]

(In reply to comment #23)
> (From update of attachment 182447 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182447&action=review
> 
> This looks much clearer to me! Please see more comments below.
> 
> > Source/WebKit2/UIProcess/WebContext.cpp:186
> > +    updateProcessSuppressionEnabledForAnyAndAllContexts();
> 
> I still don't think that this is descriptive enough. Given that there are two actions to take here, perhaps this can be spit into two function calls? Something like
> 
>     installOcclusionNotificationHandlerIfNeeded();
>     disableOcclusionHandlingForGlobalProcesses(); // FIXME (106804): Remove when m_processSuppressionEnabled is changed to start with true.
> 
> Starting from here, I think that you'll need to refactor other functions a bit more.
> 

I dropped the update method completely and moved initialization code to platformInitialize.  The initialization code now just calls couple of setters to update the value of s_processSuppressionEnabledForAnyContext and s_processSuppressionEnabledForAllContexts.  This way, it works correctly regardless of whether the default value is true or false.  But, I am not sure if I sacrificed readability.  Please let me know if you think so and I will find better method names.

> > Source/WebKit2/UIProcess/WebContext.h:358
> > +    static bool omitProcessSuppression();
> 
> Maybe processSuppressionIsDisabledViaDebugPreference()?
> 

I moved the check to one place and hence dropped this method all together.

> > Source/WebKit2/UIProcess/WebContext.h:463
> > +    static bool s_processSuppressionEnabledForAnyContext;
> > +    static bool s_processSuppressionEnabledForAllContexts;
> 
> Please consider moving these to implementation file. The reason is that future refactorings will not touch the header then, and recompiles will be faster.
> 
> For the same reason, we like to have static functions declared in .cpp files, too, if they don't access member variables through a pointer.
> 

I moved all private methods and static data members to the implementation file.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:298
> > +    bool isOccluded = m_processSuppressionEnabled && s_applicationIsOccluded;
> 
> isOccluded probably needs a better name now, because it combines "is occluded" with "this context tracks occlusion".
> 

I modified this method to take a isOccluded parameter.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:307
> > +{
> 
> I'd put a comment here, explaining that we intend to get rid of global processes eventually.
> 

Added a comment.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:331
> > +    updateProcessSuppressionEnabledForAnyAndAllContexts();
> 
> This needs to be split into more specific function calls, too.
> 

I broke this up into two getters and two setters.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:407
> > +    if (omitProcessSuppression())
> > +        return;
> 
> This function is only called once. It would be nice to make it do exactly what its name says  by moving the check out.

Fixed, moved the check out of these methods.

[Build Bot]

Comment on attachment 182682 [details]
Patch

Attachment 182682 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15871751

[Alexey Proskuryakov]

Comment on attachment 182682 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182682&action=review

This looks good to me. Since you are not a committer, and need to update the patch anyway to fix the build, I'll say r-, but there is nothing really else blocking this from being landed.

Comments below are optional, although I think that you'll want to address some of them.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:65
> +static bool s_applicationIsOccluded = false;

I'm not sure if this variable's value is accurate. Do we get an occlusion notification if the application launches in background in an invisible space (e.g. as part of relaunching applications after a restart)? Even if we get one, that might happen before we installed a handler.

This is more like a stored value of latest occlusion handler invocation than accurate information about occlusion state, unless I'm missing something. This variable name is too tempting, and someone could use it in an incorrect way later.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:67
> +static bool s_processSuppressionEnabledForAllContexts = true; // Initializing to true simplifies WebContext::platformInitialize.

I don't think that this comment is helpful. The value is logically true when there are no contexts (because it's a result of statement applied to an empty set).

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:85
> +static void setApplicationIsOccludedForChildProcessesInContext(bool isOccluded, WebContext* context)

Despite what I said about moving functions to .cpp files, this might be going further than we usually go, an is thus surprising.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:109
> +static void setApplicationIsOccluded(bool isOccluded)

I think that behavior of this function is just fine, but the name makes it confusingly look like a simple setter (same applies to specialized versions above). Perhaps "notifyProcessesIfOcclusionStateChanged()" would be correct and more helpful?

Alternatively, one can think about this as common code for applicationBecameVisible and applicationBecameOccluded functions, and name this "applicationBecameOccluded()".

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:114
> +    if (s_applicationIsOccluded == isOccluded)
> +        return;
> +
> +    s_applicationIsOccluded = isOccluded;

In fact, I'd consider moving this logic to callers, even though this means a few lines of duplicated code. That way, this function would only do one thing, not two.

Also, this would make it clearer that we don't rely on these notifications to not come twice, and perhaps trigger an interesting investigation of why we can't rely on them.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:161
> +static void setProcessSuppressionEnabledForAnyContext(bool enabled)

Here as well, the name is more appropriate for a plain setter, not for a function whose primary goal is to register or unregister handlers. It's an implementation detail that a static variable tracks having the handlers currently installed.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:193
> +static bool isProcessSuppressionEnabledForAnyContext()

We normally try to make function names are grammatically correct as feasible. Here, processSuppressionIsEnabledForAnyContext() would be more in accordance with WebKit style.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:226
> +    if (m_processSuppressionEnabled)
> +        setProcessSuppressionEnabledForAnyContext(true);
> +    else
> +        setProcessSuppressionEnabledForAllContexts(false);

I don't get this. Isn't this condition a constant? I think that platformInitialize() is called before a client could have a chance to change the value of m_processSuppressionEnabled.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:436
> +    return s_applicationIsOccluded;

This function is dead code AFAICT, and should be removed.

[Kiran Muppala]

Created attachment 182894 [details]
Patch

[Kiran Muppala]

(In reply to comment #27)
> (From update of attachment 182682 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182682&action=review
> 
> This looks good to me. Since you are not a committer, and need to update the patch anyway to fix the build, I'll say r-, but there is nothing really else blocking this from being landed.
> 
> Comments below are optional, although I think that you'll want to address some of them.
> 
> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:65
> > +static bool s_applicationIsOccluded = false;
> 
> I'm not sure if this variable's value is accurate. Do we get an occlusion notification if the application launches in background in an invisible space (e.g. as part of relaunching applications after a restart)? Even if we get one, that might happen before we installed a handler.
> 
> This is more like a stored value of latest occlusion handler invocation than accurate information about occlusion state, unless I'm missing something. This variable name is too tempting, and someone could use it in an incorrect way later.
> 

You are right about this.  In fact the occlusion notification API as initially provided did not have a mechanism to handle this correctly.  But they have since added a way to get the occlusion state, and there is a separate radar on that <rdar://problem/12929903> and would like to address it in a patch for that issue alone.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:67
> > +static bool s_processSuppressionEnabledForAllContexts = true; // Initializing to true simplifies WebContext::platformInitialize.
> 
> I don't think that this comment is helpful. The value is logically true when there are no contexts (because it's a result of statement applied to an empty set).
>
Ok, dropped the comment.
 
> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:85
> > +static void setApplicationIsOccludedForChildProcessesInContext(bool isOccluded, WebContext* context)
> 
> Despite what I said about moving functions to .cpp files, this might be going further than we usually go, an is thus surprising.
>

I made this a member function and renamed it as updateChildProcessesApplicationOcclusionState in line with the fixes for the comments below.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:109
> > +static void setApplicationIsOccluded(bool isOccluded)
> 
> I think that behavior of this function is just fine, but the name makes it confusingly look like a simple setter (same applies to specialized versions above). Perhaps "notifyProcessesIfOcclusionStateChanged()" would be correct and more helpful?
> 
> Alternatively, one can think about this as common code for applicationBecameVisible and applicationBecameOccluded functions, and name this "applicationBecameOccluded()".
> 
> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:114
> > +    if (s_applicationIsOccluded == isOccluded)
> > +        return;
> > +
> > +    s_applicationIsOccluded = isOccluded;
> 
> In fact, I'd consider moving this logic to callers, even though this means a few lines of duplicated code. That way, this function would only do one thing, not two.
> 
> Also, this would make it clearer that we don't rely on these notifications to not come twice, and perhaps trigger an interesting investigation of why we can't rely on them.
> 

Ok, I moved the setting part of the method to the applicationBecameVisible and applicationBecameOccluded.  Renamed this to applicationOcclusionStateChanged().

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:161
> > +static void setProcessSuppressionEnabledForAnyContext(bool enabled)
> 
> Here as well, the name is more appropriate for a plain setter, not for a function whose primary goal is to register or unregister handlers. It's an implementation detail that a static variable tracks having the handlers currently installed.
> 

This comment is the single most helpful feedback I got during this review.  Until now I was refactoring with the only goal being to reduce function complexity, but never paid attention to the part that I was naming them after implementation details.  Thanks for this very insightful comment.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:193
> > +static bool isProcessSuppressionEnabledForAnyContext()
> 
> We normally try to make function names are grammatically correct as feasible. Here, processSuppressionIsEnabledForAnyContext() would be more in accordance with WebKit style.
>

Fixed.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:226
> > +    if (m_processSuppressionEnabled)
> > +        setProcessSuppressionEnabledForAnyContext(true);
> > +    else
> > +        setProcessSuppressionEnabledForAllContexts(false);
> 
> I don't get this. Isn't this condition a constant? I think that platformInitialize() is called before a client could have a chance to change the value of m_processSuppressionEnabled.
>

Yes, I was trying handle both cases because it gave me a sense, probably false, of robustness, but I guess less code is better code, so I removed the true case.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:436
> > +    return s_applicationIsOccluded;
> 
> This function is dead code AFAICT, and should be removed.

This getter is used by WebProcessProxy, NetworkProcessProxy e.t.c in didFinishLaunching(), just not used within this file.

[Build Bot]

Comment on attachment 182894 [details]
Patch

Attachment 182894 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15910139

[Alexey Proskuryakov]

> You are right about this.  In fact the occlusion notification API as initially provided did not have a mechanism to handle this correctly.  But they have since added a way to get the occlusion state, and there is a separate radar on that <rdar://problem/12929903> and would like to address it in a patch for that issue alone.

I think that we will still need a variable with the semantics of s_applicationIsOccluded - it's not helpful to know true current occlusion state when the goal is to decide whether to update handler registration. It still appears that this variable (and its getter) need more precise names.

[Alexey Proskuryakov]

> Yes, I was trying handle both cases because it gave me a sense, probably false, of robustness, but I guess less code is better code, so I removed the true case.

You could ASSERT that m_processSuppressionEnabled has expected value. In fact, I suggest that you add such an assertion.

[Kiran Muppala]

Created attachment 182906 [details]
Patch

[Kiran Muppala]

(In reply to comment #31)
> > You are right about this.  In fact the occlusion notification API as initially provided did not have a mechanism to handle this correctly.  But they have since added a way to get the occlusion state, and there is a separate radar on that <rdar://problem/12929903> and would like to address it in a patch for that issue alone.
> 
> I think that we will still need a variable with the semantics of s_applicationIsOccluded - it's not helpful to know true current occlusion state when the goal is to decide whether to update handler registration. It still appears that this variable (and its getter) need more precise names.

I don't follow.  Handler registration is based on whether any context has process suppression enabled.  s_applicationIsOccluded does represent if the application is currently occluded although some child processes may not think it is if process suppression is disabled for them.  I plan to rename the variable used by a individual process to something like processVisibility to make that more clear.

[Alexey Proskuryakov]

Yes, I misspoke, handler registration is unrelated. 

Anyway, this variable feel more more like "latestOcclusionNotification" or "savedOcclusionState" to me than a plain "applicationIsOccluded".

[Kiran Muppala]

(In reply to comment #35)
> Yes, I misspoke, handler registration is unrelated. 
> 
> Anyway, this variable feel more more like "latestOcclusionNotification" or "savedOcclusionState" to me than a plain "applicationIsOccluded".

Yeah, I see the danger now.  I will separate process suppression from occlusion notifications, which need to be enabled all the time for other reasons.  Then this variable will truly reflect the current state.  I will leave this as is for now.

[Kiran Muppala]

Created attachment 182913 [details]
Patch

[Kiran Muppala]

(In reply to comment #32)
> > Yes, I was trying handle both cases because it gave me a sense, probably false, of robustness, but I guess less code is better code, so I removed the true case.
> 
> You could ASSERT that m_processSuppressionEnabled has expected value. In fact, I suggest that you add such an assertion.

Added ASSERT in latest patch.  Besides that fixed a bug, which is that didFinishLaunching() functions were not checking if process suppression is enabled for their context or globally before messaging the process.  Added the additional check for the four didFinishLaunching() methods.  Also fixed the build failure due to unused method.

[Alexey Proskuryakov]

Comment on attachment 182913 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182913&action=review

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:68
> +static bool s_applicationIsOccluded = false;

I still think that this should have a less misleading name, but OK, let's deal with this later.

[WebKit Review Bot]

Comment on attachment 182913 [details]
Patch

Clearing flags on attachment: 182913

Committed r139888: <http://trac.webkit.org/changeset/139888>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.