[webkit-reviews] review denied: [Bug 126356] Propagate WindowServer modifications state to WebProcess : [Attachment 220182] GTK fix

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 126356: Propagate WindowServer modifications state to WebProcess
https://bugs.webkit.org/show_bug.cgi?id=126356

Attachment 220182: GTK fix
https://bugs.webkit.org/attachment.cgi?id=220182&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
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);