Bug 139108 – Track pages preventing suppression in WebProcessProxy using RefCounter (instead of HashSet)

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

[Gavin Barraclough]

The networking process is allowed to app nap if all web pages are also currently in app nap.*

In order to detect whether any page in any process currently requires the networking process to be active we:
 - maintains hash sets in every WebProcessProxy of pages that are okay with suppression.
 - if anything changes, the WebContext iterates every WebProcessProxy to recompute state.
This is all crazy - all we actually need is a simple count of the number of pages that need to prevent the networking process from entering app nap.

This patch gets us half way there - replace the HashSet with a RefCounter. Next step will be to hoist the RefCounters from the process proxies up to the context to do away with the iteration.

(* Actually, more specifically the networking process will app nap if all pages are visually idle - that's probably a bug, we're failing to take media & page load state into account.)

[Gavin Barraclough]

Created attachment 242289 [details]
Fix

[Gavin Barraclough]

Created attachment 243151 [details]
Fix

[Benjamin Poulain]

Comment on attachment 243151 [details]
Fix

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

We should consider removing all the #ifdefs. The concepts seems useful for other platforms.

Something I don't see: shouldn't there be code updating the counter when the settings change?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:391
> +    updateProccessSuppressionState();

Why not make updateProccessSuppressionState() a step of updateActivityToken()? (and maybe rename updateActivityToken)

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1283
> +        m_preventProcessSuppression.clear();

m_preventProcessSuppression = nullptr?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:4567
> +    m_preventProcessSuppression.clear();

m_preventProcessSuppression = nullptr?

> Source/WebKit2/UIProcess/WebProcessProxy.h:123
> +    PassRefPtr<RefCounter::Count> preventProcessSuppressionForPage()

The function name is an action, that is no good for a getter.

Not sure what would be a better name. Maybe processSuppressionCounter()?

> Source/WebKit2/UIProcess/WebProcessProxy.h:234
> +    RefCounter m_pagesPreventingSuppression;

m_pagesPreventingSuppression -> m_pagesPreventingSuppressionCount/m_pagesPreventingSuppressionCounter/m_processSupressionCounter?

[Gavin Barraclough]

(In reply to comment #3)
> Comment on attachment 243151 [details]
> Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243151&action=review
> 
> We should consider removing all the #ifdefs. The concepts seems useful for
> other platforms.

Yep, that seems reasonable.
 
> Something I don't see: shouldn't there be code updating the counter when the
> settings change?

That happens from WebPageProxy::preferencesDidChange().
(though this can probably go away after in the next patch).
 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:391
> > +    updateProccessSuppressionState();
> 
> Why not make updateProccessSuppressionState() a step of
> updateActivityToken()? (and maybe rename updateActivityToken)

Yeah, the repeated separate calls aren't great. :-(
 
This should probably be merged out to a separate subclass, similar to PageThrotter in WebCore. I'm gonna leave them separate for now.

> > Source/WebKit2/UIProcess/WebPageProxy.cpp:1283
> > +        m_preventProcessSuppression.clear();
> 
> m_preventProcessSuppression = nullptr?
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:4567
> > +    m_preventProcessSuppression.clear();
> 
> m_preventProcessSuppression = nullptr?

Done.
 
> > Source/WebKit2/UIProcess/WebProcessProxy.h:123
> > +    PassRefPtr<RefCounter::Count> preventProcessSuppressionForPage()
> 
> The function name is an action, that is no good for a getter.
> 
> Not sure what would be a better name. Maybe processSuppressionCounter()?

Done.

> > Source/WebKit2/UIProcess/WebProcessProxy.h:234
> > +    RefCounter m_pagesPreventingSuppression;
> 
> m_pagesPreventingSuppression ->
> m_pagesPreventingSuppressionCount/m_pagesPreventingSuppressionCounter/
> m_processSupressionCounter?

Done.
 
https://trac.webkit.org/changeset/177209