Bug 126356 – Propagate WindowServer modifications state to WebProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Gavin Barraclough]

This will be necessary to move control of process suppression to the WebProcess.

[Gavin Barraclough]

Created attachment 220178 [details]
Fix

[EFL EWS Bot]

Comment on attachment 220178 [details]
Fix

Attachment 220178 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/5880595670368256

[Gavin Barraclough]

Created attachment 220182 [details]
GTK fix

[Sam Weinig]

Comment on attachment 220182 [details]
GTK fix

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

> Source/WebKit2/UIProcess/WebContext.h:304
> +#if PLATFORM(MAC) && !PLATFORM(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090
> +    void windowModificationsStateChanged();
> +    static WindowModificationsState windowModificationsState();
> +#else
> +    static WindowModificationsState windowModificationsState() { return WindowModificationsStarted; }
> +#endif

I don't get why something called WindowModificationsState would be on the WebContext (or static).  "Window" level concepts should really a PageClient thing if possible.

> Source/WebKit2/UIProcess/mac/WebContextMac.mm:137
> +    for (auto *context : contexts) {

* on the wrong side.

> Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm:85
> +void WebProcessProxy::windowModificationsStateChanged()

Even though this isn't currently used by other platforms, there isn't anything really mac specific about it.  Can this just go in WebProcessProxy.cpp?

> Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm:88
> +    for (WebPageProxyMap::iterator it = m_pageMap.begin(), end = m_pageMap.end(); it != end; ++it)
> +        it->value->viewStateDidChange(ViewState::IsModifying);

This can be written as:

for (const auto&  page : m_pageMap.values())
    page->viewStateDidChange(ViewState::IsModifying);

[Gavin Barraclough]

(In reply to comment #4)
> (From update of attachment 220182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220182&action=review
> 
> > Source/WebKit2/UIProcess/WebContext.h:304
> > +#if PLATFORM(MAC) && !PLATFORM(IOS) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090
> > +    void windowModificationsStateChanged();
> > +    static WindowModificationsState windowModificationsState();
> > +#else
> > +    static WindowModificationsState windowModificationsState() { return WindowModificationsStarted; }
> > +#endif
> 
> I don't get why something called WindowModificationsState would be on the WebContext (or static).  "Window" level concepts should really a PageClient thing if possible.

The WindowModifications state is not per window, as you might expect from the name – it is per WindowServer connection (i.e. process wide).

The code registers a single handler, and then broadcasts the message out to all WebPageProxies. The alternative would be to redundantly register multiple handlers which would all be receiving exactly the same message, which seemed inefficient.

It looked like the best way to find all WebPageProxies in existence was from the WebContext – the WebContext statically know all WebContexts in existence, which in turn can find all WebPageProxies, which in turn can find all WebPageProxies. Did I miss a more direct route?
 
> * on the wrong side.
> 
> Even though this isn't currently used by other platforms, there isn't anything really mac specific about it.  Can this just go in WebProcessProxy.cpp?
>
> This can be written as:

Yep, yep, yep - will fix.

[Gavin Barraclough]

Created attachment 220269 [details]
WIP

[Gavin Barraclough]

Created attachment 220275 [details]
Fix

[Gavin Barraclough]

Committed revision 161246.

[Geoffrey Garen]

Comment on attachment 220275 [details]
Fix

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

> Source/WebKit2/ChangeLog:11
> +        WindowServer connection basis, and as such may produce (safe) false positives.

Did you mean "safe false negatives"? I don't think it's ever safe to falsely say that something is not visible to the user when in reality it is.

[Gavin Barraclough]

(In reply to comment #9)
> (From update of attachment 220275 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220275&action=review
> 
> > Source/WebKit2/ChangeLog:11
> > +        WindowServer connection basis, and as such may produce (safe) false positives.
> 
> Did you mean "safe false negatives"? I don't think it's ever safe to falsely say that something is not visible to the user when in reality it is.

Ugh, yes you're right, this is now "safe false negatives" – the comment reflects the initial implementation.

The flag was inverted – in the former version of the patch the value was called "IsModifying", which meant !IsVisuallyIdle – and as such we would sometimes produce safe false positives.