Bug 100997 – Add application occlusion criterion to enable/disable process suppression on Mac

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

[Kiran Muppala]

When App is occluded all WebKit2 processes should be eligible for suppression.  This behavior should be disabled by default.

[Kiran Muppala]

<rdar://problem/12583536>

[Kiran Muppala]

Created attachment 171954 [details]
Patch

[Mark Rowe (bdash)]

Comment on attachment 171954 [details]
Patch

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

I'll let someone with more WebKit2 familiarity comment on the bulk of the patch, which is all about the IPC side of things. I've provided feedback on some aspects related to process suppression.

> Source/WebKit2/ChangeLog:3
> +        Add App occlusion criterion to enable/disable process suppression on mac

App -> application
mac -> Mac

> Source/WebKit2/NetworkProcess/NetworkProcess.messages.in:33
> +    SetAppIsOccluded(bool flag)

The abbreviation here isn't worth it IMO. App should be "Application" here and elsewhere in this patch.

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:29
> +#import <Foundation/Foundation.h>

This #import isn't necessary. This is pulled in by our prefix headers.

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:36
> +    if (m_appIsOccluded != flag) {
> +        m_appIsOccluded = flag;

Our usual style is to prefer an early return rather than nesting the body of the method in an if like this.

> Source/WebKit2/Shared/mac/ChildProcessMac.mm:41
> +            enableSuppression(@"App became Occluded");
> +        else
> +            disableSuppression(@"App became Visible");

I don't think occluded and visible should be capitalized in this message.

> Source/WebKit2/UIProcess/API/C/mac/WKContextPrivateMac.cpp:29
> +#include "WKAPICast.h"

Is this include needed? I don't see anything that obviously uses it in this file.

> Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:168
> +    if (WebContext::appIsOccluded())
> +        m_connection->send(Messages::NetworkProcess::SetAppIsOccluded(true), 0);

The network process already disables suppression when it launches. Why does it need to be done again in didFinishLaunching?

> Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:234
> +    if (WebContext::appIsOccluded())
> +        m_connection->send(Messages::PluginProcess::SetAppIsOccluded(true), 0);

Same question here as with the network process case.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:278
> +        for (size_t i = 0, count = contexts.size(); i < count; ++i) {
> +            contexts[i]->sendToAllProcesses(Messages::WebProcess::SetAppIsOccluded(false));
> +        }

The braces aren't needed around the body of this loop.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:299
> +        for (size_t i = 0, count = contexts.size(); i < count; ++i) {
> +            contexts[i]->sendToAllProcesses(Messages::WebProcess::SetAppIsOccluded(true));
> +        }

The braces aren't needed around the body of this loop.

[Kiran Muppala]

Created attachment 171969 [details]
Patch

[Kiran Muppala]

(In reply to comment #3)
> (From update of attachment 171954 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171954&action=review
> 
> I'll let someone with more WebKit2 familiarity comment on the bulk of the patch, which is all about the IPC side of things. I've provided feedback on some aspects related to process suppression.
> 
> > Source/WebKit2/ChangeLog:3
> > +        Add App occlusion criterion to enable/disable process suppression on mac
> 
> App -> application
> mac -> Mac
> 

Fixed.

> > Source/WebKit2/NetworkProcess/NetworkProcess.messages.in:33
> > +    SetAppIsOccluded(bool flag)
> 
> The abbreviation here isn't worth it IMO. App should be "Application" here and elsewhere in this patch.
> 

Fixed.

> > Source/WebKit2/Shared/mac/ChildProcessMac.mm:29
> > +#import <Foundation/Foundation.h>
> 
> This #import isn't necessary. This is pulled in by our prefix headers.
> 

You are right, the file compiled without this and hence removed it.

> > Source/WebKit2/Shared/mac/ChildProcessMac.mm:36
> > +    if (m_appIsOccluded != flag) {
> > +        m_appIsOccluded = flag;
> 
> Our usual style is to prefer an early return rather than nesting the body of the method in an if like this.
> 

Added early return and eliminated nesting.

> > Source/WebKit2/Shared/mac/ChildProcessMac.mm:41
> > +            enableSuppression(@"App became Occluded");
> > +        else
> > +            disableSuppression(@"App became Visible");
> 
> I don't think occluded and visible should be capitalized in this message.
> 

Fixed.

> > Source/WebKit2/UIProcess/API/C/mac/WKContextPrivateMac.cpp:29
> > +#include "WKAPICast.h"
> 
> Is this include needed? I don't see anything that obviously uses it in this file.
> 

Yeah, this wasn't necessary.  Removed in updated patch.

> > Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:168
> > +    if (WebContext::appIsOccluded())
> > +        m_connection->send(Messages::NetworkProcess::SetAppIsOccluded(true), 0);
> 
> The network process already disables suppression when it launches. Why does it need to be done again in didFinishLaunching?
> 
> > Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:234
> > +    if (WebContext::appIsOccluded())
> > +        m_connection->send(Messages::PluginProcess::SetAppIsOccluded(true), 0);
> 
> Same question here as with the network process case.

If during process launch, the application became occluded, then this will let the process know about it so that suppression can be enabled.  Unlike WebProcess, PluginProcess, NetworkProcess and SharedWorkerProcess don't queue messages during launch, so adding this check in didFinishLaunching is a sure way to update the process after launch.

> 
> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:278
> > +        for (size_t i = 0, count = contexts.size(); i < count; ++i) {
> > +            contexts[i]->sendToAllProcesses(Messages::WebProcess::SetAppIsOccluded(false));
> > +        }
> 
> The braces aren't needed around the body of this loop.
> 
> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:299
> > +        for (size_t i = 0, count = contexts.size(); i < count; ++i) {
> > +            contexts[i]->sendToAllProcesses(Messages::WebProcess::SetAppIsOccluded(true));
> > +        }
> 
> The braces aren't needed around the body of this loop.

Fixed.

[Mark Rowe (bdash)]

(In reply to comment #5)
> (In reply to comment #3)
> > Same question here as with the network process case.
> 
> If during process launch, the application became occluded, then this will let the process know about it so that suppression can be enabled.  Unlike WebProcess, PluginProcess, NetworkProcess and SharedWorkerProcess don't queue messages during launch, so adding this check in didFinishLaunching is a sure way to update the process after launch.

My concern is that the calls to -disableAutomaticTermination: / -enableAutomaticTermination: need to be balanced. If -disableAutomaticTermination: is called twice during launch, once by the process during its launch, and once as a result of receiving the message sent within didFinishLaunching, how will the extra call be balanced out?

[Kiran Muppala]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > Same question here as with the network process case.
> > 
> > If during process launch, the application became occluded, then this will let the process know about it so that suppression can be enabled.  Unlike WebProcess, PluginProcess, NetworkProcess and SharedWorkerProcess don't queue messages during launch, so adding this check in didFinishLaunching is a sure way to update the process after launch.
> 
> My concern is that the calls to -disableAutomaticTermination: / -enableAutomaticTermination: need to be balanced. If -disableAutomaticTermination: is called twice during launch, once by the process during its launch, and once as a result of receiving the message sent within didFinishLaunching, how will the extra call be balanced out?

The member variable m_isApplicationOccluded protects against this.  Only when a message with occlusion state differing from the current value of m_isApplicationOccluded is received, is the assertion taken or released.  If the state matches current value, then the message has no effect.  This keeps the assertion balanced even if the messages are not balanced.  In fact the didFinishLaunching method first checks if the application has become occluded before sending a message, since otherwise the message has no effect.

[Mark Rowe (bdash)]

(In reply to comment #7)
> In fact the didFinishLaunching method first checks if the application has become occluded before sending a message, since otherwise the message has no effect.

Yeah, looking over the patch again I can see that my concern is nonsense. The application suppression parts of it look good. Hopefully you can find someone to look at the WebKit2 aspects soon!

[Alexey Proskuryakov]

Comment on attachment 171969 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkProcess.messages.in:33
> +    SetApplicationIsOccluded(bool flag)

Receiving this message with true flag results in an -enableAutomaticTermination call. Why is it OK to ever automatically terminate NetworkProcess (or any of the other processes, for that matter)?

> Source/WebKit2/Shared/ChildProcess.h:32
> +OBJC_CLASS NSString;

I think that this should be in PLATFORM(MAC) for clarity.

> Source/WebKit2/Shared/ChildProcess.h:67
> +    static void disableSuppression(NSString *reason);
> +    static void enableSuppression(NSString *reason);

Not sure why this is in ChildProcess. Won't main process need the same functionality?

[Kiran Muppala]

Created attachment 172185 [details]
Patch

[Kiran Muppala]

(In reply to comment #9)
> (From update of attachment 171969 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171969&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkProcess.messages.in:33
> > +    SetApplicationIsOccluded(bool flag)
> 
> Receiving this message with true flag results in an -enableAutomaticTermination call. Why is it OK to ever automatically terminate NetworkProcess (or any of the other processes, for that matter)?

The naming of the assertion is causing unnecessary confusion.

> 
> > Source/WebKit2/Shared/ChildProcess.h:32
> > +OBJC_CLASS NSString;
> 
> I think that this should be in PLATFORM(MAC) for clarity.

I agree, although moving the below methods to a new Mac specific file made this issue go away.

> 
> > Source/WebKit2/Shared/ChildProcess.h:67
> > +    static void disableSuppression(NSString *reason);
> > +    static void enableSuppression(NSString *reason);
> 
> Not sure why this is in ChildProcess. Won't main process need the same functionality?

Although it isn't needed in this patch, you are right, UI process will also need these.  So, I moved them to a new file called ProcessUtilities.h  under Platform/mac.

[Kiran Muppala]

Created attachment 172188 [details]
Patch

[Kiran Muppala]

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

Added a comment to the assertion helper methods to reduce confusion.  Could not use a assertion because the UI process can have automatic termination support enabled already.

[Alexey Proskuryakov]

Comment on attachment 172188 [details]
Patch

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.

[Kiran Muppala]

Created attachment 172410 [details]
Patch

[Kiran Muppala]

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

Fixed. Moved ProcessUtilities.h to Platform folder and renamed ProcessUtilities.mm to ProcessUtilitiesMac.mm.

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

Yeah, my comment did incorrectly suggest that AutoTerminationSupport should be enabled for ChildProcesses.  Thanks for providing a better one to replace it with.

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

I did look into WKPreferences after your feedback, but I could not move the function since WKPreferences is tied to a page group, but this particular setting is global and applies to all contexts and processes.  ProcessModel is a similar setting that applies to contexts but unlike ProcessModel process suppression support is easier to set across all child processes regardless of context or process type.

It is possible to provide a setting on a per context basis too, but then a similar setting would be needed for each process type.  This added granularity of enabling or disabling process suppression is unlikely to be used.  Since, the goal is to enable it for all processes and fix any bugs using assertions rather than disabling it on a per context or process type basis.

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

This is only there because NetworkProcessManager does not expose the pointer to the NetworkProcess itself.  Since PluginProcessManager and SharedWorkerProcessManager also didn't expose pointers to their underlying processes, I didn't add a getter to NetworkProcessManager either.

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

Ok, I renamed this and SharedWorkerProcessManger::sendToAllProcesses to "sendToAllValidProcesses" and also added as similar method to WebContext.

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

Yeah, the copy/pasted code speaks to a more fundamental problem.  As far as I know, it isn't possible to specify messages common to all child processes.  So, the actual message type is distinct even though they all convey the same information.  This means, even if I add a base class, say ChildProcessProxy, the actual message needs to be sent in the derived class to preserve the type information of the Message.  i.e Messages::SharedWorkerProcess::SetApplicationIsOccluded vs Messages::WebProcess::SetApplicationIsOccluded.  I am sure it would be possible to add support for specifying messages received by ChildProcess itself.  But, that is not a trivial change and I don't claim to know how to do it either.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:63
> > +bool WebContext::s_processSuppressionSupportEnabled = false;
> 
> This too should be in preferences, not a standalone boolean.

This is tied to the method WKContextSetProcessSuppressionSupportEnabled(bool) that I commented on above.

[Kiran Muppala]

Created attachment 172433 [details]
Patch

[Kiran Muppala]

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

Fixed merge conflict in WebKit2.xcodeproj/project.pbxproj.  No other changes.

[Alexey Proskuryakov]

Comment on attachment 172433 [details]
Patch

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

> This is only there because NetworkProcessManager does not expose the pointer to the NetworkProcess itself.  Since PluginProcessManager and SharedWorkerProcessManager also didn't expose pointers to their underlying processes, I didn't add a getter to NetworkProcessManager either.

I see. I suggest following the existing model then, and adding a function that sends the message internally, instead of exposing a "sendToProcess" function. For example, it may well make sense to send a blunt SIGSTOP to some of the processes instead of enabling automatic suspension. Knowledge about this should be encapsulated in process manager.

> Source/WebKit2/Platform/mac/ProcessUtilitiesMac.mm:34
> +    // The following call assumes that AutoTerminationSupport is not enabled for ChildProcesses.

This is the text I suggested, but mentioning ChildProcess does not make sense in ProcessUtilities. This applies to any process where this is called, whether it is a child process or not.

> Source/WebKit2/UIProcess/API/C/WKContext.cpp:332
> +void WKContextSetProcessSuppressionSupportEnabled(bool enable)
> +{
> +    WebContext::setProcessSuppressionSupportEnabled(enable);
> +}

As long as the function name starts with "WKContext", it should operate on WKContexts. It's just a naming scheme.

But this doesn't really feel like a per-context API.

I'm sorry that I cannot suggest what to do here. Perhaps you should consider how the feature will evolve in near future, and make an API for that, even if not fully implemented.

[Kiran Muppala]

Created attachment 172647 [details]
Patch

[Kiran Muppala]

(In reply to comment #19)
> (From update of attachment 172433 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172433&action=review
> 
> > This is only there because NetworkProcessManager does not expose the pointer to the NetworkProcess itself.  Since PluginProcessManager and SharedWorkerProcessManager also didn't expose pointers to their underlying processes, I didn't add a getter to NetworkProcessManager either.
> 
> I see. I suggest following the existing model then, and adding a function that sends the message internally, instead of exposing a "sendToProcess" function. For example, it may well make sense to send a blunt SIGSTOP to some of the processes instead of enabling automatic suspension. Knowledge about this should be encapsulated in process manager.

Removed generic send to process methods and replaced it with specific method for application occlusion message on network, plugin and shared worker process.  For WebProcess, since the generic message send function already exists, i used that itself.

> 
> > Source/WebKit2/Platform/mac/ProcessUtilitiesMac.mm:34
> > +    // The following call assumes that AutoTerminationSupport is not enabled for ChildProcesses.
> 
> This is the text I suggested, but mentioning ChildProcess does not make sense in ProcessUtilities. This applies to any process where this is called, whether it is a child process or not.
> 

I replaced these two functions with a class to manage the assertion.  Added a more general comment too.

> > Source/WebKit2/UIProcess/API/C/WKContext.cpp:332
> > +void WKContextSetProcessSuppressionSupportEnabled(bool enable)
> > +{
> > +    WebContext::setProcessSuppressionSupportEnabled(enable);
> > +}
> 
> As long as the function name starts with "WKContext", it should operate on WKContexts. It's just a naming scheme.
> 
> But this doesn't really feel like a per-context API.
> 
> I'm sorry that I cannot suggest what to do here. Perhaps you should consider how the feature will evolve in near future, and make an API for that, even if not fully implemented.

I removed the API methods.  As you said, they should operate on WKContexts and making the implementation complex, I am using a private default for now.  Sam Weinig said it is ok to do this when we don't expect users to change the setting.  Once process suppression is enabled by default, it will be easier to correctly interpret a per-context setting and hence I will add a API at that time.

[Kiran Muppala]

(In reply to comment #21)
> (In reply to comment #19)
> > (From update of attachment 172433 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=172433&action=review
> > 
> > > This is only there because NetworkProcessManager does not expose the pointer to the NetworkProcess itself.  Since PluginProcessManager and SharedWorkerProcessManager also didn't expose pointers to their underlying processes, I didn't add a getter to NetworkProcessManager either.
> > 
> > I see. I suggest following the existing model then, and adding a function that sends the message internally, instead of exposing a "sendToProcess" function. For example, it may well make sense to send a blunt SIGSTOP to some of the processes instead of enabling automatic suspension. Knowledge about this should be encapsulated in process manager.
> 
> Removed generic send to process methods and replaced it with specific method for application occlusion message on network, plugin and shared worker process.  For WebProcess, since the generic message send function already exists, i used that itself.
> 
> > 
> > > Source/WebKit2/Platform/mac/ProcessUtilitiesMac.mm:34
> > > +    // The following call assumes that AutoTerminationSupport is not enabled for ChildProcesses.
> > 
> > This is the text I suggested, but mentioning ChildProcess does not make sense in ProcessUtilities. This applies to any process where this is called, whether it is a child process or not.
> > 
> 
> I replaced these two functions with a class to manage the assertion.  Added a more general comment too.
> 
> > > Source/WebKit2/UIProcess/API/C/WKContext.cpp:332
> > > +void WKContextSetProcessSuppressionSupportEnabled(bool enable)
> > > +{
> > > +    WebContext::setProcessSuppressionSupportEnabled(enable);
> > > +}
> > 
> > As long as the function name starts with "WKContext", it should operate on WKContexts. It's just a naming scheme.
> > 
> > But this doesn't really feel like a per-context API.
> > 
> > I'm sorry that I cannot suggest what to do here. Perhaps you should consider how the feature will evolve in near future, and make an API for that, even if not fully implemented.
> 
> I removed the API methods.  As you said, they should operate on WKContexts and making the implementation complex, I am using a private default for now.  Sam Weinig said it is ok to do this when we don't expect users to change the setting.  Once process suppression is enabled by default, it will be easier to correctly interpret a per-context setting and hence I will add a API at that time.

I meant to say, and *instead of* making the implementation complex, above.

[Alexey Proskuryakov]

Comment on attachment 172647 [details]
Patch

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

> I replaced these two functions with a class to manage the assertion.

I don't like this change. In object oriented design, a class is supposed to be something that encapsulates state and behaviors. A  "suppression disabler" is hardly such a thing, and it barely parses as English.

This design pattern is so useful for RAII that it's widely used anyway, but there is no RAII here as far as I can tell, just a function call disguised as method call.

Looks good to me otherwise.

> Source/WebKit2/ChangeLog:33
> +        * Shared/ChildProcess.h:
> +        (WebKit):
> +        (ChildProcess):

ChangeLogs are meant to be read (and sometimes searched) by humans, so if prepare-ChangeLog generates useless lines like these, it's desirable to correct it manually for best results.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:314
> +    // A secret default until process suppression is enabled by default, at which point a context setting can be added with the

I would have said "temporary", not "secret". It's right here in plain sight, not secret at all.

[Kiran Muppala]

Created attachment 172667 [details]
Patch

[Kiran Muppala]

(In reply to comment #23)
> (From update of attachment 172647 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172647&action=review
> 
> > I replaced these two functions with a class to manage the assertion.
> 
> I don't like this change. In object oriented design, a class is supposed to be something that encapsulates state and behaviors. A  "suppression disabler" is hardly such a thing, and it barely parses as English.
> 
> This design pattern is so useful for RAII that it's widely used anyway, but there is no RAII here as far as I can tell, just a function call disguised as method call.
> 
> Looks good to me otherwise.
> 

I removed this class and replaced it with member functions within ChildProcess.  You commented in a previous patch that these functions could be used in UI process and hence I had moved them out of here.  But i don't foresee using these functions without the use of a RAII class in the UI process.  I will add the class back when needed for the UI process.

> > Source/WebKit2/ChangeLog:33
> > +        * Shared/ChildProcess.h:
> > +        (WebKit):
> > +        (ChildProcess):
> 
> ChangeLogs are meant to be read (and sometimes searched) by humans, so if prepare-ChangeLog generates useless lines like these, it's desirable to correct it manually for best results.
> 

Fixed.  Thanks for explaining that output of prepare-changelog acts as a guide rather than being final.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:314
> > +    // A secret default until process suppression is enabled by default, at which point a context setting can be added with the
> 
> I would have said "temporary", not "secret". It's right here in plain sight, not secret at all.

Changed to "temporary".

[Alexey Proskuryakov]

Comment on attachment 172667 [details]
Patch

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

r=mw, thank you for your patience.

> Source/WebKit2/ChangeLog:38
> +        (NetworkProcessProxy):

I didn't make it clear that my comment was general, not just about those two lines.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:293
> +            contexts[i]->sendToAllProcesses(Messages::WebProcess::SetApplicationIsOccluded(true));

Yes, it makes sense to keep the message sent directly to WebProcesses, because WebContext is a "manager" for those.

[Kiran Muppala]

Created attachment 172673 [details]
Patch

[Kiran Muppala]

(In reply to comment #26)
> (From update of attachment 172667 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172667&action=review
> 
> r=mw, thank you for your patience.
> 

No worries.  I learnt a lot about WKPreferences, API convention among other things due to your feedback.

> > Source/WebKit2/ChangeLog:38
> > +        (NetworkProcessProxy):
> 
> I didn't make it clear that my comment was general, not just about those two lines.
> 

My bad.  I thought I removed all of them.  I removed this and couple more similar lines.

> > Source/WebKit2/UIProcess/mac/WebContextMac.mm:293
> > +            contexts[i]->sendToAllProcesses(Messages::WebProcess::SetApplicationIsOccluded(true));
> 
> Yes, it makes sense to keep the message sent directly to WebProcesses, because WebContext is a "manager" for those.

[Kiran Muppala]

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

Removed unhelpful lines in ChangeLog.  No other changes.

[WebKit Review Bot]

Comment on attachment 172673 [details]
Patch

Clearing flags on attachment: 172673

Committed r133699: <http://trac.webkit.org/changeset/133699>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> I see. I suggest following the existing model then, and adding a function that sends the message internally, instead of exposing a "sendToProcess" function. For example, it may well make sense to send a blunt SIGSTOP to some of the processes instead of enabling automatic suspension. Knowledge about this should be encapsulated in process manager.

FWIW, I'm adding a process() accessor to NetworkProcessManager now, so that messages to it could be sent easier.

But it probably still makes sense to keep setApplicationIsOccluded() as function on NetworkProcessManager, as it's part of process management that may or may not be implemented by sending a regular message.

[Kiran Muppala]

(In reply to comment #32)
> > I see. I suggest following the existing model then, and adding a function that sends the message internally, instead of exposing a "sendToProcess" function. For example, it may well make sense to send a blunt SIGSTOP to some of the processes instead of enabling automatic suspension. Knowledge about this should be encapsulated in process manager.
> 
> FWIW, I'm adding a process() accessor to NetworkProcessManager now, so that messages to it could be sent easier.
> 
> But it probably still makes sense to keep setApplicationIsOccluded() as function on NetworkProcessManager, as it's part of process management that may or may not be implemented by sending a regular message.

Another reason to keep it in the process manager is if there is a network process per context in the future.